Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834493007:
MacViews: Allows the toolkit-views Zoom Dialog to be used (Closed)
DescriptionMacViews: Allows the toolkit-views Zoom Dialog to be used
Updates Mac tests to test both views and Cocoa versions of the Zoom Bubble.
BUG=662128, 177940
TEST=browser_tests --gtest_filter=*ZoomDecorationTest*
unit_tests --gtest_filter=*ZoomDecorationTest*
Review-Url: https://codereview.chromium.org/2834493007
Cr-Commit-Position: refs/heads/master@{#467508}
Committed: https://chromium.googlesource.com/chromium/src/+/948625158c31a467a776ca18e58d470a1c62565d
Patch Set 1 : MacViews: Allows the toolkit-views Zoom Dialog to be used #Patch Set 2 : MacViews: Allows the toolkit-views Zoom Dialog to be used (tests) #
Total comments: 21
Patch Set 3 : MacViews: Allows the toolkit-views Zoom Dialog to be used (comments) #
Total comments: 6
Patch Set 4 : MacViews: Allows the toolkit-views Zoom Dialog to be used (keep anchoring) #Messages
Total messages: 54 (44 generated)
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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) 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...
Patchset #3 (id:100001) 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...
Description was changed from
==========
MacViews: Allows the toolkit-views Zoom Dialog to be used
BUG=662128
==========
to
==========
MacViews: Allows the toolkit-views Zoom Dialog to be used
Updates Mac tests to test both views and Cocoa versions of the Zoom Bubble.
BUG=662128
TEST=browser_tests --gtest_filter=*ZoomDecorationTest*
unit_tests --gtest_filter=*ZoomDecorationTest*
==========
varkha@chromium.org changed reviewers: + msw@chromium.org
msw@, can you please take a first look at the views Zoom Bubble on MacOS (tapted@ is OOO till the end of the week)? This does not yet change the look of it (we will be adding +/- buttons to it), just making it show on Mac (behind a flag). Thanks!
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm (right): https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:187: BOOL auto_close = !ui::MaterialDesignController::IsSecondaryUiMaterial(); Note: prefer this to inlining for being self-documenting. self review - const.
Trent or someone else should review the Cocoa changes too. https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:136: // Shows a views zoom bubble at the | anchor_point|. This occurs when the zoom nit: no space inside vertical bars https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:137: // icon is clicked or or via a key equivalent. nit: "or or", consider "or when a shortcut key is pressed." (it's ok to leave "or via a key shortcut" to match above) https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:146: void RefreshZoomBubbleViews(); aside: not for this CL, but it would be nice to minimize and standardize the APIs across various dialogs. Ideally we'd just have Show*() and Get*() plus per-dialog methods like ZoomBubbleView::Refresh (& GetWidget + Widget::Close). https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm (right): https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:94: BrowserWindowController* bwc = nit: avoid acronym identifiers. https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:96: NSPoint anchor = [bwc bookmarkBubblePoint]; Why show this at the bookmark bubble point? Don't we still show the zoom icon? Can we re-use the |NSPoint anchor| and GetBubblePointInFrame below? https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:187: BOOL auto_close = !ui::MaterialDesignController::IsSecondaryUiMaterial(); Why would we change the auto-close behavior depending on MD/views? https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm (right): https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm:28: // An enum to parameterize the tests so that the tests can be run with and nit: simplify this to a bool param or extract the duplicated enum and string function from chrome/browser/download/download_danger_prompt_browsertest.cc to a separate file. Duplicating this pattern across all similar test files would add lots of temporary overhead/cleanup (and potential for extra maintenance/bugs). Maybe make a SecondaryUiMdInProcessBrowserTest base class? https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/zoom_decoration_unittest.mm (right): https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/zoom_decoration_unittest.mm:15: // An enum to parameterize the tests so that the tests can be run with and Yikes, ditto. Maybe use a test utils file and/or test subclass(es). https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:219: label_->SetFontList(GetTitleFontList()); aside: when updating the bubble appearance in a follow up CL to add the +/- buttons, maybe make this a proper title? https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right): https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:72: // BubbleView anchoring to |anchor_view| or |anchor_point| as available. nit: "The bubble will anchor to |anchor_view| or |anchor_point| as available." (or a more complete description of ctor/args/behavior)
Description was changed from
==========
MacViews: Allows the toolkit-views Zoom Dialog to be used
Updates Mac tests to test both views and Cocoa versions of the Zoom Bubble.
BUG=662128
TEST=browser_tests --gtest_filter=*ZoomDecorationTest*
unit_tests --gtest_filter=*ZoomDecorationTest*
==========
to
==========
MacViews: Allows the toolkit-views Zoom Dialog to be used
Updates Mac tests to test both views and Cocoa versions of the Zoom Bubble.
BUG=662128, 177940
TEST=browser_tests --gtest_filter=*ZoomDecorationTest*
unit_tests --gtest_filter=*ZoomDecorationTest*
==========
varkha@chromium.org changed reviewers: + ellyjones@chromium.org
> Trent or someone else should review the Cocoa changes too. +ellyjones@, can you please take a look at chrome/browser/ui/cocoa/ ? Thanks! https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:136: // Shows a views zoom bubble at the | anchor_point|. This occurs when the zoom On 2017/04/24 18:53:38, msw wrote: > nit: no space inside vertical bars Done. https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:137: // icon is clicked or or via a key equivalent. On 2017/04/24 18:53:38, msw wrote: > nit: "or or", consider "or when a shortcut key is pressed." > (it's ok to leave "or via a key shortcut" to match above) Done. https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:146: void RefreshZoomBubbleViews(); On 2017/04/24 18:53:38, msw wrote: > aside: not for this CL, but it would be nice to minimize and standardize the > APIs across various dialogs. Ideally we'd just have Show*() and Get*() plus > per-dialog methods like ZoomBubbleView::Refresh (& GetWidget + Widget::Close). Acknowledged. We would need to move the declarations of per-dialog methods to an interface in a file outside of chrome/browser/ui/views as that cannot be used (see chrome/browser/ui/cocoa/DEPS). Yes it would be better to do that for all such bridge methods at once. https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm (right): https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:94: BrowserWindowController* bwc = On 2017/04/24 18:53:38, msw wrote: > nit: avoid acronym identifiers. Done. https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:96: NSPoint anchor = [bwc bookmarkBubblePoint]; On 2017/04/24 18:53:38, msw wrote: > Why show this at the bookmark bubble point? Don't we still show the zoom icon? > Can we re-use the |NSPoint anchor| and GetBubblePointInFrame below? My guess is that this is called bookmark bubble point because the bookmark bubble was the first one converted. The anchoring alignment is now same for all the bubbles (the right edge of the location bar) so the name probably needs to be updated. I will discuss with tapted@ what name would be good for this, maybe locationBarBubblePoint or something like that. GetBubblePointInFrame is a bubble-specific override and should go away after MD is default - we should not need per-dialog override. I'd rather not prolong its use here since we no longer anchor to a specific point in the specific icon. https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:187: BOOL auto_close = !ui::MaterialDesignController::IsSecondaryUiMaterial(); On 2017/04/24 18:53:38, msw wrote: > Why would we change the auto-close behavior depending on MD/views? I don't think this should be any different from other platforms when the views implementation becomes default. Other platforms don't auto-close the bubble when it is opened by a click. See also http://crbug/177940 . I've added a comment now, but it should probably go away together with that variable and we should just do ShowBubble(true /*user_action */) in the end. https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm (right): https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm:28: // An enum to parameterize the tests so that the tests can be run with and On 2017/04/24 18:53:38, msw wrote: > nit: simplify this to a bool param or extract the duplicated enum and string > function from chrome/browser/download/download_danger_prompt_browsertest.cc to a > separate file. Duplicating this pattern across all similar test files would add > lots of temporary overhead/cleanup (and potential for extra maintenance/bugs). > Maybe make a SecondaryUiMdInProcessBrowserTest base class? Done. Not sure about refactoring this, this is used across the test hierarchy so it may get tricky. Simplifying it to bool should be good. We can also look at other places where "SecondaryUiMdStatusToString" is found and trim it. https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/zoom_decoration_unittest.mm (right): https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/zoom_decoration_unittest.mm:15: // An enum to parameterize the tests so that the tests can be run with and On 2017/04/24 18:53:39, msw wrote: > Yikes, ditto. Maybe use a test utils file and/or test subclass(es). Done. https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:219: label_->SetFontList(GetTitleFontList()); On 2017/04/24 18:53:39, msw wrote: > aside: when updating the bubble appearance in a follow up CL to add the +/- > buttons, maybe make this a proper title? Not sure. The new mocks still show this not above the rest of the elements so maybe just a label. https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right): https://codereview.chromium.org/2834493007/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:72: // BubbleView anchoring to |anchor_view| or |anchor_point| as available. On 2017/04/24 18:53:39, msw wrote: > nit: "The bubble will anchor to |anchor_view| or |anchor_point| as available." > (or a more complete description of ctor/args/behavior) Ouch, looks like some errant keystroke has erased the beginning of the sentence. Thanks for catching this. 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.
Patchset #3 (id:140001) 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: This issue passed the CQ dry run.
lgtm with two questions https://codereview.chromium.org/2834493007/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm (right): https://codereview.chromium.org/2834493007/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm:184: INSTANTIATE_TEST_CASE_P(, ZoomDecorationTest, testing::Bool()); Ooh, this is a very clever technique! I like it. https://codereview.chromium.org/2834493007/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2834493007/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:39: #include "chrome/browser/ui/views/frame/browser_view.h" hm, is it not safe to just always include this, even if it's unused when OS_MACOSX && !MAC_VIEWS_BROWSER? https://codereview.chromium.org/2834493007/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:114: views::BubbleDialogDelegateView::CreateBubble(zoom_bubble_); if we don't have the zoom view on the Cocoa location bar observe the zoom bubble, how does it stay up to date?
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...
Thanks, good eye! https://codereview.chromium.org/2834493007/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm (right): https://codereview.chromium.org/2834493007/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm:184: INSTANTIATE_TEST_CASE_P(, ZoomDecorationTest, testing::Bool()); On 2017/04/26 13:11:13, Elly Fong-Jones wrote: > Ooh, this is a very clever technique! I like it. Acknowledged. https://codereview.chromium.org/2834493007/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/2834493007/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:39: #include "chrome/browser/ui/views/frame/browser_view.h" On 2017/04/26 13:11:13, Elly Fong-Jones wrote: > hm, is it not safe to just always include this, even if it's unused when > OS_MACOSX && !MAC_VIEWS_BROWSER? I like this. Done. https://codereview.chromium.org/2834493007/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:114: views::BubbleDialogDelegateView::CreateBubble(zoom_bubble_); On 2017/04/26 13:11:13, Elly Fong-Jones wrote: > if we don't have the zoom view on the Cocoa location bar observe the zoom > bubble, how does it stay up to date? Yes, thanks for noticing this! On Mac I had to but forgot to use KeepBubbleAnchored(bubble_view). Fixed.
FYI - https://codereview.chromium.org/2845593002 has a draft for adding +/- buttons and a single-row layout.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ellyjones@chromium.org Link to the patchset: https://codereview.chromium.org/2834493007/#ps180001 (title: "MacViews: Allows the toolkit-views Zoom Dialog to be used (keep anchoring)")
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": 180001, "attempt_start_ts": 1493249072282960,
"parent_rev": "0c5dce88667d9ceeed1d90daa8485aa6c6009176", "commit_rev":
"948625158c31a467a776ca18e58d470a1c62565d"}
Message was sent while issue was closed.
Description was changed from
==========
MacViews: Allows the toolkit-views Zoom Dialog to be used
Updates Mac tests to test both views and Cocoa versions of the Zoom Bubble.
BUG=662128, 177940
TEST=browser_tests --gtest_filter=*ZoomDecorationTest*
unit_tests --gtest_filter=*ZoomDecorationTest*
==========
to
==========
MacViews: Allows the toolkit-views Zoom Dialog to be used
Updates Mac tests to test both views and Cocoa versions of the Zoom Bubble.
BUG=662128, 177940
TEST=browser_tests --gtest_filter=*ZoomDecorationTest*
unit_tests --gtest_filter=*ZoomDecorationTest*
Review-Url: https://codereview.chromium.org/2834493007
Cr-Commit-Position: refs/heads/master@{#467508}
Committed:
https://chromium.googlesource.com/chromium/src/+/948625158c31a467a776ca18e58d...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001) as https://chromium.googlesource.com/chromium/src/+/948625158c31a467a776ca18e58d... |
