Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2196203002:
[Mac] The infobar's top arrow should be hidden while the omnibox popup is shown (Closed)
|
Created:
4 years, 4 months ago by Sungmann Cho Modified:
3 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Mac] The infobar's top arrow should be hidden while the omnibox popup is shown
Unlike on Windows, the infobar's top arrow still remains on a screen even after
the omnibox popup is displayed over it. This CL fixes this problem based on the
idea (https://codereview.chromium.org/18859004) previously applied to Windows
version.
BUG=174077
Review-Url: https://codereview.chromium.org/2196203002
Cr-Original-Commit-Position: refs/heads/master@{#464700}
Committed: https://chromium.googlesource.com/chromium/src/+/d1040f6d95719c52a3ec442c76be29d5c0630bf4
Review-Url: https://codereview.chromium.org/2196203002
Cr-Commit-Position: refs/heads/master@{#464883}
Committed: https://chromium.googlesource.com/chromium/src/+/7b81dd151531dcd6b5cdbbd078fbf37e4b102adf
Patch Set 1 : The initial approach #Patch Set 2 : The second approach #Patch Set 3 : Sort #includes #Patch Set 4 : Revert to the patch set 1 #
Total comments: 6
Patch Set 5 : Address comments #
Total comments: 1
Patch Set 6 : The third approach #
Total comments: 4
Patch Set 7 : Use ScopedObserver #Patch Set 8 : Fix crasher #
Total comments: 2
Patch Set 9 : Address comments #
Messages
Total messages: 41 (12 generated)
Description was changed from ========== WIP BUG=174077 ========== to ========== [Mac] The infobar's top arrow should be hidden while the omnibox popup is shown Unlike on Windows, the infobar's top arrow still remains on a screen even after the omnibox popup is displayed over it. This CL fixes this problem based on the idea (https://codereview.chromium.org/18859004) previously applied to Windows version. BUG=174077 ==========
sungmann.cho@navercorp.com changed reviewers: + rsesek@chromium.org
@rsesek: I'm fixing crbug.com/174077 and this is my first try, but I'm not convinced this is a right approach to do this in terms of design and relationship between related classes. Would you please take a look at this? I want to hear your advice. Thanks!
I think this is definitely in the right direction. Did you consider implementing the OmniboxPopupModelObserver interface on BrowserWindowCocoa instead? That'd remove the use of the one-off class.
On 2016/08/02 13:44:08, Robert Sesek wrote: > I think this is definitely in the right direction. Did you consider implementing > the OmniboxPopupModelObserver interface on BrowserWindowCocoa instead? That'd > remove the use of the one-off class. OK. I'll give it a shot.
On 2016/08/03 12:52:16, Sungmann Cho wrote: > On 2016/08/02 13:44:08, Robert Sesek wrote: > > I think this is definitely in the right direction. Did you consider > implementing > > the OmniboxPopupModelObserver interface on BrowserWindowCocoa instead? That'd > > remove the use of the one-off class. > > OK. I'll give it a shot. I implemented OmniboxPopupModelObserver on BrowserWindowCocoa. For me, the first approach seems more clear and readable as all the relevant codes are gathered in one place(OmniboxPopupModelObserverBridge), and besides we can get benefits from using RAII. What do you think?
On 2016/08/03 13:55:14, Sungmann Cho wrote: > On 2016/08/03 12:52:16, Sungmann Cho wrote: > > On 2016/08/02 13:44:08, Robert Sesek wrote: > > > I think this is definitely in the right direction. Did you consider > > implementing > > > the OmniboxPopupModelObserver interface on BrowserWindowCocoa instead? > That'd > > > remove the use of the one-off class. > > > > OK. I'll give it a shot. > > I implemented OmniboxPopupModelObserver on BrowserWindowCocoa. For me, the first > approach seems more clear and readable as all the relevant codes are gathered in > one place(OmniboxPopupModelObserverBridge), and besides we can get benefits from > using RAII. What do you think? I see your point. The problem seems to stem from the fact that you can't AddObserver in BrowserWindowCocoa's ctor because the location bar isn't yet constructed. I agree with you that the earlier patch set is more clear.
On 2016/08/04 14:41:50, Robert Sesek wrote: > On 2016/08/03 13:55:14, Sungmann Cho wrote: > > On 2016/08/03 12:52:16, Sungmann Cho wrote: > > > On 2016/08/02 13:44:08, Robert Sesek wrote: > > > > I think this is definitely in the right direction. Did you consider > > > implementing > > > > the OmniboxPopupModelObserver interface on BrowserWindowCocoa instead? > > That'd > > > > remove the use of the one-off class. > > > > > > OK. I'll give it a shot. > > > > I implemented OmniboxPopupModelObserver on BrowserWindowCocoa. For me, the > first > > approach seems more clear and readable as all the relevant codes are gathered > in > > one place(OmniboxPopupModelObserverBridge), and besides we can get benefits > from > > using RAII. What do you think? > > I see your point. The problem seems to stem from the fact that you can't > AddObserver in BrowserWindowCocoa's ctor because the location bar isn't yet > constructed. I agree with you that the earlier patch set is more clear. You're right, indeed. I resubmitted the earlier patch set.
sungmann.cho@navercorp.com changed reviewers: + pkasting@chromium.org
@pkasting: PTAL at chrome/browser/ui/infobar_container_delegate.{h|cc}
https://codereview.chromium.org/2196203002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller.h (right): https://codereview.chromium.org/2196203002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.h:211: omnibox_popup_model_observer_bridge_; naming: use camelCase since this is an ObjC ivar (the variable on line 207 is wrongly named). https://codereview.chromium.org/2196203002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2196203002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.mm:201: omnibox_popup_model_ = [controller_ locationBarBridge] You could move this to the initializer list and remove the DCHECK on line 200. https://codereview.chromium.org/2196203002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.mm:225: }; DISALLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/2196203002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller.h (right): https://codereview.chromium.org/2196203002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.h:211: omnibox_popup_model_observer_bridge_; On 2016/08/04 17:13:40, Robert Sesek wrote: > naming: use camelCase since this is an ObjC ivar (the variable on line 207 is > wrongly named). Done. https://codereview.chromium.org/2196203002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2196203002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.mm:201: omnibox_popup_model_ = [controller_ locationBarBridge] On 2016/08/04 17:13:41, Robert Sesek wrote: > You could move this to the initializer list and remove the DCHECK on line 200. Done. https://codereview.chromium.org/2196203002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.mm:225: }; On 2016/08/04 17:13:41, Robert Sesek wrote: > DISALLOW_COPY_AND_ASSIGN Done.
LGTM
https://codereview.chromium.org/2196203002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2196203002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.mm:215: [infobar_container_controller defaultMaxTopArrowHeight]; Won't this do the wrong thing when the current arrow height is not the default height? We want to toggle between 0 and the current height, not 0 and the default height, I think. Also, seems like instead of plumbing the accessor on the infobar controller, we should plumb this from the browser window controller or similar, so the code that sets the max top arrow height in this case can just call the same function as the code that already sets it today for Mac. This is basically what Views does with the function in BrowserView.
What's the status of this CL?
On 2017/02/11 02:08:34, Peter Kasting wrote: > What's the status of this CL? Oh my days! I totally forgot about this CL. :( I'll fix this ASAP. Thanks for letting me this!
On 2017/02/11 02:08:34, Peter Kasting wrote: > What's the status of this CL? I'm really sorry for being late. I just uploaded a follow up CL. I would appreciate if you could take a look at it.
LGTM https://codereview.chromium.org/2196203002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2196203002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:204: omnibox_popup_model_->AddObserver(this); Nit: Prefer using a ScopedObserver to manual add/remove calls https://codereview.chromium.org/2196203002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:435: new OmniboxPopupModelObserverBridge(self)); Is it safe to construct this in the constructor, or is there no popup model at that point? That would let us use a by-value member instead of a unique_ptr.
https://codereview.chromium.org/2196203002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2196203002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:204: omnibox_popup_model_->AddObserver(this); On 2017/04/10 21:53:56, Peter Kasting wrote: > Nit: Prefer using a ScopedObserver to manual add/remove calls Done. https://codereview.chromium.org/2196203002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:435: new OmniboxPopupModelObserverBridge(self)); On 2017/04/10 21:53:56, Peter Kasting wrote: > Is it safe to construct this in the constructor, or is there no popup model at > that point? That would let us use a by-value member instead of a unique_ptr. I think |initWithBrowser:browser:| is the right place to create the bridge as it corresponds to BrowserView::InitViews(), and it's safe to create it here since the popup model already exists at that point.
I uploaded the patch set #7 that resolves the last comments, and left my thought on the question. Is the previous l-g-t-m still valid?
On 2017/04/14 01:48:34, Sungmann Cho wrote: > I uploaded the patch set #7 that resolves the last comments, and left my thought > on the question. Is the previous l-g-t-m still valid? Yes.
The CQ bit was checked by sungmann.cho@navercorp.com
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2196203002/#ps120001 (title: "Use ScopedObserver")
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": 120001, "attempt_start_ts": 1492154263101110,
"parent_rev": "6ea2f510147f3bca05aea7024752e95dcf559912", "commit_rev":
"d1040f6d95719c52a3ec442c76be29d5c0630bf4"}
Message was sent while issue was closed.
Description was changed from ========== [Mac] The infobar's top arrow should be hidden while the omnibox popup is shown Unlike on Windows, the infobar's top arrow still remains on a screen even after the omnibox popup is displayed over it. This CL fixes this problem based on the idea (https://codereview.chromium.org/18859004) previously applied to Windows version. BUG=174077 ========== to ========== [Mac] The infobar's top arrow should be hidden while the omnibox popup is shown Unlike on Windows, the infobar's top arrow still remains on a screen even after the omnibox popup is displayed over it. This CL fixes this problem based on the idea (https://codereview.chromium.org/18859004) previously applied to Windows version. BUG=174077 Review-Url: https://codereview.chromium.org/2196203002 Cr-Commit-Position: refs/heads/master@{#464700} Committed: https://chromium.googlesource.com/chromium/src/+/d1040f6d95719c52a3ec442c76be... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/d1040f6d95719c52a3ec442c76be...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2823743002/ by skym@chromium.org. The reason for reverting is: Causing sync integ tests to fail, see crbug.com/711791.
Message was sent while issue was closed.
Description was changed from ========== [Mac] The infobar's top arrow should be hidden while the omnibox popup is shown Unlike on Windows, the infobar's top arrow still remains on a screen even after the omnibox popup is displayed over it. This CL fixes this problem based on the idea (https://codereview.chromium.org/18859004) previously applied to Windows version. BUG=174077 Review-Url: https://codereview.chromium.org/2196203002 Cr-Commit-Position: refs/heads/master@{#464700} Committed: https://chromium.googlesource.com/chromium/src/+/d1040f6d95719c52a3ec442c76be... ========== to ========== [Mac] The infobar's top arrow should be hidden while the omnibox popup is shown Unlike on Windows, the infobar's top arrow still remains on a screen even after the omnibox popup is displayed over it. This CL fixes this problem based on the idea (https://codereview.chromium.org/18859004) previously applied to Windows version. BUG=174077 Review-Url: https://codereview.chromium.org/2196203002 Cr-Commit-Position: refs/heads/master@{#464700} Committed: https://chromium.googlesource.com/chromium/src/+/d1040f6d95719c52a3ec442c76be... ==========
The segmentation fault is stemmed from the fact that the scoped observer in |OmniboxPopupModelObserverBridge| tries to remove the observer from |omnibox_popup_model_| already deleted during the destruction of |toolbarController_|. So, to prevent this from happening, we must explicitly delete |OmniboxPopupModelObserverBridge| before |toolbarController_| is deleted. I uploaded the new patch set #8 that resolves the problem. When I ran sync_integration_tests --gtest_filter=TwoClientAutofillSyncTest.* with this CL, I got the successful result. Would you please take a look again? I apologize for causing you trouble.
LGTM In the future, prefer to upload the original landed patch to a new CL, then upload your fixed version as a new patchset, rather than reuploading to the original CL -- this makes it clearer what happened when tracing down landed/reverted/relanded later. (And when we switch to Gerrit it will only support that workflow.) https://codereview.chromium.org/2196203002/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2196203002/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:435: new OmniboxPopupModelObserverBridge(self)); Nit: Prefer = base::MakeUnique<> over reset(new ...) https://codereview.chromium.org/2196203002/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:466: omniboxPopupModelObserverBridge_.reset(); Nit: Add comment about why this has to be reset here.
On 2017/04/15 17:37:03, Peter Kasting wrote: > LGTM > > In the future, prefer to upload the original landed patch to a new CL, then > upload your fixed version as a new patchset, rather than reuploading to the > original CL -- this makes it clearer what happened when tracing down > landed/reverted/relanded later. (And when we switch to Gerrit it will only > support that workflow.) Aha! I got it. I'll keep this in mind. > https://codereview.chromium.org/2196203002/diff/140001/chrome/browser/ui/coco... > File chrome/browser/ui/cocoa/browser_window_controller.mm (right): > > https://codereview.chromium.org/2196203002/diff/140001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/browser_window_controller.mm:435: new > OmniboxPopupModelObserverBridge(self)); > Nit: Prefer = base::MakeUnique<> over reset(new ...) Done. > https://codereview.chromium.org/2196203002/diff/140001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/browser_window_controller.mm:466: > omniboxPopupModelObserverBridge_.reset(); > Nit: Add comment about why this has to be reset here. Done.
The CQ bit was checked by sungmann.cho@navercorp.com
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2196203002/#ps160001 (title: "Address comments")
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": 160001, "attempt_start_ts": 1492315235963760,
"parent_rev": "3e1cedb46b52e6c09243993671a5a0ddb7a4eeed", "commit_rev":
"7b81dd151531dcd6b5cdbbd078fbf37e4b102adf"}
Message was sent while issue was closed.
Description was changed from ========== [Mac] The infobar's top arrow should be hidden while the omnibox popup is shown Unlike on Windows, the infobar's top arrow still remains on a screen even after the omnibox popup is displayed over it. This CL fixes this problem based on the idea (https://codereview.chromium.org/18859004) previously applied to Windows version. BUG=174077 Review-Url: https://codereview.chromium.org/2196203002 Cr-Commit-Position: refs/heads/master@{#464700} Committed: https://chromium.googlesource.com/chromium/src/+/d1040f6d95719c52a3ec442c76be... ========== to ========== [Mac] The infobar's top arrow should be hidden while the omnibox popup is shown Unlike on Windows, the infobar's top arrow still remains on a screen even after the omnibox popup is displayed over it. This CL fixes this problem based on the idea (https://codereview.chromium.org/18859004) previously applied to Windows version. BUG=174077 Review-Url: https://codereview.chromium.org/2196203002 Cr-Original-Commit-Position: refs/heads/master@{#464700} Committed: https://chromium.googlesource.com/chromium/src/+/d1040f6d95719c52a3ec442c76be... Review-Url: https://codereview.chromium.org/2196203002 Cr-Commit-Position: refs/heads/master@{#464883} Committed: https://chromium.googlesource.com/chromium/src/+/7b81dd151531dcd6b5cdbbd078fb... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/7b81dd151531dcd6b5cdbbd078fb...
Message was sent while issue was closed.
On 2017/04/16 04:19:33, commit-bot: I haz the power wrote: > Committed patchset #9 (id:160001) as > https://chromium.googlesource.com/chromium/src/+/7b81dd151531dcd6b5cdbbd078fb... Hi, According to Findit's flake analyzer this CL may have introduced flakiness in the test SingleClientPasswordManagerSettingMigratorServiceSyncTest.LocalOffOffSyncOffOn on Mac10.10 Tests, can you please help verify? The link to the analysis is pasted below: https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV... Thanks, Jeff on behalf of Findit team
Message was sent while issue was closed.
On 2017/04/17 07:51:06, lijeffrey wrote: > On 2017/04/16 04:19:33, commit-bot: I haz the power wrote: > > Committed patchset #9 (id:160001) as > > > https://chromium.googlesource.com/chromium/src/+/7b81dd151531dcd6b5cdbbd078fb... > > Hi, > > According to Findit's flake analyzer this CL may have introduced flakiness in > the test > SingleClientPasswordManagerSettingMigratorServiceSyncTest.LocalOffOffSyncOffOn > on Mac10.10 Tests, can you please help verify? The link to the analysis is > pasted below: > > https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV... > > Thanks, > Jeff on behalf of Findit team Hi! The CL causing the problem was reverted at 2017-04-15 00:19:05 UTC, and the flaky test report indicates that the request time of the test is 2017-04-15 00:03:48 UTC. I think the problem will be not observed in the next tries.
Message was sent while issue was closed.
On 2017/04/17 09:12:10, Sungmann Cho wrote: > On 2017/04/17 07:51:06, lijeffrey wrote: > > On 2017/04/16 04:19:33, commit-bot: I haz the power wrote: > > > Committed patchset #9 (id:160001) as > > > > > > https://chromium.googlesource.com/chromium/src/+/7b81dd151531dcd6b5cdbbd078fb... > > > > Hi, > > > > According to Findit's flake analyzer this CL may have introduced flakiness in > > the test > > SingleClientPasswordManagerSettingMigratorServiceSyncTest.LocalOffOffSyncOffOn > > on Mac10.10 Tests, can you please help verify? The link to the analysis is > > pasted below: > > > > > https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV... > > > > Thanks, > > Jeff on behalf of Findit team > > Hi! The CL causing the problem was reverted at 2017-04-15 00:19:05 UTC, and the > flaky test report indicates that the request time of the test is 2017-04-15 > 00:03:48 UTC. I think the problem will be not observed in the next tries. Great! Many thanks. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
