Make code path for bounds changes getting to renderer less brittle

Make code path for bounds changes getting to renderer less brittle

Currently when a move of the browser occurs WebContentsViewAura has an
observer for its parent since it does not directly get a notification
about the change, since it bounds are relative to the hiearchy and only
the top level window's bound change in this case. This allows the class
to send its new screen coordinates to the renderer. This is brittle,
because it assumes that the parent is the top level window, which is
currently true. I am introducing changes to NVHA
(https://codereview.chromium.org/48113013/) to enabled fast resize which
break this assumption.

This CL changes the observer to track all of the ancestors for the
window in question instead of just the parent, since any one of them
moving could cause the need for an update. This CL also moves the
observer class out of being a nested definition into its own class
NativeViewScreenBoundsObserver and adds a delegate interface between
WCVA and NVSBO to make testing easier.

BUG=282463
BUG=312228
TEST=Confirmed that moving the browser window around no longer breaks
     the positioning of the drop down described in 312228.
     Ran new tests in content_unittests.
     <TBD> Confirm the following are fine:
          browser_tests: PrintPreviewTest.WindowedNPAPIPluginHidden
          interactive_ui_tests:
          StarViewTestNoDWM.WindowedNPAPIPluginHidden

[rharrison, 2013-10-31 20:04:23]

PTAL

[sadrul, 2013-10-31 20:27:04]

+sky@ for owner opinion

https://codereview.chromium.org/54623007/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/54623007/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_view_aura.cc:688: SendScreenRects();
As I think about it, this needs to be invoked whenever the bounds of the window
changes in screen coordinate, right? If that is the case, then this should
really be watching all the ancestor windows for the bounds-changed signal. So
not just |parent|, and not just |top-level|. I don't know if there's a better
way of doing that.

[sky, 2013-10-31 21:55:29]

https://codereview.chromium.org/54623007/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/54623007/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_view_aura.cc:688: SendScreenRects();
On 2013/10/31 20:27:05, sadrul wrote:
> As I think about it, this needs to be invoked whenever the bounds of the
window
> changes in screen coordinate, right? If that is the case, then this should
> really be watching all the ancestor windows for the bounds-changed signal. So
> not just |parent|, and not just |top-level|. I don't know if there's a better
> way of doing that.

I think you're right. We have something like this in View. See
NeedsNotificationWhenVisibleBoundsChange.

[rharrison, 2013-11-04 14:24:17]

On 2013/10/31 21:55:29, sky wrote:
>
https://codereview.chromium.org/54623007/diff/1/content/browser/web_contents/...
> File content/browser/web_contents/web_contents_view_aura.cc (right):
> 
>
https://codereview.chromium.org/54623007/diff/1/content/browser/web_contents/...
> content/browser/web_contents/web_contents_view_aura.cc:688: SendScreenRects();
> On 2013/10/31 20:27:05, sadrul wrote:
> > As I think about it, this needs to be invoked whenever the bounds of the
> window
> > changes in screen coordinate, right? If that is the case, then this should
> > really be watching all the ancestor windows for the bounds-changed signal.
So
> > not just |parent|, and not just |top-level|. I don't know if there's a
better
> > way of doing that.
> 
> I think you're right. We have something like this in View. See
> NeedsNotificationWhenVisibleBoundsChange.

ok, I will look at resolving this and upload a new CL.

[rharrison, 2013-11-04 20:47:40]

Looking at the views code it looks like NeedsNotificationWhenVisibleBoundsChange
deals with controlling when Layout is propagated down the hierarchy. To my
understanding there isn't an equivalent mechanism that propagates information
about these changes automatically for aura::Window. Thus they cannot to react to
a change without explicitly registering to observe changed windows.

I have modified the CL to track all of the ancestors of the window that we are
interested in and keep the set of ancestors up to date.

[sky, 2013-11-04 21:30:25]

On Mon, Nov 4, 2013 at 12:47 PM,  <rharrison@chromium.org> wrote:
> Looking at the views code it looks like
> NeedsNotificationWhenVisibleBoundsChange
> deals with controlling when Layout is propagated down the hierarchy.

Not quite. If you override it to return true then
OnVisibleBoundsChanged is invoked any time bounds relative to the root
changes.

> To my
> understanding there isn't an equivalent mechanism that propagates
> information
> about these changes automatically for aura::Window. Thus they cannot to
> react to
> a change without explicitly registering to observe changed windows.

I agree that Window does not provide a similar mechanism. I mentioned
the Views approach as it seems similar to what you need.

> I have modified the CL to track all of the ancestors of the window that we
> are
> interested in and keep the set of ancestors up to date.

I'll take a look.

  -Scott
>
> https://codereview.chromium.org/54623007/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2013-11-04 21:33:23]

https://codereview.chromium.org/54623007/diff/290001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/54623007/diff/290001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:653: class
WebContentsViewAura::WindowObserver
Can you move this into its own standalone object so that you can easily write a
test?

https://codereview.chromium.org/54623007/diff/290001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:683:
view_->touch_editable_->UpdateEditingController();
Does this really need to be done any time the bounds of an ancestor changes?

https://codereview.chromium.org/54623007/diff/290001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:741:
std::set<aura::Window*> RelativeComplement(std::set<aura::Window*> set_a,
Does std offer some function that does this?

[sky, 2013-11-04 21:34:00]

https://codereview.chromium.org/54623007/diff/290001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/54623007/diff/290001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:718: void
AddToObserved(std::set<aura::Window*> windows) {
const std::set&

https://codereview.chromium.org/54623007/diff/290001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:729: void
RemoveFromObserved(std::set<aura::Window*> windows) {
const std::set&

https://codereview.chromium.org/54623007/diff/290001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:741:
std::set<aura::Window*> RelativeComplement(std::set<aura::Window*> set_a,
const std::set&

[rharrison, 2013-11-06 19:26:16]

I have made all of the desired changes except for adding new tests. I am
planning on doing that in the next CL. No need to review again yet.

sadrul, what would be a better name/location for
WebContentsViewAuraWindowObserver*?

https://codereview.chromium.org/54623007/diff/290001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/54623007/diff/290001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:653: class
WebContentsViewAura::WindowObserver
On 2013/11/04 21:33:23, sky wrote:
> Can you move this into its own standalone object so that you can easily write
a
> test?

Done.

https://codereview.chromium.org/54623007/diff/290001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:683:
view_->touch_editable_->UpdateEditingController();
Talked to varunjain@ about this and I think it does. It has the same basic
requirement as sending screen rects, it needs to occur whenever the visible
bounds may have changed.

https://codereview.chromium.org/54623007/diff/290001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:718: void
AddToObserved(std::set<aura::Window*> windows) {
On 2013/11/04 21:34:01, sky wrote:
> const std::set&

Done.

https://codereview.chromium.org/54623007/diff/290001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:729: void
RemoveFromObserved(std::set<aura::Window*> windows) {
On 2013/11/04 21:34:01, sky wrote:
> const std::set&

Done.

https://codereview.chromium.org/54623007/diff/290001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:741:
std::set<aura::Window*> RelativeComplement(std::set<aura::Window*> set_a,
On 2013/11/04 21:33:23, sky wrote:
> Does std offer some function that does this?

Apparently it is std::set_difference... I probably would not have guessed that
on my own.

[sadrul, 2013-11-06 20:24:05]

https://codereview.chromium.org/54623007/diff/540001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/54623007/diff/540001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1203:
window_observer_.reset(new WebContentsViewAuraWindowObserver(this));
I was thinking of something more like a NativeViewScreenBoundsObserver. So,

  NativeViewSreenBoundsObserver:
    NativeViewScreenBoundsObserver(WebContentsView*)

    void OnScreenPositionChanged();
    void OnScreenBoundsChanged();

The NVSBO installs itself as a WindowObserver to the WCV::window_ and its
ancestor windows, and calls into OnScreenPositionChanged/OnScreenBoundsChanged
from the relevant callbacks. OnScreenPoisitionChanged() calls into
SendScreenRects(), and OnScreenBoundsChanged() calls into both SendScreenRects()
and touch-editable.

This NVSBO can live in web_contents/aura/, with its own _unittest.cc

WCVA simply creates this NVSBO here, and does not need to care about anything
else (so no delegate implementation etc.).

[rharrison, 2013-11-07 22:26:09]

PTAL, refactored code and added tests.

sadrul, I kept the delegate interface but moved the obsever out into its own
file. Without the delegate interface and instead just passing in the
WebContentsView, the NVSBO would have to do a lot of reaching into the view,
which couples those classes tightly and makes proper unit tests impossible.

[sadrul, 2013-11-08 02:16:25]

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
File content/browser/web_contents/aura/native_view_screen_bounds_observer.cc
(right):

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
content/browser/web_contents/aura/native_view_screen_bounds_observer.cc:95:
std::set<aura::Window*> NativeViewScreenBoundsObserver::GenerateAncestors(
Call this CollectAllWindowsToRoot (or something like that), and include |window|
in the returned set, instead of calling this function and then adding it
explicitly after the call.

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
File content/browser/web_contents/aura/native_view_screen_bounds_observer.h
(right):

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
content/browser/web_contents/aura/native_view_screen_bounds_observer.h:26:
virtual void RemoveRootWindowObserver(aura::RootWindowObserver* observer) = 0;
You don't need any of the above in this interface.

When you create the the NativeViewScreenBoundsObserver, give it the
aura::Window*, and the delegate.

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
content/browser/web_contents/aura/native_view_screen_bounds_observer.h:41:
virtual void Init(NativeViewScreenBoundsObserverDelegate* delegate);
This doesn't need to be virtual?

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
File
content/browser/web_contents/aura/native_view_screen_bounds_observer_unittest.cc
(right):

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
content/browser/web_contents/aura/native_view_screen_bounds_observer_unittest.cc:78:
return IsWindowInSet(window, observed());
Use aura::Window::HasObserver() to determine this

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
content/browser/web_contents/aura/native_view_screen_bounds_observer_unittest.cc:106:
EXPECT_EQ(static_cast<unsigned int>(0), observed().size());
EXPECT_EQ(0u, observed().size())

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
content/browser/web_contents/aura/native_view_screen_bounds_observer_unittest.cc:169:
EXPECT_TRUE(IsWindowObserved(new_parent_window));
Add some expectations on Position/BoundsChangedCount before/after re-parenting,
since that is the primary thing we want to test.

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
content/browser/web_contents/aura/native_view_screen_bounds_observer_unittest.cc:255:
TEST_F(NativeViewScreenBoundsObserverTest, RemoveFromObserved) {
I am not sure what we are testing in these last three tests.

[rharrison, 2013-11-11 19:41:57]

Unfortunately https://codereview.chromium.org/53153003 made significant changes
to ::WindowObserver, basically merging ::ChildWindowObserver into it. This is
going to require me to make significant changes to this CL. Hopefully I will
post an updated version tomorrow.

[rharrison, 2013-11-12 22:16:28]

jam, I have moved the NPAPI related parts of ::WindowObserver out into its own
class as part of refactoring ::WindowObserver, PTAL.
I also have some gypi changes that will need review.

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
File content/browser/web_contents/aura/native_view_screen_bounds_observer.cc
(right):

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
content/browser/web_contents/aura/native_view_screen_bounds_observer.cc:95:
std::set<aura::Window*> NativeViewScreenBoundsObserver::GenerateAncestors(
On 2013/11/08 02:16:26, sadrul wrote:
> Call this CollectAllWindowsToRoot (or something like that), and include
|window|
> in the returned set, instead of calling this function and then adding it
> explicitly after the call.

Done.

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
File content/browser/web_contents/aura/native_view_screen_bounds_observer.h
(right):

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
content/browser/web_contents/aura/native_view_screen_bounds_observer.h:26:
virtual void RemoveRootWindowObserver(aura::RootWindowObserver* observer) = 0;
On 2013/11/08 02:16:26, sadrul wrote:
> You don't need any of the above in this interface.
> 
> When you create the the NativeViewScreenBoundsObserver, give it the
> aura::Window*, and the delegate.

Done.

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
content/browser/web_contents/aura/native_view_screen_bounds_observer.h:41:
virtual void Init(NativeViewScreenBoundsObserverDelegate* delegate);
On 2013/11/08 02:16:26, sadrul wrote:
> This doesn't need to be virtual?

Done.

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
File
content/browser/web_contents/aura/native_view_screen_bounds_observer_unittest.cc
(right):

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
content/browser/web_contents/aura/native_view_screen_bounds_observer_unittest.cc:78:
return IsWindowInSet(window, observed());
On 2013/11/08 02:16:26, sadrul wrote:
> Use aura::Window::HasObserver() to determine this

Done.

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
content/browser/web_contents/aura/native_view_screen_bounds_observer_unittest.cc:106:
EXPECT_EQ(static_cast<unsigned int>(0), observed().size());
On 2013/11/08 02:16:26, sadrul wrote:
> EXPECT_EQ(0u, observed().size())

Done.

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
content/browser/web_contents/aura/native_view_screen_bounds_observer_unittest.cc:169:
EXPECT_TRUE(IsWindowObserved(new_parent_window));
On 2013/11/08 02:16:26, sadrul wrote:
> Add some expectations on Position/BoundsChangedCount before/after
re-parenting,
> since that is the primary thing we want to test.

Added additional additional cases in OnWindowBoundsChanged for this.

https://codereview.chromium.org/54623007/diff/600001/content/browser/web_cont...
content/browser/web_contents/aura/native_view_screen_bounds_observer_unittest.cc:255:
TEST_F(NativeViewScreenBoundsObserverTest, RemoveFromObserved) {
On 2013/11/08 02:16:26, sadrul wrote:
> I am not sure what we are testing in these last three tests.

I don't think they are adding more coverage, removing.

https://codereview.chromium.org/54623007/diff/850001/content/browser/web_cont...
File content/browser/web_contents/aura/constrained_windows_observer.h (right):

https://codereview.chromium.org/54623007/diff/850001/content/browser/web_cont...
content/browser/web_contents/aura/constrained_windows_observer.h:15: 
jam,
Since this class should go away with NPAPI support, is there a bug tracking that
effort or someone that owns it so I can put a proper TODO about that here?

[jam, 2013-11-13 20:47:53]

On 2013/11/12 22:16:28, rharrison wrote:
> jam, I have moved the NPAPI related parts of ::WindowObserver out into its own
> class as part of refactoring ::WindowObserver, PTAL.
> I also have some gypi changes that will need review.

I have added more stuff to this class in r234823 to fix M32 bugs. Now instead of
just watching the parent, we also watch the root window. Apologies that this is
conflicting with your changes.

I realize that we're going to remove all the NPAPI code in a year. Until then,
do we want separate observers? I ask because if you're watching all the parents
of a web contents view, and win aura also watches parent and root window, that
is a bit of duplication. Why not keep one observer?

[rharrison, 2013-11-13 21:05:54]

On 2013/11/13 20:47:53, jam wrote:
> On 2013/11/12 22:16:28, rharrison wrote:
> > jam, I have moved the NPAPI related parts of ::WindowObserver out into its
own
> > class as part of refactoring ::WindowObserver, PTAL.
> > I also have some gypi changes that will need review.
> 
> I have added more stuff to this class in r234823 to fix M32 bugs. Now instead
of
> just watching the parent, we also watch the root window. Apologies that this
is
> conflicting with your changes.
> 
> I realize that we're going to remove all the NPAPI code in a year. Until then,
> do we want separate observers? I ask because if you're watching all the
parents
> of a web contents view, and win aura also watches parent and root window, that
> is a bit of duplication. Why not keep one observer?


n/p, I understand it is a release blocking bug. Are there any other changes that
you foresee landing soon or should I be safe to rebase at this point?

My thought is to keep the NPAPI code that is going to be removed separated (in
another observer), so that when it comes time to remove it the process will be a
mechanical removal instead of having to untangle to the two behaviours.
Additionally this allows for relatively simple modular classes so I can bring
the NVSBO under test. I would like the NVSBO implementation to only contain code
that is expected to continue exist, so removal of the NPAPI code will have
little if any impact on it.

[jam, 2013-11-14 02:26:20]

On 2013/11/13 21:05:54, rharrison wrote:
> On 2013/11/13 20:47:53, jam wrote:
> > On 2013/11/12 22:16:28, rharrison wrote:
> > > jam, I have moved the NPAPI related parts of ::WindowObserver out into its
> own
> > > class as part of refactoring ::WindowObserver, PTAL.
> > > I also have some gypi changes that will need review.
> > 
> > I have added more stuff to this class in r234823 to fix M32 bugs. Now
instead
> of
> > just watching the parent, we also watch the root window. Apologies that this
> is
> > conflicting with your changes.
> > 
> > I realize that we're going to remove all the NPAPI code in a year. Until
then,
> > do we want separate observers? I ask because if you're watching all the
> parents
> > of a web contents view, and win aura also watches parent and root window,
that
> > is a bit of duplication. Why not keep one observer?
> 
> 
> n/p, I understand it is a release blocking bug. Are there any other changes
that
> you foresee landing soon or should I be safe to rebase at this point?

No changes that I know of. If we find out about any bugs in the next week or two
though, I'd have to make those changes on branch and trunk, and I wouldn't be
able to merge them.

> 
> My thought is to keep the NPAPI code that is going to be removed separated (in
> another observer), so that when it comes time to remove it the process will be
a
> mechanical removal instead of having to untangle to the two behaviours.

Honestly, I think that code is going to be easy to remove either way because
it's behind an OS_WIN ifdef...

> Additionally this allows for relatively simple modular classes so I can bring
> the NVSBO under test. 

That can happen either way, no?

> I would like the NVSBO implementation to only contain code
> that is expected to continue exist, so removal of the NPAPI code will have
> little if any impact on it.

Given that we're not deprecating NPAPI for over a year, it's unclear how any of
the code in chrome will look like at that time. I guess I have a slight
preference for having one observer per WebContentsViewAura, given that they'll
be watching the same windows. If you feel strongly about having multiple
observers, up to you.

[jam, 2013-11-14 17:28:20]

btw I thought about it more, and I don't feel strongly enough either way. I
think I was guilty of trying to do some premature optimizations. If you think
it's cleaner to have two different observers, it's your call.

[jam, 2013-11-14 18:56:12]

one more update: looks like there's one or two bugs related to this code that
have to be landed on trunk and then merged. so if you're splitting this code,
please hold off. sorry :(

[rharrison, 2013-11-14 19:00:32]

On 2013/11/14 18:56:12, jam wrote:
> one more update: looks like there's one or two bugs related to this code that
> have to be landed on trunk and then merged. so if you're splitting this code,
> please hold off. sorry :(

Thanks for letting me know, please CC on the CLs so I can know when they are
finalized.

[jam, 2013-11-15 17:52:36]

On 2013/11/14 19:00:32, rharrison wrote:
> On 2013/11/14 18:56:12, jam wrote:
> > one more update: looks like there's one or two bugs related to this code
that
> > have to be landed on trunk and then merged. so if you're splitting this
code,
> > please hold off. sorry :(
> 
> Thanks for letting me know, please CC on the CLs so I can know when they are
> finalized.

so far, r235267, r235305, and https://codereview.chromium.org/72283002/. I'm
really hoping there are no more.

After all the changes, the bookeeping involved in watching just the parent and
root window in win aura has become very complicated. it would be much simpler to
just always watch all parent windows, which you're doing afaik. in that case, it
seems better to have one observer instead of having two that do that work? i
have added a lot of tests for the win aura case over the last week, so changing
this code should hopefully be much less worrying now.

[rharrison, 2013-11-18 21:12:11]

Sync'd to ToT today and the specific issue that I was attempting to resolve has
been fixed as a side effect of jam's changes. I am going to abandon this, unless
the bug returns.

[rharrison, 2013-11-20 15:43:47]

On 2013/11/18 21:12:11, rharrison wrote:
> Sync'd to ToT today and the specific issue that I was attempting to resolve
has
> been fixed as a side effect of jam's changes. I am going to abandon this,
unless
> the bug returns.

It looks like the test I did of this was flawed or an annomaly. I am able to
reproduce the issue I was seeing consistently again. Re-opening the bug.

[rharrison, 2013-11-20 15:44:00]

On 2013/11/20 15:43:47, rharrison wrote:
> On 2013/11/18 21:12:11, rharrison wrote:
> > Sync'd to ToT today and the specific issue that I was attempting to resolve
> has
> > been fixed as a side effect of jam's changes. I am going to abandon this,
> unless
> > the bug returns.
> 
> It looks like the test I did of this was flawed or an annomaly. I am able to
> reproduce the issue I was seeing consistently again. Re-opening the bug.

err.. CL

[rharrison, 2013-11-20 16:40:54]

https://codereview.chromium.org/54623007/diff/850001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (left):

https://codereview.chromium.org/54623007/diff/850001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:678: // Constrained
windows are added as children of the parent's parent's view
jam, are the views that are of concern here being hard coded added as the
parent's parent's children or is it that they are actually the root view's
children and that happens to be the parent of the parent right now? Am I going
to have to make changes in other places for the NPAPI code since the NVHA CL I
am working on, https://codereview.chromium.org/48113013/, is going to add
another window to the hiearchy and that is going to break assumptions?

I am trying to figure out how to integrate this with my ancestor work at the
moment and I am not familiar with how the NPAPI code works.
I am thinking of three different sets of windows that could be the constrained
window set, can you clarify which is the correct one?

1) All children of windows about this up to the root, since they could
potentially overlap.
2) Just all children of the root window.
3) Children of the parent's parent, which is actually something different then
the root window.

[rharrison, 2013-11-20 21:20:22]

I have kept the separation between the two observers in this CL because to be
honest I find it hard to reason about what each one is doing when they are in a
single class. Once I have finished iterating on this CL I can merge them if that
is essential to landing.

For the CVO my understanding of what exists in the current code is tracking the
children of the RootWindow and the siblings of the window associated with the
WebContentsView. I have tried to preserve this as much as possible. I am going
to add to the NVHA CL adjusting the value of constrained_parent_ in CVO to be
the parent of the clipping_window_, since this should get the same tracking
siblings behaviour.

What tests should I be running to confirm that I haven't broken the NPAPI code?

[jam, 2013-11-21 18:32:30]

Right now, constrained windows are peers of WebContentsView or children of the
root window, depending on the the compositing mode. It looks like your change
needs to be updated from trunk, i.e. the UpdateConstrainedWindows method in your
new class doesn't have the enumeration of the root window's children.

If you look at all the changes that have happened to the NPAPI case (see all the
cls in crbug.com/299224), you'll see how complicated that code has become in
order to watch the root window and the parent. There are all these corner cases
when the parent becomes the root window and switches to something else. It would
be much easier to read the code now if the observer just watched all the parent
windows. It sounds like that's what you're doing in your observer. So we can
leave the constrained window observer code in a separate one, and it can be very
hard to read as it is right now, or we can merge them. I prefer the latter at
this point. If I could write the constrained window watching code again, that's
what I would do, but for the M32 fixes I didn't want to rewrite it so that
merges are less risky.

As for tests, here are the ones that I added:
browser_tests: PrintPreviewTest.WindowedNPAPIPluginHidden
interactive_ui_tests: StarViewTestNoDWM.WindowedNPAPIPluginHidden

[rharrison, 2013-11-21 18:40:52]

On 2013/11/21 18:32:30, jam wrote:
> Right now, constrained windows are peers of WebContentsView or children of the
> root window, depending on the the compositing mode. It looks like your change
> needs to be updated from trunk, i.e. the UpdateConstrainedWindows method in
your
> new class doesn't have the enumeration of the root window's children.
> 
> If you look at all the changes that have happened to the NPAPI case (see all
the
> cls in crbug.com/299224), you'll see how complicated that code has become in
> order to watch the root window and the parent. There are all these corner
cases
> when the parent becomes the root window and switches to something else. It
would
> be much easier to read the code now if the observer just watched all the
parent
> windows. It sounds like that's what you're doing in your observer. So we can
> leave the constrained window observer code in a separate one, and it can be
very
> hard to read as it is right now, or we can merge them. I prefer the latter at
> this point. If I could write the constrained window watching code again,
that's
> what I would do, but for the M32 fixes I didn't want to rewrite it so that
> merges are less risky.
> 
> As for tests, here are the ones that I added:
> browser_tests: PrintPreviewTest.WindowedNPAPIPluginHidden
> interactive_ui_tests: StarViewTestNoDWM.WindowedNPAPIPluginHidden

ok, I think I understand things now. I will merge the two observers, since I
agree that this would make the code cleaner/simpler.

[rharrison, 2013-11-21 20:20:29]

PTAL, I have merged the two classes. I still have to run the tests to confirm
everything is good, but at least a high level review would be appreciated.

[jam, 2013-11-23 01:59:15]

On 2013/11/21 20:20:29, rharrison wrote:
> PTAL, I have merged the two classes. I still have to run the tests to confirm
> everything is good, but at least a high level review would be appreciated.

I'll wait till you have the tests passing on Windows, since that could change
what the reviewed files look like

[jam, 2013-11-23 01:59:33]

On 2013/11/23 01:59:15, jam wrote:
> On 2013/11/21 20:20:29, rharrison wrote:
> > PTAL, I have merged the two classes. I still have to run the tests to
confirm
> > everything is good, but at least a high level review would be appreciated.
> 
> I'll wait till you have the tests passing on Windows, since that could change
> what the reviewed files look like

and to be very clear: this can happen on the trybots

[rharrison, 2013-11-26 04:52:04]

On 2013/11/23 01:59:33, jam wrote:
> On 2013/11/23 01:59:15, jam wrote:
> > On 2013/11/21 20:20:29, rharrison wrote:
> > > PTAL, I have merged the two classes. I still have to run the tests to
> confirm
> > > everything is good, but at least a high level review would be appreciated.
> > 
> > I'll wait till you have the tests passing on Windows, since that could
change
> > what the reviewed files look like
> 
> and to be very clear: this can happen on the trybots

PTAL, the CL is passing for sure on 2 out of 3 windows bots and I am pretty sure
the third isn't due to the CL.

[rharrison, 2013-11-26 16:58:54]

On 2013/11/26 04:52:04, rharrison wrote:
> On 2013/11/23 01:59:33, jam wrote:
> > On 2013/11/23 01:59:15, jam wrote:
> > > On 2013/11/21 20:20:29, rharrison wrote:
> > > > PTAL, I have merged the two classes. I still have to run the tests to
> > confirm
> > > > everything is good, but at least a high level review would be
appreciated.
> > > 
> > > I'll wait till you have the tests passing on Windows, since that could
> change
> > > what the reviewed files look like
> > 
> > and to be very clear: this can happen on the trybots
> 
> PTAL, the CL is passing for sure on 2 out of 3 windows bots and I am pretty
sure
> the third isn't due to the CL.

Additionally the linux_asan failure does not reproduce for me locally.

[jam, 2013-11-27 01:17:57]

On 2013/11/26 04:52:04, rharrison wrote:
> On 2013/11/23 01:59:33, jam wrote:
> > On 2013/11/23 01:59:15, jam wrote:
> > > On 2013/11/21 20:20:29, rharrison wrote:
> > > > PTAL, I have merged the two classes. I still have to run the tests to
> > confirm
> > > > everything is good, but at least a high level review would be
appreciated.
> > > 
> > > I'll wait till you have the tests passing on Windows, since that could
> change
> > > what the reviewed files look like
> > 
> > and to be very clear: this can happen on the trybots
> 
> PTAL, the CL is passing for sure on 2 out of 3 windows bots and I am pretty
sure
> the third isn't due to the CL.

The two tests I added (StarViewTestNoDWM.WindowedNPAPIPluginHidden and
FindInPageControllerTest.WindowedNPAPIPluginHidden) are failing, which indicates
that something went wrong in the porting of the code.

[rharrison, 2013-11-27 04:15:34]

On 2013/11/27 01:17:57, jam wrote:
> On 2013/11/26 04:52:04, rharrison wrote:
> > On 2013/11/23 01:59:33, jam wrote:
> > > On 2013/11/23 01:59:15, jam wrote:
> > > > On 2013/11/21 20:20:29, rharrison wrote:
> > > > > PTAL, I have merged the two classes. I still have to run the tests to
> > > confirm
> > > > > everything is good, but at least a high level review would be
> appreciated.
> > > > 
> > > > I'll wait till you have the tests passing on Windows, since that could
> > change
> > > > what the reviewed files look like
> > > 
> > > and to be very clear: this can happen on the trybots
> > 
> > PTAL, the CL is passing for sure on 2 out of 3 windows bots and I am pretty
> sure
> > the third isn't due to the CL.
> 
> The two tests I added (StarViewTestNoDWM.WindowedNPAPIPluginHidden and
> FindInPageControllerTest.WindowedNPAPIPluginHidden) are failing, which
indicates
> that something went wrong in the porting of the code.

Looking at the failures, it looks like the failure is due to an assert which is
trying to confirm that the the region associated with the NPAPI window is simple
v. complex. My understanding is that this means that the window associated with
the plugin is not a rectangle, which it should be. I am assuming this means that
the set of constrained windows that are being sent is incorrect and is causing
some funky shaped cut out. I am going to look at this tomorrow, but if it is
obvious to you what is going wrong or if I am heading down an incorrect path
please let me know.

[jam, 2013-11-27 17:13:50]

On 2013/11/27 04:15:34, rharrison wrote:
> On 2013/11/27 01:17:57, jam wrote:
> > On 2013/11/26 04:52:04, rharrison wrote:
> > > On 2013/11/23 01:59:33, jam wrote:
> > > > On 2013/11/23 01:59:15, jam wrote:
> > > > > On 2013/11/21 20:20:29, rharrison wrote:
> > > > > > PTAL, I have merged the two classes. I still have to run the tests
to
> > > > confirm
> > > > > > everything is good, but at least a high level review would be
> > appreciated.
> > > > > 
> > > > > I'll wait till you have the tests passing on Windows, since that could
> > > change
> > > > > what the reviewed files look like
> > > > 
> > > > and to be very clear: this can happen on the trybots
> > > 
> > > PTAL, the CL is passing for sure on 2 out of 3 windows bots and I am
pretty
> > sure
> > > the third isn't due to the CL.
> > 
> > The two tests I added (StarViewTestNoDWM.WindowedNPAPIPluginHidden and
> > FindInPageControllerTest.WindowedNPAPIPluginHidden) are failing, which
> indicates
> > that something went wrong in the porting of the code.
> 
> Looking at the failures, it looks like the failure is due to an assert which
is
> trying to confirm that the the region associated with the NPAPI window is
simple
> v. complex. My understanding is that this means that the window associated
with
> the plugin is not a rectangle, which it should be. I am assuming this means
that
> the set of constrained windows that are being sent is incorrect and is causing
> some funky shaped cut out. I am going to look at this tomorrow, but if it is
> obvious to you what is going wrong or if I am heading down an incorrect path
> please let me know.

Looking at the error output, say
http://build.chromium.org/p/tryserver.chromium/builders/win_swarm_triggered/b...,
it's expecting SIMPLEREGION but instead gets NULLREGION. That means that the
plugin isn't even shown.

I patched your change and can confirm that the plugin dissappears after a rezie
of the browser window, and that also it's not covered up by print preview or
other dialogs like the find-in-page.

[rharrison, 2013-11-27 17:43:23]

On 2013/11/27 17:13:50, jam wrote:
> On 2013/11/27 04:15:34, rharrison wrote:
> > On 2013/11/27 01:17:57, jam wrote:
> > > On 2013/11/26 04:52:04, rharrison wrote:
> > > > On 2013/11/23 01:59:33, jam wrote:
> > > > > On 2013/11/23 01:59:15, jam wrote:
> > > > > > On 2013/11/21 20:20:29, rharrison wrote:
> > > > > > > PTAL, I have merged the two classes. I still have to run the tests
> to
> > > > > confirm
> > > > > > > everything is good, but at least a high level review would be
> > > appreciated.
> > > > > > 
> > > > > > I'll wait till you have the tests passing on Windows, since that
could
> > > > change
> > > > > > what the reviewed files look like
> > > > > 
> > > > > and to be very clear: this can happen on the trybots
> > > > 
> > > > PTAL, the CL is passing for sure on 2 out of 3 windows bots and I am
> pretty
> > > sure
> > > > the third isn't due to the CL.
> > > 
> > > The two tests I added (StarViewTestNoDWM.WindowedNPAPIPluginHidden and
> > > FindInPageControllerTest.WindowedNPAPIPluginHidden) are failing, which
> > indicates
> > > that something went wrong in the porting of the code.
> > 
> > Looking at the failures, it looks like the failure is due to an assert which
> is
> > trying to confirm that the the region associated with the NPAPI window is
> simple
> > v. complex. My understanding is that this means that the window associated
> with
> > the plugin is not a rectangle, which it should be. I am assuming this means
> that
> > the set of constrained windows that are being sent is incorrect and is
causing
> > some funky shaped cut out. I am going to look at this tomorrow, but if it is
> > obvious to you what is going wrong or if I am heading down an incorrect path
> > please let me know.
> 
> Looking at the error output, say
>
http://build.chromium.org/p/tryserver.chromium/builders/win_swarm_triggered/b...,
> it's expecting SIMPLEREGION but instead gets NULLREGION. That means that the
> plugin isn't even shown.
> 
> I patched your change and can confirm that the plugin dissappears after a
rezie
> of the browser window, and that also it's not covered up by print preview or
> other dialogs like the find-in-page.

Thanks for catching that it is reporting NULLREGION, and not COMPLEXREGION.

[rharrison, 2013-11-28 19:57:59]

I am having a real hard time getting the NPAPI tests to pass. Specifically it
appears that the code is now adding a constrained rectangle for the entire
browser window immediately after creating the observer, which obviously causes
the entire plugin to be clipped. I also think that are some cases are being
missed wrt siblings.

I am going to see if the two observer design passes the tests. If so I am going
to recommend landing that and there be a clean bug and CL for merging the two
observers created, since this is blocking the scroll end effect work I am
handing off.

[rharrison, 2013-11-29 15:53:17]

Well that experiment was a bit of a bust :-/

I am going to be on leave/vacation for December while I work on my thesis and
for the winter holidays. In the new year I am expecting to be leaving the Chrome
team, so I won't be working on this CL anymore. There are people in WAT that are
going to be taking over the scroll end effect work. I am leaving this CL up for
them to reference, but there will be a new version from one of them in the
future.