Add item and document sequence numbers to FrameNavigationEntry.

Add item and document sequence numbers to FrameNavigationEntry.

This will be used for identifying which frames need to navigate
on back/forward navigations in --site-per-process.

BUG=236848
TEST=No behavior change yet.

Committed: https://crrev.com/8b5cd4c7d51dec913908d91dbc19bedda293e708
Cr-Commit-Position: refs/heads/master@{#335173}

[Charlie Reis, 2015-06-17 22:56:22]

creis@chromium.org changed reviewers:
+ avi@chromium.org

[Charlie Reis, 2015-06-17 22:56:23]

Avi, can you take a look?  This is one piece I know I'll need for porting
GoToEntry from HistoryController to NavigationController, since it's used to
determine which frames need to navigate (and which of those are same document vs
different document).

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:811: bool
is_site_per_process = base::CommandLine::ForCurrentProcess()->HasSwitch(
I'll undo this.  (I was using it below in an earlier version of the patch.)

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1587: //
TODO(creis): Add a subframe test.
I'll add this as well.

(I was debating checking these in the existing NavigationTypeClassification
tests since those are pretty exhaustive on the types of navigations, but it also
felt like it would pollute them with checks unrelated to the classifier.  Not
sure what the best practice here should be for features that should be tested
across all types of navigations.)

[Avi (use Gerrit), 2015-06-18 00:57:01]

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_navigation_entry.h (right):

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_navigation_entry.h:30: // frame sequence
numbers or unique names would work instead, similar to
How is this TODO changed, with its reference to sequence numbers, if you are
introducing them below?

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:811: bool
is_site_per_process = base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/06/17 22:56:23, Charlie Reis (OOO soon) wrote:
> I'll undo this.  (I was using it below in an earlier version of the patch.)

Acknowledged.

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1572:
EXPECT_NE(-1, dsn_1);
-1? I would have thought that would be a "not assigned" value, not one you'd see
for a committed load.

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1585:
EXPECT_EQ(dsn_1, dsn_2);
So document sequence numbers are sometimes shared across navigation entries. Is
there a design document of some kind explaining what the larger picture here is?

https://codereview.chromium.org/1184423005/diff/1/content/public/common/frame...
File content/public/common/frame_navigate_params.h (right):

https://codereview.chromium.org/1184423005/diff/1/content/public/common/frame...
content/public/common/frame_navigate_params.h:45: 
If these are generated deep in Blink in a renderer process (coming off of
WebHistoryItem), how in the world are they supposed to end up being "globally
unique"?

[Charlie Reis, 2015-06-18 20:29:47]

Patchset #2 (id:20001) has been deleted

[Charlie Reis, 2015-06-18 20:30:18]

Thanks!  New patch up.

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_navigation_entry.h (right):

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_navigation_entry.h:30: // frame sequence
numbers or unique names would work instead, similar to
On 2015/06/18 00:57:00, Avi wrote:
> How is this TODO changed, with its reference to sequence numbers, if you are
> introducing them below?

Good question.  Nate killed frame sequence numbers recently, and HistoryNode
only uses unique name now.  I update the TODO to suggest moving to unique names.
 (On my plate, but less pressing than the basic back/forward case.)

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:811: bool
is_site_per_process = base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/06/18 00:57:00, Avi wrote:
> On 2015/06/17 22:56:23, Charlie Reis (OOO soon) wrote:
> > I'll undo this.  (I was using it below in an earlier version of the patch.)
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1572:
EXPECT_NE(-1, dsn_1);
On 2015/06/18 00:57:00, Avi wrote:
> -1? I would have thought that would be a "not assigned" value, not one you'd
see
> for a committed load.

Right: EXPECT_NE.

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1585:
EXPECT_EQ(dsn_1, dsn_2);
On 2015/06/18 00:57:00, Avi wrote:
> So document sequence numbers are sometimes shared across navigation entries.
Is
> there a design document of some kind explaining what the larger picture here
is?

It's from Blink, so I assume the answer is no.  Though actually, I'm impressed
that these values are documented in HistoryItem.h!
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I've added similar documentation in FrameNavigationEntry (on the accessors) and
frame_navigate_params.h.

https://codereview.chromium.org/1184423005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1587: //
TODO(creis): Add a subframe test.
On 2015/06/17 22:56:23, Charlie Reis (OOO soon) wrote:
> I'll add this as well.
> 
> (I was debating checking these in the existing NavigationTypeClassification
> tests since those are pretty exhaustive on the types of navigations, but it
also
> felt like it would pollute them with checks unrelated to the classifier.  Not
> sure what the best practice here should be for features that should be tested
> across all types of navigations.)

Done.

https://codereview.chromium.org/1184423005/diff/1/content/public/common/frame...
File content/public/common/frame_navigate_params.h (right):

https://codereview.chromium.org/1184423005/diff/1/content/public/common/frame...
content/public/common/frame_navigate_params.h:45: 
On 2015/06/18 00:57:00, Avi wrote:
> If these are generated deep in Blink in a renderer process (coming off of
> WebHistoryItem), how in the world are they supposed to end up being "globally
> unique"?

They get serialized into session restore data, so I think the goal was for them
to still be usable when restored into a new session.  Looking closer, it's not
actually guaranteed-- it's just an attempt to avoid overlap:

static long long generateSequenceNumber()
{
    // Initialize to the current time to reduce the likelihood of generating
    // identifiers that overlap with those from past/future browser sessions.
    static long long next = static_cast<long long>(currentTime() * 1000000.0);
    return ++next;
}

I've rephrased the comment, and I'll be sure to include a SiteInstance check
when comparing them for navigation.

https://codereview.chromium.org/1184423005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1184423005/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1228:
new_entry->GetFrameEntry(rfh->frame_tree_node());
I went ahead and added GetFrameEntry, since it's cleaner not to use
NavigationEntryImpl::TreeNode out here and I'll need it anyway in the next CL.

[Avi (use Gerrit), 2015-06-18 20:36:07]

lgtm

https://codereview.chromium.org/1184423005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_navigation_entry.h (right):

https://codereview.chromium.org/1184423005/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.h:61: // item sequence number
but the same document sequence number.
Update this comment to explain "globally unique" like you do below.

https://codereview.chromium.org/1184423005/diff/40001/content/public/common/f...
File content/public/common/frame_navigate_params.h (right):

https://codereview.chromium.org/1184423005/diff/40001/content/public/common/f...
content/public/common/frame_navigate_params.h:41: // The document sequence
number is ssed to identify cross-document navigations
s/ssed/used/

[Charlie Reis, 2015-06-18 20:50:01]

creis@chromium.org changed reviewers:
+ dcheng@chromium.org

[Charlie Reis, 2015-06-18 20:50:11]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[Charlie Reis, 2015-06-18 20:50:12]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1184423005/#ps60001
(title: "Fix typo")

[Charlie Reis, 2015-06-18 20:51:05]

Thanks!

Daniel, can you review frame_messages.h (and frame_navigate_params.h) for IPC
owners?

[commit-bot: I haz the power, 2015-06-18 20:51:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1184423005/60001

[commit-bot: I haz the power, 2015-06-18 21:53:23]

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

[commit-bot: I haz the power, 2015-06-18 21:53:23]

Dry run: This issue passed the CQ dry run.

[dcheng, 2015-06-18 21:55:52]

lgtm

https://codereview.chromium.org/1184423005/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_navigation_entry.h (right):

https://codereview.chromium.org/1184423005/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.h:69: int64
document_sequence_number() { return document_sequence_number_; }
Nit: maket he getters const.

[Charlie Reis, 2015-06-18 22:08:21]

Patchset #4 (id:80001) has been deleted

[Charlie Reis, 2015-06-18 22:10:01]

Thanks!

https://codereview.chromium.org/1184423005/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_navigation_entry.h (right):

https://codereview.chromium.org/1184423005/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.h:69: int64
document_sequence_number() { return document_sequence_number_; }
On 2015/06/18 21:55:52, dcheng wrote:
> Nit: maket he getters const.

Done.

[Charlie Reis, 2015-06-18 22:10:05]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2015-06-18 22:10:05]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1184423005/#ps100001
(title: "Add const; git cl format")

[commit-bot: I haz the power, 2015-06-18 22:10:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1184423005/100001

[commit-bot: I haz the power, 2015-06-19 00:11:15]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-06-19 00:12:55]

Patchset 4 (id:??) landed as
https://crrev.com/8b5cd4c7d51dec913908d91dbc19bedda293e708
Cr-Commit-Position: refs/heads/master@{#335173}