Refactor PageLoadExtraInfo::committed_url to url and did_commit fields.

chrome/browser/page_load_metrics/page_load_tracker.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/page_load_metrics/page_load_tracker.h"
 
 #include <algorithm>
 #include <ostream>
 #include <string>
 #include <utility>
@@ skipping 276 matching lines @@
     bool in_foreground,
     PageLoadMetricsEmbedderInterface* embedder_interface,
     const GURL& currently_committed_url,
     content::NavigationHandle* navigation_handle,
     UserInitiatedInfo user_initiated_info,
     int aborted_chain_size,
     int aborted_chain_size_same_url)
     : 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),
       page_transition_(navigation_handle->GetPageTransition()),
       user_initiated_info_(user_initiated_info),
       aborted_chain_size_(aborted_chain_size),
       aborted_chain_size_same_url_(aborted_chain_size_same_url),
       embedder_interface_(embedder_interface) {
   DCHECK(!navigation_handle->HasCommitted());
   embedder_interface_->RegisterObservers(this);
   INVOKE_AND_PRUNE_OBSERVERS(observers_, OnStart, navigation_handle,
                              currently_committed_url, started_in_foreground_);
 }
 
 PageLoadTracker::~PageLoadTracker() {
   if (app_entered_background_) {
     RecordAppBackgroundPageLoadCompleted(true);
   }
 
   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);
 
     // Don't include any aborts that resulted in a new navigation, as the chain
     // length will be included in the aborter PageLoadTracker.
     if (abort_type_ != ABORT_RELOAD && abort_type_ != ABORT_FORWARD_BACK &&
         abort_type_ != ABORT_NEW_NAVIGATION) {
       LogAbortChainHistograms(nullptr);
     }
   } else if (timing_.IsEmpty()) {
     RecordInternalError(ERR_NO_IPCS_RECEIVED);
   }
 
   const PageLoadExtraInfo info = ComputePageLoadExtraInfo();
   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);
     }
   }
 }
 
 void PageLoadTracker::LogAbortChainHistograms(
     content::NavigationHandle* final_navigation) {
   if (aborted_chain_size_ == 0)
     return;
   // Note that this could be broken out by this navigation's abort type, if more
   // granularity is needed. Add one to the chain size to count the current
   // navigation. In the other cases, the current navigation is the final
   // navigation (which commits).
   if (!final_navigation) {
     UMA_HISTOGRAM_COUNTS(internal::kAbortChainSizeNoCommit,
                          aborted_chain_size_ + 1);
     LogAbortChainSameURLHistogram(aborted_chain_size_same_url_ + 1);
     return;
   }
 
   // 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.
   LogAbortChainSameURLHistogram(aborted_chain_size_same_url_);
 
   ui::PageTransition committed_transition =
       final_navigation->GetPageTransition();
   switch (AbortTypeForPageTransition(committed_transition)) {
     case ABORT_RELOAD:
       UMA_HISTOGRAM_COUNTS(internal::kAbortChainSizeReload,
@@ skipping 62 matching lines @@
   // crbug.com/680841 for details.
   if (content::IsBrowserSideNavigationEnabled() &&
       navigation_handle->GetGlobalRequestID() == content::GlobalRequestID())
     return;
 
   DCHECK(!navigation_request_id_.has_value());
   navigation_request_id_ = navigation_handle->GetGlobalRequestID();
   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();
 
   INVOKE_AND_PRUNE_OBSERVERS(observers_, OnCommit, navigation_handle);
   LogAbortChainHistograms(navigation_handle);
 }
 
 void PageLoadTracker::FailedProvisionalLoad(
     content::NavigationHandle* navigation_handle) {
   DCHECK(!failed_provisional_load_info_);
   failed_provisional_load_info_.reset(new FailedProvisionalLoadInfo(
       base::TimeTicks::Now() - navigation_handle->NavigationStart(),
       navigation_handle->GetNetErrorCode()));
 }
 
 void PageLoadTracker::Redirect(content::NavigationHandle* navigation_handle) {
+  MaybeUpdateURL(navigation_handle);
   INVOKE_AND_PRUNE_OBSERVERS(observers_, OnRedirect, navigation_handle);
 }
 
 void PageLoadTracker::OnInputEvent(const blink::WebInputEvent& event) {
   input_tracker_.OnInputEvent(event);
   for (const auto& observer : observers_) {
     observer->OnUserInput(event);
   }
 }
 
@@ skipping 32 matching lines @@
   // different start time from an earlier struct is considered invalid.
   bool valid_timing_descendent =
       timing_.navigation_start.is_null() ||
       timing_.navigation_start == new_timing.navigation_start;
   // Ensure flags sent previously are still present in the new metadata fields.
   bool valid_behavior_descendent =
       (metadata_.behavior_flags & new_metadata.behavior_flags) ==
       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
     // metadata_ before calling GetPageLoadMetricsInfo. Thus, we make a copy of
     // timing here, update timing_ and metadata_, and then proceed to dispatch
     // the observer timing callbacks.
     const PageLoadTiming last_timing = timing_;
     timing_ = new_timing;
 
     const PageLoadMetadata last_metadata = metadata_;
@@ skipping 75 matching lines @@
   }
 
   // abort_type_ == ABORT_NONE implies abort_user_initiated_info_ is not user
   // initiated.
   DCHECK(abort_type_ != ABORT_NONE ||
          (!abort_user_initiated_info_.browser_initiated &&
           !abort_user_initiated_info_.user_gesture &&
           !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_);
 }
 
 bool PageLoadTracker::HasMatchingNavigationRequestID(
     const content::GlobalRequestID& request_id) const {
   DCHECK(request_id != content::GlobalRequestID());
   return navigation_request_id_.has_value() &&
          navigation_request_id_.value() == request_id;
 }
 
@@ skipping 30 matching lines @@
     base::TimeTicks abort_cause_time) const {
   // Note that |abort_cause_time - abort_time| can be negative.
   return abort_type_ == ABORT_OTHER &&
          (abort_cause_time - abort_time_).InMilliseconds() < 100;
 }
 
 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,
                                           UserInitiatedInfo user_initiated_info,
                                           base::TimeTicks timestamp,
                                           bool is_certainly_browser_timestamp) {
   // When a provisional navigation commits, that navigation's start time is
   // interpreted as the abort time for other provisional loads in the tab.
   // However, this only makes sense if the committed load started after the
   // aborted provisional loads started. Thus we ignore cases where the committed
@@ skipping 20 matching lines @@
   // user initiated.
   if (abort_type != ABORT_CLIENT_REDIRECT)
     abort_user_initiated_info_ = user_initiated_info;
 
   if (is_certainly_browser_timestamp) {
     ClampBrowserTimestampIfInterProcessTimeTickSkew(&abort_time_);
   }
 }
 
 }  // namespace page_load_metrics