Updated toolbar download button style #17769

simonhong · 2023-03-27T06:46:05Z

Normal state -

In-progress -

Completed and user doesn't click the button yet.

fix brave/brave-browser#29324

Resolves

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

simonhong · 2023-03-27T23:54:30Z

patches/chrome-browser-ui-views-download-bubble-download_toolbar_button_view.cc.patch

+ namespace {
+
+-constexpr int kProgressRingRadius = 9;
+constexpr int kProgressRingRadius = 12;


Made patch here. I think this is the most simplest way.
As this value is used in the middle of PaintButtonContents(),
we need to override that method and copy the most of that method's impls.

simonhong · 2023-03-27T23:54:56Z

chromium_src/chrome/browser/ui/views/download/bubble/download_toolbar_button_view.h

+  }                                                       \
+                                                          \
+ protected:                                               \
+  DownloadDisplayController* display_controller() const { \


Added to call this from the const method (GetIconColor()).

nit: returning mutable from const is not great, so another option here is to implement this as DownloadDisplayController::IconInfo GetIconInfo() const { return controller_->GetIconInfo(); }.

I think the return type should be const DownloadDisplayController* based on https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/const.md - The returned member could be changed so this getter doesn't seem to be logically const.

But changing return type like so would make us use const_cast<>, so I think we can remove this getter here and just use const_cast<> from the callsite.

I just saw @goodov right after I'd written the comment above. +1 for @goodov 's suggestion.

Thanks both! https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/const.md is nice doc :)

goodov · 2023-03-28T05:58:16Z

chromium_src/chrome/browser/ui/views/download/bubble/download_toolbar_button_view.h

+  }                                                       \
+                                                          \
+ protected:                                               \
+  DownloadDisplayController* display_controller() const { \


nit: returning mutable from const is not great, so another option here is to implement this as DownloadDisplayController::IconInfo GetIconInfo() const { return controller_->GetIconInfo(); }.

sangwoo108 · 2023-03-28T06:07:36Z

chromium_src/chrome/browser/ui/views/download/bubble/download_toolbar_button_view.cc

+
+#define DownloadToolbarButtonView DownloadToolbarButtonViewChromium
+
+#include "src/chrome/browser/ui/views/download/bubble/download_toolbar_button_view.cc"
+
+#undef DownloadToolbarButtonView
+


I'm not sure if it's worth, but can we remove the patch file by overriding gfx::RectFToSkRect() like this?

Suggested change

#define DownloadToolbarButtonView DownloadToolbarButtonViewChromium

#include "src/chrome/browser/ui/views/download/bubble/download_toolbar_button_view.cc"

#undef DownloadToolbarButtonView

#include "ui/gfx/geometry/skia_conversions.h"

namespace gfx {

SkRect AdjustRingBounds(const gfx::RectF& ring_bounds);

}

#define RectFToSkRect(ring_bounds) AdjustRingBounds(ring_bounds)

#define DownloadToolbarButtonView DownloadToolbarButtonViewChromium

#include "src/chrome/browser/ui/views/download/bubble/download_toolbar_button_view.cc"

#undef DownloadToolbarButtonView

#undef RectFToSkRect

namespace gfx {

SkRect AdjustRingBounds(const gfx::RectF& ring_bounds) {

const int chromium_ring_radius = ui::TouchUiController::Get()->touch_ui()

? kProgressRingRadiusTouchMode

: kProgressRingRadius;

constexpr auto kBraveRingRadius = 12;

const auto delta = kBraveRingRadius - chromium_ring_radius;

gfx::RectF new_bounds(ring_bounds);

new_bounds.Outset(gfx::OutsetsF(delta));

return gfx::RectFToSkRect(new_bounds);

}

} // namespace gfx

Nice, removed patch!

sangwoo108 · 2023-03-28T06:14:00Z

chromium_src/chrome/browser/ui/views/download/bubble/download_toolbar_button_view.h

+  }                                                       \
+                                                          \
+ protected:                                               \
+  DownloadDisplayController* display_controller() const { \


I think the return type should be const DownloadDisplayController* based on https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/const.md - The returned member could be changed so this getter doesn't seem to be logically const.

But changing return type like so would make us use const_cast<>, so I think we can remove this getter here and just use const_cast<> from the callsite.

fix brave/brave-browser#29324

Updated toolbar download button style

kjozwiak · 2023-03-29T21:54:29Z

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.52.2 Chromium: 112.0.5615.39 (Official Build) nightly (64-bit)
-- | --
Revision | a0e7b9718a92bcd1cf33b7c95316caff3fc20714-refs/branch-heads/5615@{#753}
OS | Windows 11 Version 22H2 (Build 22621.1413)

Using the examples mentioned via #17769 (comment) & brave/brave-browser#29324 (comment), verified the following:

Light Theme

DarkTheme

Merge pull request #17769 from brave/update_download_button_style Updated toolbar download button style

simonhong self-assigned this Mar 27, 2023

simonhong force-pushed the update_download_button_style branch from 88c8bb4 to f1b137d Compare March 27, 2023 07:46

simonhong commented Mar 27, 2023

View reviewed changes

simonhong marked this pull request as ready for review March 27, 2023 23:55

simonhong requested review from a team as code owners March 27, 2023 23:55

simonhong requested a review from sangwoo108 March 27, 2023 23:55

goodov approved these changes Mar 28, 2023

View reviewed changes

sangwoo108 reviewed Mar 28, 2023

View reviewed changes

simonhong added 2 commits March 28, 2023 15:25

Updated toolbar download button style

02f38f7

fix brave/brave-browser#29324

Removed patch file and use overriding to apply custom radius

9ba33a2

simonhong force-pushed the update_download_button_style branch from f1b137d to 9ba33a2 Compare March 28, 2023 08:32

sangwoo108 approved these changes Mar 28, 2023

View reviewed changes

simonhong merged commit 44553a8 into master Mar 28, 2023

simonhong deleted the update_download_button_style branch March 28, 2023 12:03

github-actions bot added this to the 1.51.x - Nightly milestone Mar 28, 2023

simonhong added a commit that referenced this pull request Mar 29, 2023

Merge pull request #17769 from brave/update_download_button_style

88714ef

Updated toolbar download button style

simonhong mentioned this pull request Mar 29, 2023

Updated toolbar download button style (uplift to 1.50.x) #17811

Merged

7 tasks

kjozwiak pushed a commit that referenced this pull request Mar 29, 2023

Updated toolbar download button style (uplift to 1.50.x) (#17811)

13f5504

Merge pull request #17769 from brave/update_download_button_style Updated toolbar download button style

kjozwiak mentioned this pull request Mar 29, 2023

Update toolbar's download button style brave/brave-browser#29324

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated toolbar download button style #17769

Updated toolbar download button style #17769

simonhong commented Mar 27, 2023 •

edited

Loading

simonhong Mar 27, 2023

simonhong Mar 27, 2023 •

edited

Loading

goodov Mar 28, 2023

sangwoo108 Mar 28, 2023

sangwoo108 Mar 28, 2023 •

edited

Loading

simonhong Mar 28, 2023

goodov Mar 28, 2023

sangwoo108 Mar 28, 2023

simonhong Mar 28, 2023

sangwoo108 Mar 28, 2023

kjozwiak commented Mar 29, 2023

-#define DownloadToolbarButtonView DownloadToolbarButtonViewChromium
-#include "src/chrome/browser/ui/views/download/bubble/download_toolbar_button_view.cc"
-#undef DownloadToolbarButtonView
+#include "ui/gfx/geometry/skia_conversions.h"
+namespace gfx {
+SkRect AdjustRingBounds(const gfx::RectF& ring_bounds);
+}
+#define RectFToSkRect(ring_bounds) AdjustRingBounds(ring_bounds)
+#define DownloadToolbarButtonView DownloadToolbarButtonViewChromium
+#include "src/chrome/browser/ui/views/download/bubble/download_toolbar_button_view.cc"
+#undef DownloadToolbarButtonView
+#undef RectFToSkRect
+namespace gfx {
+SkRect AdjustRingBounds(const gfx::RectF& ring_bounds) {
+  const int chromium_ring_radius = ui::TouchUiController::Get()->touch_ui()
+                        ? kProgressRingRadiusTouchMode
+                        : kProgressRingRadius;
+  constexpr auto kBraveRingRadius = 12;
+  const auto delta = kBraveRingRadius - chromium_ring_radius;
+  gfx::RectF new_bounds(ring_bounds);
+  new_bounds.Outset(gfx::OutsetsF(delta));
+  return gfx::RectFToSkRect(new_bounds);
+}
+}  // namespace gfx

Updated toolbar download button style #17769

Updated toolbar download button style #17769

Conversation

simonhong commented Mar 27, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

simonhong Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangwoo108 Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjozwiak commented Mar 29, 2023

simonhong commented Mar 27, 2023 •

edited

Loading

simonhong Mar 27, 2023 •

edited

Loading

sangwoo108 Mar 28, 2023 •

edited

Loading