Removing listening for repaints (OnUpdateRect) from OverscrollNavigationOverlay.

The overlay is now dismissed after the first non-empty paint or the page load, whichever comes first. Removed all listening for repaints (OnUpdateRect).

Also updated the code for overlay fadeout to avoid the need to repaint the overlay layer before the animation is started - we now fadeout the WindowSlider's layer if it was the one being shown when the fadeout was started.

BUG=NONE

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273961

[mfomitchev, 2014-05-14 17:46:34]

Sadrul, can you please take a look? Thanks!

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

https://codereview.chromium.org/278173005/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:215:
RenderViewHostTester::TestOnMessageReceived(rvh, msg);
It would be much better if we could actually enable a real paint to occur
instead of faking it. Is there any way I can do it?

[sadrul, 2014-05-14 18:20:46]

I seem to remember we don't get a first-paint message for in-page navigations.
Can you confirm that this works correctly for these cases?

[mfomitchev, 2014-05-14 19:34:37]

You are right, it's broken for GMail.
WHat do you think is the best way to fix this?
I guess in those cases it would be best to dismiss the overlay on the first
paint? Perhaps we should bring back something along the lines of PaintObserver?
If so, it would be nice to hook into DidFlushPaint() somehow (or a method which
is guaranteed to be called as a result of that) instead of sprinkling a bunch of
callback in a bunch of different places though.. Also, I see that RWHVA has
changed quite a bit since PaintObserver was removed. A bunch of methods we had
hooks in a re not even there anymore.

PaintObserver removal so you don't have to search for it:
https://codereview.chromium.org/153913004/

[sadrul, 2014-05-15 21:36:35]

On 2014/05/14 19:34:37, mfomitchev wrote:
> You are right, it's broken for GMail.
> WHat do you think is the best way to fix this?
> I guess in those cases it would be best to dismiss the overlay on the first
> paint? Perhaps we should bring back something along the lines of
PaintObserver?
> If so, it would be nice to hook into DidFlushPaint() somehow (or a method
which
> is guaranteed to be called as a result of that) instead of sprinkling a bunch
of
> callback in a bunch of different places though.. Also, I see that RWHVA has
> changed quite a bit since PaintObserver was removed. A bunch of methods we had
> hooks in a re not even there anymore.
> 
> PaintObserver removal so you don't have to search for it:
> https://codereview.chromium.org/153913004/

PaintObserver as it was wasn't great, and was relatively flaky. It may be better
to have some 'first paint after navigation' notification instead. Although
paint-notification may not be good enough, e.g. what if the in-page navigation
does not actually change the content at all?

[mfomitchev, 2014-05-15 21:45:50]

On 2014/05/15 21:36:35, sadrul wrote:
> PaintObserver as it was wasn't great, and was relatively flaky. It may be
better
> to have some 'first paint after navigation' notification instead. Although
> paint-notification may not be good enough, e.g. what if the in-page navigation
> does not actually change the content at all?

Ok, so sounds like we need to keep DocumentOnLoadCompletedInMainFrame(). I think
we can still get rid of DidStopLoading and OnUpdateRect?
It would be nice to have the "first paint after navigation" notification so that
we can dismiss the overlay quicker for in-page navigations. Do you have any
thoughts on how to do that? ViewHostMsg_DidFirstVisuallyNonEmptyPaint is sent
from RenderViewImpl::DidFlushPaint(), could we perhaps do something similar
here?

[sadrul, 2014-05-20 16:58:09]

On 2014/05/15 21:45:50, mfomitchev wrote:
> On 2014/05/15 21:36:35, sadrul wrote:
> > PaintObserver as it was wasn't great, and was relatively flaky. It may be
> better
> > to have some 'first paint after navigation' notification instead. Although
> > paint-notification may not be good enough, e.g. what if the in-page
navigation
> > does not actually change the content at all?
> 
> Ok, so sounds like we need to keep DocumentOnLoadCompletedInMainFrame(). I
think
> we can still get rid of DidStopLoading and OnUpdateRect?
> It would be nice to have the "first paint after navigation" notification so
that
> we can dismiss the overlay quicker for in-page navigations. Do you have any
> thoughts on how to do that? ViewHostMsg_DidFirstVisuallyNonEmptyPaint is sent
> from RenderViewImpl::DidFlushPaint(), could we perhaps do something similar
> here?

Perhaps. We would need to maintain the right set of information to identify a
'first-paint-after-nav'. This probably still wouldn't address the issue where
in-page nav doesn't trigger a paint at all?

[mfomitchev, 2014-05-21 17:37:23]

Ok, I just got rid of OnUpdateRect (and that was actually a bit more tricky than
I expected because of the fadeout). I think this is fine for now - we may be
able to dismiss a bit quicker for in-page navigations that do trigger paints,
but I'd expect most in-page navigations to complete the full load much quicker
than inter-page navigations.

[sadrul, 2014-05-27 16:48:28]

https://codereview.chromium.org/278173005/diff/20001/content/browser/web_cont...
File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (left):

https://codereview.chromium.org/278173005/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:304: void
OverscrollNavigationOverlay::DocumentOnLoadCompletedInMainFrame() {
This is related to document loading. Why are we removing this? (IIRC this allows
removing the overlay earlier?)

https://codereview.chromium.org/278173005/diff/20001/content/browser/web_cont...
File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right):

https://codereview.chromium.org/278173005/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:176:
overlay_dismiss_layer = overlay_dismiss_layer_.Pass();
Does this work correctly?

From what I can see, |overlay_dismiss_layer_| is parented to |window_|'s layer.
If you destroy |window_| below in line 181, then |overlay_dismiss_layer| would
become un-parented, and the layer wouldn't show up. Is that not the case?

https://codereview.chromium.org/278173005/diff/20001/content/browser/web_cont...
File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right):

https://codereview.chromium.org/278173005/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/overscroll_navigation_overlay.h:71: // if the
the page has been painted or the page-load has completed, and a
s/the the/the/

[mfomitchev, 2014-05-27 21:21:05]

https://codereview.chromium.org/278173005/diff/20001/content/browser/web_cont...
File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (left):

https://codereview.chromium.org/278173005/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:304: void
OverscrollNavigationOverlay::DocumentOnLoadCompletedInMainFrame() {
It was helping dismiss the page sooner when we required both a paint and a load,
but now that the logic changed I don't think we want this:
- for inter-page navigation we want to dismiss the overlay after the first paint
happens. If this method is somehow called before that (I couldn't make that
happen btw), we still want to wait until the paint. 
- For in-page navigation I don't see this called at all - I guess because the
document has already been loaded and isn't being reloaded again. I just see
DidStopLoading. 

So I didn't think there was any need to keep this.

https://codereview.chromium.org/278173005/diff/20001/content/browser/web_cont...
File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right):

https://codereview.chromium.org/278173005/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:176:
overlay_dismiss_layer = overlay_dismiss_layer_.Pass();
It works. In this case overlay_dismiss_layer_ is the slider's layer. I don't
think window's layer is the parent of slider window's layer. It's parent is the
parent of window's layer. 
From SetupSlider():
ui::Layer* parent = owner_->layer()->parent();
..
parent->StackAbove(slider_.get(), owner_->layer());

https://codereview.chromium.org/278173005/diff/20001/content/browser/web_cont...
File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right):

https://codereview.chromium.org/278173005/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/overscroll_navigation_overlay.h:71: // if the
the page has been painted or the page-load has completed, and a
On 2014/05/27 16:48:29, sadrul wrote:
> s/the the/the/

Done.

[sadrul, 2014-05-30 15:33:12]

LGTM!

[mfomitchev, 2014-05-30 15:34:44]

@sky - can you please review as the owner? Thanks!

[sky, 2014-05-30 16:40:40]

LGTM

[mfomitchev, 2014-05-30 17:40:49]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-05-30 17:46:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/278173005/40001

[commit-bot: I haz the power, 2014-05-31 00:40:25]

Change committed as 273961