Making sure we don't take a GestureNav screenshot unless the history position changes.

Making sure we don't take a GestureNav screenshot unless the history position changes.

Only take a screenshot if the page id changes.

Looking at NavigationControllerImpl::ClassifyNavigation(), I think this is the
right check. If we could move the code for taking the screenshot post
controller_->RendererDidNavigate() call, we'd be able to use the NavigationType
calculated in that method, but based on the comments for the screenshot code,
this doesn't seem like a safe thing to do.


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

[mfomitchev, 2014-06-03 22:36:56]

nasko, japhet: Please review. Thanks!
sadrul: FYI

[Nate Chapin, 2014-06-03 23:13:17]

On 2014/06/03 22:36:56, mfomitchev wrote:
> nasko, japhet: Please review. Thanks!
> sadrul: FYI

LGTM, but I'm not an owner.

[nasko, 2014-06-03 23:18:15]

avi@ is actively working on containing usage of page_id. I think this case is
fine, but adding him as heads-up.

[sadrul, 2014-06-04 05:02:37]

A couple of questions: (1) does the page-id change for in-page navigations? (2)
Can we plumb through the info that the navigation is happening in response to a
push or replace history-api call, and not take the screenshot when that is the
case?

[nasko, 2014-06-04 17:17:38]

On 2014/06/04 05:02:37, sadrul wrote:
> A couple of questions: (1) does the page-id change for in-page navigations?
(2)
> Can we plumb through the info that the navigation is happening in response to
a
> push or replace history-api call, and not take the screenshot when that is the
> case?

1. Any navigation that adds a history entry will change the page id.
2. pushState() creates a history entry and needs to have a screenshot created.
In general, any history entry added should have a screenshot created, otherwise
overscroll has nothing to show for history navigation.

[mfomitchev, 2014-06-04 18:06:32]

Also, replaceState will not result in the page id change, so we have that
covered.

[nasko, 2014-06-04 18:53:47]

The code itself looks good with a couple of nits. It will be good to write a
test case for this, so we don't accidentally regress it, especially with all the
upcoming work on session history refactoring.

https://codereview.chromium.org/316553007/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/316553007/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:430: int32 current_page_id =
current_entry ? current_entry->GetPageID() : -1;
nit: it will be a bit more readable with an empty line following.

https://codereview.chromium.org/316553007/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:433: // screenshot.
nit: For back/forward navigations we still want to take screenshots, so what
this is trying to single out is exactly the case where the same history entry is
used. The comment as is implies that only going back to the previous history
entry or new navigations are of interest.

[mfomitchev, 2014-06-04 19:26:20]

Given that this is a stable blocker, would you be okay with landing the unit
test in a separate CL? I would create an issue to track and should implement the
unit test before the end of the next week.

https://codereview.chromium.org/316553007/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/316553007/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:430: int32 current_page_id =
current_entry ? current_entry->GetPageID() : -1;
On 2014/06/04 18:53:48, nasko wrote:
> nit: it will be a bit more readable with an empty line following.

Done.

https://codereview.chromium.org/316553007/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:433: // screenshot.
Reworded

[nasko, 2014-06-04 19:48:37]

I'm OK with a test in a separate CL if this needs to be merged into stable.
LGTM

[mfomitchev, 2014-06-04 19:49:05]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-06-04 19:50:39]

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

[mfomitchev, 2014-06-04 19:52:56]

Cool! crbug.com/380830 created for the unit test.

[mfomitchev, 2014-06-04 20:07:00]

The CQ bit was unchecked by mfomitchev@chromium.org

[mfomitchev, 2014-06-04 20:13:44]

@nasko - I've rearranged the code a bit so that we don't execute unnecessary
code when GestureNav is not enabled, and also so that I can tell the PMs that
this CL has no impact on non-aura builds. Does it still look okay?

[nasko, 2014-06-04 20:23:41]

Still LGTM

[mfomitchev, 2014-06-04 20:24:05]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-06-04 20:26:09]

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

[commit-bot: I haz the power, 2014-06-04 23:44:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-04 23:52:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-04 23:52:04]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[mfomitchev, 2014-06-05 00:23:34]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-06-05 00:25:51]

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

[commit-bot: I haz the power, 2014-06-05 01:53:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-05 01:57:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-05 01:57:56]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[mfomitchev, 2014-06-05 02:07:47]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-06-05 02:10:43]

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

[commit-bot: I haz the power, 2014-06-05 05:46:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-05 06:45:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-05 06:45:38]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[mfomitchev, 2014-06-05 18:55:33]

When the navigation is performed by entering the url into the omnibox, the page
id doesn't change, however we should capture the screenshot. Addressing this
case by also checking if the URL changed or not.

[Avi (use Gerrit), 2014-06-05 19:39:53]

My thoughts on reading this...

There is a FrameHostMsg_DidCommitProvisionalLoad_Params. Why not use the
|was_within_same_page| value?

[Avi (use Gerrit), 2014-06-05 19:45:24]

This looks reasonable to me as a fix for an immediate problem. LGTM

But I am creeped out by the fact, though, that this is a chunk of intricate
logic about how to take a screenshot that lives inside the navigation code. Is
there a way to improve this?

[mfomitchev, 2014-06-05 19:46:16]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-06-05 19:47:54]

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

[mfomitchev, 2014-06-05 19:59:55]

Agreed. Do you think we should try to tackle this now, or perhaps wait until
creis@ comes back? Also, I am wondering if site isolation make any of this
easier?

I am not an expert in this area, but here are my 2c:
- On the other side of the IPC, I think we can tell if this is a navigation we
want to ignore simply base on the commit type. If the commit type is
WebHistoryInertCommit, I don't think we need to take a screenshot. Would sending
the commit type across IPC or encapsulating it in some other form be worth
exploring?
- We can also determine whether we need to take the screenshot based on the
NavigationType. Is there any way we can calculate it before calling
DidNavigateFrame()?

[commit-bot: I haz the power, 2014-06-05 23:25:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-06 04:52:30]

Change committed as 275324