Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2845593002:
Updates Zoom bubble layout and adds +/- buttons (Closed)
DescriptionUpdates Zoom bubble layout and adds +/- buttons
Adds + and - buttons to the Zoom bubble.
Updates layout to only have one row.
With this change the zoom icon (ZoomView) in location bar
can be shown even at 100% zoom as long as the zoom bubble
is also shown (otherwise the bubble loses its anchor when
using + / - buttons to go over 100% zoom without closing
the bubble.
In this scenario a kZoomPlusIcon is used (rather than none).
Whenever the zoom bubble gets closed the location bar gets
updated to update layout and visibility of the zoom icon.
BUG=651641
TEST=no new tests but a few existing tests updated or checked
ZoomControllerTest
ZoomDecorationTest
ZoomControllerBrowserTest
PDFExtensionTest.PdfZoomWithoutBubble
TBR=phajdan.jr@chromium.org (for a trivial change in chrome/test/base/test_browser_window.h)
Review-Url: https://codereview.chromium.org/2845593002
Cr-Original-Commit-Position: refs/heads/master@{#472611}
Committed: https://chromium.googlesource.com/chromium/src/+/c4d839e6e1f0963b2760620cb0ed072b802bf225
Review-Url: https://codereview.chromium.org/2845593002
Cr-Commit-Position: refs/heads/master@{#472891}
Committed: https://chromium.googlesource.com/chromium/src/+/f6f47b5a77854b023c3ccd6c57d1584051f2280a
Patch Set 1 #Patch Set 2 : Updates Zoom bubble layout and adds +/- buttons (mac-views - rebased) #Patch Set 3 : Updates Zoom bubble layout and adds +/- buttons (mac-views) - fixing the non-mac build #Patch Set 4 : Updates Zoom bubble layout and adds +/- buttons (rebased) #Patch Set 5 : Updates Zoom bubble layout and adds +/- buttons (vector images) #
Total comments: 18
Patch Set 6 : Updates Zoom bubble layout and adds +/- buttons (comments) #
Total comments: 33
Patch Set 7 : Updates Zoom bubble layout and adds +/- buttons (comments) #
Total comments: 16
Patch Set 8 : Updates Zoom bubble layout and adds +/- buttons (comments) #
Total comments: 10
Patch Set 9 : Updates Zoom bubble layout and adds +/- buttons (layout) #Patch Set 10 : Keep zoom icon hidden when zoom is 100% #Patch Set 11 : Always shows zoom icon, even when zoom is 100% #Patch Set 12 : Always shows zoom icon, even when zoom is 100% (rebased) #
Total comments: 20
Patch Set 13 : Comments and nits #Patch Set 14 : Hide zoom icon at 100% #Patch Set 15 : Make a fix for 100% zoom case portable to Mac #Patch Set 16 : Making tests happy #Patch Set 17 : Fix ZoomController to never show the bubble when zoom level is not changed from default #
Total comments: 2
Patch Set 18 : Updates Zoom bubble layout and adds +/- buttons (reland) #Patch Set 19 : Fix compilation error on gcc #Messages
Total messages: 115 (82 generated)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Patchset #3 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
Patchset #5 (id:180001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
varkha@chromium.org changed reviewers: + estade@chromium.org
estade@, can you please take a first look? One thing that can be simplified is using a simpler layout manager like BoxLayout, not sure if this would make the state with both MD and pre-MD code paths simpler though. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:154: label_->SetText(label_text); Self review: carve out utility UpdateZoomPercent() method and reuse here and in Init(). https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:257: l10n_util::GetStringUTF16(IDS_ACCNAME_ZOOM_MINUS2)); Self review: carve out utility ImageButton* CreateZoomButton(icon, tooltip_id). https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:290: } nit: no newline.
lgtm https://codereview.chromium.org/2845593002/diff/200001/chrome/app/vector_icon... File chrome/app/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2845593002/diff/200001/chrome/app/vector_icon... chrome/app/vector_icons/BUILD.gn:53: "minus.1x.icon", could you call this zoom_minus and zoom_plus? There are likely many places we might use plus and minus icons in Chrome but their exact appearance might differ. Plus this makes it immediately obvious how these are used for now, and if we later want to reuse them for something not zoom related we can make that switch easily. (other icons with generic names here like "code" are copying the names on go/icons) https://codereview.chromium.org/2845593002/diff/200001/chrome/app/vector_icon... File chrome/app/vector_icons/plus.1x.icon (right): https://codereview.chromium.org/2845593002/diff/200001/chrome/app/vector_icon... chrome/app/vector_icons/plus.1x.icon:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2017 (etc) https://codereview.chromium.org/2845593002/diff/200001/chrome/app/vector_icon... File chrome/app/vector_icons/plus.icon (right): https://codereview.chromium.org/2845593002/diff/200001/chrome/app/vector_icon... chrome/app/vector_icons/plus.icon:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:45: constexpr int kVerticalInset = 2; I would prefer these be defined where they are used. That gives them context. Also I don't really see an advantage to listing them on different lines, as opposed to const gfx::Insets kInsetsForSomething(2, 0, 2, 10); https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:48: constexpr int kResetButtonPadding = 10; ditto https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:289: l10n_util::GetStringUTF16(IDS_ZOOM_SET_DEFAULT)); so it's normally "Reset" with a tooltip of "Reset to Default". That doesn't seem that useful, I'd either skip the tooltip or, if "reset" is really ambiguous, use "Reset to default zoom level", or even better, "Reset to [whatever the default zoom level is as per settings]" this is a product decision so I wouldn't block this CL on it.
varkha@chromium.org changed reviewers: + pkasting@chromium.org
+pkasting@ for OWNERS in c/b/ui/views/ (assume that Evan's comments are addressed, working across time zones). Thanks!
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
https://codereview.chromium.org/2845593002/diff/200001/chrome/app/vector_icon... File chrome/app/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2845593002/diff/200001/chrome/app/vector_icon... chrome/app/vector_icons/BUILD.gn:53: "minus.1x.icon", On 2017/04/27 17:29:38, Evan Stade wrote: > could you call this zoom_minus and zoom_plus? There are likely many places we > might use plus and minus icons in Chrome but their exact appearance might > differ. Plus this makes it immediately obvious how these are used for now, and > if we later want to reuse them for something not zoom related we can make that > switch easily. > > (other icons with generic names here like "code" are copying the names on > go/icons) Those are already taken (by magnifying glass icons). Since I based those on the icons I've found on go/icons I will rename them to generic add / remove. https://codereview.chromium.org/2845593002/diff/200001/chrome/app/vector_icon... File chrome/app/vector_icons/plus.1x.icon (right): https://codereview.chromium.org/2845593002/diff/200001/chrome/app/vector_icon... chrome/app/vector_icons/plus.1x.icon:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/04/27 17:29:39, Evan Stade wrote: > 2017 (etc) Done. https://codereview.chromium.org/2845593002/diff/200001/chrome/app/vector_icon... File chrome/app/vector_icons/plus.icon (right): https://codereview.chromium.org/2845593002/diff/200001/chrome/app/vector_icon... chrome/app/vector_icons/plus.icon:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/04/27 17:29:39, Evan Stade wrote: > 2017 Done. https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:45: constexpr int kVerticalInset = 2; On 2017/04/27 17:29:39, Evan Stade wrote: > I would prefer these be defined where they are used. That gives them context. > Also I don't really see an advantage to listing them on different lines, as > opposed to > > const gfx::Insets kInsetsForSomething(2, 0, 2, 10); Done. https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:48: constexpr int kResetButtonPadding = 10; On 2017/04/27 17:29:39, Evan Stade wrote: > ditto Done. https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:154: label_->SetText(label_text); On 2017/04/27 11:42:39, varkha wrote: > Self review: carve out utility UpdateZoomPercent() method and reuse here and in > Init(). Done. https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:257: l10n_util::GetStringUTF16(IDS_ACCNAME_ZOOM_MINUS2)); On 2017/04/27 11:42:39, varkha wrote: > Self review: carve out utility ImageButton* > CreateZoomButton(icon, tooltip_id). Done. https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:289: l10n_util::GetStringUTF16(IDS_ZOOM_SET_DEFAULT)); On 2017/04/27 17:29:39, Evan Stade wrote: > so it's normally "Reset" with a tooltip of "Reset to Default". That doesn't seem > that useful, I'd either skip the tooltip or, if "reset" is really ambiguous, use > "Reset to default zoom level", or even better, "Reset to [whatever the default > zoom level is as per settings]" > > this is a product decision so I wouldn't block this CL on it. Acknowledged. I've added a todo to change that when IDS_ZOOM_SET_DEFAULTS is no longer used. I do find "Reset" to be a bit ambiguous here and to include a numeric value in "Reset to []" version I think, might be confusing as well since where that value comes from may not be obvious to a user in this context. https://codereview.chromium.org/2845593002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:290: } On 2017/04/27 11:42:39, varkha wrote: > nit: no newline. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/harmony_layout_provider.cc (right): https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_layout_provider.cc:99: for (int snap_point : {220, 320, 448, 512}) { Don't add this, as this isn't a Harmony snap point. This surface should not be snapped to any width. Are you seeing it snapped to 320? I thought this would not have a dialog delegate. Maybe this is a case where we need to add the virtual override for dialogs to ask not to be snapped, which Elly had suggested when adding dialog snapping? I'd check with ellyjones/tapted about this if you're seeing dialog snapping. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:324: (zoom_view_->visible() || ZoomBubbleView::GetZoomBubble())) { When would GetZoomBubble() be true but |zoom_view_| not be visible? https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:201: const bool material = ui::MaterialDesignController::IsSecondaryUiMaterial(); Do we have to key this off secondary-ui-material? I thought this was a UI we'd wanted for several years now. I'd like to avoid the conditionals and just change this. This would hopefully also let us switch to a box layout. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:203: const int kResetButtonPadding = 10; This should be using a constant from the layout provider, presumably the unrelated control horizontal spacing. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:204: const gfx::Insets kBubbleContentsInsets(2, 0, 2, 10); These numbers are very magic (and I don't recognize them). It seems like these should be using the toast insets from the harmony spec ( https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(... ). I would think the best way to do this would be to provide a new InsetsMetric in the layout provider for this. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:237: ui::NativeTheme::kColorId_ButtonEnabledColor)); The latest Harmony mock I see shows this in normal text color, which is what I'd expect. Using a button color for a label is surprising. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:292: base::AutoReset<bool> auto_ignore_close_bubble(&ignore_close_bubble_, true); Is this so if the timer is already running, it won't do anything? If so, why not just cancel the timer here instead of adding this variable? https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:293: auto_close_ = false; Won't this mean once you click a button the bubble never auto-hides? Seems like clicking a button should just reset the timer (to a longer value like 5 seconds). https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:372: views::Button* ZoomBubbleView::CreateZoomButton(const gfx::VectorIcon& icon, Nit: It would be nice if this returned a unique_ptr to make it clear it was transferring ownership back.
Thanks Peter, will post updates but wanted to answer the questions first. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/harmony_layout_provider.cc (right): https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_layout_provider.cc:99: for (int snap_point : {220, 320, 448, 512}) { On 2017/04/28 05:58:48, Peter Kasting wrote: > Don't add this, as this isn't a Harmony snap point. > > This surface should not be snapped to any width. Are you seeing it snapped to > 320? I thought this would not have a dialog delegate. Maybe this is a case > where we need to add the virtual override for dialogs to ask not to be snapped, > which Elly had suggested when adding dialog snapping? I'd check with > ellyjones/tapted about this if you're seeing dialog snapping. Yes the dialog was snapping to 320 and this went away when I added another snap point. I thought this snapping made sense since the zoom percent label could have different width. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:324: (zoom_view_->visible() || ZoomBubbleView::GetZoomBubble())) { On 2017/04/28 05:58:48, Peter Kasting wrote: > When would GetZoomBubble() be true but |zoom_view_| not be visible? When going over 100% with +/- buttons. Consider that setting zoom to 100% would have hidden the bubble. I block that with the auto-reset var and then the code below updates the label when the bubble is still shown. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:201: const bool material = ui::MaterialDesignController::IsSecondaryUiMaterial(); On 2017/04/28 05:58:48, Peter Kasting wrote: > Do we have to key this off secondary-ui-material? I thought this was a UI we'd > wanted for several years now. I'd like to avoid the conditionals and just > change this. > > This would hopefully also let us switch to a box layout. Yes, I'll do that if this is not blocked on Harmony launch. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:203: const int kResetButtonPadding = 10; On 2017/04/28 05:58:48, Peter Kasting wrote: > This should be using a constant from the layout provider, presumably the > unrelated control horizontal spacing. Acknowledged. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:204: const gfx::Insets kBubbleContentsInsets(2, 0, 2, 10); On 2017/04/28 05:58:48, Peter Kasting wrote: > These numbers are very magic (and I don't recognize them). > > It seems like these should be using the toast insets from the harmony spec ( > https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(... > ). I would think the best way to do this would be to provide a new InsetsMetric > in the layout provider for this. Acknowledged. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:237: ui::NativeTheme::kColorId_ButtonEnabledColor)); On 2017/04/28 05:58:48, Peter Kasting wrote: > The latest Harmony mock I see shows this in normal text color, which is what I'd > expect. Using a button color for a label is surprising. Acknowledged. On Mac the labels are all muted gray so I was trying to preserve that I guess. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:292: base::AutoReset<bool> auto_ignore_close_bubble(&ignore_close_bubble_, true); On 2017/04/28 05:58:48, Peter Kasting wrote: > Is this so if the timer is already running, it won't do anything? If so, why > not just cancel the timer here instead of adding this variable? No, this is to skip over 100% without closing the dialog. I think any button interaction here should never result in the bubble getting closed so I've added this here rather than plumbing do_not_close_me variable up and down. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:293: auto_close_ = false; On 2017/04/28 05:58:48, Peter Kasting wrote: > Won't this mean once you click a button the bubble never auto-hides? Seems like > clicking a button should just reset the timer (to a longer value like 5 > seconds). Yes. This is similar to ET_GESTURE_TAP handling that also blocks auto_close. I wasn't expecting a dialog to close on me after interacting with it, especially with the buttons that invite repetition like +/- where repetition may be rare for some people and any auto-close timer might come when a user is about to press or tap +/- again. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:372: views::Button* ZoomBubbleView::CreateZoomButton(const gfx::VectorIcon& icon, On 2017/04/28 05:58:48, Peter Kasting wrote: > Nit: It would be nice if this returned a unique_ptr to make it clear it was > transferring ownership back. Acknowledged.
https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/harmony_layout_provider.cc (right): https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_layout_provider.cc:99: for (int snap_point : {220, 320, 448, 512}) { On 2017/04/28 06:21:19, varkha wrote: > On 2017/04/28 05:58:48, Peter Kasting wrote: > > Don't add this, as this isn't a Harmony snap point. > > > > This surface should not be snapped to any width. Are you seeing it snapped to > > 320? I thought this would not have a dialog delegate. Maybe this is a case > > where we need to add the virtual override for dialogs to ask not to be > snapped, > > which Elly had suggested when adding dialog snapping? I'd check with > > ellyjones/tapted about this if you're seeing dialog snapping. > > Yes the dialog was snapping to 320 and this went away when I added another snap > point. > I thought this snapping made sense since the zoom percent label could have > different width. One problem is that this isn't a snap width in the spec, and adding it here will affect every dialog in Chrome. If you're trying to force the label to have a constant width, I'd do that by adjusting how the zoom bubble computes the label position and the dialog preferred size, e.g. by measuring the width needed to actually show "100%". https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:324: (zoom_view_->visible() || ZoomBubbleView::GetZoomBubble())) { On 2017/04/28 06:21:19, varkha wrote: > On 2017/04/28 05:58:48, Peter Kasting wrote: > > When would GetZoomBubble() be true but |zoom_view_| not be visible? > > When going over 100% with +/- buttons. Consider that setting zoom to > 100% would have hidden the bubble. I block that with the auto-reset var > and then the code below updates the label when the bubble is still > shown. Hmm. I'm inclined to say the zoom view should always be visible when the bubble is visible, even if the bubble is at 100% (or it's not really anchored to anything in that case). Then when the bubble disappears, if the zoom level is 100%, the zoom view can go away too. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:237: ui::NativeTheme::kColorId_ButtonEnabledColor)); On 2017/04/28 06:21:19, varkha wrote: > On 2017/04/28 05:58:48, Peter Kasting wrote: > > The latest Harmony mock I see shows this in normal text color, which is what > I'd > > expect. Using a button color for a label is surprising. > > Acknowledged. On Mac the labels are all muted gray so I was trying to preserve > that > I guess. What do you mean by "on Mac the labels are all muted gray"? Are you referring to the screenshot in the bug? That is from an outdated mock and should basically be ignored at this point. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:292: base::AutoReset<bool> auto_ignore_close_bubble(&ignore_close_bubble_, true); On 2017/04/28 06:21:19, varkha wrote: > On 2017/04/28 05:58:48, Peter Kasting wrote: > > Is this so if the timer is already running, it won't do anything? If so, why > > not just cancel the timer here instead of adding this variable? > > No, this is to skip over 100% without closing the dialog. I think any button > interaction here should never result in the bubble getting closed so I've > added this here rather than plumbing do_not_close_me variable up and > down. My brain hurts trying to understand how closing works because we now seem to have multiple levels of callbacks, guarding each other's functionality using auto-resetting boolean members. Can you find a more direct way of plumbing things? I'm sorry, it's too late at night for me to have concrete suggestions on precisely how to do this. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:293: auto_close_ = false; On 2017/04/28 06:21:19, varkha wrote: > On 2017/04/28 05:58:48, Peter Kasting wrote: > > Won't this mean once you click a button the bubble never auto-hides? Seems > like > > clicking a button should just reset the timer (to a longer value like 5 > > seconds). > > Yes. This is similar to ET_GESTURE_TAP handling that also blocks auto_close. I > wasn't expecting a dialog to close on me after interacting with it, especially > with the > buttons that invite repetition like +/- where repetition may be rare for some > people > and any auto-close timer might come when a user is about to press or tap +/- > again. Stopping auto-close permanently is potentially annoying. If you think the timer reset idea I have (using a longer timeout) is not the right way to go, I would bring this up with hwi.
varkha@chromium.org changed reviewers: + tapted@chromium.org
https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/harmony_layout_provider.cc (right): https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_layout_provider.cc:99: for (int snap_point : {220, 320, 448, 512}) { On 2017/04/28 06:47:26, Peter Kasting (OOO thru May 3) wrote: > On 2017/04/28 06:21:19, varkha wrote: > > On 2017/04/28 05:58:48, Peter Kasting wrote: > > > Don't add this, as this isn't a Harmony snap point. > > > > > > This surface should not be snapped to any width. Are you seeing it snapped > to > > > 320? I thought this would not have a dialog delegate. Maybe this is a case > > > where we need to add the virtual override for dialogs to ask not to be > > snapped, > > > which Elly had suggested when adding dialog snapping? I'd check with > > > ellyjones/tapted about this if you're seeing dialog snapping. > > > > Yes the dialog was snapping to 320 and this went away when I added another > snap > > point. > > I thought this snapping made sense since the zoom percent label could have > > different width. > > One problem is that this isn't a snap width in the spec, and adding it here will > affect every dialog in Chrome. > > If you're trying to force the label to have a constant width, I'd do that by > adjusting how the zoom bubble computes the label position and the dialog > preferred size, e.g. by measuring the width needed to actually show "100%". Done. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:324: (zoom_view_->visible() || ZoomBubbleView::GetZoomBubble())) { On 2017/04/28 06:47:26, Peter Kasting (OOO thru May 3) wrote: > On 2017/04/28 06:21:19, varkha wrote: > > On 2017/04/28 05:58:48, Peter Kasting wrote: > > > When would GetZoomBubble() be true but |zoom_view_| not be visible? > > > > When going over 100% with +/- buttons. Consider that setting zoom to > > 100% would have hidden the bubble. I block that with the auto-reset var > > and then the code below updates the label when the bubble is still > > shown. > > Hmm. I'm inclined to say the zoom view should always be visible when the bubble > is visible, even if the bubble is at 100% (or it's not really anchored to > anything in that case). Then when the bubble disappears, if the zoom level is > 100%, the zoom view can go away too. I think this creates more complexity. Consider that for 100% case we would need an additional icon in location bar (without + or -). Even more confusing is what should happen when a user opens a zoom bubble, then uses + / - buttons to get zoom to 100% and then clicks the location bar icon. Presumably the bubble will close and presumably the icon will get hidden. So we will have a button that gets hidden when clicked, probably suboptimal. Interestingly enough on Mac we now allow the bubble to get through 100% zoom using + / - buttons and the zoom icon in location bar disappears when zoom is set to 100%. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:201: const bool material = ui::MaterialDesignController::IsSecondaryUiMaterial(); On 2017/04/28 06:21:19, varkha wrote: > On 2017/04/28 05:58:48, Peter Kasting wrote: > > Do we have to key this off secondary-ui-material? I thought this was a UI > we'd > > wanted for several years now. I'd like to avoid the conditionals and just > > change this. > > > > This would hopefully also let us switch to a box layout. > > Yes, I'll do that if this is not blocked on Harmony launch. Done. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:203: const int kResetButtonPadding = 10; On 2017/04/28 06:21:19, varkha wrote: > On 2017/04/28 05:58:48, Peter Kasting wrote: > > This should be using a constant from the layout provider, presumably the > > unrelated control horizontal spacing. > > Acknowledged. Done. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:204: const gfx::Insets kBubbleContentsInsets(2, 0, 2, 10); On 2017/04/28 06:21:19, varkha wrote: > On 2017/04/28 05:58:48, Peter Kasting wrote: > > These numbers are very magic (and I don't recognize them). > > > > It seems like these should be using the toast insets from the harmony spec ( > > > https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(... > > ). I would think the best way to do this would be to provide a new > InsetsMetric > > in the layout provider for this. > > Acknowledged. Done. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:237: ui::NativeTheme::kColorId_ButtonEnabledColor)); On 2017/04/28 06:47:26, Peter Kasting (OOO thru May 3) wrote: > On 2017/04/28 06:21:19, varkha wrote: > > On 2017/04/28 05:58:48, Peter Kasting wrote: > > > The latest Harmony mock I see shows this in normal text color, which is what > > I'd > > > expect. Using a button color for a label is surprising. > > > > Acknowledged. On Mac the labels are all muted gray so I was trying to preserve > > that > > I guess. > > What do you mean by "on Mac the labels are all muted gray"? Are you referring > to the screenshot in the bug? That is from an outdated mock and should > basically be ignored at this point. Done. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:292: base::AutoReset<bool> auto_ignore_close_bubble(&ignore_close_bubble_, true); On 2017/04/28 06:47:26, Peter Kasting (OOO thru May 3) wrote: > On 2017/04/28 06:21:19, varkha wrote: > > On 2017/04/28 05:58:48, Peter Kasting wrote: > > > Is this so if the timer is already running, it won't do anything? If so, > why > > > not just cancel the timer here instead of adding this variable? > > > > No, this is to skip over 100% without closing the dialog. I think any button > > interaction here should never result in the bubble getting closed so I've > > added this here rather than plumbing do_not_close_me variable up and > > down. > > My brain hurts trying to understand how closing works because we now seem to > have multiple levels of callbacks, guarding each other's functionality using > auto-resetting boolean members. Can you find a more direct way of plumbing > things? I'm sorry, it's too late at night for me to have concrete suggestions > on precisely how to do this. I've added a comment explaining the reasoning. I will look some more if it is possible to make something that doesn't complicate things too much but this actually seemed to me more understandable than passing an additional UI-related parameter via multiple levels of stack. e.g. Browser::OnZoomChanged would need an additional field (on top of existing data.can_show_bubble) to mark when zoom is changing via the bubble buttons. The chain of invocations currently leading to this is quite wide so this could mushroom in something much larger. Certainly seems like at least a separate task. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:293: auto_close_ = false; On 2017/04/28 06:47:26, Peter Kasting (OOO thru May 3) wrote: > On 2017/04/28 06:21:19, varkha wrote: > > On 2017/04/28 05:58:48, Peter Kasting wrote: > > > Won't this mean once you click a button the bubble never auto-hides? Seems > > like > > > clicking a button should just reset the timer (to a longer value like 5 > > > seconds). > > > > Yes. This is similar to ET_GESTURE_TAP handling that also blocks auto_close. I > > wasn't expecting a dialog to close on me after interacting with it, especially > > with the > > buttons that invite repetition like +/- where repetition may be rare for some > > people > > and any auto-close timer might come when a user is about to press or tap +/- > > again. > > Stopping auto-close permanently is potentially annoying. If you think the timer > reset idea I have (using a longer timeout) is not the right way to go, I would > bring this up with hwi. I've played a bit more with it and I find that 5 seconds is long enough to still be comfortable for repeated presses. Done. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:372: views::Button* ZoomBubbleView::CreateZoomButton(const gfx::VectorIcon& icon, On 2017/04/28 06:21:19, varkha wrote: > On 2017/04/28 05:58:48, Peter Kasting wrote: > > Nit: It would be nice if this returned a unique_ptr to make it clear it was > > transferring ownership back. > > Acknowledged. Done.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Updates Zoom bubble layout and adds +/- buttons (mac-views) Adds + and - buttons to the Zoom bubble. Updates layout to only have one row. The code is behind --secondary-ui-md flag. Includes https://codereview.chromium.org/2834493007/ BUG=651641 ========== to ========== Updates Zoom bubble layout and adds +/- buttons Adds + and - buttons to the Zoom bubble. Updates layout to only have one row. BUG=651641 ==========
https://codereview.chromium.org/2845593002/diff/240001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2845593002/diff/240001/chrome/app/generated_r... chrome/app/generated_resources.grd:5652: Reset to dafault zoom level dafault -> default https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:323: if (can_show_bubble && web_contents && Perhaps comment here? Like // Note that at 100% Zoom, the ZoomBubbleView may be present without the // corresponding location bar decoration, |zoom_view_|. https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:245: views::View* zoom_in_button_padding = new views::View(); Rather than a container View with a border and layout, would it be nicer to have a sibling View that overrides GetPreferredSize with the desired padding? There's actually a PaddingView in ash/system/tray/system_tray.cc already. Otherwise... it feels like maybe we should be using GridLayout :/ or add BoxLayout::SetPaddingForView(..). But I guess this is OK too. https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:286: const gfx::Size kPreferredZoomBubbleSize = gfx::Size(208, 32); constexpr? (the geometry classes have the syntax voodoo to make this work I think) https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:398: if (timer_.IsRunning()) { I don't think this case adds anything. Timer::Start(..) is literally `AssignMembers() && Reset()`, so we can remove this and the call to StopTimer() in ButtonPressed(). https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right): https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:95: gfx::Size GetPreferredSize() const override; nit: GetPreferredSize() comes from views::View, so I'd order it at the top, before the OnFooEvent methods https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:114: std::unique_ptr<views::Button> CreateZoomButton(const gfx::VectorIcon& icon, This doesn't manipulate any members - can it be put in an anonymous namespace in the .cc? std::unique_ptr<views::Button> CreateZoomButton(ButtonListener* listener, const gfx::VectorIcon& icon, int tooltip_id); The gfx::VectorIcon forward declare might not be needed then either https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:137: int timer_duration_; Add units? (e.g. timer_duration_ms_), or use a base::TimeDelta? Or this could also be a bool `any_button_pressed_` and an `if` in StartTimerIfNecessary
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2845593002/diff/240001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2845593002/diff/240001/chrome/app/generated_r... chrome/app/generated_resources.grd:5652: Reset to dafault zoom level On 2017/05/02 01:28:02, tapted wrote: > dafault -> default Done. https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:323: if (can_show_bubble && web_contents && On 2017/05/02 01:28:02, tapted wrote: > Perhaps comment here? Like > > // Note that at 100% Zoom, the ZoomBubbleView may be present without the > // corresponding location bar decoration, |zoom_view_|. Done. https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:245: views::View* zoom_in_button_padding = new views::View(); On 2017/05/02 01:28:02, tapted wrote: > Rather than a container View with a border and layout, would it be nicer to have > a sibling View that overrides GetPreferredSize with the desired padding? > There's actually a PaddingView in ash/system/tray/system_tray.cc already. > > Otherwise... it feels like maybe we should be using GridLayout :/ or add > BoxLayout::SetPaddingForView(..). But I guess this is OK too. Yes, I actually went back and forth on something like this. I think this is better than one more override with a fixed size. We have a few of those, they are never big enough to warrant factoring them into something reusable and so they end up multiplying. I think this is also more explicit. But I don't feel super strongly about it. https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:286: const gfx::Size kPreferredZoomBubbleSize = gfx::Size(208, 32); On 2017/05/02 01:28:02, tapted wrote: > constexpr? (the geometry classes have the syntax voodoo to make this work I > think) Done. https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:398: if (timer_.IsRunning()) { On 2017/05/02 01:28:02, tapted wrote: > I don't think this case adds anything. Timer::Start(..) is literally > `AssignMembers() && Reset()`, so we can remove this and the call to StopTimer() > in ButtonPressed(). Done. https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right): https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:95: gfx::Size GetPreferredSize() const override; On 2017/05/02 01:28:02, tapted wrote: > nit: GetPreferredSize() comes from views::View, so I'd order it at the top, > before the OnFooEvent methods Done. https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:114: std::unique_ptr<views::Button> CreateZoomButton(const gfx::VectorIcon& icon, On 2017/05/02 01:28:02, tapted wrote: > This doesn't manipulate any members - can it be put in an anonymous namespace in > the .cc? > > std::unique_ptr<views::Button> CreateZoomButton(ButtonListener* listener, const > gfx::VectorIcon& icon, int tooltip_id); > > The gfx::VectorIcon forward declare might not be needed then either Done. https://codereview.chromium.org/2845593002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:137: int timer_duration_; On 2017/05/02 01:28:02, tapted wrote: > Add units? (e.g. timer_duration_ms_), or use a base::TimeDelta? > > Or this could also be a bool `any_button_pressed_` and an `if` in > StartTimerIfNecessary I like the self documenting base::TimeDelta consts. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:200: constexpr gfx::Size kPreferredZoomBubbleSize = gfx::Size(208, 32); update: we chatted offline about whether these numbers make sense. They're from the mock, but it's possible weird font settings or long translated strings could cause problems. Investigation pending.
I LGTMed tapted's patch to add the "should snap" bit, so if that lands first, make sure to rebase atop that. https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2845593002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:324: (zoom_view_->visible() || ZoomBubbleView::GetZoomBubble())) { On 2017/05/01 10:11:59, varkha wrote: > On 2017/04/28 06:47:26, Peter Kasting (OOO thru May 3) wrote: > > On 2017/04/28 06:21:19, varkha wrote: > > > On 2017/04/28 05:58:48, Peter Kasting wrote: > > > > When would GetZoomBubble() be true but |zoom_view_| not be visible? > > > > > > When going over 100% with +/- buttons. Consider that setting zoom to > > > 100% would have hidden the bubble. I block that with the auto-reset var > > > and then the code below updates the label when the bubble is still > > > shown. > > > > Hmm. I'm inclined to say the zoom view should always be visible when the > bubble > > is visible, even if the bubble is at 100% (or it's not really anchored to > > anything in that case). Then when the bubble disappears, if the zoom level is > > 100%, the zoom view can go away too. > > I think this creates more complexity. Consider that for 100% case we would need > an additional icon in location bar (without + or -). Yes, but I think that's OK, and if the icon is implemented as "magnifier icon plus optional +/- badge", as is easy in our current system, it wouldn't even be very difficult to implement. > Even more confusing is what > should happen when a user opens a zoom bubble, then uses + / - buttons to get > zoom to 100% and then clicks the location bar icon. Presumably the bubble will > close and presumably the icon will get hidden. So we will have a button that > gets > hidden when clicked, probably suboptimal. That's a good case to have pointed out, but I'm not sure it would actually feel weird as you describe. Maybe I'm wrong? It kinda feels weird either way to me. Being able to have a bubble that doesn't have an anchor element (with visible "ripple" activation present) is pretty weird from a Harmony perspective. I still think doing it the way I suggest would be better, and I still think it might be worth talking to Hwi about it, but I'm not prepared to block on this. Up to you what to do. If you leave it implemented as it is currently, can we simplify the conditional to just check GetZoomBubble()? It seems like that should always be true when |zoom_view_| is visible so checking that separately feels redundant. This would also hopefully avoid the need for much of the comment you added. https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:235: box_layout->SetDefaultFlex(1); Why does this layout need any flex? https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:262: // button has fixed size. I don't really understand this. Why do we need a "related button" margin on the right side of this button? That ought to be provided by the layout manager. https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:267: zoom_in_button_padding->SetBorder(views::CreateEmptyBorder(0, 0, 0, margin)); Even if we did need a margin, BoxLayout can do per-child margins; I'd think that'd be a more appropriate way to do this. https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right): https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:149: // True when handling a button click event. Nit: Maybe "Used to ignore close requests generated automatically in response to button presses, since pressing a button in the bubble should not trigger closing."
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:200: constexpr gfx::Size kPreferredZoomBubbleSize = gfx::Size(208, 32); On 2017/05/04 00:24:11, tapted wrote: > update: we chatted offline about whether these numbers make sense. They're from > the mock, but it's possible weird font settings or long translated strings could > cause problems. Investigation pending. Switched to sizing the bubble based on the label width. https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:235: box_layout->SetDefaultFlex(1); On 2017/05/05 23:50:40, Peter Kasting wrote: > Why does this layout need any flex? Not if the label is set to fixed size based on the maximum zoom. https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:262: // button has fixed size. On 2017/05/05 23:50:40, Peter Kasting wrote: > I don't really understand this. Why do we need a "related button" margin on the > right side of this button? That ought to be provided by the layout manager. Yes, the updated mock seems symmetrical so I've just removed this. https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:267: zoom_in_button_padding->SetBorder(views::CreateEmptyBorder(0, 0, 0, margin)); On 2017/05/05 23:50:40, Peter Kasting wrote: > Even if we did need a margin, BoxLayout can do per-child margins; I'd think > that'd be a more appropriate way to do this. Done. https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right): https://codereview.chromium.org/2845593002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:149: // True when handling a button click event. On 2017/05/05 23:50:40, Peter Kasting wrote: > Nit: Maybe "Used to ignore close requests generated automatically in response to > button presses, since pressing a button in the bubble should not trigger > closing." Done.
LGTM with comments https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:74: SetText(base::FormatPercent(web_contents->GetMaximumZoomPercent())); I worry a little about this, in the sense that it's not clear to me the maximum zoom percent will always be the maximum width string. The max percent is currently 500, and if, say, "4" is 1 px wider than "5" in the user's font, then a 400% zoom will be 1 px wider than a 500%. The safest thing to do is probably to actually iterate through the zoom levels (at least the ones at default and above) and get the width for each, then use the max of all those. There should be a small number of these so this should still be fast. https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:284: box_layout->SetFlexForView(reset_button_, 0); This call shouldn't be necessary, I don't think. https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:307: bool ZoomBubbleView::ShouldSnapFrameWidth() const { You no longer need to provide this -- Elly's recently-landed change should mean you get this behavior by default.
lgtm https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:74: SetText(base::FormatPercent(web_contents->GetMaximumZoomPercent())); this should also take a font style (it currently works out the same, but it's good to be explicit, and if UX later decide to refine the mock to have a different style, we'll know where to look). The best way of doing this will be to call the Label constructor in an initializer list explicit ZoomValue(const content::WebContents* web_contents) : Label(base::FormatPercent(web_contents->GetMaximumZoomPercent()), views::style::CONTEXT_LABEL, views::style::STYLE_PRIMARY), preferred_size_(Label::GetPreferredSize()) { SetText(base::string16()); } I guess preferred_size_ could be const then too (edit: unless we need to iterate) https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:74: SetText(base::FormatPercent(web_contents->GetMaximumZoomPercent())); On 2017/05/16 00:14:35, Peter Kasting wrote: > I worry a little about this, in the sense that it's not clear to me the maximum > zoom percent will always be the maximum width string. The max percent is > currently 500, and if, say, "4" is 1 px wider than "5" in the user's font, then > a 400% zoom will be 1 px wider than a 500%. > > The safest thing to do is probably to actually iterate through the zoom levels > (at least the ones at default and above) and get the width for each, then use > the max of all those. There should be a small number of these so this should > still be fast. true it would be safer. Although I suspect it would be unusual for any digital UI font to use non-"tabular" [1] numerals. Even for languages like Persian, which don't use western-arabic numerals. [1] https://www.fonts.com/content/learning/fontology/level-3/numbers/proportional... https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:75: preferred_size_ = views::Label::GetPreferredSize(); I tend to drop the views:: prefix (inheriting effectively acts as a `using views::Label;` directive) https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:78: ~ZoomValue() override{}; nit: space before {}, no semicolon after https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:80: gfx::Size GetPreferredSize() const override { return preferred_size_; } nit: // Label: https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:313: // No buttons presses in this dialog should cause the dialog to close, "No buttons pressed" or "No button presses" or "A button press.. should not" https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right): https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:90: bool ShouldSnapFrameWidth() const override; since LocationBarBubbleDelegateView::GetDialogButtons() returns NONE I think with Elly's recent patch ( r471471 ) this override is no longer needed. (although the longer story is that this dialog probably should return ui::DIALOG_BUTTON_CANCEL to help with a11y and OS integration, but it would need to do that in a way that didn't cause DialogClientView to add a button row... - something for me to investigate later)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
With this CL the zoom icon in location bar is always shown in views browser. On Mac with --secondary-ui-md the views zoom bubble is used but the zoom decoration is hidden for zoom=100%. I think it is much simpler to just always show the icon and I find it hard to reason what is special for the 100% (or default rather) case that makes the icon unnecessary if we think it is necessary for other values. So those are I think our choices: 1. make the zoom icon always show and simplify the code on Mac. 2. make zoom icon auto-hide soon after closing the bubble (and deal with complexity around delayed states). 3. make zoom icon always shown on touchscreen devices 4. some combination of 2 and 3 I could do the Mac part if necessary (#1) in a separate CL. WDYT? https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:74: SetText(base::FormatPercent(web_contents->GetMaximumZoomPercent())); On 2017/05/16 06:29:02, tapted wrote: > this should also take a font style (it currently works out the same, but it's > good to be explicit, and if UX later decide to refine the mock to have a > different style, we'll know where to look). The best way of doing this will be > to call the Label constructor in an initializer list > > explicit ZoomValue(const content::WebContents* web_contents) > : Label(base::FormatPercent(web_contents->GetMaximumZoomPercent()), > views::style::CONTEXT_LABEL, > views::style::STYLE_PRIMARY), > preferred_size_(Label::GetPreferredSize()) { > SetText(base::string16()); > } > > I guess preferred_size_ could be const then too (edit: unless we need to > iterate) Done. https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:74: SetText(base::FormatPercent(web_contents->GetMaximumZoomPercent())); On 2017/05/16 00:14:35, Peter Kasting wrote: > I worry a little about this, in the sense that it's not clear to me the maximum > zoom percent will always be the maximum width string. The max percent is > currently 500, and if, say, "4" is 1 px wider than "5" in the user's font, then > a 400% zoom will be 1 px wider than a 500%. > > The safest thing to do is probably to actually iterate through the zoom levels > (at least the ones at default and above) and get the width for each, then use > the max of all those. There should be a small number of these so this should > still be fast. This is something we also do in [1] (we also cache the value there but we don't need that here since this is only invoked once). [1] - https://cs.chromium.org/chromium/src/chrome/browser/ui/views/toolbar/app_menu... https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:74: SetText(base::FormatPercent(web_contents->GetMaximumZoomPercent())); On 2017/05/16 06:29:01, tapted wrote: > On 2017/05/16 00:14:35, Peter Kasting wrote: > > I worry a little about this, in the sense that it's not clear to me the > maximum > > zoom percent will always be the maximum width string. The max percent is > > currently 500, and if, say, "4" is 1 px wider than "5" in the user's font, > then > > a 400% zoom will be 1 px wider than a 500%. > > > > The safest thing to do is probably to actually iterate through the zoom levels > > (at least the ones at default and above) and get the width for each, then use > > the max of all those. There should be a small number of these so this should > > still be fast. > > true it would be safer. Although I suspect it would be unusual for any digital > UI font to use non-"tabular" [1] numerals. Even for languages like Persian, > which don't use western-arabic numerals. > > > [1] > https://www.fonts.com/content/learning/fontology/level-3/numbers/proportional... Acknowledged. https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:75: preferred_size_ = views::Label::GetPreferredSize(); On 2017/05/16 06:29:01, tapted wrote: > I tend to drop the views:: prefix (inheriting effectively acts as a `using > views::Label;` directive) Acknowledged. https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:78: ~ZoomValue() override{}; On 2017/05/16 06:29:02, tapted wrote: > nit: space before {}, no semicolon after Done. https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:80: gfx::Size GetPreferredSize() const override { return preferred_size_; } On 2017/05/16 06:29:02, tapted wrote: > nit: // Label: Done. https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:284: box_layout->SetFlexForView(reset_button_, 0); On 2017/05/16 00:14:35, Peter Kasting wrote: > This call shouldn't be necessary, I don't think. Done. https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:307: bool ZoomBubbleView::ShouldSnapFrameWidth() const { On 2017/05/16 00:14:35, Peter Kasting wrote: > You no longer need to provide this -- Elly's recently-landed change should mean > you get this behavior by default. Done. https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:313: // No buttons presses in this dialog should cause the dialog to close, On 2017/05/16 06:29:01, tapted wrote: > "No buttons pressed" or "No button presses" or "A button press.. should not" Done. https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right): https://codereview.chromium.org/2845593002/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:90: bool ShouldSnapFrameWidth() const override; On 2017/05/16 06:29:02, tapted wrote: > since LocationBarBubbleDelegateView::GetDialogButtons() returns NONE I think > with Elly's recent patch ( r471471 ) this override is no longer needed. > > > (although the longer story is that this dialog probably should return > ui::DIALOG_BUTTON_CANCEL to help with a11y and OS integration, but it would need > to do that in a way that didn't cause DialogClientView to add a button row... - > something for me to investigate later) Done.
On 2017/05/16 22:53:07, varkha wrote: > I think it is much simpler to just always show the icon > and I find it hard to reason what is special for the 100% (or default rather) > case that makes the icon unnecessary if we think it is necessary for other > values. I would strongly argue against the icon always showing. I would sooner never show it than show it at 100%. Like the content settings icons, we put an icon there only for non-default values. I zoom in a couple sites because the fonts are too small. It's fine to be reminded of that unusual/active setting. That doesn't mean I want the other 99% of the web to show this icon which I'll never use.
On 2017/05/16 23:00:20, Evan Stade wrote: > On 2017/05/16 22:53:07, varkha wrote: > > I think it is much simpler to just always show the icon > > and I find it hard to reason what is special for the 100% (or default rather) > > case that makes the icon unnecessary if we think it is necessary for other > > values. > > I would strongly argue against the icon always showing. I would sooner never > show it than show it at 100%. Like the content settings icons, we put an icon > there only for non-default values. I zoom in a couple sites because the fonts > are too small. It's fine to be reminded of that unusual/active setting. That > doesn't mean I want the other 99% of the web to show this icon which I'll never > use. And I vote for: 2. make zoom icon auto-hide soon after closing the bubble (and deal with complexity around delayed states). (possibly only auto-hiding if at the default value)
On 2017/05/16 23:03:20, Evan Stade wrote: > On 2017/05/16 23:00:20, Evan Stade wrote: > > On 2017/05/16 22:53:07, varkha wrote: > > > I think it is much simpler to just always show the icon > > > and I find it hard to reason what is special for the 100% (or default > rather) > > > case that makes the icon unnecessary if we think it is necessary for other > > > values. > > > > I would strongly argue against the icon always showing. I would sooner never > > show it than show it at 100%. Like the content settings icons, we put an icon > > there only for non-default values. I zoom in a couple sites because the fonts > > are too small. It's fine to be reminded of that unusual/active setting. That > > doesn't mean I want the other 99% of the web to show this icon which I'll > never > > use. > > And I vote for: > > 2. make zoom icon auto-hide soon after closing the bubble (and deal with > complexity around delayed states). > > (possibly only auto-hiding if at the default value) And one more question (I will wait for opinions though): Should I block this CL on the delayed hiding of the icon?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/05/16 23:37:12, varkha wrote: > On 2017/05/16 23:03:20, Evan Stade wrote: > > On 2017/05/16 23:00:20, Evan Stade wrote: > > > On 2017/05/16 22:53:07, varkha wrote: > > > > I think it is much simpler to just always show the icon > > > > and I find it hard to reason what is special for the 100% (or default > > rather) > > > > case that makes the icon unnecessary if we think it is necessary for other > > > > values. > > > > > > I would strongly argue against the icon always showing. I would sooner never > > > show it than show it at 100%. Like the content settings icons, we put an > icon > > > there only for non-default values. I zoom in a couple sites because the > fonts > > > are too small. It's fine to be reminded of that unusual/active setting. That > > > doesn't mean I want the other 99% of the web to show this icon which I'll > > never > > > use. > > > > And I vote for: > > > > 2. make zoom icon auto-hide soon after closing the bubble (and deal with > > complexity around delayed states). > > > > (possibly only auto-hiding if at the default value) > > And one more question (I will wait for opinions though): > Should I block this CL on the delayed hiding of the icon? The only thing I'd really care about is if this one landed in m60 and the other one didn't (branch is 9 days away). Otherwise it doesn't seem like a big deal either way.
I agree with Evan that we shouldn't be showing the zoom icon all the time. I would hide the zoom icon immediately (not on a delay) if the bubble closes and the zoom value is not the default value. Basically, show the icon if (zoom value not default || bubble open). Yes, this means clicking the icon while the bubble is open at the default zoom level will close the bubble and hide the icon you just clicked. I am fine with that consequence -- it seems better than delaying the hiding of the icon. If you know how to do this and intend to do it imminently, it doesn't matter if it lands in this CL or not. If you're not sure, I would figure it out first, only to minimize the chance of landing this and then having that get stalled, especially past a branch.
Patchset #14 (id:380001) has been deleted
Thanks, does the last PS make sense for views implementation?
Patchset #15 (id:420001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
And PS 15 should also fix it such that Mac works the same (icon disappears at 100% but only after the bubble is closed).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #17 (id:480001) has been deleted
Patchset #15 (id:440001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #17 (id:520001) has been deleted
Patchset #17 (id:540001) has been deleted
varkha@chromium.org changed reviewers: + wjmaclean@chromium.org
+wjmaclean@ for changes in components/zoom/zoom_controller.cc and chrome/browser/ui/zoom/zoom_controller_unittest.cc
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/17 17:28:01, varkha wrote: > +wjmaclean@ for changes in components/zoom/zoom_controller.cc and > chrome/browser/ui/zoom/zoom_controller_unittest.cc lgtm
Description was changed from
==========
Updates Zoom bubble layout and adds +/- buttons
Adds + and - buttons to the Zoom bubble.
Updates layout to only have one row.
BUG=651641
==========
to
==========
Updates Zoom bubble layout and adds +/- buttons
Adds + and - buttons to the Zoom bubble.
Updates layout to only have one row.
With this change the zoom icon (ZoomView) in location bar
can be shown even at 100% zoom as long as the zoom bubble
is also shown (otherwise the bubble loses its anchor when
using + / - buttons to go over 100% zoom without closing
the bubble.
In this scenario a kZoomPlusIcon is used (rather than none).
Whenever the zoom bubble gets closed the location bar gets
updated to update layout and visibility of the zoom icon.
BUG=651641
TEST=no new tests but a few existing tests updated or checked
ZoomControllerTest
ZoomDecorationTest
ZoomControllerBrowserTest
PDFExtensionTest.PdfZoomWithoutBubble
TBR=phajdan.jr@chromium.org (for a trivial change in
chrome/test/base/test_browser_window.h)
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org, tapted@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2845593002/#ps560001 (title: "Fix ZoomController to never show the bubble when zoom level is not changed from default")
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": 560001, "attempt_start_ts": 1495066185795100,
"parent_rev": "8bb22114ceb999bf61364b6bf54c22dc973065a0", "commit_rev":
"c4d839e6e1f0963b2760620cb0ed072b802bf225"}
Message was sent while issue was closed.
Description was changed from
==========
Updates Zoom bubble layout and adds +/- buttons
Adds + and - buttons to the Zoom bubble.
Updates layout to only have one row.
With this change the zoom icon (ZoomView) in location bar
can be shown even at 100% zoom as long as the zoom bubble
is also shown (otherwise the bubble loses its anchor when
using + / - buttons to go over 100% zoom without closing
the bubble.
In this scenario a kZoomPlusIcon is used (rather than none).
Whenever the zoom bubble gets closed the location bar gets
updated to update layout and visibility of the zoom icon.
BUG=651641
TEST=no new tests but a few existing tests updated or checked
ZoomControllerTest
ZoomDecorationTest
ZoomControllerBrowserTest
PDFExtensionTest.PdfZoomWithoutBubble
TBR=phajdan.jr@chromium.org (for a trivial change in
chrome/test/base/test_browser_window.h)
==========
to
==========
Updates Zoom bubble layout and adds +/- buttons
Adds + and - buttons to the Zoom bubble.
Updates layout to only have one row.
With this change the zoom icon (ZoomView) in location bar
can be shown even at 100% zoom as long as the zoom bubble
is also shown (otherwise the bubble loses its anchor when
using + / - buttons to go over 100% zoom without closing
the bubble.
In this scenario a kZoomPlusIcon is used (rather than none).
Whenever the zoom bubble gets closed the location bar gets
updated to update layout and visibility of the zoom icon.
BUG=651641
TEST=no new tests but a few existing tests updated or checked
ZoomControllerTest
ZoomDecorationTest
ZoomControllerBrowserTest
PDFExtensionTest.PdfZoomWithoutBubble
TBR=phajdan.jr@chromium.org (for a trivial change in
chrome/test/base/test_browser_window.h)
Review-Url: https://codereview.chromium.org/2845593002
Cr-Commit-Position: refs/heads/master@{#472611}
Committed:
https://chromium.googlesource.com/chromium/src/+/c4d839e6e1f0963b2760620cb0ed...
==========
Message was sent while issue was closed.
Committed patchset #17 (id:560001) as https://chromium.googlesource.com/chromium/src/+/c4d839e6e1f0963b2760620cb0ed...
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2845593002/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:71: return button; I think you need to std::move(button). This broken a bunch of bots, e.g. https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.chromiumos%2FCh... error: cannot bind 'std::unique_ptr<views::ImageButton>' lvalue to 'std::unique_ptr<views::ImageButton>&&'
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:560001) has been created in https://codereview.chromium.org/2891883002/ by thestig@chromium.org. The reason for reverting is: Broke "ChromiumOS x86-generic Compile" bot. The bot uses gcc instead of clang, and needs more std::move()s..
Message was sent while issue was closed.
Description was changed from
==========
Updates Zoom bubble layout and adds +/- buttons
Adds + and - buttons to the Zoom bubble.
Updates layout to only have one row.
With this change the zoom icon (ZoomView) in location bar
can be shown even at 100% zoom as long as the zoom bubble
is also shown (otherwise the bubble loses its anchor when
using + / - buttons to go over 100% zoom without closing
the bubble.
In this scenario a kZoomPlusIcon is used (rather than none).
Whenever the zoom bubble gets closed the location bar gets
updated to update layout and visibility of the zoom icon.
BUG=651641
TEST=no new tests but a few existing tests updated or checked
ZoomControllerTest
ZoomDecorationTest
ZoomControllerBrowserTest
PDFExtensionTest.PdfZoomWithoutBubble
TBR=phajdan.jr@chromium.org (for a trivial change in
chrome/test/base/test_browser_window.h)
Review-Url: https://codereview.chromium.org/2845593002
Cr-Commit-Position: refs/heads/master@{#472611}
Committed:
https://chromium.googlesource.com/chromium/src/+/c4d839e6e1f0963b2760620cb0ed...
==========
to
==========
Updates Zoom bubble layout and adds +/- buttons
Adds + and - buttons to the Zoom bubble.
Updates layout to only have one row.
With this change the zoom icon (ZoomView) in location bar
can be shown even at 100% zoom as long as the zoom bubble
is also shown (otherwise the bubble loses its anchor when
using + / - buttons to go over 100% zoom without closing
the bubble.
In this scenario a kZoomPlusIcon is used (rather than none).
Whenever the zoom bubble gets closed the location bar gets
updated to update layout and visibility of the zoom icon.
BUG=651641
TEST=no new tests but a few existing tests updated or checked
ZoomControllerTest
ZoomDecorationTest
ZoomControllerBrowserTest
PDFExtensionTest.PdfZoomWithoutBubble
TBR=phajdan.jr@chromium.org (for a trivial change in
chrome/test/base/test_browser_window.h)
Review-Url: https://codereview.chromium.org/2845593002
Cr-Commit-Position: refs/heads/master@{#472611}
Committed:
https://chromium.googlesource.com/chromium/src/+/c4d839e6e1f0963b2760620cb0ed...
==========
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2845593002/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2845593002/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:71: return button; On 2017/05/18 01:59:28, Lei Zhang wrote: > I think you need to std::move(button). > > This broken a bunch of bots, e.g. > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.chromiumos%2FCh... > > error: cannot bind 'std::unique_ptr<views::ImageButton>' lvalue to > 'std::unique_ptr<views::ImageButton>&&' Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, wjmaclean@chromium.org, pkasting@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2845593002/#ps600001 (title: "Fix compilation error on gcc")
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": 600001, "attempt_start_ts": 1495132956162510,
"parent_rev": "89c24d4a46c1f0e7ad03891a659e6d23dc776c36", "commit_rev":
"f6f47b5a77854b023c3ccd6c57d1584051f2280a"}
Message was sent while issue was closed.
Description was changed from
==========
Updates Zoom bubble layout and adds +/- buttons
Adds + and - buttons to the Zoom bubble.
Updates layout to only have one row.
With this change the zoom icon (ZoomView) in location bar
can be shown even at 100% zoom as long as the zoom bubble
is also shown (otherwise the bubble loses its anchor when
using + / - buttons to go over 100% zoom without closing
the bubble.
In this scenario a kZoomPlusIcon is used (rather than none).
Whenever the zoom bubble gets closed the location bar gets
updated to update layout and visibility of the zoom icon.
BUG=651641
TEST=no new tests but a few existing tests updated or checked
ZoomControllerTest
ZoomDecorationTest
ZoomControllerBrowserTest
PDFExtensionTest.PdfZoomWithoutBubble
TBR=phajdan.jr@chromium.org (for a trivial change in
chrome/test/base/test_browser_window.h)
Review-Url: https://codereview.chromium.org/2845593002
Cr-Commit-Position: refs/heads/master@{#472611}
Committed:
https://chromium.googlesource.com/chromium/src/+/c4d839e6e1f0963b2760620cb0ed...
==========
to
==========
Updates Zoom bubble layout and adds +/- buttons
Adds + and - buttons to the Zoom bubble.
Updates layout to only have one row.
With this change the zoom icon (ZoomView) in location bar
can be shown even at 100% zoom as long as the zoom bubble
is also shown (otherwise the bubble loses its anchor when
using + / - buttons to go over 100% zoom without closing
the bubble.
In this scenario a kZoomPlusIcon is used (rather than none).
Whenever the zoom bubble gets closed the location bar gets
updated to update layout and visibility of the zoom icon.
BUG=651641
TEST=no new tests but a few existing tests updated or checked
ZoomControllerTest
ZoomDecorationTest
ZoomControllerBrowserTest
PDFExtensionTest.PdfZoomWithoutBubble
TBR=phajdan.jr@chromium.org (for a trivial change in
chrome/test/base/test_browser_window.h)
Review-Url: https://codereview.chromium.org/2845593002
Cr-Original-Commit-Position: refs/heads/master@{#472611}
Committed:
https://chromium.googlesource.com/chromium/src/+/c4d839e6e1f0963b2760620cb0ed...
Review-Url: https://codereview.chromium.org/2845593002
Cr-Commit-Position: refs/heads/master@{#472891}
Committed:
https://chromium.googlesource.com/chromium/src/+/f6f47b5a77854b023c3ccd6c57d1...
==========
Message was sent while issue was closed.
Committed patchset #19 (id:600001) as https://chromium.googlesource.com/chromium/src/+/f6f47b5a77854b023c3ccd6c57d1... |
/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-07a-layout.png){kind=link}