Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 5 days ago by spqchan
Modified:
1 day 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 80 (58 generated)
spqchan
Hey Trent, PTAL. Let me know if you have any questions!
1 week, 1 day 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: ...
1 week, 1 day 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: ...
1 week 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 ...
1 week 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. ...
6 days, 17 hours 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 ...
6 days, 14 hours 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: > ...
5 days, 15 hours 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
5 days, 15 hours 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)
5 days, 15 hours ago (2017-05-19 02:01:22 UTC) #43
spqchan
+ sky for browser_dialogs.h and chrome/browser/ui/views ownership
5 days, 15 hours 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 ...
5 days, 2 hours 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 ...
5 days 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 ...
4 days, 21 hours 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): > ...
4 days, 19 hours 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 days, 9 hours 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 ...
1 day, 10 hours 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 ...
1 day, 10 hours ago (2017-05-23 07:12:25 UTC) #72
spqchan
Thanks! sky, can you take another look?
1 day, 10 hours ago (2017-05-23 07:13:37 UTC) #73
sky
LGTM
1 day ago (2017-05-23 17:31:20 UTC) #74
spqchan
On 2017/05/23 17:31:20, sky wrote: > LGTM thanks!
1 day 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
1 day ago (2017-05-23 17:33:17 UTC) #77
commit-bot: I haz the power
1 day 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06