Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2853143003:
MacViews: Allows the toolkit-views Device Chooser bubble to be used (Closed)
DescriptionMacViews: Allows the toolkit-views Device Chooser bubble to be used
BUG=610430
TBR=raymes@chromium.org
Review-Url: https://codereview.chromium.org/2853143003
Cr-Commit-Position: refs/heads/master@{#469822}
Committed: https://chromium.googlesource.com/chromium/src/+/ec11daa13a86c2535a123ea45e91e5b82d4d7d1e
Patch Set 1 : MacViews: Allows the toolkit-views Device Chooser bubble to be used #
Total comments: 20
Patch Set 2 : MacViews: Allows the toolkit-views Device Chooser bubble to be used (separated Mac specifics) #
Total comments: 10
Patch Set 3 : MacViews: Allows the toolkit-views Device Chooser bubble to be used (files rename) #Patch Set 4 : MacViews: Allows the toolkit-views Device Chooser bubble to be used (swizzling tests) #
Total comments: 2
Patch Set 5 : MacViews: Allows the toolkit-views Device Chooser bubble to be used (swizzling tests) #
Total comments: 8
Patch Set 6 : MacViews: Allows the toolkit-views Device Chooser bubble to be used (nits) #
Total comments: 8
Patch Set 7 : MacViews: Allows the toolkit-views Device Chooser bubble to be used (nits) #Messages
Total messages: 69 (54 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 varkha@chromium.org
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Patchset #1 (id:1) 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...
varkha@chromium.org changed reviewers: + tapted@chromium.org
tapted@, can you please take a first look? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm (right): https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm:23: NSPoint AnchorPointForWindow(NSWindow* parent) { Self review: simplify with inlines. https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm:40: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) { Self review: simplify by reversing the condition and inlining const temp vars.
darn this one is trickier than I thought :/. and components/bubble seems kinda half-baked and incomplete (but untouched for over a year). That might be where we need to tackle some of this in the long term.. There are steps in https://codereview.chromium.org/1935993004 we can follow to avoid the #ifdefs and layering stuff. Since you're short on time, feel free to pass the baton on for this one :) https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_cocoa.mm (right): https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_cocoa.mm:13: #include "base/memory/ptr_util.h" nit: remove? https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm (right): https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm:31: } Do we need an else? Maybe we can move this method to its own header (bubble_anchor_helper_views.h perhaps) There's logic in chooser_bubble_ui_cocoa.mm that falls back to centering when there is no location bar - it should be able to call the helper too. (There's logic in permission_bubble_controller.mm too... (except it falls back to "left of screen")) We can probably do a similar thing for things anchored on the right of the location bar, but we can worry about that later. https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm:38: #if !BUILDFLAG(MAC_VIEWS_BROWSER) Can we exclude the entire file under the mac_views_browser build in .gn? https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.cc:21: #include "chrome/browser/ui/views/frame/browser_view.h" this header shouldn't be compiled on mac :/ https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.cc:23: #include "chrome/browser/ui/views/location_bar/location_bar_view.h" these either https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.cc:34: #import "chrome/browser/ui/cocoa/bubble_anchor_helper_views.h" I don't think there's a precedent for #including stuff in chrome/browser/ui/cocoa in folders under chrome/browser/ui/views -- it's probably some kind of layering violation https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.cc:245: #if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER) I think we need something similar to permission_prompt_impl_views_mac.mm to solve this one https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.h (right): https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.h:12: #include "ui/gfx/geometry/point.h" nit: forward declare
Patchset #2 (id:40001) 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...
Thanks for the pointers! I've refactored this CL similar to https://codereview.chromium.org/1935993004. PTAL. https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_cocoa.mm (right): https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_cocoa.mm:13: #include "base/memory/ptr_util.h" On 2017/05/03 01:10:53, tapted wrote: > nit: remove? Done. https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm (right): https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm:23: NSPoint AnchorPointForWindow(NSWindow* parent) { On 2017/05/02 12:25:48, varkha wrote: > Self review: simplify with inlines. Acknowledged. https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm:31: } On 2017/05/03 01:10:53, tapted wrote: > Do we need an else? > > Maybe we can move this method to its own header (bubble_anchor_helper_views.h > perhaps) > > There's logic in chooser_bubble_ui_cocoa.mm that falls back to centering when > there is no location bar - it should be able to call the helper too. > > (There's logic in permission_bubble_controller.mm too... (except it falls back > to "left of screen")) > > We can probably do a similar thing for things anchored on the right of the > location bar, but we can worry about that later. Acknowledged. I'll look into this. https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm:38: #if !BUILDFLAG(MAC_VIEWS_BROWSER) On 2017/05/03 01:10:53, tapted wrote: > Can we exclude the entire file under the mac_views_browser build in .gn? Please see if the refactoring I've done makes sense. It's similar to the CL you've linked and avoids #ifdefs. https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm:40: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) { On 2017/05/02 12:25:48, varkha wrote: > Self review: simplify by reversing the condition and inlining const temp vars. Done. https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.cc:21: #include "chrome/browser/ui/views/frame/browser_view.h" On 2017/05/03 01:10:53, tapted wrote: > this header shouldn't be compiled on mac :/ Done (refactored out). https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.cc:23: #include "chrome/browser/ui/views/location_bar/location_bar_view.h" On 2017/05/03 01:10:53, tapted wrote: > these either Done (refactored out). https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.cc:34: #import "chrome/browser/ui/cocoa/bubble_anchor_helper_views.h" On 2017/05/03 01:10:53, tapted wrote: > I don't think there's a precedent for #including stuff in > chrome/browser/ui/cocoa in folders under chrome/browser/ui/views -- it's > probably some kind of layering violation Done (refactored out). https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.cc:245: #if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER) On 2017/05/03 01:10:53, tapted wrote: > I think we need something similar to permission_prompt_impl_views_mac.mm to > solve this one Done. https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.h (right): https://codereview.chromium.org/2853143003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.h:12: #include "ui/gfx/geometry/point.h" On 2017/05/03 01:10:53, tapted wrote: > nit: forward declare Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:1632: "views/permission_bubble/chooser_bubble_ui_view_views.cc", This won't be compiled on ChromeOS (also perhaps chooser_bubble_ui_impl_views.cc ) https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:1848: "views/permission_bubble/permission_prompt_impl_views.cc", chooser_bubble_ui_view_views.cc -> chooser_bubble_ui_impl_views.cc should go here, above this line https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:2929: "cocoa/permission_bubble/chooser_bubble_ui_view_cocoa_views.mm", perhaps chooser_bubble_ui_impl_views_mac.mm (in keeping with permission_prompt_impl_views_mac.mm) https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_view_cocoa_views.mm (right): https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_view_cocoa_views.mm:60: gfx::Point anchor_point = gfx::ScreenPointFromNSPoint(origin); yah it I really think this needs to go into the helper file/header, if it's the same or close enough -[ChooserBubbleUiController getExpectedAnchorPoint] should call it too, in this CL, but we can worry about moving other permisisons/site_settings bubbles over in a later CL. https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.cc:20: #include "chrome/browser/ui/views/frame/browser_view.h" we should just be able to delete these 3-4 #includes now
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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/2853143003/diff/60001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:1632: "views/permission_bubble/chooser_bubble_ui_view_views.cc", On 2017/05/03 05:23:09, tapted wrote: > This won't be compiled on ChromeOS > > (also perhaps chooser_bubble_ui_impl_views.cc ) Done. https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:1848: "views/permission_bubble/permission_prompt_impl_views.cc", On 2017/05/03 05:23:09, tapted wrote: > chooser_bubble_ui_view_views.cc -> chooser_bubble_ui_impl_views.cc should go > here, above this line Done. https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:2929: "cocoa/permission_bubble/chooser_bubble_ui_view_cocoa_views.mm", On 2017/05/03 05:23:09, tapted wrote: > perhaps chooser_bubble_ui_impl_views_mac.mm (in keeping with > permission_prompt_impl_views_mac.mm) Done (keeping the style and shortening). https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_view_cocoa_views.mm (right): https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_view_cocoa_views.mm:60: gfx::Point anchor_point = gfx::ScreenPointFromNSPoint(origin); On 2017/05/03 05:23:09, tapted wrote: > yah it I really think this needs to go into the helper file/header, if it's the > same or close enough > -[ChooserBubbleUiController getExpectedAnchorPoint] should call it too, in this > CL, but we can worry about moving other permisisons/site_settings bubbles over > in a later CL. Done. https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/2853143003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_view.cc:20: #include "chrome/browser/ui/views/frame/browser_view.h" On 2017/05/03 05:23:09, tapted wrote: > we should just be able to delete these 3-4 #includes now Done. https://codereview.chromium.org/2853143003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/permission_bubble/permission_prompt_impl_views_mac.mm (right): https://codereview.chromium.org/2853143003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/permission_bubble/permission_prompt_impl_views_mac.mm:29: browser_, HasVisibleLocationBarForBrowser(browser_))); I could be not making this change, see which looks better to you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm - just a few nits https://codereview.chromium.org/2853143003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/permission_bubble/permission_prompt_impl_views_mac.mm (right): https://codereview.chromium.org/2853143003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/permission_bubble/permission_prompt_impl_views_mac.mm:29: browser_, HasVisibleLocationBarForBrowser(browser_))); On 2017/05/03 08:47:03, varkha wrote: > I could be not making this change, see which looks better to you. this is good - it's probably showing up somewhere odd for some window types.. https://codereview.chromium.org/2853143003/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bubble_anchor_helper.h (right): https://codereview.chromium.org/2853143003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper.h:8: #import <Foundation/NSGeometry.h> Foundation/Foundation.h (there's a policy to always include the "Framework" header) https://codereview.chromium.org/2853143003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper.h:15: // Returns a point at the bottom left of the location bar when location bar is Returns a point in screen coordinates... https://codereview.chromium.org/2853143003/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bubble_anchor_helper.mm (right): https://codereview.chromium.org/2853143003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper.mm:23: const NSInteger kFullscreenLeftOffset = 40; nit: constexpr Also, comment here like // Offset from the screen edge to show dialogs when there is no location bar. Don't center, since that could obscure a fullscreen bubble. https://codereview.chromium.org/2853143003/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/permission_bubble/permission_bubble_controller_unittest.mm (right): https://codereview.chromium.org/2853143003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/permission_bubble/permission_bubble_controller_unittest.mm:395: // Constant offset from the left to use for fullscreen permission bubbles. See bubble_anchor_helper.mm?
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Patchset #6 (id:140001) 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...
varkha@chromium.org changed reviewers: + raymes@chromium.org
+raymes@ for OWNERS in chrome/browser/ui/views/permission_bubble/ Thanks! https://codereview.chromium.org/2853143003/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bubble_anchor_helper.h (right): https://codereview.chromium.org/2853143003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper.h:8: #import <Foundation/NSGeometry.h> On 2017/05/03 09:09:11, tapted wrote: > Foundation/Foundation.h > > (there's a policy to always include the "Framework" header) Done. https://codereview.chromium.org/2853143003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper.h:15: // Returns a point at the bottom left of the location bar when location bar is On 2017/05/03 09:09:11, tapted wrote: > Returns a point in screen coordinates... Done. https://codereview.chromium.org/2853143003/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bubble_anchor_helper.mm (right): https://codereview.chromium.org/2853143003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper.mm:23: const NSInteger kFullscreenLeftOffset = 40; On 2017/05/03 09:09:11, tapted wrote: > nit: constexpr > > Also, comment here like > > // Offset from the screen edge to show dialogs when there is no location bar. > Don't center, since that could obscure a fullscreen bubble. > Done. https://codereview.chromium.org/2853143003/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/permission_bubble/permission_bubble_controller_unittest.mm (right): https://codereview.chromium.org/2853143003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/permission_bubble/permission_bubble_controller_unittest.mm:395: // Constant offset from the left to use for fullscreen permission bubbles. On 2017/05/03 09:09:11, tapted wrote: > See bubble_anchor_helper.mm? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Patchset #6 (id:160001) has been deleted
Patchset #6 (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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
raymes@chromium.org changed reviewers: + juncai@chromium.org
I defer to juncai who wrote the code.
Can you also post some screenshots of the choosers from Mac and ChromeOS (can build the ChromeOS on Linux machine) following the steps at: https://bugs.chromium.org/p/chromium/issues/detail?id=610430#c29 https://codereview.chromium.org/2853143003/diff/200001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm (right): https://codereview.chromium.org/2853143003/diff/200001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm:16: #include "chrome/browser/ui/views/permission_bubble/chooser_bubble_ui.h" maybe move this include to the top of the include list and add an extra line after it since this .mm file implements it. https://codereview.chromium.org/2853143003/diff/200001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm:36: // Set |parent_window| because some valid anchors can become hidden. nit: s/|parent_window|/|parent_window_| https://codereview.chromium.org/2853143003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_views.cc (right): https://codereview.chromium.org/2853143003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_views.cc:16: #include "chrome/browser/ui/views/permission_bubble/chooser_bubble_ui.h" maybe move this include to the top of the include list and add an extra line after it since this .cc file implements it. https://codereview.chromium.org/2853143003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_views.cc:29: // Set |parent_window| because some valid anchors can become hidden. nit: s/|parent_window|/|parent_window_|
LGTM with the above comments addressed.
Patchset #7 (id:220001) has been deleted
Patchset #7 (id:240001) 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...
On 2017/05/05 17:05:26, juncai wrote: > LGTM with the above comments addressed. Thanks, raymes@, can you rubber stamp it please for OWNERS? Thanks!
https://codereview.chromium.org/2853143003/diff/200001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm (right): https://codereview.chromium.org/2853143003/diff/200001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm:16: #include "chrome/browser/ui/views/permission_bubble/chooser_bubble_ui.h" On 2017/05/04 18:27:15, juncai wrote: > maybe move this include to the top of the include list and add an extra line > after it since this .mm file implements it. Done. https://codereview.chromium.org/2853143003/diff/200001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/permission_bubble/chooser_bubble_ui_views_mac.mm:36: // Set |parent_window| because some valid anchors can become hidden. On 2017/05/04 18:27:15, juncai wrote: > nit: s/|parent_window|/|parent_window_| Done. https://codereview.chromium.org/2853143003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_views.cc (right): https://codereview.chromium.org/2853143003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_views.cc:16: #include "chrome/browser/ui/views/permission_bubble/chooser_bubble_ui.h" On 2017/05/04 18:27:15, juncai wrote: > maybe move this include to the top of the include list and add an extra line > after it since this .cc file implements it. Done. https://codereview.chromium.org/2853143003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/permission_bubble/chooser_bubble_ui_views.cc:29: // Set |parent_window| because some valid anchors can become hidden. On 2017/05/04 18:27:15, juncai wrote: > nit: s/|parent_window|/|parent_window_| Done.
Description was changed from ========== MacViews: Allows the toolkit-views Device Chooser bubble to be used BUG=610430 ========== to ========== MacViews: Allows the toolkit-views Device Chooser bubble to be used BUG=610430 TBR=raymes@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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.
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, juncai@chromium.org Link to the patchset: https://codereview.chromium.org/2853143003/#ps260001 (title: "MacViews: Allows the toolkit-views Device Chooser bubble to be used (nits)")
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": 260001, "attempt_start_ts": 1494029023570970,
"parent_rev": "11cfe2438623a20f5ba74be863b6719b7525be18", "commit_rev":
"ec11daa13a86c2535a123ea45e91e5b82d4d7d1e"}
Message was sent while issue was closed.
Description was changed from ========== MacViews: Allows the toolkit-views Device Chooser bubble to be used BUG=610430 TBR=raymes@chromium.org ========== to ========== MacViews: Allows the toolkit-views Device Chooser bubble to be used BUG=610430 TBR=raymes@chromium.org Review-Url: https://codereview.chromium.org/2853143003 Cr-Commit-Position: refs/heads/master@{#469822} Committed: https://chromium.googlesource.com/chromium/src/+/ec11daa13a86c2535a123ea45e91... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:260001) as https://chromium.googlesource.com/chromium/src/+/ec11daa13a86c2535a123ea45e91... |
