Refactor PageLoadExtraInfo::committed_url to url and did_commit fields.

Refactor PageLoadExtraInfo::committed_url to url and did_commit fields.

Historically we exposed the start_url and committed_url to observers.
This makes it impossible to know the most recent URL in cases where a
page redirects but doesn't commit.

This patch fixes this. We now report the start_url and most recently known
URL, as well as a separate bit indicating whether the page committed.

This is needed for an upcoming UKM metrics tracking change where we 
want to know the most recent URL in cases where a page load redirects but
fails to commit.

BUG=692543
Review-Url: https://codereview.chromium.org/2692373003
Cr-Commit-Position: refs/heads/master@{#451046}
Committed: https://chromium.googlesource.com/chromium/src/+/5503d4eec7c0bbcf26078e86f0236582e999c10d

[Bryan McQuade, 2017-02-15 13:11:49]

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

[commit-bot: I haz the power, 2017-02-15 13:12:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-02-15 13:37:29]

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

[commit-bot: I haz the power, 2017-02-15 13:37:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 14:25:00]

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

[commit-bot: I haz the power, 2017-02-15 14:25:00]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-02-15 14:25:38]

Description was changed from

==========
Refactor PageLoadExtraInfo::committed_url to url and did_commit fields.

BUG=
==========

to

==========
Refactor PageLoadExtraInfo::committed_url to url and did_commit fields.

Historically we exposed the start_url and committed_url to observers.
This makes it impossible to know the most recent URL in cases where a
page redirects but doesn't commit.

This patch fixes this. We now report the start_url and most recently known
URL, as well as a separate bit indicating whether the page committed.

This is needed for an upcoming UKM metrics tracking change where we 
want to know the most recent URL in cases where a page load redirects but
fails to commit.

BUG=692543
==========

[Bryan McQuade, 2017-02-15 14:35:52]

bmcquade@chromium.org changed reviewers:
+ jkarlin@chromium.org, pasko@chromium.org

[Bryan McQuade, 2017-02-15 14:36:16]

jkarlin, PTAL for page_load_metrics
pasko, PTAL for small prerender change

Thanks!

[jkarlin, 2017-02-15 19:55:36]

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:160:
committed_load_ && committed_load_->did_commit() ? committed_load_->url()
committed_load_->did_commit() must be true, right? Can we dcheck that?

This code could be: committed_load_ ? committed_load_->url() : opener_url;

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:116: const bool
did_commit,
You have const here but not in the .cc

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:297:
url_(navigation_handle->GetURL()),
Can we set start_url_ here as well? It imposes logic overhead to be clever about
the bytes.

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:449: start_url_ = url_;
This can be removed with above change.

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.h (right):

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.h:203: const GURL&
start_url() const {
With suggestion in .cc file this function can be removed.

[Bryan McQuade, 2017-02-15 20:06:06]

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

[commit-bot: I haz the power, 2017-02-15 20:07:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-02-15 20:27:10]

Addressed comments, thanks!

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:160:
committed_load_ && committed_load_->did_commit() ? committed_load_->url()
On 2017/02/15 at 19:55:36, jkarlin wrote:
> committed_load_->did_commit() must be true, right? Can we dcheck that?
> 
> This code could be: committed_load_ ? committed_load_->url() : opener_url;

Done, thanks!

I added the DCHECK at the location in the code where we assign to
committed_load_.

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:116: const bool
did_commit,
On 2017/02/15 at 19:55:36, jkarlin wrote:
> You have const here but not in the .cc

Ah, this is unnecessary. I removed it. Thanks!

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:297:
url_(navigation_handle->GetURL()),
On 2017/02/15 at 19:55:36, jkarlin wrote:
> Can we set start_url_ here as well? It imposes logic overhead to be clever
about the bytes.

Done

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:449: start_url_ = url_;
On 2017/02/15 at 19:55:36, jkarlin wrote:
> This can be removed with above change.

Done

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.h (right):

https://codereview.chromium.org/2692373003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.h:203: const GURL&
start_url() const {
On 2017/02/15 at 19:55:36, jkarlin wrote:
> With suggestion in .cc file this function can be removed.

Thanks! Removed.

[commit-bot: I haz the power, 2017-02-15 21:35:18]

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

[commit-bot: I haz the power, 2017-02-15 21:35:19]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[jkarlin, 2017-02-16 12:38:43]

lgtm

[Bryan McQuade, 2017-02-16 16:56:27]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2017-02-16 16:57:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 17:12:05]

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

[commit-bot: I haz the power, 2017-02-16 17:12:07]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Bryan McQuade, 2017-02-16 17:45:57]

bmcquade@chromium.org changed reviewers:
+ mattcary@chromium.org

[Bryan McQuade, 2017-02-16 17:46:32]

Adding mattcary@ as secondary prerender reviewer. PTAL for small prerender
change, thanks!

[mattcary, 2017-02-16 17:57:15]

lgtm

[pasko, 2017-02-16 17:58:46]

lgtm, sorry for slow response

[Bryan McQuade, 2017-02-16 18:18:29]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2017-02-16 18:19:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 19:23:23]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487269109160910,
"parent_rev": "eda32ab0041fe3ca8ba09b4ef89f02bf90754e30", "commit_rev":
"5503d4eec7c0bbcf26078e86f0236582e999c10d"}

[commit-bot: I haz the power, 2017-02-16 19:23:59]

Description was changed from

==========
Refactor PageLoadExtraInfo::committed_url to url and did_commit fields.

Historically we exposed the start_url and committed_url to observers.
This makes it impossible to know the most recent URL in cases where a
page redirects but doesn't commit.

This patch fixes this. We now report the start_url and most recently known
URL, as well as a separate bit indicating whether the page committed.

This is needed for an upcoming UKM metrics tracking change where we 
want to know the most recent URL in cases where a page load redirects but
fails to commit.

BUG=692543
==========

to

==========
Refactor PageLoadExtraInfo::committed_url to url and did_commit fields.

Historically we exposed the start_url and committed_url to observers.
This makes it impossible to know the most recent URL in cases where a
page redirects but doesn't commit.

This patch fixes this. We now report the start_url and most recently known
URL, as well as a separate bit indicating whether the page committed.

This is needed for an upcoming UKM metrics tracking change where we 
want to know the most recent URL in cases where a page load redirects but
fails to commit.

BUG=692543

Review-Url: https://codereview.chromium.org/2692373003
Cr-Commit-Position: refs/heads/master@{#451046}
Committed:
https://chromium.googlesource.com/chromium/src/+/5503d4eec7c0bbcf26078e86f023...
==========

[commit-bot: I haz the power, 2017-02-16 19:24:01]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5503d4eec7c0bbcf26078e86f023...