Remove page id from "page capture".

chrome/renderer/chrome_render_view_observer.cc

 // Copyright (c) 2012 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/renderer/chrome_render_view_observer.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 78 matching lines @@
 
 // maximum number of characters in the document to index, any text beyond this
 // point will be clipped
 static const size_t kMaxIndexChars = 65535;
 
 // Constants for UMA statistic collection.
 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.
 // |found_tag| is set to true if the tag was successfully found.
 // Returns true if the document was parsed without errors.
 bool RetrieveMetaTagContent(const WebFrame* main_frame,
                             const GURL& expected_url,
                             const std::string& meta_tag_name,
                             bool* found_tag,
                             std::string* meta_tag_content) {
@@ skipping 39 matching lines @@
 
 }  // namespace
 
 ChromeRenderViewObserver::ChromeRenderViewObserver(
     content::RenderView* render_view,
     ChromeRenderProcessObserver* chrome_render_process_observer)
     : content::RenderViewObserver(render_view),
       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))
     OnSetClientSidePhishingDetection(true);
 }
 
 ChromeRenderViewObserver::~ChromeRenderViewObserver() {
 }
 
 bool ChromeRenderViewObserver::OnMessageReceived(const IPC::Message& message) {
@@ skipping 171 matching lines @@
     Send(new ChromeViewHostMsg_PageHasOSDD(
         routing_id(), main_frame->document().url(), osdd_url,
         search_provider::AUTODETECTED_PROVIDER));
   }
 
   // Don't capture pages including refresh meta tag.
   if (HasRefreshMetaTag(main_frame))
     return;
 
   CapturePageInfoLater(
-      render_view()->GetPageId(),
       false,  // preliminary_capture
       base::TimeDelta::FromMilliseconds(
           render_view()->GetContentStateImmediately() ?
               0 : kDelayForCaptureMs));
 }
 
 void ChromeRenderViewObserver::DidCommitProvisionalLoad(
     WebLocalFrame* frame, bool is_new_navigation) {
   // Don't capture pages being not new, or including refresh meta tag.
   if (!is_new_navigation || HasRefreshMetaTag(frame))
     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;
 
   WebFrame* main_frame = render_view()->GetWebView()->mainFrame();
   if (!main_frame)
     return;
 
   // Don't index/capture pages that are in view source mode.
   if (main_frame->isViewSourceModeEnabled())
     return;
@@ skipping 13 matching lines @@
   // Retrieve the frame's full text (up to kMaxIndexChars), and pass it to the
   // translate helper for language detection and possible translation.
   base::string16 contents;
   base::TimeTicks capture_begin_time = base::TimeTicks::Now();
   CaptureText(main_frame, &contents);
   UMA_HISTOGRAM_TIMES(kTranslateCaptureText,
                       base::TimeTicks::Now() - capture_begin_time);
   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)
   // Will swap out the string.
   if (phishing_classifier_)
     phishing_classifier_->PageCaptured(&contents, preliminary_capture);
 #endif
 }
 
 void ChromeRenderViewObserver::CaptureText(WebFrame* frame,
@@ skipping 45 matching lines @@
     WebElement element = node.to<WebElement>();
     if (!element.hasHTMLTagName(tag_name))
       continue;
     WebString value = element.getAttribute(attribute_name);
     if (value.isNull() || !LowerCaseEqualsASCII(value, "refresh"))
       continue;
     return true;
   }
   return false;
 }