|
|
Description[Mac] Hover/Active Omnibox Icon States for Secondary UI MD
When a secondary UI dialog is opened, the omnibox icon it's anchored to
need to set its state as active. When the dialog is dismissed, the icon is
set to inactive.
BubbleAnchorHelper provides functions to retrieve the omnibox icons
and sets the active state.
BUG=715863
Review-Url: https://codereview.chromium.org/2882533003
Cr-Commit-Position: refs/heads/master@{#473963}
Committed: https://chromium.googlesource.com/chromium/src/+/358c00f72ae3bcf3911da69b48403116dc267712
Patch Set 1 : . #Patch Set 2 : Added comments #
Total comments: 15
Patch Set 3 : Addressed tapted's comments #Patch Set 4 : Nit #
Total comments: 2
Patch Set 5 : Fixes for tapted #
Total comments: 4
Patch Set 6 : Addressed tapted's comments #
Total comments: 3
Patch Set 7 : Move LocationBarDecoration to cocoa #
Total comments: 4
Patch Set 8 : Rebased and applied tapted's changes #Messages
Total messages: 80 (58 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by spqchan@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 spqchan@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_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 spqchan@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 #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Description was changed from ========== [Mac] Hover/Active Omnibox Icon States for Secondary MD UI BUG=715863 ========== to ========== [Mac] Hover/Active Omnibox Icon States for Secondary UI MD When a secondary UI dialog is opened, the omnibox icon it's anchored to need to set its state as active. When the dialog is dismissed, the icon is set to inactive. BUG=715863 ==========
spqchan@chromium.org changed reviewers: + tapted@chromium.org
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #2 (id:80001) 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...
Hey Trent, PTAL. Let me know if you have any questions!
Description was changed from ========== [Mac] Hover/Active Omnibox Icon States for Secondary UI MD When a secondary UI dialog is opened, the omnibox icon it's anchored to need to set its state as active. When the dialog is dismissed, the icon is set to inactive. BUG=715863 ========== to ========== [Mac] Hover/Active Omnibox Icon States for Secondary UI MD When a secondary UI dialog is opened, the omnibox icon it's anchored to need to set its state as active. When the dialog is dismissed, the icon is set to inactive. BubbleAnchorHelper provides functions to retrieve the omnibox icons and sets the active state. BUG=715863 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm (right): https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm:25: explicit BubbleAnchorHelper(views::BubbleDialogDelegateView* bubble, nit: explicit not required https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm:59: LocationBarViewMac* locationBarBridge = nit: locationBarBridge -> location_bar (camel case only for new declarations between @foo/@end) https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm:60: [[[bubble->parent_window() window] windowController] locationBarBridge]; I don't think this will compile in the 10.11 or 10.12 SDK. [NSWindow windowController] used to be declared to return `id`, but now it's NSWindowController*, which won't have locationBarBridge declared on it. I think we need a helper method like LocationBarForBubbleParent(..) which uses +[BrowserWindowController browserWindowControllerForWindow] (and also checks if that returned nil) https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm:144: if (decoration_) This raises a new lifetime issue that didn't previously exist - BubbleAnchorHelper::OnWidgetDestroying needs a guarantee that |decoration_| hasn't been deleted. I *think* WidgetOwnerNSWindowAdapter provides this guarantee, but we should put an ownership comment up around line 50. E.g. // Weak. Lifetime is tied to the controller of bubble's parent window, which owns a LocationBarViewMac that directly owns |decoration_|. And here, a comment like, // NativeWidgetMac guarantees that a child's OnWidgetDestroying() is invoked before the parent NSWindow has closed. https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right): https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:305: model->set_decoration(this); this doesn't look right - can we just pass |this| through to ContentSettingBubbleViewsBridge::Show(..) below, and avoid changing the model? https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:67: if (bubble) is there a known case where this is null? (comment? or DCHECK)
The CQ bit was checked by spqchan@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 spqchan@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm (right): https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm:25: explicit BubbleAnchorHelper(views::BubbleDialogDelegateView* bubble, On 2017/05/16 07:39:37, tapted wrote: > nit: explicit not required Done. https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm:59: LocationBarViewMac* locationBarBridge = On 2017/05/16 07:39:37, tapted wrote: > nit: locationBarBridge -> location_bar (camel case only for new declarations > between @foo/@end) Done. https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm:60: [[[bubble->parent_window() window] windowController] locationBarBridge]; On 2017/05/16 07:39:37, tapted wrote: > I don't think this will compile in the 10.11 or 10.12 SDK. > > [NSWindow windowController] used to be declared to return `id`, but now it's > NSWindowController*, which won't have locationBarBridge declared on it. > > I think we need a helper method like > > LocationBarForBubbleParent(..) which uses > > +[BrowserWindowController browserWindowControllerForWindow] > > (and also checks if that returned nil) Done. https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm:144: if (decoration_) On 2017/05/16 07:39:37, tapted wrote: > This raises a new lifetime issue that didn't previously exist - > BubbleAnchorHelper::OnWidgetDestroying needs a guarantee that |decoration_| > hasn't been deleted. > > I *think* WidgetOwnerNSWindowAdapter provides this guarantee, but we should put > an ownership comment up around line 50. E.g. > > // Weak. Lifetime is tied to the controller of bubble's parent window, which > owns a LocationBarViewMac that directly owns |decoration_|. > > And here, a comment like, > > // NativeWidgetMac guarantees that a child's OnWidgetDestroying() is invoked > before the parent NSWindow has closed. Done. Are there any other steps that we could perhaps take to make sure this is guaranteed? https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right): https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:305: model->set_decoration(this); On 2017/05/16 07:39:37, tapted wrote: > this doesn't look right - can we just pass |this| through to > ContentSettingBubbleViewsBridge::Show(..) below, and avoid changing the model? Good point. I implemented a method in LocationBarViewMac that can return the content setting decoration based on the anchor point. https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:67: if (bubble) On 2017/05/16 07:39:37, tapted wrote: > is there a known case where this is null? (comment? or DCHECK) Whoops, no. I removed it
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/2882533003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right): https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:310: [web_contents->GetTopLevelNativeWindow() contentView], Is there a reason we can't pass |this| to ::Show(..) on this line? https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:322: decoration:this i.e. similar to the call here, for Cocoa https://codereview.chromium.org/2882533003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/2882533003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:102: void ContentSettingBubbleViewsBridge::Show(gfx::NativeView parent_view, (is it possible to add a LocationBarDecoration* argument to the method here? I think a forward dec is all you need, so we don't need this file to be .mm)
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 spqchan@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...
PTAL https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right): https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:322: decoration:this On 2017/05/17 01:57:49, tapted wrote: > i.e. similar to the call here, for Cocoa https://codereview.chromium.org/2882533003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/2882533003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:102: void ContentSettingBubbleViewsBridge::Show(gfx::NativeView parent_view, On 2017/05/17 01:57:49, tapted wrote: > (is it possible to add a LocationBarDecoration* argument to the method here? > > I think a forward dec is all you need, so we don't need this file to be .mm) Done. I was hesitant to add a platform exclusive param to a method in browser_dialog, but I just realized that this method is actually exclusive to Mac. I added the LocationBarDecoration* param to this method, as well to the methods for the Bookmark and Page Info bubbles.
lgtm https://codereview.chromium.org/2882533003/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm (right): https://codereview.chromium.org/2882533003/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm:14: #include "ui/gfx/mac/coordinate_conversion.h" remove? https://codereview.chromium.org/2882533003/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm (right): https://codereview.chromium.org/2882533003/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:1278: LocationBarViewMac* locationBar = [controller locationBarBridge]; nit: locationBar -> location_bar. and-hm - this duplicates some logic in AnchorPointForWindow, but there are other call sites. You could pass in a LocationBarDecoration** to AnchorPointForWindow, but that might not look better. Up to you. This is fine.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Thanks! https://codereview.chromium.org/2882533003/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm (right): https://codereview.chromium.org/2882533003/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm:14: #include "ui/gfx/mac/coordinate_conversion.h" On 2017/05/18 03:40:38, tapted wrote: > remove? Done. https://codereview.chromium.org/2882533003/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm (right): https://codereview.chromium.org/2882533003/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:1278: LocationBarViewMac* locationBar = [controller locationBarBridge]; On 2017/05/18 03:40:39, tapted wrote: > nit: locationBar -> location_bar. > > and-hm - this duplicates some logic in AnchorPointForWindow, but there are other > call sites. You could pass in a LocationBarDecoration** to AnchorPointForWindow, > but that might not look better. Up to you. This is fine. Done.
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2882533003/#ps180001 (title: "Addressed tapted's comments")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
spqchan@chromium.org changed reviewers: + sky@chromium.org
+ sky for browser_dialogs.h and chrome/browser/ui/views ownership
https://codereview.chromium.org/2882533003/diff/180001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2882533003/diff/180001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:131: LocationBarDecoration* decoration); The code in c/b/ui shouldn't know about platform specific classes such as LocationBarDecoration. Is there some other way to inject the decoration? I'm not clear on why you need decoration here? Presumably only the mac code is going to be calling this, so can't the mac code call the function defined in the mac side? I'm suggesting you leave this functions as is here and in the mac code you define the function that takes a decoration.
Patchset #7 (id:200001) has been deleted
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #7 (id:220001) 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...
PTAL +tapted, I moved LocationBarViewMac back to cocoa. Can you look over this again? https://codereview.chromium.org/2882533003/diff/180001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2882533003/diff/180001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:131: LocationBarDecoration* decoration); On 2017/05/19 14:53:22, sky wrote: > The code in c/b/ui shouldn't know about platform specific classes such as > LocationBarDecoration. Is there some other way to inject the decoration? I'm not > clear on why you need decoration here? Presumably only the mac code is going to > be calling this, so can't the mac code call the function defined in the mac > side? I'm suggesting you leave this functions as is here and in the mac code you > define the function that takes a decoration. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2882533003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/page_info/page_info_bubble_view.h (right): https://codereview.chromium.org/2882533003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/page_info_bubble_view.h:78: // If |anchor_view| is null, |anchor_rect| is used to anchor the bubble. Document return value, and in particular document who owns it.
On 2017/05/19 20:03:22, sky wrote: > LGTM > > https://codereview.chromium.org/2882533003/diff/240001/chrome/browser/ui/view... > File chrome/browser/ui/views/page_info/page_info_bubble_view.h (right): > > https://codereview.chromium.org/2882533003/diff/240001/chrome/browser/ui/view... > chrome/browser/ui/views/page_info/page_info_bubble_view.h:78: // If > |anchor_view| is null, |anchor_rect| is used to anchor the bubble. > Document return value, and in particular document who owns it. Thanks! Just need another LGTM from tapted
https://codereview.chromium.org/2882533003/diff/180001/chrome/browser/ui/brow... File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2882533003/diff/180001/chrome/browser/ui/brow... chrome/browser/ui/browser_dialogs.h:131: LocationBarDecoration* decoration); On 2017/05/19 17:44:55, spqchan wrote: > On 2017/05/19 14:53:22, sky wrote: > > The code in c/b/ui shouldn't know about platform specific classes such as > > LocationBarDecoration. Is there some other way to inject the decoration? I'm > not > > clear on why you need decoration here? Presumably only the mac code is going > to > > be calling this, so can't the mac code call the function defined in the mac > > side? I'm suggesting you leave this functions as is here and in the mac code > you > > define the function that takes a decoration. > > Done. Ooh interesting. I think the fact that we can even pass a LocationBarDecoration* in to these methods (which only exists in a Cocoa browser window), is an indication that these declarations are just in the wrong place, and should be moved under c/b/ui/cocoa. I toyed around with PatchSet 6 in https://codereview.chromium.org/2899583002/ - I think that's what we want. Basically it's move: chrome/browser/ui/{views => cocoa}/browser_dialogs_views_mac.cc - then we make a chrome/browser/ui/cocoa/browser_dialogs_views_mac.h to match, which has the declarations that are only in use by the cocoa browser and can be deleted when things shift to mac_views_browser https://codereview.chromium.org/2882533003/diff/240001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/2882533003/diff/240001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:594: ui::ConvertPointFromScreenToWindow([field_ window], anchor); This kind of hit testing is usually good to avoid - it can lead to less robust solutions. Eg there are already issues when bubbles pop up whilst an NSWindow is being actively dragged by the user -- Cocoa doesn't give continuous updates to [window frame] until the window is released, since the drag events are intercepted by the window server process. Mapping from screen to window coordinates can do weird things in these cases. https://codereview.chromium.org/2882533003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/2882533003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/browser_dialogs_views_mac.cc:10: #include "chrome/browser/ui/cocoa/bubble_anchor_helper_views.h" (this was here before, but I think a c/b/ui/cocoa include doesn't really belong under c/b/ui/views, hence I think moving the entire browser_dialogs_views_mac.cc file makes the most sense)
The CQ bit was checked by spqchan@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #8 (id:260001) has been deleted
The CQ bit was checked by spqchan@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 spqchan@chromium.org to run a CQ dry run
Patchset #8 (id:280001) has been deleted
Patchset #8 (id:300001) has been deleted
Patchset #8 (id:320001) 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.
Great, thanks! I applied the changes you made to this CL and rebased it. https://codereview.chromium.org/2882533003/diff/240001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/2882533003/diff/240001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:594: ui::ConvertPointFromScreenToWindow([field_ window], anchor); On 2017/05/21 08:15:28, tapted wrote: > This kind of hit testing is usually good to avoid - it can lead to less robust > solutions. Eg there are already issues when bubbles pop up whilst an NSWindow is > being actively dragged by the user -- Cocoa doesn't give continuous updates to > [window frame] until the window is released, since the drag events are > intercepted by the window server process. Mapping from screen to window > coordinates can do weird things in these cases. That makes sense, I'll avoid this
lgtm - thanks! but make sure sky@ is on board with the chrome/browser/ui/browser_dialogs.h changes (now just deleting stuff \o/)
Thanks! sky, can you take another look?
LGTM
On 2017/05/23 17:31:20, sky wrote: > LGTM thanks!
The CQ bit was checked by spqchan@chromium.org
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": 340001, "attempt_start_ts": 1495560742924120, "parent_rev": "0b3fe7fdedf2eb52860ccac16bd8c04822c156b2", "commit_rev": "358c00f72ae3bcf3911da69b48403116dc267712"}
Message was sent while issue was closed.
Description was changed from ========== [Mac] Hover/Active Omnibox Icon States for Secondary UI MD When a secondary UI dialog is opened, the omnibox icon it's anchored to need to set its state as active. When the dialog is dismissed, the icon is set to inactive. BubbleAnchorHelper provides functions to retrieve the omnibox icons and sets the active state. BUG=715863 ========== to ========== [Mac] Hover/Active Omnibox Icon States for Secondary UI MD When a secondary UI dialog is opened, the omnibox icon it's anchored to need to set its state as active. When the dialog is dismissed, the icon is set to inactive. BubbleAnchorHelper provides functions to retrieve the omnibox icons and sets the active state. BUG=715863 Review-Url: https://codereview.chromium.org/2882533003 Cr-Commit-Position: refs/heads/master@{#473963} Committed: https://chromium.googlesource.com/chromium/src/+/358c00f72ae3bcf3911da69b4840... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:340001) as https://chromium.googlesource.com/chromium/src/+/358c00f72ae3bcf3911da69b4840... |