Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(75)

Issue 2882533003: [Mac] Hover/Active Omnibox Icon States for Secondary UI MD (Closed)

Created:
3 years, 7 months ago by spqchan
Modified:
3 years, 7 months ago
Reviewers:
tapted, sky
CC:
chromium-reviews, mac-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+163 lines, -191 lines) Patch
M chrome/browser/ui/BUILD.gn View 1 2 3 4 5 6 7 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/browser_dialogs.h View 1 2 3 4 5 6 7 4 chunks +0 lines, -43 lines 0 comments Download
A chrome/browser/ui/cocoa/browser_dialogs_views_mac.h View 1 2 3 4 5 6 7 1 chunk +70 lines, -0 lines 0 comments Download
A + chrome/browser/ui/cocoa/browser_dialogs_views_mac.cc View 1 2 3 4 5 6 7 6 chunks +19 lines, -9 lines 0 comments Download
M chrome/browser/ui/cocoa/browser_window_controller.mm View 1 2 3 4 5 6 7 4 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/ui/cocoa/bubble_anchor_helper_views.h View 1 2 3 4 7 1 chunk +11 lines, -2 lines 0 comments Download
M chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm View 1 2 3 4 5 7 4 chunks +34 lines, -5 lines 0 comments Download
M chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm View 1 2 3 4 5 6 7 2 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm View 1 2 3 4 5 6 7 2 chunks +7 lines, -2 lines 0 comments Download
M chrome/browser/ui/cocoa/tab_dialogs_views_mac.mm View 1 chunk +1 line, -1 line 0 comments Download
D chrome/browser/ui/views/browser_dialogs_views_mac.cc View 1 2 3 4 5 6 7 1 chunk +0 lines, -112 lines 0 comments Download
M chrome/browser/ui/views/page_info/page_info_bubble_view.h View 1 2 3 4 5 6 7 1 chunk +8 lines, -8 lines 0 comments Download
M chrome/browser/ui/views/page_info/page_info_bubble_view.cc View 1 2 3 4 5 6 7 3 chunks +3 lines, -2 lines 0 comments Download

Messages

Total messages: 80 (58 generated)
spqchan
Hey Trent, PTAL. Let me know if you have any questions!
3 years, 7 months ago (2017-05-16 03:34:00 UTC) #19
tapted
https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm File chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm (right): https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm#newcode25 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/cocoa/bubble_anchor_helper_views.mm#newcode59 chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm:59: ...
3 years, 7 months ago (2017-05-16 07:39:38 UTC) #23
spqchan
PTAL https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm File chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm (right): https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm#newcode25 chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm:25: explicit BubbleAnchorHelper(views::BubbleDialogDelegateView* bubble, On 2017/05/16 07:39:37, tapted wrote: ...
3 years, 7 months ago (2017-05-17 01:39:40 UTC) #27
tapted
https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right): https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm#newcode310 chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:310: [web_contents->GetTopLevelNativeWindow() contentView], Is there a reason we can't pass ...
3 years, 7 months ago (2017-05-17 01:57:49 UTC) #29
spqchan
PTAL https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right): https://codereview.chromium.org/2882533003/diff/100001/chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm#newcode322 chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:322: decoration:this On 2017/05/17 01:57:49, tapted wrote: > i.e. ...
3 years, 7 months ago (2017-05-17 23:58:46 UTC) #34
tapted
lgtm https://codereview.chromium.org/2882533003/diff/160001/chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm File chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm (right): https://codereview.chromium.org/2882533003/diff/160001/chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm#newcode14 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/cocoa/page_info/page_info_bubble_controller.mm File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm (right): https://codereview.chromium.org/2882533003/diff/160001/chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm#newcode1278 ...
3 years, 7 months ago (2017-05-18 03:40:39 UTC) #35
spqchan
Thanks! https://codereview.chromium.org/2882533003/diff/160001/chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm File chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm (right): https://codereview.chromium.org/2882533003/diff/160001/chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm#newcode14 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: > ...
3 years, 7 months ago (2017-05-19 01:47:28 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2882533003/180001
3 years, 7 months ago (2017-05-19 01:48:25 UTC) #41
commit-bot: I haz the power
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_presubmit/builds/441958)
3 years, 7 months ago (2017-05-19 02:01:22 UTC) #43
spqchan
+ sky for browser_dialogs.h and chrome/browser/ui/views ownership
3 years, 7 months ago (2017-05-19 02:03:17 UTC) #45
sky
https://codereview.chromium.org/2882533003/diff/180001/chrome/browser/ui/browser_dialogs.h File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2882533003/diff/180001/chrome/browser/ui/browser_dialogs.h#newcode131 chrome/browser/ui/browser_dialogs.h:131: LocationBarDecoration* decoration); The code in c/b/ui shouldn't know about ...
3 years, 7 months ago (2017-05-19 14:53:22 UTC) #46
spqchan
PTAL +tapted, I moved LocationBarViewMac back to cocoa. Can you look over this again? https://codereview.chromium.org/2882533003/diff/180001/chrome/browser/ui/browser_dialogs.h ...
3 years, 7 months ago (2017-05-19 17:44:55 UTC) #51
sky
LGTM https://codereview.chromium.org/2882533003/diff/240001/chrome/browser/ui/views/page_info/page_info_bubble_view.h File chrome/browser/ui/views/page_info/page_info_bubble_view.h (right): https://codereview.chromium.org/2882533003/diff/240001/chrome/browser/ui/views/page_info/page_info_bubble_view.h#newcode78 chrome/browser/ui/views/page_info/page_info_bubble_view.h:78: // If |anchor_view| is null, |anchor_rect| is used ...
3 years, 7 months ago (2017-05-19 20:03:22 UTC) #54
spqchan
On 2017/05/19 20:03:22, sky wrote: > LGTM > > https://codereview.chromium.org/2882533003/diff/240001/chrome/browser/ui/views/page_info/page_info_bubble_view.h > File chrome/browser/ui/views/page_info/page_info_bubble_view.h (right): > ...
3 years, 7 months ago (2017-05-19 22:03:25 UTC) #55
tapted
https://codereview.chromium.org/2882533003/diff/180001/chrome/browser/ui/browser_dialogs.h File chrome/browser/ui/browser_dialogs.h (right): https://codereview.chromium.org/2882533003/diff/180001/chrome/browser/ui/browser_dialogs.h#newcode131 chrome/browser/ui/browser_dialogs.h:131: LocationBarDecoration* decoration); On 2017/05/19 17:44:55, spqchan wrote: > On ...
3 years, 7 months ago (2017-05-21 08:15:28 UTC) #56
spqchan
Great, thanks! I applied the changes you made to this CL and rebased it. https://codereview.chromium.org/2882533003/diff/240001/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm ...
3 years, 7 months ago (2017-05-23 06:52:37 UTC) #71
tapted
lgtm - thanks! but make sure sky@ is on board with the chrome/browser/ui/browser_dialogs.h changes (now ...
3 years, 7 months ago (2017-05-23 07:12:25 UTC) #72
spqchan
Thanks! sky, can you take another look?
3 years, 7 months ago (2017-05-23 07:13:37 UTC) #73
sky
LGTM
3 years, 7 months ago (2017-05-23 17:31:20 UTC) #74
spqchan
On 2017/05/23 17:31:20, sky wrote: > LGTM thanks!
3 years, 7 months ago (2017-05-23 17:32:15 UTC) #75
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2882533003/340001
3 years, 7 months ago (2017-05-23 17:33:17 UTC) #77
commit-bot: I haz the power
3 years, 7 months ago (2017-05-23 17:40:37 UTC) #80
Message was sent while issue was closed.
Committed patchset #8 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/358c00f72ae3bcf3911da69b4840...

Powered by Google App Engine
This is Rietveld 408576698