Ensure that unique ids are unique.

Ensure that unique ids are unique.

BUG=369661
TEST=NavigationControllerBrowserTest.UniqueIDs

Committed: https://crrev.com/954dff9e0261be9d1d691b65d070b0ec70caf1b8
Cr-Commit-Position: refs/heads/master@{#330893}

[Avi (use Gerrit), 2015-05-18 20:46:38]

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

[Avi (use Gerrit), 2015-05-18 20:46:39]

Charlie, Peter: for review.

[Peter Kasting, 2015-05-18 20:49:23]

I'm concerned about whether this will be a behavior change for infobars.  For
the infobars whose expiry functions rely on checking unique IDs, will this mean
we now expire infobars on subframe navigations when we didn't before, or
something similar?

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

https://codereview.chromium.org/1139823006/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:344: // Copy all state over,
unless cleared in ResetForCommit.
Nit: all -> most

https://codereview.chromium.org/1139823006/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:345: // Not unique_id_; even
identical twins have different fingerprints.
Nit: Perhaps "Don't copy |unique_id_|, or it won't be unique."

[Avi (use Gerrit), 2015-05-18 20:51:43]

It's quite possible that there would be an effect. If there is a noticed effect,
I'm willing to go into the infobar code and tweak it to return to the existing
behavior.

But it's hard to do this without tests that are confirming what the expected
behavior is.

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

https://codereview.chromium.org/1139823006/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:344: // Copy all state over,
unless cleared in ResetForCommit.
On 2015/05/18 20:49:23, Peter Kasting wrote:
> Nit: all -> most

Done.

https://codereview.chromium.org/1139823006/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:345: // Not unique_id_; even
identical twins have different fingerprints.
On 2015/05/18 20:49:23, Peter Kasting wrote:
> Nit: Perhaps "Don't copy |unique_id_|, or it won't be unique."

Done.

[Peter Kasting, 2015-05-18 20:53:31]

On 2015/05/18 20:51:43, Avi wrote:
> It's quite possible that there would be an effect. If there is a noticed
effect,
> I'm willing to go into the infobar code and tweak it to return to the existing
> behavior.
> 
> But it's hard to do this without tests that are confirming what the expected
> behavior is.

What I'm looking for here is for you to audit the code and look at what the
current behavior is and whether this will change it.  If there's a behavior
change at all, we need to worry about this.  I'm less worried about having you
figure out the "expected" behavior, I can help with that if it turns out there
is, in fact, a behavior change.

[Avi (use Gerrit), 2015-05-18 21:41:01]

On 2015/05/18 20:53:31, Peter Kasting wrote:
> What I'm looking for here is for you to audit the code and look at what the
> current behavior is and whether this will change it.  If there's a behavior
> change at all, we need to worry about this.  I'm less worried about having you
> figure out the "expected" behavior, I can help with that if it turns out there
> is, in fact, a behavior change.

InfoBarDelegate::ShouldExpireInternal is:

return (contents_unique_id_ != details.entry_id) || details.is_reload;

My first instinct is that it should be re-written as:

return !details.is_main_frame || details.is_reload;

But here's the problem, Peter. I don't know the infobar code incredibly well.
And I'm refactoring lots of core code. If there's a test for a feature that
breaks with a change I make, I can fix it, because the test guides me as to what
the expected behavior is.

But there are no tests here. All I have is the existing infobar code, and I'm
left to guess the intent. And I have no idea what it is, nor am I particularly
qualified to guess.

So Peter, how should I rewrite ShouldExpireInternal? If you provide me with
tests, I will make sure they stay passing. If you provide me with a rewrite of
the infobar code, I will include it. But I don't have the knowledge or
qualification to keep things working when there is no indication in the code of
what the expected behavior is.

[Peter Kasting, 2015-05-19 01:03:33]

I wrote https://codereview.chromium.org/1142153002/ which should remove the
infobar dependence on unique IDs.  Once that lands, feel free to move forward
here.

[Avi (use Gerrit), 2015-05-19 03:14:36]

On 2015/05/19 01:03:33, Peter Kasting wrote:
> I wrote https://codereview.chromium.org/1142153002/ which should remove the
> infobar dependence on unique IDs.  Once that lands, feel free to move forward
> here.

Peter, that is an amazing CL. Thank you for taking this on!

[Charlie Reis, 2015-05-19 16:35:44]

LGTM once Peter's CL lands.  (Thanks Peter!)

[Avi (use Gerrit), 2015-05-21 03:44:30]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2015-05-21 03:45:27]

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

[commit-bot: I haz the power, 2015-05-21 04:42:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-21 04:43:34]

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