Refactor PageLoadExtraInfo::committed_url to url and did_commit fields.

chrome/browser/page_load_metrics/page_load_tracker.cc

Index: chrome/browser/page_load_metrics/page_load_tracker.cc
diff --git a/chrome/browser/page_load_metrics/page_load_tracker.cc b/chrome/browser/page_load_metrics/page_load_tracker.cc
index 26c591db52774d451f66e09bd28c313654f7cb18..1d1e224f9f58a715f84d70a287012ccee670cb3b 100644
--- a/chrome/browser/page_load_metrics/page_load_tracker.cc
+++ b/chrome/browser/page_load_metrics/page_load_tracker.cc
@@ -294,7 +294,8 @@ PageLoadTracker::PageLoadTracker(
     : did_stop_tracking_(false),
       app_entered_background_(false),
       navigation_start_(navigation_handle->NavigationStart()),
-      start_url_(navigation_handle->GetURL()),
+      url_(navigation_handle->GetURL()),

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

Can we set start_url_ here as well? It imposes logic overhead to be clever about
the bytes.

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

Done

+      did_commit_(false),
       abort_type_(ABORT_NONE),
       abort_user_initiated_info_(UserInitiatedInfo::NotUserInitiated()),
       started_in_foreground_(in_foreground),
@@ -317,7 +318,7 @@ PageLoadTracker::~PageLoadTracker() {
   if (did_stop_tracking_)
     return;
 
-  if (committed_url_.is_empty()) {
+  if (!did_commit_) {
     if (!failed_provisional_load_info_)
       RecordInternalError(ERR_NO_COMMIT_OR_FAILED_PROVISIONAL_LOAD);
 
@@ -335,7 +336,7 @@ PageLoadTracker::~PageLoadTracker() {
   for (const auto& observer : observers_) {
     if (failed_provisional_load_info_) {
       observer->OnFailedProvisionalLoad(*failed_provisional_load_info_, info);
-    } else if (!info.committed_url.is_empty()) {
+    } else if (did_commit_) {
       observer->OnComplete(timing_, info);
     }
   }
@@ -357,7 +358,7 @@ void PageLoadTracker::LogAbortChainHistograms(
   }
 
   // The following is only executed for committing trackers.
-  DCHECK(!committed_url_.is_empty());
+  DCHECK(did_commit_);
 
   // Note that histograms could be separated out by this commit's transition
   // type, but for simplicity they will all be bucketed together.
@@ -440,8 +441,18 @@ void PageLoadTracker::WillProcessNavigationResponse(
   DCHECK(navigation_request_id_.value() != content::GlobalRequestID());
 }
 
+void PageLoadTracker::MaybeUpdateURL(
+    content::NavigationHandle* navigation_handle) {
+  if (navigation_handle->GetURL() == url_)
+    return;
+  if (start_url_.is_empty())
+    start_url_ = url_;

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

This can be removed with above change.

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

Done

+  url_ = navigation_handle->GetURL();
+}
+
 void PageLoadTracker::Commit(content::NavigationHandle* navigation_handle) {
-  committed_url_ = navigation_handle->GetURL();
+  did_commit_ = true;
+  MaybeUpdateURL(navigation_handle);
   // Some transitions (like CLIENT_REDIRECT) are only known at commit time.
   page_transition_ = navigation_handle->GetPageTransition();
   user_initiated_info_.user_gesture = navigation_handle->HasUserGesture();
@@ -459,6 +470,7 @@ void PageLoadTracker::FailedProvisionalLoad(
 }
 
 void PageLoadTracker::Redirect(content::NavigationHandle* navigation_handle) {
+  MaybeUpdateURL(navigation_handle);
   INVOKE_AND_PRUNE_OBSERVERS(observers_, OnRedirect, navigation_handle);
 }
 
@@ -511,7 +523,7 @@ bool PageLoadTracker::UpdateTiming(const PageLoadTiming& new_timing,
       metadata_.behavior_flags;
   if (IsValidPageLoadTiming(new_timing) && valid_timing_descendent &&
       valid_behavior_descendent) {
-    DCHECK(!committed_url_.is_empty());  // OnCommit() must be called first.
+    DCHECK(did_commit_);  // OnCommit() must be called first.
     // There are some subtle ordering constraints here. GetPageLoadMetricsInfo()
     // must be called before DispatchObserverTimingCallbacks, but its
     // implementation depends on the state of metadata_, so we need to update
@@ -607,7 +619,7 @@ PageLoadExtraInfo PageLoadTracker::ComputePageLoadExtraInfo() {
           !abort_user_initiated_info_.user_input_event));
   return PageLoadExtraInfo(
       first_background_time, first_foreground_time, started_in_foreground_,
-      user_initiated_info_, committed_url_, start_url_, abort_type_,
+      user_initiated_info_, url(), start_url(), did_commit_, abort_type_,
       abort_user_initiated_info_, time_to_abort, metadata_);
 }
 
@@ -658,8 +670,8 @@ bool PageLoadTracker::MatchesOriginalNavigation(
     content::NavigationHandle* navigation_handle) {
   // Neither navigation should have committed.
   DCHECK(!navigation_handle->HasCommitted());
-  DCHECK(committed_url_.is_empty());
-  return navigation_handle->GetURL() == start_url_;
+  DCHECK(!did_commit_);
+  return navigation_handle->GetURL() == start_url();
 }
 
 void PageLoadTracker::UpdateAbortInternal(UserAbortType abort_type,