Remove page id from "page capture".

chrome/renderer/chrome_render_view_observer.cc

Index: chrome/renderer/chrome_render_view_observer.cc
diff --git a/chrome/renderer/chrome_render_view_observer.cc b/chrome/renderer/chrome_render_view_observer.cc
index f3603e7b181fb26c964c5ff514c2dd867b164b2e..b911e7a500e5813e0083154fc025cf4f3088fc3b 100644
--- a/chrome/renderer/chrome_render_view_observer.cc
+++ b/chrome/renderer/chrome_render_view_observer.cc
@@ -96,12 +96,6 @@ static const char kTranslateCaptureText[] = "Translate.CaptureText";
 
 namespace {
 
-GURL StripRef(const GURL& url) {
-  GURL::Replacements replacements;
-  replacements.ClearRef();
-  return url.ReplaceComponents(replacements);
-}
-
 #if defined(OS_ANDROID)
 // Parses the DOM for a <meta> tag with a particular name.
 // |meta_tag_content| is set to the contents of the 'content' attribute.
@@ -161,7 +155,6 @@ ChromeRenderViewObserver::ChromeRenderViewObserver(
       chrome_render_process_observer_(chrome_render_process_observer),
       translate_helper_(new TranslateHelper(render_view)),
       phishing_classifier_(NULL),
-      last_indexed_page_id_(-1),
       capture_timer_(false, false) {
   const CommandLine& command_line = *CommandLine::ForCurrentProcess();
   if (!command_line.HasSwitch(switches::kDisableClientSidePhishingDetection))
@@ -353,7 +346,6 @@ void ChromeRenderViewObserver::DidStopLoading() {
     return;
 
   CapturePageInfoLater(
-      render_view()->GetPageId(),
       false,  // preliminary_capture
       base::TimeDelta::FromMilliseconds(
           render_view()->GetContentStateImmediately() ?
@@ -367,29 +359,21 @@ void ChromeRenderViewObserver::DidCommitProvisionalLoad(
     return;
 
   CapturePageInfoLater(
-      render_view()->GetPageId(),
       true,  // preliminary_capture
       base::TimeDelta::FromMilliseconds(kDelayForForcedCaptureMs));
 }
 
-void ChromeRenderViewObserver::CapturePageInfoLater(int page_id,
-                                                    bool preliminary_capture,
+void ChromeRenderViewObserver::CapturePageInfoLater(bool preliminary_capture,
                                                     base::TimeDelta delay) {
   capture_timer_.Start(
       FROM_HERE,
       delay,
       base::Bind(&ChromeRenderViewObserver::CapturePageInfo,
                  base::Unretained(this),
-                 page_id,
                  preliminary_capture));
 }
 
-void ChromeRenderViewObserver::CapturePageInfo(int page_id,
-                                               bool preliminary_capture) {
-  // If |page_id| is obsolete, we should stop indexing and capturing a page.
-  if (render_view()->GetPageId() != page_id)

[awong, 2014/07/07 18:36:59]

This looks like a guard against the RenderView loading a new page between the
post task in CapturePageInfoLater() and the invocation. I would expect us to
need to replace the page_id based cancellation with a WeakPtrFactory or
something other similar flag.

Might even be able to just remove the timer and use PostDelayedTask directly.

[Avi (use Gerrit), 2014/07/07 18:49:46]

I'm not convinced this check for page id is even necessary.

When a navigation happens, DidCommitProvisionalLoad will get called, and it will
call CapturePageInfoLater, which will reset the timer. How is is possible that a
timed capture request with the wrong page id would even happen?

[Lei Zhang, 2014/07/07 19:37:04]

Can the page id change if |is_new_navigation| in DidCommitProvisionalLoad() is
false? That would be the only possible cause.

[Avi (use Gerrit), 2014/07/07 19:51:22]

I don't know. I can reset the timer no matter what.

-    return;
-
+void ChromeRenderViewObserver::CapturePageInfo(bool preliminary_capture) {
   if (!render_view()->GetWebView())
     return;
 
@@ -423,39 +407,6 @@ void ChromeRenderViewObserver::CapturePageInfo(int page_id,
   if (translate_helper_)
     translate_helper_->PageCaptured(contents);
 
-  // TODO(shess): Is indexing "Full text search" indexing?  In that
-  // case more of this can go.
-  // Skip indexing if this is not a new load.  Note that the case where
-  // page_id == last_indexed_page_id_ is more complicated, since we need to
-  // reindex if the toplevel URL has changed (such as from a redirect), even
-  // though this may not cause the page id to be incremented.
-  if (page_id < last_indexed_page_id_)
-    return;
-
-  bool same_page_id = last_indexed_page_id_ == page_id;
-  if (!preliminary_capture)
-    last_indexed_page_id_ = page_id;
-
-  // Get the URL for this page.
-  GURL url(main_frame->document().url());
-  if (url.is_empty()) {
-    if (!preliminary_capture)
-      last_indexed_url_ = GURL();
-    return;
-  }
-
-  // If the page id is unchanged, check whether the URL (ignoring fragments)
-  // has changed.  If so, we need to reindex.  Otherwise, assume this is a
-  // reload, in-page navigation, or some other load type where we don't want to
-  // reindex.  Note: subframe navigations after onload increment the page id,
-  // so these will trigger a reindex.
-  GURL stripped_url(StripRef(url));

[awong, 2014/07/07 18:36:59]

In this observer, the page_id seems to be used as a coarse grain indication of
"we've been here before."

In particular, this block of code seems to be avoiding reclassification of pages
with the same top-level URL. I think the classifier itself does some squashing
here, but not 100% sure if we've done the same job of de-duping.

[Avi (use Gerrit), 2014/07/07 18:49:46]

The classifier does check for PAGE_TRANSITION_FORWARD_BACK. It's roughly
equivalent.

What's irritating is that that check for PAGE_TRANSITION_FORWARD_BACK could
never have been used before, because forward-back pages were never passed to it
in the first place. I'd rather rely on the checks that the classifier people put
in rather than the upstream checks that it appears that they weren't even aware
of.

[Lei Zhang, 2014/07/07 19:37:04]

The PAGE_TRANSITION_FORWARD_BACK was added in r83366 and this check here was
added afterwards in r99025 for bug 80742.

Given a foo.html that calls location.replace("foo.html"), currently this check
gets hit and the phishing classifier code never runs. When removed, the phishing
classifier code runs and it passes the PAGE_TRANSITION_FORWARD_BACK test.

[Avi (use Gerrit), 2014/07/07 19:51:22]

So the phishing classifier runs an extra time if the page does a
location.replace? I would think that's a good thing; what if different, fishier,
content was loaded this next time?

It would be nice to get a cleaner signal here as to whether things changed or
not.

-  if (same_page_id && stripped_url == last_indexed_url_)
-    return;
-
-  if (!preliminary_capture)
-    last_indexed_url_ = stripped_url;
-
   TRACE_EVENT0("renderer", "ChromeRenderViewObserver::CapturePageInfo");
 
 #if defined(FULL_SAFE_BROWSING)