Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(22)

Issue 1406613002: For some vector icons, get the size from the vector definition. (Closed)

Created:
5 years, 2 months ago by Evan Stade
Modified:
4 years, 7 months ago
Reviewers:
Peter Kasting, sky
CC:
chromium-reviews, asanka, extensions-reviews_chromium.org, tfarina, noyau+watch_chromium.org, chromium-apps-reviews_chromium.org, miu+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

For some vector icons, get the size from the vector definition. Sometimes various icons intended to be shown in the same place in the UI have different dimensions in their .icon definition, so those call sites can't be updated. For example, GetTabAlertIndicatorImage. This patch is probably not comprehensive but it covers many CreateVectorIcon cases that are possible to update. BUG=505953 Committed: https://crrev.com/3b7f55d085ee8bb37b7124d18778a7b24428c1ff Cr-Commit-Position: refs/heads/master@{#389952}

Patch Set 1 #

Total comments: 1

Patch Set 2 : rebase #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -28 lines) Patch
M ash/frame/caption_buttons/frame_caption_button.cc View 1 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/ui/bookmarks/bookmark_utils.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/bar_control_button.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/extensions/extension_install_dialog_view.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/frame/browser_non_client_frame_view.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/location_bar/selected_keyword_view.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/tabs/tab.cc View 1 3 chunks +3 lines, -5 lines 0 comments Download
M chrome/browser/ui/views/toolbar/app_menu_button.cc View 1 1 chunk +1 line, -4 lines 0 comments Download
M chrome/browser/ui/views/toolbar/toolbar_view.cc View 1 2 chunks +5 lines, -6 lines 0 comments Download
M ui/gfx/paint_vector_icon.h View 1 1 chunk +6 lines, -1 line 0 comments Download
M ui/gfx/paint_vector_icon.cc View 1 1 chunk +7 lines, -0 lines 0 comments Download
M ui/views/controls/menu/menu_image_util.cc View 1 2 chunks +2 lines, -4 lines 1 comment Download

Messages

Total messages: 31 (7 generated)
Evan Stade
As previously discussed, I think it makes sense to treat the primary UI icons differently ...
5 years, 2 months ago (2015-10-13 19:20:00 UTC) #2
sky
+pkasting LGTM, but wait for Peter to be happy (he may have strong opinions on ...
5 years, 2 months ago (2015-10-13 20:15:06 UTC) #3
Evan Stade
+pkasting
5 years, 2 months ago (2015-10-14 20:59:21 UTC) #5
Peter Kasting
I don't think introducing the concept of a "primary" versus "secondary" icon into the codebase ...
5 years, 2 months ago (2015-10-14 23:39:04 UTC) #6
Evan Stade
On 2015/10/14 23:39:04, Peter Kasting wrote: > I don't think introducing the concept of a ...
5 years, 2 months ago (2015-10-14 23:44:46 UTC) #7
Peter Kasting
On 2015/10/14 23:44:46, Evan Stade wrote: > Nope, I still disagree that the secondary icons ...
5 years, 2 months ago (2015-10-15 00:05:54 UTC) #8
Evan Stade
On 2015/10/15 00:05:54, Peter Kasting wrote: > On 2015/10/14 23:44:46, Evan Stade wrote: > > ...
5 years, 2 months ago (2015-10-20 00:29:26 UTC) #9
Peter Kasting
Then let's put this on hold pending the other change, since if that goes through, ...
5 years, 2 months ago (2015-10-20 00:51:54 UTC) #10
Evan Stade
On 2015/10/20 00:51:54, Peter Kasting wrote: > Then let's put this on hold pending the ...
5 years, 2 months ago (2015-10-20 01:02:11 UTC) #11
Evan Stade
ping on this. If you don't foresee having time to work on your change in ...
5 years ago (2015-12-10 23:42:13 UTC) #12
Peter Kasting
On 2015/12/10 23:42:13, Evan Stade wrote: > ping on this. If you don't foresee having ...
5 years ago (2015-12-10 23:49:11 UTC) #13
Evan Stade
On 2015/12/10 23:49:11, Peter Kasting wrote: > On 2015/12/10 23:42:13, Evan Stade wrote: > > ...
4 years, 10 months ago (2016-01-28 05:07:01 UTC) #14
Peter Kasting
On 2016/01/28 05:07:01, Evan Stade wrote: > On 2015/12/10 23:49:11, Peter Kasting wrote: > > ...
4 years, 10 months ago (2016-01-28 05:19:01 UTC) #15
Evan Stade
On 2016/01/28 05:19:01, Peter Kasting wrote: > On 2016/01/28 05:07:01, Evan Stade wrote: > > ...
4 years, 8 months ago (2016-04-20 21:32:05 UTC) #16
Peter Kasting
The only thing I'm reluctant on here is the terminology "primary UI icon". It seems ...
4 years, 8 months ago (2016-04-21 23:06:04 UTC) #17
Evan Stade
On 2016/04/21 23:06:04, Peter Kasting wrote: > Just CreateVectorIcon(), and talk about the other stuff ...
4 years, 7 months ago (2016-04-25 22:31:30 UTC) #18
Evan Stade
ptal, also note updated CL description. https://codereview.chromium.org/1406613002/diff/20001/ui/views/controls/menu/menu_image_util.cc File ui/views/controls/menu/menu_image_util.cc (left): https://codereview.chromium.org/1406613002/diff/20001/ui/views/controls/menu/menu_image_util.cc#oldcode15 ui/views/controls/menu/menu_image_util.cc:15: return gfx::CreateVectorIcon(gfx::VectorIconId::MENU_CHECK, kMenuCheckSize, ...
4 years, 7 months ago (2016-04-26 16:58:29 UTC) #20
Peter Kasting
LGTM, but it looks like the set of affected callsites changed significantly between the two ...
4 years, 7 months ago (2016-04-26 20:16:39 UTC) #21
Evan Stade
On 2016/04/26 20:16:39, Peter Kasting wrote: > LGTM, but it looks like the set of ...
4 years, 7 months ago (2016-04-26 22:52:04 UTC) #22
Evan Stade
Scott, did you want to take a look at the new patchset? It's not much ...
4 years, 7 months ago (2016-04-26 22:54:06 UTC) #23
sky
SLGTM
4 years, 7 months ago (2016-04-26 23:41:36 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1406613002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406613002/20001
4 years, 7 months ago (2016-04-26 23:42:20 UTC) #27
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 7 months ago (2016-04-27 00:21:34 UTC) #29
commit-bot: I haz the power
4 years, 7 months ago (2016-04-27 00:22:45 UTC) #31
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/3b7f55d085ee8bb37b7124d18778a7b24428c1ff
Cr-Commit-Position: refs/heads/master@{#389952}

Powered by Google App Engine
This is Rietveld 408576698