Don't capture screenshots for in-page navigations.

Don't capture screenshots for in-page navigations.

This is an optimization to speed up in-page navigations. We want those to be
blazingly fast, and currently the pixel readback done while taking a
screenshot takes 40ms on a GPU on Pixel. This may be reverted once
screenshotting is optimizaed.

Also changed the background color shown bu GestureNav when there is no
screenshot available from gray to white to make it look less broken.

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

[mfomitchev, 2014-07-25 21:51:37]

Urgent small patch for M37 - can you please review?

[sadrul, 2014-07-25 21:54:25]

lgtm

[Charlie Reis, 2014-07-25 22:07:40]

Typos in CL description: optimizaed, bu GestureNav

https://codereview.chromium.org/417343003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/417343003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:451: if
(new_url.ReplaceComponents(clear_ref) ==
We probably shouldn't add yet another way to detect in-page navigations.  For
example, this would miss pushState navigations.

In this case, params.was_within_same_page is probably the right thing to use.  I
seem to remember that it doesn't handle pushState either, but there was some
discussion of fixing it and then it would be fixed for this use case as well.

https://codereview.chromium.org/417343003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/417343003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:567: // and
histrory.pushState do not capture screenshots.
history

https://codereview.chromium.org/417343003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:590:
EXPECT_FALSE(screenshot_manager()->screenshot_taken_for());
How did this pass?  pushState changes the URL path (even when ignoring the ref).

[sky, 2014-07-25 22:08:06]

https://codereview.chromium.org/417343003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/417343003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:444: bool same_page_navigation =
false;
Is this the same as params.was_within_same_page?

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

https://codereview.chromium.org/417343003/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/image_window_delegate.cc:69:
canvas->DrawColor(SK_ColorWHITE);
Why are we changing colors?

[mfomitchev, 2014-07-25 22:29:15]

https://codereview.chromium.org/417343003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/417343003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:444: bool same_page_navigation =
false;
Please see my reply to creis@ below.

https://codereview.chromium.org/417343003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:451: if
(new_url.ReplaceComponents(clear_ref) ==
Ok, so the pushState test does fail if I use params.was_within_same_page. If I
leave the raw URL comparison, it passes. The URL in that case doesn't change at
all:

new_url=http://127.0.0.1:45820/files/overscroll_navigation.html, 
cur_url=http://127.0.0.1:45820/files/overscroll_navigation.html

Here's push_state() function:
function use_push_state() {
  window.history.pushState({}, 'foo2');
}

Does this make sense?
If using URL comparison covers pushState, while was_within_same_page doesn't
would we want to keep the current way, or still change it?

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

https://codereview.chromium.org/417343003/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/image_window_delegate.cc:69:
canvas->DrawColor(SK_ColorWHITE);
Kuscher asked for this. Testing it out, I agree with him that it does look
better with white for GMail, which is our main concern here.

https://codereview.chromium.org/417343003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/417343003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:567: // and
histrory.pushState do not capture screenshots.
On 2014/07/25 22:07:40, Charlie Reis wrote:
> history

Done.

https://codereview.chromium.org/417343003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:590:
EXPECT_FALSE(screenshot_manager()->screenshot_taken_for());
responded above

[sky, 2014-07-25 22:44:49]

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

https://codereview.chromium.org/417343003/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/image_window_delegate.cc:69:
canvas->DrawColor(SK_ColorWHITE);
On 2014/07/25 22:29:15, mfomitchev wrote:
> Kuscher asked for this. Testing it out, I agree with him that it does look
> better with white for GMail, which is our main concern here.

Won't this impact *all* sites though? And won't it impact windows? My main
concern here is your desire to merge to 37. I'm fine with landing this on trunk,
but I'm not so happy about the desire to merge to 37 until this shakes out for a
while. How about separating the two patches out?

[Charlie Reis, 2014-07-25 22:45:28]

[+japhet for pushState / in-page fun]

https://codereview.chromium.org/417343003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/417343003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:451: if
(new_url.ReplaceComponents(clear_ref) ==
On 2014/07/25 22:29:15, mfomitchev wrote:
> Ok, so the pushState test does fail if I use params.was_within_same_page. If I
> leave the raw URL comparison, it passes. The URL in that case doesn't change
at
> all:
> 
> new_url=http://127.0.0.1:45820/files/overscroll_navigation.html, 
> cur_url=http://127.0.0.1:45820/files/overscroll_navigation.html
> 
> Here's push_state() function:
> function use_push_state() {
>   window.history.pushState({}, 'foo2');
> }

That's kind of a no-op example for pushState, though, since it doesn't change
the URL.  If you try it with the following I would expect your test to fail:
window.history.pushState({}, 'foo2', 'newpath');


> Does this make sense?
> If using URL comparison covers pushState, while was_within_same_page doesn't
> would we want to keep the current way, or still change it?

I'd like to have fewer ways to compute whether a navigation is in-page or not. 
It's already too complicated inside NavigationController, where we have complex
logic around {Is,Are}URLsInPageNavigation, which ends up in
LoadCommittedDetails::is_in_page below.  (This came up in
http://crbug.com/138324.)

I suggest using was_within_same_page or calling IsURLInPageNavigation, unless
japhet@ has a better idea.

[mfomitchev, 2014-07-25 23:09:58]

https://codereview.chromium.org/417343003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/417343003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:451: if
(new_url.ReplaceComponents(clear_ref) ==
Yep, it fails. Changing it to use params.was_within_same_page. Thanks! Is there
a bug for pushState/was_within_same_page which I can quote in the browsertest?

It appears was_within_same_page is also set to false when I reload the page. So
that's another test I am going to disable. Would that be fixed as well?

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

https://codereview.chromium.org/417343003/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/image_window_delegate.cc:69:
canvas->DrawColor(SK_ColorWHITE);
Yes it would. I think it would look better for any site that uses white
background. It would look worse for sites that use gray background. For other
sites I don't think it would make a difference. And there's more sites that use
white background than gray background...
What's your concern with using the white background? 

Either way, I will separate the background patch out.

[Charlie Reis, 2014-07-25 23:55:55]

LGTM.  I don't think there's a bug on file yet.  Nate, can you confirm that we
want the renderer to report true for reloads and pushState in
was_within_same_page, or am I mistaken?  If that's true, we should file a bug
and cite it here.

https://codereview.chromium.org/417343003/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/417343003/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:412: //
navigations
nit: End with period.

[mfomitchev, 2014-07-26 01:50:58]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-07-26 05:00:28]

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

[mfomitchev1, 2014-07-26 18:55:37]

The CQ bit was unchecked by mfomitchev@google.com

[mfomitchev1, 2014-07-26 18:55:40]

The CQ bit was checked by mfomitchev@google.com

[commit-bot: I haz the power, 2014-07-26 18:55:59]

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

[commit-bot: I haz the power, 2014-07-26 19:58:20]

Change committed as 285797