Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1062273002:
Snap RWHVA and resize the legacy window on Windows whenever the ancestor window's bounds change. (Closed)
|
Created:
5 years, 8 months ago by ananta Modified:
5 years, 8 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, jam, penghuang+watch_chromium.org, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSnap RWHVA and resize the legacy window on Windows whenever the ancestor window's bounds change.
The bookmark bar is hidden through the fast resize code path where in the Renderer is informed about
the actual size minus the bookmar bar before the bookmark bar is hidden. To ensure that the RWHVA is snapped
to pixel boundaries and the legacy window is resized we observe bounds changed events on the parent of the WCVA
window which is the NativeViewHost clipping window. Reason for doing this is the layer for the window hosted by WCVA
verifies if the stored bounds is the same as the one coming in and ignores the change if yes.
For more context and a general discussion of the reason why this bug happens please visit this link
https://codereview.chromium.org/1009533005/
BUG=469857, 468669
Committed: https://crrev.com/e6a9b7fff4802ac6796250987be4d52204716392
Cr-Commit-Position: refs/heads/master@{#324743}
Patch Set 1 #Patch Set 2 : Added comments #Patch Set 3 : Rebased to tip #Patch Set 4 : Fixed comment #
Total comments: 2
Patch Set 5 : Observe all ancestors. #
Total comments: 8
Patch Set 6 : Move the ancestor observer to another class and handle hierarchy changed notifications. #Patch Set 7 : Reverted change to ~WindowObserver #Patch Set 8 : Fix content_browsertests redness and a crasher while removing observers #
Total comments: 14
Patch Set 9 : Call RWHVA on Hierachy changed and recreate the ancestor window observer #Patch Set 10 : Fixed comments #
Total comments: 12
Patch Set 11 : Address review comments #Patch Set 12 : Add DCHECK in WindowAncestorObserver::OnWindowBoundsChanged #
Messages
Total messages: 29 (5 generated)
ananta@chromium.org changed reviewers: + scottmg@chromium.org, sky@chromium.org
https://codereview.chromium.org/1062273002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1062273002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:363: view_ancestor_->AddObserver(this); Don't you want to know when the bounds of any ancestor changes?
https://codereview.chromium.org/1062273002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1062273002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:363: view_ancestor_->AddObserver(this); On 2015/04/08 20:13:09, sky wrote: > Don't you want to know when the bounds of any ancestor changes? Done.
https://codereview.chromium.org/1062273002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1062273002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:336: class RenderWidgetHostViewAura::WindowObserver : public aura::WindowObserver { Using the same class for two very different purposes is error prone. I would prefer another class that manages listening to all the ancestors. And update the documentation for this class. https://codereview.chromium.org/1062273002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:349: void OnWindowAddedToRootWindow(aura::Window* window) override { I don't think you're handling the case of ancestors changing correctly but the root remains the same. I don't think you care OnWindowAddedToRoot/RemovedFromRoot, but rather any time an ancestor is added/removed. This is another reason to remove this logic into a standalone class rather than here. https://codereview.chromium.org/1062273002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:394: std::map<aura::Window*, bool> ancestor_observer_map_; I don't think you need a map, how about a set? https://codereview.chromium.org/1062273002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:776: SnapToPhysicalPixelBoundary(); Does this mean we can remove some calls to SnapToPhysicalPixelBoundary.
https://codereview.chromium.org/1062273002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1062273002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:336: class RenderWidgetHostViewAura::WindowObserver : public aura::WindowObserver { On 2015/04/09 00:01:14, sky wrote: > Using the same class for two very different purposes is error prone. I would > prefer another class that manages listening to all the ancestors. > > And update the documentation for this class. Done. https://codereview.chromium.org/1062273002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:349: void OnWindowAddedToRootWindow(aura::Window* window) override { On 2015/04/09 00:01:14, sky wrote: > I don't think you're handling the case of ancestors changing correctly but the > root remains the same. I don't think you care > OnWindowAddedToRoot/RemovedFromRoot, but rather any time an ancestor is > added/removed. This is another reason to remove this logic into a standalone > class rather than here. Done. https://codereview.chromium.org/1062273002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:394: std::map<aura::Window*, bool> ancestor_observer_map_; On 2015/04/09 00:01:14, sky wrote: > I don't think you need a map, how about a set? Done. https://codereview.chromium.org/1062273002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:776: SnapToPhysicalPixelBoundary(); On 2015/04/09 00:01:14, sky wrote: > Does this mean we can remove some calls to SnapToPhysicalPixelBoundary. Removed the call from WCVA.
On 2015/04/09 03:37:04, ananta wrote: Please hold off on the review. Looking at the test redness. Thanks
Patch is ready for a look. Thanks Ananta
https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:388: if (!params.target->Contains(view_->window_) || !params.new_parent) I'm wondering if you could simplify things like the following: . In WindowObserver override OnWindowHierarchyChanged() call through to RWHVA and tell it the WindowHierarchyChanged. . RWHVA::WindowHierarchyChanged would destroy the current WindowAncestorObserver and create a new one. My rationale is how you have things now you'll get OnWindowHierarchyChanged n! times. Additionally what I've outlined is much simpler and less error prone. It should also mean you don't need to worry about OnWindowDestroyed either, it's handle by virtue of creating/destroying when the hierarchy changes. https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:413: if (ancestor_observer_tracker_.find(window) != This should be a DCHECK. https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:415: view_->HandleParentBoundsChanged(); Don't you only care about this if the origin changes? Size changes won't effect the location on screen, right? https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:425: for (auto ancestor_observer_iterator : ancestor_observer_tracker_) ancestor_observer_iterator is a misleading name here. ancestor_observer_iterator is really a window. https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:431: std::set<aura::Window*>::iterator index = index->iter https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:510: class WindowObserver; nit: I believe we try and group class and friend declarations at the beginning of a section. https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:518: // Tracks the ancestors of the RWHVA window for bounds changed notifications. nit: it isn't bounds change, rather screen bounds changing.
https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:388: if (!params.target->Contains(view_->window_) || !params.new_parent) On 2015/04/09 23:44:38, sky wrote: > I'm wondering if you could simplify things like the following: > > . In WindowObserver override OnWindowHierarchyChanged() call through to RWHVA > and tell it the WindowHierarchyChanged. > . RWHVA::WindowHierarchyChanged would destroy the current WindowAncestorObserver > and create a new one. > > My rationale is how you have things now you'll get OnWindowHierarchyChanged n! > times. Additionally what I've outlined is much simpler and less error prone. It > should also mean you don't need to worry about OnWindowDestroyed either, it's > handle by virtue of creating/destroying when the hierarchy changes. Done. I added the hierarchy changed function in the current WindowObserver class. https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:413: if (ancestor_observer_tracker_.find(window) != On 2015/04/09 23:44:38, sky wrote: > This should be a DCHECK. Done. https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:415: view_->HandleParentBoundsChanged(); On 2015/04/09 23:44:38, sky wrote: > Don't you only care about this if the origin changes? Size changes won't effect > the location on screen, right? Yes. Added a check for origin change. https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:425: for (auto ancestor_observer_iterator : ancestor_observer_tracker_) On 2015/04/09 23:44:38, sky wrote: > ancestor_observer_iterator is a misleading name here. ancestor_observer_iterator > is really a window. changed to ancestor https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:431: std::set<aura::Window*>::iterator index = On 2015/04/09 23:44:38, sky wrote: > index->iter Function has been removed. https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:510: class WindowObserver; On 2015/04/09 23:44:38, sky wrote: > nit: I believe we try and group class and friend declarations at the beginning > of a section. Moved back to where it was before. I thought it would be better to have it where it was needed. https://codereview.chromium.org/1062273002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:518: // Tracks the ancestors of the RWHVA window for bounds changed notifications. On 2015/04/09 23:44:38, sky wrote: > nit: it isn't bounds change, rather screen bounds changing. Reworded.
https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:367: // This class provides functionality to observe the ancestors of the RWHVA This comment is misleading. Isn't the issue that we need to position on pixel pixel boundaries, which may change when the bounds relative to the root changes? https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:382: aura::Window* parent = view_->window_->parent(); Isn't this a lot simpler! https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:383: while (parent) { Who handles positioning when the bounds of view_->window_ changes? https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:397: if (ancestor_observer_tracker_.find(window) != Shouldn't this be a DCHECK? https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:414: std::set<aura::Window*> ancestor_observer_tracker_; Why the observer_tracker_? How about just ancestors_? https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:808: // Snap when we receive a hierarchy changed. http://crbug.com/388908. I shouldn't have to look at a bug to see why we need to snap when hierarchy changes. Document why.
https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:367: // This class provides functionality to observe the ancestors of the RWHVA On 2015/04/10 15:20:21, sky wrote: > This comment is misleading. Isn't the issue that we need to position on pixel > pixel boundaries, which may change when the bounds relative to the root changes? Reworded https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:382: aura::Window* parent = view_->window_->parent(); On 2015/04/10 15:20:21, sky wrote: > Isn't this a lot simpler! Yes. Thanks https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:383: while (parent) { On 2015/04/10 15:20:21, sky wrote: > Who handles positioning when the bounds of view_->window_ changes? RWHVA in its OnBoundsChanged function via the Window delegate. https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:397: if (ancestor_observer_tracker_.find(window) != On 2015/04/10 15:20:21, sky wrote: > Shouldn't this be a DCHECK? There is a DCHECK in the else. Were you indicating something else? https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:414: std::set<aura::Window*> ancestor_observer_tracker_; On 2015/04/10 15:20:21, sky wrote: > Why the observer_tracker_? How about just ancestors_? Done.
https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:397: if (ancestor_observer_tracker_.find(window) != On 2015/04/10 17:51:21, ananta wrote: > On 2015/04/10 15:20:21, sky wrote: > > Shouldn't this be a DCHECK? > > There is a DCHECK in the else. Were you indicating something else? I'm saying you should convert the if to a DCHECK and remove the DCHECK on 402.
On 2015/04/10 19:36:25, sky wrote: > https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_aura.cc (right): > > https://codereview.chromium.org/1062273002/diff/180001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_aura.cc:397: if > (ancestor_observer_tracker_.find(window) != > On 2015/04/10 17:51:21, ananta wrote: > > On 2015/04/10 15:20:21, sky wrote: > > > Shouldn't this be a DCHECK? > > > > There is a DCHECK in the else. Were you indicating something else? > > I'm saying you should convert the if to a DCHECK and remove the DCHECK on 402. ok. Done.
LGTM
The CQ bit was checked by ananta@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1062273002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ananta@chromium.org changed reviewers: + cpu@chromium.org
hmm.. complicated, but I given ananta's explanation I can't see why not. sky: I wonder if the bookmark bar can be removed of the hierarchy? instead of working around it. lgtm
The CQ bit was checked by ananta@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1062273002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/e6a9b7fff4802ac6796250987be4d52204716392 Cr-Commit-Position: refs/heads/master@{#324743}
Message was sent while issue was closed.
The issue isn't specific to the bookmark bar, right? My understanding is we need to know any time the bounds relative to the root changes so that we snap to pixel. Again I'll say the real issue here is we should stop using dips. Without dips none of this would be necessary. I had started some work on moving away from dips, but it's going to require a lot of changes. -Scott On Fri, Apr 10, 2015 at 6:35 PM, <cpu@chromium.org> wrote: > hmm.. complicated, but I given ananta's explanation I can't see why not. > > sky: I wonder if the bookmark bar can be removed of the hierarchy? instead > of > working around it. > > > lgtm > > > https://codereview.chromium.org/1062273002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1063083003/ by thakis@chromium.org. The reason for reverting is: Speculative, possibly caused lots of browsert_tests redness on a bot that runs browser_tests with an official build (and a different compiler, but all the redness we fixed on official builds so far were caused by the builds being official, not because of the compiler): http://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds/285 (see http://crbug.com/467929). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
