Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2039403002:
Renamed AppMenuBadgeController to AppMenuIconController (Closed)
DescriptionRenamed AppMenuBadgeController to AppMenuIconController
Since the app menu doesn't use "badges" under MD, it uses separate icons in place of the three vertically oriented dots. This is a follow-on CL to this one: https://codereview.chromium.org/1966643002/
BUG=584342
TBR=sky@chromium.org
Committed: https://crrev.com/03fb0f755b76b5b7fcef11e21548b98a0f9c6468
Cr-Commit-Position: refs/heads/master@{#399295}
Patch Set 1 #
Total comments: 4
Patch Set 2 : More badge->icon renames and comment changes #Patch Set 3 : Reword comment #
Total comments: 9
Patch Set 4 : Some more renames and updates. #Patch Set 5 : Renamed identifiers in Cocoa file #Messages
Total messages: 53 (21 generated)
Description was changed from ========== Renamed AppMenuBadgeController to AppMenuIconController BUG= ========== to ========== Along with the requisite files (.cc & .h). ==========
kylixrd@chromium.org changed reviewers: + estade@chromium.org, msw@chromium.org
I went ahead and also updated toolbar_controller.mm even though I'm not able to test whether it builds right now. I presume a try-bot will help there.
Removed some XP specific code. https://codereview.chromium.org/2039403002/diff/1/chrome/browser/ui/toolbar/a... File chrome/browser/ui/toolbar/app_menu_icon_controller.cc (left): https://codereview.chromium.org/2039403002/diff/1/chrome/browser/ui/toolbar/a... chrome/browser/ui/toolbar/app_menu_icon_controller.cc:94: } Since we're removing XP/Vista specific code, I went ahead and got rid of this.
lgtm with nits https://codereview.chromium.org/2039403002/diff/1/chrome/browser/ui/toolbar/a... File chrome/browser/ui/toolbar/app_menu_icon_controller.h (right): https://codereview.chromium.org/2039403002/diff/1/chrome/browser/ui/toolbar/a... chrome/browser/ui/toolbar/app_menu_icon_controller.h:17: // AppMenuBadgeController encapsulates the logic for badging the app menu icon nit: update all mentions of badge in comments, please. https://codereview.chromium.org/2039403002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/2039403002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_view.cc:256: // Add any necessary icons to the menu item based on the system state. nit: Should this be "Set the button icon based on the system state."?
Changed more 'badge' to 'icon'.
lgtm https://codereview.chromium.org/2039403002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/2039403002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_view.cc:256: // Add any necessary icons to the menu item based on the system state. On 2016/06/07 01:01:38, msw wrote: > nit: Should this be "Set the button icon based on the system state."? ping
On 2016/06/07 16:56:49, msw wrote: > On 2016/06/07 01:01:38, msw wrote: > > nit: Should this be "Set the button icon based on the system state."? > > ping Done.
kylixrd@chromium.org changed reviewers: + finnur@chromium.org
kylixrd@chromium.org changed reviewers: + shess@chromium.org
Added a couple of reviewers. finnur@, review changes related to changing of NOTIFICATION_MODULE_INCOMPATIBILITY_BADGE_CHANGE to NOTIFICATION_MODULE_INCOMPATIBILITY_ICON_CHANGE shess@, review Objective C changes related to rename of AppMenuBadgeController to AppMenuIconController.
lgtm for Objective-C changes, but your CL should absolutely not be checked in with its current description. Note that only the bit under "Description" will end up in the git check-in. The Subject does _not_ end up in the git log. Poke around at checked-in CLs to see what I mean. It also wouldn't hurt to include a reason for why you're doing this, perhaps with a BUG reference. The goal should be that if your CL is in the window of a build break, someone reviewing the CLs should be able to easily tell that your CL is (or is not) at fault without having to drill down and review it.
Description was changed from ========== Along with the requisite files (.cc & .h). ========== to ========== Renamed AppMenuBadgeController to AppMenuIconController since the app menu doesn't use "badges" under MD. It uses separate icons in place of the three vertically oriented dots. This is a follow-on CL to this one: https://codereview.chromium.org/1966643002/ ==========
On 2016/06/07 19:46:40, Scott Hess wrote: > lgtm for Objective-C changes, but your CL should absolutely not be checked in > with its current description. > > Note that only the bit under "Description" will end up in the git check-in. The > Subject does _not_ end up in the git log. Poke around at checked-in CLs to see > what I mean. It also wouldn't hurt to include a reason for why you're doing > this, perhaps with a BUG reference. The goal should be that if your CL is in > the window of a build break, someone reviewing the CLs should be able to easily > tell that your CL is (or is not) at fault without having to drill down and > review it. I actually thought the subject and description were included. That is good to know, thanks. Description has been updated with a reference to the preceding CL which prompted this one.
On 2016/06/07 20:22:17, kylix_rd wrote: > On 2016/06/07 19:46:40, Scott Hess wrote: > > lgtm for Objective-C changes, but your CL should absolutely not be checked in > > with its current description. > > > > Note that only the bit under "Description" will end up in the git check-in. > The > > Subject does _not_ end up in the git log. Poke around at checked-in CLs to > see > > what I mean. It also wouldn't hurt to include a reason for why you're doing > > this, perhaps with a BUG reference. The goal should be that if your CL is in > > the window of a build break, someone reviewing the CLs should be able to > easily > > tell that your CL is (or is not) at fault without having to drill down and > > review it. > > I actually thought the subject and description were included. That is good to > know, thanks. Nope! Generally, CL's should be something like: Subject-like line on the order of 72 chars. Meatier description paragraph-style, unless it's pretty trivial. BUG=# The Subject-like line is picked off by our infrastructure to be the Rietveld Subject: and email Subject:. git-cl-upload will do the right thing if you write like above in your local git checkin. For a CL like this which is just renaming, the meatier description is likely optional - though it hardly ever hurts to have it. The BUG=# stitches the CL into the indicates bug(s), and should generally be there unless it's a really trivial isolated cleanup (in which case BUG=none is recommended). If you have a series of inter-related CLs, the BUG= is very useful, because if someone needs to revert one of them, the bug can act as a central place to see that the CL may have later dependent CLs to worry about. > Description has been updated with a reference to the preceding CL which prompted > this one. Please include the previous CL's BUG= line, then. I'm not _entirely_ sure about how Rietveld formatting interacts with the git log, but it seems likely it takes it entirely as-is. My preference would be to have the Subject as a standalone line around 72 chars or less, then the rest of the description however is easiest.
Description was changed from ========== Renamed AppMenuBadgeController to AppMenuIconController since the app menu doesn't use "badges" under MD. It uses separate icons in place of the three vertically oriented dots. This is a follow-on CL to this one: https://codereview.chromium.org/1966643002/ ========== to ========== Renamed AppMenuBadgeController to AppMenuIconController since the app menu doesn't use "badges" under MD. It uses separate icons in place of the three vertically oriented dots. This is a follow-on CL to this one: https://codereview.chromium.org/1966643002/ BUG=584342 ==========
Description was changed from ========== Renamed AppMenuBadgeController to AppMenuIconController since the app menu doesn't use "badges" under MD. It uses separate icons in place of the three vertically oriented dots. This is a follow-on CL to this one: https://codereview.chromium.org/1966643002/ BUG=584342 ========== to ========== Renamed AppMenuBadgeController to AppMenuIconController Since the app menu doesn't use "badges" under MD. It uses separate icons in place of the three vertically oriented dots. This is a follow-on CL to this one: https://codereview.chromium.org/1966643002/ BUG=584342 ==========
> > Description has been updated with a reference to the preceding CL which > prompted > > this one. > > Please include the previous CL's BUG= line, then. Added BUG= entry and included the title with the description so it ends up in the git logs.
On 2016/06/07 20:38:19, kylix_rd wrote: > > > Description has been updated with a reference to the preceding CL which > > prompted > > > this one. > > > > Please include the previous CL's BUG= line, then. > > Added BUG= entry and included the title with the description so it ends up in > the git logs. LGTM, I've nit-picked enough :-).
LGTM
Description was changed from ========== Renamed AppMenuBadgeController to AppMenuIconController Since the app menu doesn't use "badges" under MD. It uses separate icons in place of the three vertically oriented dots. This is a follow-on CL to this one: https://codereview.chromium.org/1966643002/ BUG=584342 ========== to ========== Renamed AppMenuBadgeController to AppMenuIconController Since the app menu doesn't use "badges" under MD, it uses separate icons in place of the three vertically oriented dots. This is a follow-on CL to this one: https://codereview.chromium.org/1966643002/ BUG=584342 ==========
thanks for this cleanup. My nits are mostly around the use of "icon". It's better than badge now but even better would be something generic enough to survive the next UI redesign. (I would still use AppMenuIconController but some places that use "icon" can be more generic.) https://codereview.chromium.org/2039403002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/2039403002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:648: void ToolbarView::UpdateIconSeverity(AppMenuIconController::IconType type, nit: would be easier to grok if you copied the pattern of DoesIntersectRect (stating what interface this is for) https://codereview.chromium.org/2039403002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:682: incompatibility_icon_showing = true; I would call this the more generic/logical "incompatibility_warning_showing" https://codereview.chromium.org/2039403002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.h (right): https://codereview.chromium.org/2039403002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.h:168: void UpdateIconSeverity(AppMenuIconController::IconType type, in cases like this I would go ahead and remove the "badge" and just leave it at UpdateSeverity, which is generic enough to apply to multiple possible UI treatments. (Or you can call it something like UpdateSeverityIndication) https://codereview.chromium.org/2039403002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.h:211: AppMenuIconController icon_controller_; can you rename to app_menu_icon_controller_? In the context of the toolbar view, there are many icons.
thanks for this cleanup. My nits are mostly around the use of "icon". It's better than badge now but even better would be something generic enough to survive the next UI redesign. (I would still use AppMenuIconController but some places that use "icon" can be more generic.)
https://codereview.chromium.org/2039403002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/2039403002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:648: void ToolbarView::UpdateIconSeverity(AppMenuIconController::IconType type, On 2016/06/10 18:01:57, Evan Stade wrote: > nit: would be easier to grok if you copied the pattern of DoesIntersectRect > (stating what interface this is for) Could you be a little more specific about what you mean? There is already a comment on the declaration for the delegate's method. https://codereview.chromium.org/2039403002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:682: incompatibility_icon_showing = true; On 2016/06/10 18:01:57, Evan Stade wrote: > I would call this the more generic/logical "incompatibility_warning_showing" Done. https://codereview.chromium.org/2039403002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.h (right): https://codereview.chromium.org/2039403002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.h:168: void UpdateIconSeverity(AppMenuIconController::IconType type, On 2016/06/10 18:01:57, Evan Stade wrote: > in cases like this I would go ahead and remove the "badge" and just leave it at > UpdateSeverity, which is generic enough to apply to multiple possible UI > treatments. (Or you can call it something like UpdateSeverityIndication) Done. https://codereview.chromium.org/2039403002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.h:211: AppMenuIconController icon_controller_; On 2016/06/10 18:01:57, Evan Stade wrote: > can you rename to app_menu_icon_controller_? In the context of the toolbar view, > there are many icons. Done.
lgtm https://codereview.chromium.org/2039403002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/2039403002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:648: void ToolbarView::UpdateIconSeverity(AppMenuIconController::IconType type, On 2016/06/10 19:08:53, kylix_rd wrote: > On 2016/06/10 18:01:57, Evan Stade wrote: > > nit: would be easier to grok if you copied the pattern of DoesIntersectRect > > (stating what interface this is for) > > Could you be a little more specific about what you mean? There is already a > comment on the declaration for the delegate's method. it seems you figured it out :)
On 2016/06/10 19:38:58, Evan Stade wrote: > it seems you figured it out :) I was clearly over-thinking what you meant...
The CQ bit was checked by kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, finnur@chromium.org, shess@chromium.org Link to the patchset: https://codereview.chromium.org/2039403002/#ps60001 (title: "Some more renames and updates.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2039403002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
kylixrd@chromium.org changed reviewers: + sky@chromium.org
Need l.g.t.m. from sky@ for the chrome_notification_types.h file.
Scott is out through Thursday the 16th.
On 2016/06/10 20:09:28, msw wrote: > Scott is out through Thursday the 16th. I think it should be fine to TBR=sky@chromium.org in the CL desc since it's a one line, non-behavioral change.
kylixrd@chromium.org changed reviewers: + thestig@chromium.org - sky@chromium.org
Description was changed from ========== Renamed AppMenuBadgeController to AppMenuIconController Since the app menu doesn't use "badges" under MD, it uses separate icons in place of the three vertically oriented dots. This is a follow-on CL to this one: https://codereview.chromium.org/1966643002/ BUG=584342 ========== to ========== Renamed AppMenuBadgeController to AppMenuIconController Since the app menu doesn't use "badges" under MD, it uses separate icons in place of the three vertically oriented dots. This is a follow-on CL to this one: https://codereview.chromium.org/1966643002/ BUG=584342 TBR=sky@chromium.org ==========
kylixrd@chromium.org changed reviewers: + sky@chromium.org - thestig@chromium.org
On 2016/06/10 20:13:11, Evan Stade wrote: > On 2016/06/10 20:09:28, msw wrote: > > Scott is out through Thursday the 16th. > > I think it should be fine to mailto:TBR=sky@chromium.org in the CL desc since it's a > one line, non-behavioral change. Ok, I added that line to the description.
The CQ bit was checked by kylixrd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2039403002/60001
On 2016/06/10 20:13:11, Evan Stade wrote: > On 2016/06/10 20:09:28, msw wrote: > > Scott is out through Thursday the 16th. > > I think it should be fine to mailto:TBR=sky@chromium.org in the CL desc since it's a > one line, non-behavioral change. Hmmm... do I need commit access for this to work? I've yet to be nominated for such access.
On 2016/06/10 20:18:17, kylix_rd wrote: > On 2016/06/10 20:13:11, Evan Stade wrote: > > On 2016/06/10 20:09:28, msw wrote: > > > Scott is out through Thursday the 16th. > > > > I think it should be fine to mailto:TBR=sky@chromium.org in the CL desc since > it's a > > one line, non-behavioral change. > > Hmmm... do I need commit access for this to work? I've yet to be nominated for > such access. Nevermind... it seems to have passed the presubmit try-bot this time.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org, estade@chromium.org, shess@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2039403002/#ps80001 (title: "Renamed identifiers in Cocoa file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2039403002/80001
Message was sent while issue was closed.
Description was changed from ========== Renamed AppMenuBadgeController to AppMenuIconController Since the app menu doesn't use "badges" under MD, it uses separate icons in place of the three vertically oriented dots. This is a follow-on CL to this one: https://codereview.chromium.org/1966643002/ BUG=584342 TBR=sky@chromium.org ========== to ========== Renamed AppMenuBadgeController to AppMenuIconController Since the app menu doesn't use "badges" under MD, it uses separate icons in place of the three vertically oriented dots. This is a follow-on CL to this one: https://codereview.chromium.org/1966643002/ BUG=584342 TBR=sky@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Renamed AppMenuBadgeController to AppMenuIconController Since the app menu doesn't use "badges" under MD, it uses separate icons in place of the three vertically oriented dots. This is a follow-on CL to this one: https://codereview.chromium.org/1966643002/ BUG=584342 TBR=sky@chromium.org ========== to ========== Renamed AppMenuBadgeController to AppMenuIconController Since the app menu doesn't use "badges" under MD, it uses separate icons in place of the three vertically oriented dots. This is a follow-on CL to this one: https://codereview.chromium.org/1966643002/ BUG=584342 TBR=sky@chromium.org Committed: https://crrev.com/03fb0f755b76b5b7fcef11e21548b98a0f9c6468 Cr-Commit-Position: refs/heads/master@{#399295} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/03fb0f755b76b5b7fcef11e21548b98a0f9c6468 Cr-Commit-Position: refs/heads/master@{#399295} |
