Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1966643002:
Integrate new MD update menu icons. (Closed)
|
Created:
4 years, 7 months ago by kylix_rd Modified:
4 years, 6 months ago CC:
chromium-reviews, tfarina, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntegrate new MD update menu icons. Initial cut.
BUG=584342
Committed: https://crrev.com/de643dbfbd9001061a4cbbd1d2423d625852f1a9
Cr-Commit-Position: refs/heads/master@{#398118}
Patch Set 1 : This is here just to start getting real feedback. It is a very naive, raw integration. #
Total comments: 10
Patch Set 2 : Ensure the MD icons are the correct size #Patch Set 3 : Plumb the BadgeType through to the AppMenuButton class #Patch Set 4 : Added command-line switch to simulate discovery of bad module(s) #Patch Set 5 : Uses the BadgeType to select the severity-warning icon. #
Total comments: 5
Patch Set 6 : Many changes necessary to paint the icons with a white glyph. #Patch Set 7 : Removed simulate-bad-modules switch #Patch Set 8 : Removed unecessary include #Patch Set 9 : Merged the medium and high severity icons into one. #Patch Set 10 : Merged the high/medium/low severity icons into one. #Patch Set 11 : Removed the additional CreateVectorIconWithBadge overload #Patch Set 12 : Fully revert paint_vector_icon.cc file #Patch Set 13 : Removed 'badge' icons and added color and new path to existing icons #
Total comments: 8
Patch Set 14 : Updates based on feedback. #
Total comments: 2
Patch Set 15 : Use a switch statement instead of an if..else ladder #
Total comments: 8
Patch Set 16 : Added license headers to icon files. Cleaned up code. #
Total comments: 2
Patch Set 17 : Fixed indent #
Total comments: 13
Patch Set 18 : Renamed icons - interim name until larger refactor. #
Total comments: 1
Messages
Total messages: 64 (10 generated)
Description was changed from ========== Integrate new MD update menu icons. Initial cut. BUG= ========== to ========== Integrate new MD update menu icons. Initial cut. This is here just to start getting real feedback. It is a very naive, raw integration. BUG=584342 ==========
kylixrd@chromium.org changed reviewers: + estade@chromium.org, msw@chromium.org, pkasting@chromium.org
Description was changed from ========== Integrate new MD update menu icons. Initial cut. This is here just to start getting real feedback. It is a very naive, raw integration. BUG=584342 ========== to ========== Integrate new MD update menu icons. Initial cut. BUG=584342 ==========
https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu_button.cc:156: } TODO: Determine other state information to select "update failed", "new extension" & "severity warning" icons. Is this the right/wrong place for this?
https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu_button.cc:158: SetImage(views::Button::STATE_NORMAL, gfx::CreateVectorIcon(icon_id, color)); In seems that calling this gfx::CreateVectorIcon() overload doesn't scale the icon down the the correct size as shown here: https://drive.google.com/a/google.com/file/d/0B6LQg8CWo5bNOGFKZ0g5OVVWVkU/vie... However, I'm reluctant to call this overload: GFX_EXPORT ImageSkia CreateVectorIcon(VectorIconId id, size_t dip_size, SkColor color); with a hard-coded value. Is there a constant or some other way to know what to pass in as the dip_size?
https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu_button.cc:156: } On 2016/05/10 14:20:10, kylix_rd wrote: > TODO: Determine other state information to select "update failed", "new > extension" & "severity warning" icons. > > Is this the right/wrong place for this? You're assuming the menu is changing color due to an update when that isn't necessarily the case. See AppMenuBadgeController::BadgeType I don't think fixing this should be a TODO because showing an upgrade arrow for a global error strikes me as a regression. https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu_button.cc:158: SetImage(views::Button::STATE_NORMAL, gfx::CreateVectorIcon(icon_id, color)); On 2016/05/10 16:23:47, kylix_rd wrote: > In seems that calling this gfx::CreateVectorIcon() overload doesn't scale the > icon down the the correct size as shown here: > https://drive.google.com/a/google.com/file/d/0B6LQg8CWo5bNOGFKZ0g5OVVWVkU/vie... > > However, I'm reluctant to call this overload: > > GFX_EXPORT ImageSkia CreateVectorIcon(VectorIconId id, > size_t dip_size, > SkColor color); > > with a hard-coded value. Is there a constant or some other way to know what to > pass in as the dip_size? hardcode a value (16)
https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu_button.cc:156: } On 2016/05/10 16:48:14, Evan Stade wrote: > On 2016/05/10 14:20:10, kylix_rd wrote: > > TODO: Determine other state information to select "update failed", "new > > extension" & "severity warning" icons. > > > > Is this the right/wrong place for this? > > You're assuming the menu is changing color due to an update when that isn't > necessarily the case. See AppMenuBadgeController::BadgeType The AppMenuBadgeController doesn't update the BadgeType on the AppMenuButton instance; only the severity is updated. Should this information now be plumbed through to this class? > I don't think fixing this should be a TODO because showing an upgrade arrow for > a global error strikes me as a regression. It's only a TODO in term of this review, not in terms of the overall patch. Subsequent patches should resolve these issues. https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu_button.cc:158: SetImage(views::Button::STATE_NORMAL, gfx::CreateVectorIcon(icon_id, color)); On 2016/05/10 16:48:14, Evan Stade wrote: > On 2016/05/10 16:23:47, kylix_rd wrote: > > In seems that calling this gfx::CreateVectorIcon() overload doesn't scale the > > icon down the the correct size as shown here: > > > https://drive.google.com/a/google.com/file/d/0B6LQg8CWo5bNOGFKZ0g5OVVWVkU/vie... > > > > However, I'm reluctant to call this overload: > > > > GFX_EXPORT ImageSkia CreateVectorIcon(VectorIconId id, > > size_t dip_size, > > SkColor color); > > > > with a hard-coded value. Is there a constant or some other way to know what to > > pass in as the dip_size? > > hardcode a value (16) Done.
https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu_button.cc:156: } On 2016/05/10 17:49:01, kylix_rd wrote: > On 2016/05/10 16:48:14, Evan Stade wrote: > > On 2016/05/10 14:20:10, kylix_rd wrote: > > > TODO: Determine other state information to select "update failed", "new > > > extension" & "severity warning" icons. > > > > > > Is this the right/wrong place for this? > > > > You're assuming the menu is changing color due to an update when that isn't > > necessarily the case. See AppMenuBadgeController::BadgeType > > The AppMenuBadgeController doesn't update the BadgeType on the AppMenuButton > instance; only the severity is updated. Should this information now be plumbed > through to this class? I suppose it must, or the logic of mapping state to icon should move out of this class. I would actually consider merging the two enums because it looks like GLOBAL_ERROR and INCOMPATIBILITY_WARNING imply a certain severity (medium) so these aren't really tangential concepts any more. All of this is made more difficult by the need to continue supporting pre-md. Also I think we should worry about how this looks in incognito. You might want slightly different numerical values there (e.g. kGoogleRed300 or something). > > > I don't think fixing this should be a TODO because showing an upgrade arrow > for > > a global error strikes me as a regression. > > It's only a TODO in term of this review, not in terms of the overall patch. > Subsequent patches should resolve these issues. sg
https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu_button.cc:156: } On 2016/05/10 18:04:13, Evan Stade wrote: > On 2016/05/10 17:49:01, kylix_rd wrote: > > On 2016/05/10 16:48:14, Evan Stade wrote: > > > On 2016/05/10 14:20:10, kylix_rd wrote: > > > > TODO: Determine other state information to select "update failed", "new > > > > extension" & "severity warning" icons. > > > > > > > > Is this the right/wrong place for this? > > > > > > You're assuming the menu is changing color due to an update when that isn't > > > necessarily the case. See AppMenuBadgeController::BadgeType > > > > The AppMenuBadgeController doesn't update the BadgeType on the AppMenuButton > > instance; only the severity is updated. Should this information now be plumbed > > through to this class? > > I suppose it must, or the logic of mapping state to icon should move out of this > class. I would actually consider merging the two enums because it looks like > GLOBAL_ERROR and INCOMPATIBILITY_WARNING imply a certain severity (medium) so > these aren't really tangential concepts any more. All of this is made more > difficult by the need to continue supporting pre-md. I've arrived at the same conclusion. A subsequent patch is forthcoming that should accomplish this. Right now, I'm keeping the enumerations separate for, as you said, simplicity in supporting pre-md. > Also I think we should worry about how this looks in incognito. You might want > slightly different numerical values there (e.g. kGoogleRed300 or something). Is Incognito mode available as a global state? > > > > > I don't think fixing this should be a TODO because showing an upgrade arrow > > for > > > a global error strikes me as a regression. > > > > It's only a TODO in term of this review, not in terms of the overall patch. > > Subsequent patches should resolve these issues. > > sg
This patch should plumb the BadgeType through to the AppMenuButton class. There is one icon for which I've not determined the proper representative state, UPDATE_MENU_NEW_EXTENSION. It appears that the orange circle-bang icon is/should be used for both the global error and incompatibility warning.
https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu_button.cc:156: } On 2016/05/10 18:10:51, kylix_rd wrote: > On 2016/05/10 18:04:13, Evan Stade wrote: > > On 2016/05/10 17:49:01, kylix_rd wrote: > > > On 2016/05/10 16:48:14, Evan Stade wrote: > > > > On 2016/05/10 14:20:10, kylix_rd wrote: > > > > > TODO: Determine other state information to select "update failed", "new > > > > > extension" & "severity warning" icons. > > > > > > > > > > Is this the right/wrong place for this? > > > > > > > > You're assuming the menu is changing color due to an update when that > isn't > > > > necessarily the case. See AppMenuBadgeController::BadgeType > > > > > > The AppMenuBadgeController doesn't update the BadgeType on the AppMenuButton > > > instance; only the severity is updated. Should this information now be > plumbed > > > through to this class? > > > > I suppose it must, or the logic of mapping state to icon should move out of > this > > class. I would actually consider merging the two enums because it looks like > > GLOBAL_ERROR and INCOMPATIBILITY_WARNING imply a certain severity (medium) so > > these aren't really tangential concepts any more. All of this is made more > > difficult by the need to continue supporting pre-md. > > I've arrived at the same conclusion. A subsequent patch is forthcoming that > should accomplish this. Right now, I'm keeping the enumerations separate for, as > you said, simplicity in supporting pre-md. > > > Also I think we should worry about how this looks in incognito. You might want > > slightly different numerical values there (e.g. kGoogleRed300 or something). > > Is Incognito mode available as a global state? no. First step would be to see if designers actually do want a different color for incognito. Second step would probably involve putting the colors in NativeTheme. > > > > > > > > I don't think fixing this should be a TODO because showing an upgrade > arrow > > > for > > > > a global error strikes me as a regression. > > > > > > It's only a TODO in term of this review, not in terms of the overall patch. > > > Subsequent patches should resolve these issues. > > > > sg >
https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu_button.cc:156: } On 2016/05/10 18:48:39, Evan Stade wrote: > On 2016/05/10 18:10:51, kylix_rd wrote: > > On 2016/05/10 18:04:13, Evan Stade wrote: > > > On 2016/05/10 17:49:01, kylix_rd wrote: > > > > On 2016/05/10 16:48:14, Evan Stade wrote: > > > > > On 2016/05/10 14:20:10, kylix_rd wrote: > > > > > > TODO: Determine other state information to select "update failed", > "new > > > > > > extension" & "severity warning" icons. > > > > > > > > > > > > Is this the right/wrong place for this? > > > > > > > > > > You're assuming the menu is changing color due to an update when that > > isn't > > > > > necessarily the case. See AppMenuBadgeController::BadgeType > > > > > > > > The AppMenuBadgeController doesn't update the BadgeType on the > AppMenuButton > > > > instance; only the severity is updated. Should this information now be > > plumbed > > > > through to this class? > > > > > > I suppose it must, or the logic of mapping state to icon should move out of > > this > > > class. I would actually consider merging the two enums because it looks like > > > GLOBAL_ERROR and INCOMPATIBILITY_WARNING imply a certain severity (medium) > so > > > these aren't really tangential concepts any more. All of this is made more > > > difficult by the need to continue supporting pre-md. > > > > I've arrived at the same conclusion. A subsequent patch is forthcoming that > > should accomplish this. Right now, I'm keeping the enumerations separate for, > as > > you said, simplicity in supporting pre-md. > > > > > Also I think we should worry about how this looks in incognito. You might > want > > > slightly different numerical values there (e.g. kGoogleRed300 or something). > > > > Is Incognito mode available as a global state? > > no. First step would be to see if designers actually do want a different color > for incognito. Second step would probably involve putting the colors in > NativeTheme. Ah, so the question was more rhetorical and speculative than an actual decision or direction to follow. Do we want to do this now or drop in a TODO and assume this patch is good-enough?
On 2016/05/10 19:17:02, kylix_rd wrote: > https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... > File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): > > https://codereview.chromium.org/1966643002/diff/1/chrome/browser/ui/views/too... > chrome/browser/ui/views/toolbar/app_menu_button.cc:156: } > On 2016/05/10 18:48:39, Evan Stade wrote: > > On 2016/05/10 18:10:51, kylix_rd wrote: > > > On 2016/05/10 18:04:13, Evan Stade wrote: > > > > On 2016/05/10 17:49:01, kylix_rd wrote: > > > > > On 2016/05/10 16:48:14, Evan Stade wrote: > > > > > > On 2016/05/10 14:20:10, kylix_rd wrote: > > > > > > > TODO: Determine other state information to select "update failed", > > "new > > > > > > > extension" & "severity warning" icons. > > > > > > > > > > > > > > Is this the right/wrong place for this? > > > > > > > > > > > > You're assuming the menu is changing color due to an update when that > > > isn't > > > > > > necessarily the case. See AppMenuBadgeController::BadgeType > > > > > > > > > > The AppMenuBadgeController doesn't update the BadgeType on the > > AppMenuButton > > > > > instance; only the severity is updated. Should this information now be > > > plumbed > > > > > through to this class? > > > > > > > > I suppose it must, or the logic of mapping state to icon should move out > of > > > this > > > > class. I would actually consider merging the two enums because it looks > like > > > > GLOBAL_ERROR and INCOMPATIBILITY_WARNING imply a certain severity (medium) > > so > > > > these aren't really tangential concepts any more. All of this is made more > > > > difficult by the need to continue supporting pre-md. > > > > > > I've arrived at the same conclusion. A subsequent patch is forthcoming that > > > should accomplish this. Right now, I'm keeping the enumerations separate > for, > > as > > > you said, simplicity in supporting pre-md. > > > > > > > Also I think we should worry about how this looks in incognito. You might > > want > > > > slightly different numerical values there (e.g. kGoogleRed300 or > something). > > > > > > Is Incognito mode available as a global state? > > > > no. First step would be to see if designers actually do want a different color > > for incognito. Second step would probably involve putting the colors in > > NativeTheme. > > Ah, so the question was more rhetorical and speculative than an actual decision > or direction to follow. > > Do we want to do this now or drop in a TODO and assume this patch is > good-enough? I think we should do it now, but the designers may say "no change necessary".
On 2016/05/10 19:25:45, Evan Stade wrote: > On 2016/05/10 19:17:02, kylix_rd wrote: > > Ah, so the question was more rhetorical and speculative than an actual > decision > > or direction to follow. > > > > Do we want to do this now or drop in a TODO and assume this patch is > > good-enough? > > I think we should do it now, but the designers may say "no change necessary". Seeing as I'm quite new here, who should I contact regarding this?
On 2016/05/10 19:31:26, kylix_rd wrote: > On 2016/05/10 19:25:45, Evan Stade wrote: > > On 2016/05/10 19:17:02, kylix_rd wrote: > > > Ah, so the question was more rhetorical and speculative than an actual > > decision > > > or direction to follow. > > > > > > Do we want to do this now or drop in a TODO and assume this patch is > > > good-enough? > > > > I think we should do it now, but the designers may say "no change necessary". > > Seeing as I'm quite new here, who should I contact regarding this? sgabriel provided the icons, so I'd start with him. Perhaps you can take some screenshots in incognito mode and post them on the bug.
kylixrd@chromium.org changed reviewers: + finnur@chromium.org
This patch added some code useful for testing. Previously could not simulate the state associated with ShouldShowIncompatibilityWarning() in the AppMenuBadgeController::UpdateDelegate method.
https://codereview.chromium.org/1966643002/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/app_menu_button.cc:155: AppMenuBadgeController::BADGE_TYPE_INCOMPATIBILITY_WARNING) The long names for the enum values are causing strange indenting/wrapping here. Maybe the BadgeType enum should be converted to an enum class, and then you can use a type alias to pull that class into the local scope, so you can just do things like "if (type_ == BadgeType::GLOBAL_ERROR)". https://codereview.chromium.org/1966643002/diff/70001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1966643002/diff/70001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:952: const char kSimulateBadModules[] = "simulate-bad-modules"; We try to avoid switches if possible. When they are necessary. they should be for short-term debugging and there should be a concrete plan on when they'll be removed. Do you really need to add this, or have you done sufficient local testing to be satisfied? If there's a need to test the icon chosen when there are bad modules, can that be tested by a unittest or similar instead of a switch?
https://codereview.chromium.org/1966643002/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/app_menu_button.cc:155: AppMenuBadgeController::BADGE_TYPE_INCOMPATIBILITY_WARNING) On 2016/05/11 19:16:46, Peter Kasting wrote: > The long names for the enum values are causing strange indenting/wrapping here. > > Maybe the BadgeType enum should be converted to an enum class, and then you can > use a type alias to pull that class into the local scope, so you can just do > things like "if (type_ == BadgeType::GLOBAL_ERROR)". Should this kind of refactoring work be a separate CL? https://codereview.chromium.org/1966643002/diff/70001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1966643002/diff/70001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:952: const char kSimulateBadModules[] = "simulate-bad-modules"; On 2016/05/11 19:16:46, Peter Kasting wrote: > We try to avoid switches if possible. When they are necessary. they should be > for short-term debugging and there should be a concrete plan on when they'll be > removed. > > Do you really need to add this, or have you done sufficient local testing to be > satisfied? If there's a need to test the icon chosen when there are bad > modules, can that be tested by a unittest or similar instead of a switch? I wasn't clear on the philosophy of adding switches. This can certainly be removed prior to the final commit version.
https://codereview.chromium.org/1966643002/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/app_menu_button.cc:155: AppMenuBadgeController::BADGE_TYPE_INCOMPATIBILITY_WARNING) On 2016/05/11 19:54:43, kylix_rd wrote: > On 2016/05/11 19:16:46, Peter Kasting wrote: > > The long names for the enum values are causing strange indenting/wrapping > here. > > > > Maybe the BadgeType enum should be converted to an enum class, and then you > can > > use a type alias to pull that class into the local scope, so you can just do > > things like "if (type_ == BadgeType::GLOBAL_ERROR)". > > Should this kind of refactoring work be a separate CL? There's no formal rule, it's basically a judgment call. This enum is used in few enough places that converting it would probably be pretty simple, and I'd probably just do it in the same CL (assuming you want to do it at all). But doing it separately first would be OK too.
Refactored BadgeType to an enum class. Added new CreateVectorIconWithBadge overload which allows the caller to specify a different color for the badge. Added "badge" icons.
On 2016/05/11 22:43:02, kylix_rd wrote: > Added new CreateVectorIconWithBadge overload which allows the caller to specify > a different color for the badge. > Added "badge" icons. Can you avoid this overload if the badge icons specify "white" in the icon file?
On 2016/05/11 23:04:18, Peter Kasting wrote: > On 2016/05/11 22:43:02, kylix_rd wrote: > > Added new CreateVectorIconWithBadge overload which allows the caller to > specify > > a different color for the badge. > > Added "badge" icons. > > Can you avoid this overload if the badge icons specify "white" in the icon file? Probably. However, I felt that this overload is more generally useful and could be used elsewhere. Also, doing it this way allows the caller to adjust the color more easily should the need arise. IOW, more flexibility at the expense of a little more code.
On 2016/05/11 23:28:37, kylix_rd wrote: > On 2016/05/11 23:04:18, Peter Kasting wrote: > > On 2016/05/11 22:43:02, kylix_rd wrote: > > > Added new CreateVectorIconWithBadge overload which allows the caller to > > specify > > > a different color for the badge. > > > Added "badge" icons. > > > > Can you avoid this overload if the badge icons specify "white" in the icon > file? > > Probably. However, I felt that this overload is more generally useful and could > be used elsewhere. Also, doing it this way allows the caller to adjust the color > more easily should the need arise. IOW, more flexibility at the expense of a > little more code. I ask because I wanted to add the same thing, for the same reasons as you, many months ago, and Evan vetoed it on the grounds that we didn't truly need it yet and should only add it once it became necessary. Since we seem to be over the hump on vectorizing icons, it seems plausible that we won't need to use this elsewhere in the future, in which case his argument would probably still hold. I will let Evan give his opinion on this :)
On 2016/05/11 23:32:31, Peter Kasting wrote: > On 2016/05/11 23:28:37, kylix_rd wrote: > > On 2016/05/11 23:04:18, Peter Kasting wrote: > > > On 2016/05/11 22:43:02, kylix_rd wrote: > > > > Added new CreateVectorIconWithBadge overload which allows the caller to > > > specify > > > > a different color for the badge. > > > > Added "badge" icons. > > > > > > Can you avoid this overload if the badge icons specify "white" in the icon > > file? > > > > Probably. However, I felt that this overload is more generally useful and > could > > be used elsewhere. Also, doing it this way allows the caller to adjust the > color > > more easily should the need arise. IOW, more flexibility at the expense of a > > little more code. > > I ask because I wanted to add the same thing, for the same reasons as you, many > months ago, and Evan vetoed it on the grounds that we didn't truly need it yet > and should only add it once it became necessary. > > Since we seem to be over the hump on vectorizing icons, it seems plausible that > we won't need to use this elsewhere in the future, in which case his argument > would probably still hold. > > I will let Evan give his opinion on this :) Honestly, I found it a little odd that you could *not* control the color of the badge separately and that is used the same color as the icon... Which would make likely mean the badge would be invisible depending on where it "overlaid" the icon. Internally, the badge is treated as a separate icon and is painted over the icon. <shrug> I'm OK with whatever is decided. Everyone knows my opinion on the matter. :)
Removed simulate-bad-modules command-line switch
Latest update: merged the low/medium/high update severity icons into one. Since the color isn't controlled by the icon itself, having 3 icons that rendered identically didn't make sense.
Removed CreateVectorIconWithBadge overload. Is there anything else needed for final l.g.t.m. on this CL?
On 2016/06/02 14:31:35, kylix_rd wrote: > Removed CreateVectorIconWithBadge overload. > > Is there anything else needed for final l.g.t.m. on this CL? you don't need badges. You want PATH_COLOR_ARGB
On 2016/06/02 22:02:42, Evan Stade wrote: > On 2016/06/02 14:31:35, kylix_rd wrote: > > Removed CreateVectorIconWithBadge overload. > > > > Is there anything else needed for final l.g.t.m. on this CL? > > you don't need badges. You want PATH_COLOR_ARGB Will that work in the middle of an existing path or should a new path be started? I tried that before, placing the PATH_COLOR_ARGB immediately following the CIRCLE entry, and the whole icon wound up white. This is because of the even-odd fill style.
On 2016/06/02 22:25:59, kylix_rd wrote: > On 2016/06/02 22:02:42, Evan Stade wrote: > > On 2016/06/02 14:31:35, kylix_rd wrote: > > > Removed CreateVectorIconWithBadge overload. > > > > > > Is there anything else needed for final l.g.t.m. on this CL? > > > > you don't need badges. You want PATH_COLOR_ARGB > > Will that work in the middle of an existing path or should a new path be > started? I tried that before, placing the PATH_COLOR_ARGB immediately following > the CIRCLE entry, and the whole icon wound up white. This is because of the > even-odd fill style. you'll need a NEW_PATH too.
On 2016/06/02 22:32:11, Evan Stade wrote: > On 2016/06/02 22:25:59, kylix_rd wrote: > > On 2016/06/02 22:02:42, Evan Stade wrote: > > > On 2016/06/02 14:31:35, kylix_rd wrote: > > > > Removed CreateVectorIconWithBadge overload. > > > > > > > > Is there anything else needed for final l.g.t.m. on this CL? > > > > > > you don't need badges. You want PATH_COLOR_ARGB > > > > Will that work in the middle of an existing path or should a new path be > > started? I tried that before, placing the PATH_COLOR_ARGB immediately > following > > the CIRCLE entry, and the whole icon wound up white. This is because of the > > even-odd fill style. > > you'll need a NEW_PATH too. Yep, that works. Thanks!
Removed the 'badge' icons.
https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/app_menu_badge_controller.h (right): https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/app_menu_badge_controller.h:17: // AppMenuBadgeController encapsulates the logic for badging the app menu icon technically I would say this is no longer really "badging" and using this term is now confusing. A badge would be a smaller icon on top of a larger icon. This is just a different icon. https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:159: icon_id = gfx::VectorIconId::UPDATE_MENU_SEVERITY_LOW_MEDIUM_HIGH; it seems like icon id and color should be orthogonal? severity controls color, "badge" type controls icon id. IOW, move icon_id assignments outside of this switch. https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.h (right): https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.h:36: AppMenuBadgeController::BadgeType type, nit: can we match the param ordering of ToolbarView::UpdateBadgeSeverity
https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/app_menu_badge_controller.h (right): https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/app_menu_badge_controller.h:17: // AppMenuBadgeController encapsulates the logic for badging the app menu icon On 2016/06/03 18:58:14, Evan Stade wrote: > technically I would say this is no longer really "badging" and using this term > is now confusing. A badge would be a smaller icon on top of a larger icon. This > is just a different icon. Do you want me to rename it to AppMenuIconController along with the files renamed to app_menu_icon_controller.h/cc? https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:159: icon_id = gfx::VectorIconId::UPDATE_MENU_SEVERITY_LOW_MEDIUM_HIGH; On 2016/06/03 18:58:14, Evan Stade wrote: > it seems like icon id and color should be orthogonal? severity controls color, > "badge" type controls icon id. IOW, move icon_id assignments outside of this > switch. Done. https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.h (right): https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.h:36: AppMenuBadgeController::BadgeType type, On 2016/06/03 18:58:14, Evan Stade wrote: > nit: can we match the param ordering of ToolbarView::UpdateBadgeSeverity Done.
https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/app_menu_badge_controller.h (right): https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/app_menu_badge_controller.h:17: // AppMenuBadgeController encapsulates the logic for badging the app menu icon On 2016/06/03 19:27:14, kylix_rd wrote: > On 2016/06/03 18:58:14, Evan Stade wrote: > > technically I would say this is no longer really "badging" and using this term > > is now confusing. A badge would be a smaller icon on top of a larger icon. > This > > is just a different icon. > > Do you want me to rename it to AppMenuIconController along with the files > renamed to app_menu_icon_controller.h/cc? that would be great. Other stuff like UpdateBadgeSeverity should be changed as well. You can do this in a follow up patch to keep this one simpler and to separate functional changes from code cleanup. https://codereview.chromium.org/1966643002/diff/250001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/250001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:159: if (severity_ != AppMenuIconPainter::SEVERITY_NONE) { why this check? Wouldn't type_ be NONE in this case? Using a switch is preferred so the compiler will complain when a new, unhandled type is added to the enum.
https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/app_menu_badge_controller.h (right): https://codereview.chromium.org/1966643002/diff/230001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/app_menu_badge_controller.h:17: // AppMenuBadgeController encapsulates the logic for badging the app menu icon On 2016/06/03 19:31:25, Evan Stade wrote: > On 2016/06/03 19:27:14, kylix_rd wrote: > > On 2016/06/03 18:58:14, Evan Stade wrote: > > > technically I would say this is no longer really "badging" and using this > term > > > is now confusing. A badge would be a smaller icon on top of a larger icon. > > This > > > is just a different icon. > > > > Do you want me to rename it to AppMenuIconController along with the files > > renamed to app_menu_icon_controller.h/cc? > > that would be great. Other stuff like UpdateBadgeSeverity should be changed as > well. You can do this in a follow up patch to keep this one simpler and to > separate functional changes from code cleanup. Ok, I'll add it to my list of thinks to do after this one is done. https://codereview.chromium.org/1966643002/diff/250001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/250001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:159: if (severity_ != AppMenuIconPainter::SEVERITY_NONE) { On 2016/06/03 19:31:26, Evan Stade wrote: > why this check? Wouldn't type_ be NONE in this case? The code was assuming that if the severity_ is NONE, then there would be no need to select a different icon_id regardless of the type_ (it's set to BROWSER_TOOLS above). IOW, it's a state that shouldn't happen. I could add a DCHECK() to the BadgeType::NONE case to ensure this. > Using a switch is preferred so the compiler will complain when a new, unhandled > type is added to the enum. That's a very good point.
https://codereview.chromium.org/1966643002/diff/270001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:139: using BadgeType = AppMenuBadgeController::BadgeType; I think we generally don't use using. Adding the qualifier below won't actually make any of the lines wrap will it? https://codereview.chromium.org/1966643002/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:141: gfx::VectorIconId icon_id = gfx::VectorIconId::BROWSER_TOOLS; nit: move to right above 2nd switch https://codereview.chromium.org/1966643002/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:161: // icon_id = gfx::VectorIconId::BROWSER_TOOLS; /* Already set above */ uncomment this, make initialization VECTOR_ICON_NONE https://codereview.chromium.org/1966643002/diff/270001/ui/gfx/vector_icons/up... File ui/gfx/vector_icons/update_menu_failed.icon (right): https://codereview.chromium.org/1966643002/diff/270001/ui/gfx/vector_icons/up... ui/gfx/vector_icons/update_menu_failed.icon:1: CANVAS_DIMENSIONS, 32, these all require license headers
https://codereview.chromium.org/1966643002/diff/270001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:139: using BadgeType = AppMenuBadgeController::BadgeType; On 2016/06/03 21:44:16, Evan Stade wrote: > I think we generally don't use using. Adding the qualifier below won't actually > make any of the lines wrap will it? Removed. https://codereview.chromium.org/1966643002/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:141: gfx::VectorIconId icon_id = gfx::VectorIconId::BROWSER_TOOLS; On 2016/06/03 21:44:17, Evan Stade wrote: > nit: move to right above 2nd switch Done. https://codereview.chromium.org/1966643002/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:161: // icon_id = gfx::VectorIconId::BROWSER_TOOLS; /* Already set above */ On 2016/06/03 21:44:17, Evan Stade wrote: > uncomment this, make initialization VECTOR_ICON_NONE Done. https://codereview.chromium.org/1966643002/diff/270001/ui/gfx/vector_icons/up... File ui/gfx/vector_icons/update_menu_failed.icon (right): https://codereview.chromium.org/1966643002/diff/270001/ui/gfx/vector_icons/up... ui/gfx/vector_icons/update_menu_failed.icon:1: CANVAS_DIMENSIONS, 32, On 2016/06/03 21:44:17, Evan Stade wrote: > these all require license headers Done.
lgtm https://codereview.chromium.org/1966643002/diff/290001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:160: icon_id = gfx::VectorIconId::BROWSER_TOOLS; nit: indent off
msw@ & finnur@ have been quiet on this review. Are there other reviewers that I should select if these folks are overloaded? https://codereview.chromium.org/1966643002/diff/290001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:160: icon_id = gfx::VectorIconId::BROWSER_TOOLS; On 2016/06/03 22:06:20, Evan Stade wrote: > nit: indent off Done.
On 2016/06/03 22:20:04, kylix_rd wrote: > msw@ & finnur@ have been quiet on this review. Are there other reviewers that I > should select if these folks are overloaded? > > https://codereview.chromium.org/1966643002/diff/290001/chrome/browser/ui/view... > File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): > > https://codereview.chromium.org/1966643002/diff/290001/chrome/browser/ui/view... > chrome/browser/ui/views/toolbar/app_menu_button.cc:160: icon_id = > gfx::VectorIconId::BROWSER_TOOLS; > On 2016/06/03 22:06:20, Evan Stade wrote: > > nit: indent off > > Done. I was waiting for Evan to do the primary review, I'll look now.
https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:161: DCHECK(severity_ == AppMenuIconPainter::SEVERITY_NONE); nit: DCHECK_EQ? https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:164: icon_id = gfx::VectorIconId::UPDATE_MENU_SEVERITY_LOW_MEDIUM_HIGH; nit: these names aren't terribly straightforward. How does LOW_MEDIUM_HIGH compare to WARNING? Should they use be prefixed with BROWSER_TOOLS to match the un-'badged' icon? Should they map directly to the BadgeType enum names? Should UPDATE_MENU_FAILED instead be UPDATE_MENU_SEVERITY_FAILED, to match the UPDATE_MENU_SEVERITY prefix of the others? Sorry I don't quite understand the naming pattern and there are no comments on what the look like or really mean. https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:175: gfx::CreateVectorIcon(icon_id, 16, color)); Hmm, sad to see most related callers using magic numbers like this; maybe at least do something like |const int kButtonIconSize = 16;|? https://codereview.chromium.org/1966643002/diff/310001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://codereview.chromium.org/1966643002/diff/310001/ui/gfx/BUILD.gn#newcod... ui/gfx/BUILD.gn:533: "vector_icons/update_menu_new_extension.icon", This doesn't appear to be used; should it be added later?
Woohoo! More review feedback and discussion. Keep 'em comin'. https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:161: DCHECK(severity_ == AppMenuIconPainter::SEVERITY_NONE); On 2016/06/03 22:34:37, msw wrote: > nit: DCHECK_EQ? I was completely unaware such a beast existed. Will Change. https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:164: icon_id = gfx::VectorIconId::UPDATE_MENU_SEVERITY_LOW_MEDIUM_HIGH; On 2016/06/03 22:34:37, msw wrote: > nit: these names aren't terribly straightforward. How does LOW_MEDIUM_HIGH > compare to WARNING? Should they use be prefixed with BROWSER_TOOLS to match the > un-'badged' icon? Should they map directly to the BadgeType enum names? Should > UPDATE_MENU_FAILED instead be UPDATE_MENU_SEVERITY_FAILED, to match the > UPDATE_MENU_SEVERITY prefix of the others? Sorry I don't quite understand the > naming pattern and there are no comments on what the look like or really mean. I used the names (or derivatives thereof) straight from the designer as per the discussion in this bug: https://bugs.chromium.org/p/chromium/issues/detail?id=584342 I'm otherwise perfectly willing to make naming changes... I just wasn't sure the protocol was and if such things should be done outside the designer's purview. https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:175: gfx::CreateVectorIcon(icon_id, 16, color)); On 2016/06/03 22:34:37, msw wrote: > Hmm, sad to see most related callers using magic numbers like this; maybe at > least do something like |const int kButtonIconSize = 16;|? Agreed. I think I asked at some point about whether there was a constant already defined... IMO, it should be more universally available and used rather than merely defined only within this module.
https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:175: gfx::CreateVectorIcon(icon_id, 16, color)); On 2016/06/03 22:42:45, kylix_rd wrote: > On 2016/06/03 22:34:37, msw wrote: > > Hmm, sad to see most related callers using magic numbers like this; maybe at > > least do something like |const int kButtonIconSize = 16;|? > > Agreed. I think I asked at some point about whether there was a constant already > defined... IMO, it should be more universally available and used rather than > merely defined only within this module. Well most top chrome icons are specified in both 1x and 2x variants and when you do that, you can just take the size from the .icon file. That's why we haven't needed a kToolbarIconSize. There is kFaviconSize but it's merely a coincidence that that's also 16.
https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:175: gfx::CreateVectorIcon(icon_id, 16, color)); On 2016/06/03 22:53:23, Evan Stade wrote: > On 2016/06/03 22:42:45, kylix_rd wrote: > > On 2016/06/03 22:34:37, msw wrote: > > > Hmm, sad to see most related callers using magic numbers like this; maybe at > > > least do something like |const int kButtonIconSize = 16;|? > > > > Agreed. I think I asked at some point about whether there was a constant > already > > defined... IMO, it should be more universally available and used rather than > > merely defined only within this module. > > Well most top chrome icons are specified in both 1x and 2x variants and when you > do that, you can just take the size from the .icon file. That's why we haven't > needed a kToolbarIconSize. There is kFaviconSize but it's merely a coincidence > that that's also 16. So should I look into creating 1x and 2x variants of these new icons then? These files all have CANVAS_DIMENSIONS, 32, which I presume is a 2x icon. https://codereview.chromium.org/1966643002/diff/310001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://codereview.chromium.org/1966643002/diff/310001/ui/gfx/BUILD.gn#newcod... ui/gfx/BUILD.gn:533: "vector_icons/update_menu_new_extension.icon", On 2016/06/03 22:34:37, msw wrote: > This doesn't appear to be used; should it be added later? Should we wait for a consensus or just remove it now?
https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:164: icon_id = gfx::VectorIconId::UPDATE_MENU_SEVERITY_LOW_MEDIUM_HIGH; On 2016/06/03 22:42:45, kylix_rd wrote: > On 2016/06/03 22:34:37, msw wrote: > > nit: these names aren't terribly straightforward. How does LOW_MEDIUM_HIGH > > compare to WARNING? Should they use be prefixed with BROWSER_TOOLS to match > the > > un-'badged' icon? Should they map directly to the BadgeType enum names? Should > > UPDATE_MENU_FAILED instead be UPDATE_MENU_SEVERITY_FAILED, to match the > > UPDATE_MENU_SEVERITY prefix of the others? Sorry I don't quite understand the > > naming pattern and there are no comments on what the look like or really mean. > > I used the names (or derivatives thereof) straight from the designer as per the > discussion in this bug: > https://bugs.chromium.org/p/chromium/issues/detail?id=584342 > > I'm otherwise perfectly willing to make naming changes... I just wasn't sure the > protocol was and if such things should be done outside the designer's purview. Hmm, that might be okay, but it adds another layer of indirection between badge types and their visual implementations. I'd suggest re-using the existing BadgeType enum value names or re-naming the BadgeType enum values to match the new icons names, or picking some new set of names that uses the best of both worlds. Maybe prefix with APP_MENU_ (or similar) instead of UPDATE_MENU_? Evan might have better advice, but I'd suggest something like this: APP_MENU_NORMAL, // The un-badged three-dots chrome app menu icon. APP_MENU_UPDATE, // The arrow shown when an update is available. APP_MENU_EXTENSION, // The puzzle piece shown for extension messages. APP_MENU_WARNING, // The exclamation shown for incompatibility warnings. APP_MENU_ERROR, // The horizontal dash shown for global errors.
https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:175: gfx::CreateVectorIcon(icon_id, 16, color)); On 2016/06/03 23:00:47, kylix_rd wrote: > On 2016/06/03 22:53:23, Evan Stade wrote: > > On 2016/06/03 22:42:45, kylix_rd wrote: > > > On 2016/06/03 22:34:37, msw wrote: > > > > Hmm, sad to see most related callers using magic numbers like this; maybe > at > > > > least do something like |const int kButtonIconSize = 16;|? > > > > > > Agreed. I think I asked at some point about whether there was a constant > > already > > > defined... IMO, it should be more universally available and used rather than > > > merely defined only within this module. > > > > Well most top chrome icons are specified in both 1x and 2x variants and when > you > > do that, you can just take the size from the .icon file. That's why we haven't > > needed a kToolbarIconSize. There is kFaviconSize but it's merely a coincidence > > that that's also 16. > > So should I look into creating 1x and 2x variants of these new icons then? These > files all have CANVAS_DIMENSIONS, 32, which I presume is a 2x icon. You might double check, but I assume Sebastien didn't think these needed a 1x/2x variant. So we're stuck in this slightly annoying situation. That said, whether we choose to hardcode one 16 here or a bunch of 16s in a bunch of .icon files doesn't seem like a huge win either way. One other option would be to change the .icons to size 16. You could do that by adding a scaling transform to the .svg before running through svgo. https://codereview.chromium.org/1966643002/diff/310001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://codereview.chromium.org/1966643002/diff/310001/ui/gfx/BUILD.gn#newcod... ui/gfx/BUILD.gn:533: "vector_icons/update_menu_new_extension.icon", On 2016/06/03 23:00:47, kylix_rd wrote: > On 2016/06/03 22:34:37, msw wrote: > > This doesn't appear to be used; should it be added later? > > Should we wait for a consensus or just remove it now? remove for now, add back when we have a use for it
> msw@ & finnur@ have been quiet on this review. Are there other reviewers that I > should select if these folks are overloaded? It is generally a good idea to find the best reviewer match for your CL and -- if you have multiple reviewers -- to state up-front what you want each person to do. Your review list has four people (including myself), all of which were supposed to review everything (since no further instructions were given). Maybe it's just me, but it felt like the net was cast a little too wide in this case and I would trust any of the three other reviewers to do a more thorough job than I would in this part of the codebase. Granted, I think I have made changes to some of the code in question, but I can't remember when I last made material changes under toolbar/* and I doubt I'm owner for any of this. So, it felt right to me to defer to the others on this.
On 2016/06/03 23:39:03, Finnur wrote: > > msw@ & finnur@ have been quiet on this review. Are there other reviewers that > I > > should select if these folks are overloaded? > > It is generally a good idea to find the best reviewer match for your CL and -- > if you have multiple reviewers -- to state up-front what you want each person to > do. Your review list has four people (including myself), all of which were > supposed to review everything (since no further instructions were given). Maybe > it's just me, but it felt like the net was cast a little too wide in this case > and I would trust any of the three other reviewers to do a more thorough job > than I would in this part of the codebase. Granted, I think I have made changes > to some of the code in question, but I can't remember when I last made material > changes under toolbar/* and I doubt I'm owner for any of this. So, it felt right > to me to defer to the others on this. In this case I followed the protocol of checking the most relevant OWNERS file for each directory involved. I do, however, agree that I may have gone a little overboard on reviewer selection. Since I'm still getting my feet wet on the whole process, i also accept that it'll be a while before I can say I fully have the process down. I know that code reviews can take a huge chunk one's time (I did a lot of code reviews in my previous job) and I endeavor to respect everyone's time. Feel free to remove yourself as a review if I've made a mistake.
finnur@chromium.org changed reviewers: - finnur@chromium.org
> In this case I followed the protocol of checking the most relevant OWNERS file > for each directory involved. I only appear in one of the OWNERS file in question, and it is a narrow per-file rule for extensions/ (which excludes these files). > Since I'm still getting my feet wet on the > whole process, i also accept that it'll be a while before I can say I fully have > the process down. No problem. We've all been there. :)
pkasting@chromium.org changed reviewers: - pkasting@chromium.org
On 2016/06/03 23:45:39, kylix_rd wrote: > On 2016/06/03 23:39:03, Finnur wrote: > > It is generally a good idea to find the best reviewer match for your CL and -- > > if you have multiple reviewers -- to state up-front what you want each person > to > > do. Your review list has four people (including myself), all of which were > > supposed to review everything (since no further instructions were given). > Maybe > > it's just me, but it felt like the net was cast a little too wide in this case > > and I would trust any of the three other reviewers to do a more thorough job > > than I would in this part of the codebase. Granted, I think I have made > changes > > to some of the code in question, but I can't remember when I last made > material > > changes under toolbar/* and I doubt I'm owner for any of this. So, it felt > right > > to me to defer to the others on this. > > In this case I followed the protocol of checking the most relevant OWNERS file > for each directory involved. I do, however, agree that I may have gone a little > overboard on reviewer selection. Since I'm still getting my feet wet on the > whole process, i also accept that it'll be a while before I can say I fully have > the process down. No worries -- just part of getting up to speed on Chromium development. The most critical bit, as Finnur mentions, is that when you specify multiple reviewers you should always say what you want each to do, e.g. estade: Review msw: OWNERS for x/y/z pkasting: OWNERS for a/b/c and d. Also please look at the code in ____ and reply to my comment there. In general, you should rarely have overlapping responsibilities, e.g. two people that you both ask for full review of the same thing. In cases where you're not sure of the right person, pick one and ask them to reassign if they're not correct. I'm going to remove myself here since I think estade's review can pretty much cover everything, but feel free to add me back if you need my review on any particular bits or need an OWNER signoff somewhere.
I've removed the currently unused "extension" icon, renamed the existing icons to "browser_tools_xxx" to match the current "browser_tools" triple-dot icon. These and the enumeration can be renamed in a later refactoring CL to match msw@'s suggestion. I've added .1x variants of the icons, also consistent with the existing pattern set by the "browser_tools.1x" and "browser_tools" icons. I can remove the 2x icons in favor of the 1x versions if folks would rather. estade@ seems to be the preeminent authority on the vectorization process. https://codereview.chromium.org/1966643002/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1966643002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:41: type_(AppMenuBadgeController::BadgeType::NONE), Oops... failed to properly initialize the type_ field in the ctor.
lgtm
The CQ bit was checked by kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org Link to the patchset: https://codereview.chromium.org/1966643002/#ps320001 (title: "Renamed icons - interim name until larger refactor.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1966643002/320001
Message was sent while issue was closed.
Description was changed from ========== Integrate new MD update menu icons. Initial cut. BUG=584342 ========== to ========== Integrate new MD update menu icons. Initial cut. BUG=584342 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Integrate new MD update menu icons. Initial cut. BUG=584342 ========== to ========== Integrate new MD update menu icons. Initial cut. BUG=584342 Committed: https://crrev.com/de643dbfbd9001061a4cbbd1d2423d625852f1a9 Cr-Commit-Position: refs/heads/master@{#398118} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/de643dbfbd9001061a4cbbd1d2423d625852f1a9 Cr-Commit-Position: refs/heads/master@{#398118} |
