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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 2 weeks ago by spqchan
Modified:
5 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
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!
5 months, 1 week 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: ...
5 months, 1 week 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: ...
5 months, 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 ...
5 months, 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. ...
5 months, 1 week 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 ...
5 months, 1 week 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 months, 1 week 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 months, 1 week 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 months, 1 week ago (2017-05-19 02:01:22 UTC) #43
spqchan
+ sky for browser_dialogs.h and chrome/browser/ui/views ownership
5 months, 1 week 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 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 ...
5 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 ...
5 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): > ...
5 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 ...
5 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 ...
5 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 ...
5 months ago (2017-05-23 07:12:25 UTC) #72
spqchan
Thanks! sky, can you take another look?
5 months ago (2017-05-23 07:13:37 UTC) #73
sky
LGTM
5 months ago (2017-05-23 17:31:20 UTC) #74
spqchan
On 2017/05/23 17:31:20, sky wrote: > LGTM thanks!
5 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
5 months ago (2017-05-23 17:33:17 UTC) #77
commit-bot: I haz the power
5 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa