Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2932373002:
Add a battery icon to the right of Tether networks in system tray. (Closed)
|
Created:
3 years, 6 months ago by lesliewatkins Modified:
3 years, 6 months ago CC:
Ryan Hansberry, chromium-reviews, kalyank, stevenjb+watch_chromium.org, sadrul, oshima+watch_chromium.org, Jeremy Klein, James Hawkins Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a battery icon to the right of Tether networks in system tray.
BUG=672257
Review-Url: https://codereview.chromium.org/2932373002
Cr-Commit-Position: refs/heads/master@{#478702}
Committed: https://chromium.googlesource.com/chromium/src/+/9e8cf3f662fb0c0ad5b662046851209f34a4b476
Patch Set 1 #
Total comments: 7
Patch Set 2 : stevenjb@ comments #
Total comments: 14
Patch Set 3 : khorimoto@ comments #
Total comments: 2
Messages
Total messages: 20 (8 generated)
lesliewatkins@chromium.org changed reviewers: + hansberry@chromium.org, khorimoto@chromium.org, stevenjb@chromium.org
The change to network_state_handler_unittest was done automatically by `git cl format`. I tried unsuccessfully to change it back. Is there some other script in the presubmit/upload process that does the formatting?
Description was changed from ========== Add a battery icon to the right of Tether networks in system tray. BUG= ========== to ========== Add a battery icon to the right of Tether networks in system tray. BUG= 672257 ==========
https://codereview.chromium.org/2932373002/diff/1/ash/system/network/network_... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2932373002/diff/1/ash/system/network/network_... ash/system/network/network_list.cc:598: NetworkStateHandler* handler = NetworkHandler::Get()->network_state_handler(); handler is only used once, so no need for local. https://codereview.chromium.org/2932373002/diff/1/ash/system/network/network_... ash/system/network/network_list.cc:603: if (NetworkTypePattern::Tether().MatchesType(network->type())) { invert and early exit https://codereview.chromium.org/2932373002/diff/1/ash/system/network/network_... ash/system/network/network_list.cc:604: views::ImageView* right_icon = TrayPopupUtils::CreateMainImageView(); Why 'right_icon'? https://codereview.chromium.org/2932373002/diff/1/chromeos/network/network_st... File chromeos/network/network_state_handler_unittest.cc (right): https://codereview.chromium.org/2932373002/diff/1/chromeos/network/network_st... chromeos/network/network_state_handler_unittest.cc:343: ->path()); This file must have been touched somehow in the CL. You should be able to revert it with 'git checkout master chromeos/network/network_state_handler_unittest.cc'.
https://codereview.chromium.org/2932373002/diff/1/ash/system/network/network_... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2932373002/diff/1/ash/system/network/network_... ash/system/network/network_list.cc:598: NetworkStateHandler* handler = NetworkHandler::Get()->network_state_handler(); On 2017/06/12 16:20:51, stevenjb wrote: > handler is only used once, so no need for local. Done. https://codereview.chromium.org/2932373002/diff/1/ash/system/network/network_... ash/system/network/network_list.cc:603: if (NetworkTypePattern::Tether().MatchesType(network->type())) { On 2017/06/12 16:20:51, stevenjb wrote: > invert and early exit Done. https://codereview.chromium.org/2932373002/diff/1/ash/system/network/network_... ash/system/network/network_list.cc:604: views::ImageView* right_icon = TrayPopupUtils::CreateMainImageView(); On 2017/06/12 16:20:51, stevenjb wrote: > Why 'right_icon'? A holdover from before the icon creation was in its own function. I changed the name to 'icon'.
https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:574: void NetworkListView::UpdateViewForNetwork(HoverHighlightView* view, Question: "Update" implies that this will be called multiple times with the same |view| parameter when the view needs to be updated. Does this mean that we could accidentally add multiple "right views"? https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:583: views::View* power_icon = CreatePowerStatusView(info); nit: Can you move the code from here onward to a helper function which sets the "right view"? If you feel that that is overkill, can you add a line break before this section of code and comment about it? https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:585: view->AddRightView(power_icon); nit: Return early to avoid the extra indentation for the else{} block. https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:606: views::ImageView* icon = TrayPopupUtils::CreateMainImageView(); Are we sure CreateMainImageView() is the right function, since this is more of a side-image? Would CreateMoreImageView() be more appropriate? (I'm not sure either way.) https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:626: icon->SetTooltipText(base::FormatPercent(network->battery_percentage())); Does this need to be localized? I could imagine that some other language would write "50%" as "%50", but I don't actually know. https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... File ash/system/network/network_list.h (right): https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... ash/system/network/network_list.h:77: // the charge level of the mobile device that is being used as a hotspot. s/charge level/battery percentage/
lgtm https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:585: view->AddRightView(power_icon); On 2017/06/12 17:22:09, Kyle Horimoto wrote: > nit: Return early to avoid the extra indentation for the else{} block. I would actually suggest against that, at least with the code as-is. If adding a right-side view was the only thing this function did (e.g. if we added a helper as you suggested), then I would agree, but currently it would be very easy for someone to come along and add something to the end of this method, in which case an early exit could cause a subtle bug. Personally I think this is fine as-is, but I would also be fine with a CreateRightView helper also. https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:626: icon->SetTooltipText(base::FormatPercent(network->battery_percentage())); On 2017/06/12 17:22:09, Kyle Horimoto wrote: > Does this need to be localized? I could imagine that some other language would > write "50%" as "%50", but I don't actually know. That's what base::FormatPercent does :)
lesliewatkins@chromium.org changed reviewers: - hansberry@chromium.org
https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:574: void NetworkListView::UpdateViewForNetwork(HoverHighlightView* view, On 2017/06/12 17:22:09, Kyle Horimoto wrote: > Question: "Update" implies that this will be called multiple times with the same > |view| parameter when the view needs to be updated. Does this mean that we could > accidentally add multiple "right views"? Reset() (called at the beginning of Update) prevents that from happening. There's also a DCHECK in AddRightView() to catch such a situation. https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:583: views::View* power_icon = CreatePowerStatusView(info); On 2017/06/12 17:22:09, Kyle Horimoto wrote: > nit: Can you move the code from here onward to a helper function which sets the > "right view"? > > If you feel that that is overkill, can you add a line break before this section > of code and comment about it? I went the line break route for now. Done. https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:585: view->AddRightView(power_icon); On 2017/06/12 17:30:04, stevenjb wrote: > On 2017/06/12 17:22:09, Kyle Horimoto wrote: > > nit: Return early to avoid the extra indentation for the else{} block. > > I would actually suggest against that, at least with the code as-is. > > If adding a right-side view was the only thing this function did (e.g. if we > added a helper as you suggested), then I would agree, but currently it would be > very easy for someone to come along and add something to the end of this method, > in which case an early exit could cause a subtle bug. > > Personally I think this is fine as-is, but I would also be fine with a > CreateRightView helper also. Acknowledged. https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:606: views::ImageView* icon = TrayPopupUtils::CreateMainImageView(); On 2017/06/12 17:22:09, Kyle Horimoto wrote: > Are we sure CreateMainImageView() is the right function, since this is more of a > side-image? Would CreateMoreImageView() be more appropriate? (I'm not sure > either way.) Looking more closely at these methods, I believe you're right! Both seem to work, but it may make a nicer image if the ImageView has the same dimensions as the Canvas. Done. https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:626: icon->SetTooltipText(base::FormatPercent(network->battery_percentage())); On 2017/06/12 17:22:09, Kyle Horimoto wrote: > Does this need to be localized? I could imagine that some other language would > write "50%" as "%50", but I don't actually know. Yes, but I think FormatPercent() does do the localization. Someone correct me if I'm wrong... https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... File ash/system/network/network_list.h (right): https://codereview.chromium.org/2932373002/diff/20001/ash/system/network/netw... ash/system/network/network_list.h:77: // the charge level of the mobile device that is being used as a hotspot. On 2017/06/12 17:22:09, Kyle Horimoto wrote: > s/charge level/battery percentage/ Done.
lgtm Nice work!
The CQ bit was checked by lesliewatkins@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2932373002/#ps40001 (title: "khorimoto@ comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497291187249660,
"parent_rev": "73a5c4c765070f73a26c33eb489fca61322c6643", "commit_rev":
"9e8cf3f662fb0c0ad5b662046851209f34a4b476"}
Message was sent while issue was closed.
Description was changed from ========== Add a battery icon to the right of Tether networks in system tray. BUG= 672257 ========== to ========== Add a battery icon to the right of Tether networks in system tray. BUG= 672257 Review-Url: https://codereview.chromium.org/2932373002 Cr-Commit-Position: refs/heads/master@{#478702} Committed: https://chromium.googlesource.com/chromium/src/+/9e8cf3f662fb0c0ad5b662046851... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9e8cf3f662fb0c0ad5b662046851...
Message was sent while issue was closed.
estade@chromium.org changed reviewers: + estade@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2932373002/diff/40001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2932373002/diff/40001/ash/system/network/netw... ash/system/network/network_list.cc:610: gfx::Size canvas_size = gfx::Size(kMenuIconSize, kMenuIconSize); Hi all, I am making some changes to the battery icon so I noticed this CL. There are a couple things I see to keep in mind for the future. The first is that this is using an icon at a size that it's not designed for. It's subtle but you can tell the icon should only be displayed at 16dip because that's what the 1x.icon file specifies. (I'm working on adding a DCHECK for this but can't do so until errant callsites are cleaned up.) Displaying at any other size won't have guaranteed sharp lines: in this case, if you zoom in you can see that it's a bit blurrier than we'd like. The second observation is that this copies a lot of code from PowerStatus, which should instead be shared. The final observation is that assuming a 1x scale and using CreateFrom1xBitmap is not appropriate as the icon for non-1x scales is slightly different. I will fold these suggestions into my change but I wanted to share them here for future reference. Thanks and sorry for the late notice.
Message was sent while issue was closed.
https://codereview.chromium.org/2932373002/diff/40001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2932373002/diff/40001/ash/system/network/netw... ash/system/network/network_list.cc:610: gfx::Size canvas_size = gfx::Size(kMenuIconSize, kMenuIconSize); On 2017/06/14 18:04:38, Evan Stade wrote: > Hi all, > > I am making some changes to the battery icon so I noticed this CL. There are a > couple things I see to keep in mind for the future. The first is that this is > using an icon at a size that it's not designed for. It's subtle but you can tell > the icon should only be displayed at 16dip because that's what the 1x.icon file > specifies. (I'm working on adding a DCHECK for this but can't do so until errant > callsites are cleaned up.) Displaying at any other size won't have guaranteed > sharp lines: in this case, if you zoom in you can see that it's a bit blurrier > than we'd like. The second observation is that this copies a lot of code from > PowerStatus, which should instead be shared. The final observation is that > assuming a 1x scale and using CreateFrom1xBitmap is not appropriate as the icon > for non-1x scales is slightly different. > > I will fold these suggestions into my change but I wanted to share them here for > future reference. Thanks and sorry for the late notice. Thanks for the heads-up, Evan! Just wanted to clarify - you'll be fixing this issue in a follow-up CL (so we don't have to submit a change ourselves), right? Please CC us on your change so we can see the correct way to use these icons - thank you! :)
Message was sent while issue was closed.
On 2017/06/15 20:14:24, Kyle Horimoto wrote: > https://codereview.chromium.org/2932373002/diff/40001/ash/system/network/netw... > File ash/system/network/network_list.cc (right): > > https://codereview.chromium.org/2932373002/diff/40001/ash/system/network/netw... > ash/system/network/network_list.cc:610: gfx::Size canvas_size = > gfx::Size(kMenuIconSize, kMenuIconSize); > On 2017/06/14 18:04:38, Evan Stade wrote: > > Hi all, > > > > I am making some changes to the battery icon so I noticed this CL. There are a > > couple things I see to keep in mind for the future. The first is that this is > > using an icon at a size that it's not designed for. It's subtle but you can > tell > > the icon should only be displayed at 16dip because that's what the 1x.icon > file > > specifies. (I'm working on adding a DCHECK for this but can't do so until > errant > > callsites are cleaned up.) Displaying at any other size won't have guaranteed > > sharp lines: in this case, if you zoom in you can see that it's a bit blurrier > > than we'd like. The second observation is that this copies a lot of code from > > PowerStatus, which should instead be shared. The final observation is that > > assuming a 1x scale and using CreateFrom1xBitmap is not appropriate as the > icon > > for non-1x scales is slightly different. > > > > I will fold these suggestions into my change but I wanted to share them here > for > > future reference. Thanks and sorry for the late notice. > > Thanks for the heads-up, Evan! Just wanted to clarify - you'll be fixing this > issue in a follow-up CL (so we don't have to submit a change ourselves), right? correct > > Please CC us on your change so we can see the correct way to use these icons - > thank you! :) In this case, I was actually busy removing this icon for a different reason when I noticed this usage, so the correct way is to delete the icon and do something else :) Usually, the correct thing would have been to ask the designer for an icon designed for 20x20dip. You can see there are many parallel icons called ash/resources/vector_icons/system_menu_foo.icon vs. ash/resources/system_tray_foo.icon which reflect the difference between designed for 16x16 vs designed for 20x20. The CL that fixes this is here: https://chromium-review.googlesource.com/c/535973/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
