DistillerRenderFrameObserver determines if page is distilled from URL.

The DistillerRenderFrameObserver::SetIsDistillerPage was sometimes
being called after DistillerRenderFrameObserver::DidCreateScriptContext,
which caused the Distiller Javascript object never to be created
and injected into the page. SetIsDistillerPage happened via a mojom
call from the Browser process when
WebContentsObserver::DidFinishNavigation occurred. It wasn't guaranteed
to happen in any particular order. This was making a test flaky;
relanding the test will happen in a separate change.

It is cleaner to simply check if the page is distilled by checking that
the URL is correctly formatted as a distilled page.

AX-Relnotes: n/a
Bug: 1016615, 1075439
Change-Id: I12cb5bb9e2975aca25a79333f251d1a28546f32b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209281Reviewed-by: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Katie Dektar <katie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771264}

chrome/renderer/chrome_content_renderer_client.cc

@@ -546,9 +546,9 @@ void ChromeContentRendererClient::RenderFrameCreated(
     }
   }
 
-  // Set up a mojo service to test if this page is a distiller page.
+  // Set up a render frame observer to test if this page is a distiller page.
   new dom_distiller::DistillerJsRenderFrameObserver(
-      render_frame, ISOLATED_WORLD_ID_CHROME_INTERNAL, registry);
+      render_frame, ISOLATED_WORLD_ID_CHROME_INTERNAL);
 
   if (dom_distiller::ShouldStartDistillabilityService()) {
     // Create DistillabilityAgent to send distillability updates to

components/dom_distiller/content/browser/dom_distiller_viewer_source.cc

@@ -20,7 +20,6 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "components/dom_distiller/content/browser/distiller_javascript_utils.h"
-#include "components/dom_distiller/content/common/mojom/distiller_page_notifier_service.mojom.h"
 #include "components/dom_distiller/core/distilled_page_prefs.h"
 #include "components/dom_distiller/core/dom_distiller_request_view_base.h"
 #include "components/dom_distiller/core/dom_distiller_service.h"
@@ -124,18 +123,6 @@ void DomDistillerViewerSource::RequestViewerHandle::DidFinishNavigation(
   bool expected_main_view_request = navigation == expected_url_;
   if (navigation_handle->IsSameDocument() || expected_main_view_request) {
     // In-page navigations, as well as the main view request can be ignored.
-    if (expected_main_view_request) {
-      content::RenderFrameHost* render_frame_host =
-          navigation_handle->GetRenderFrameHost();
-      CHECK_EQ(0, render_frame_host->GetEnabledBindings());
-
-      // Tell the renderer that this is currently a distilled page.
-      mojo::Remote<mojom::DistillerPageNotifierService> page_notifier_service;
-      render_frame_host->GetRemoteInterfaces()->GetInterface(
-          page_notifier_service.BindNewPipeAndPassReceiver());
-      DCHECK(page_notifier_service);
-      page_notifier_service->NotifyIsDistillerPage();
-    }
     return;
   }
 

components/dom_distiller/content/common/mojom/BUILD.gn

@@ -8,7 +8,6 @@ mojom("mojom") {
   sources = [
     "distillability_service.mojom",
     "distiller_javascript_service.mojom",
-    "distiller_page_notifier_service.mojom",
   ]
   deps = [ "//components/dom_distiller/core/mojom" ]
 }

components/dom_distiller/content/common/mojom/distiller_page_notifier_service.mojom

-// Copyright 2015 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.
-
-module dom_distiller.mojom;
-
-// This service is implemented in the renderer process and is used by the
-// browser to notify a renderer that is page is a distiller page.
-interface DistillerPageNotifierService {
-  NotifyIsDistillerPage();
-};

components/dom_distiller/content/renderer/BUILD.gn

@@ -10,8 +10,6 @@ static_library("renderer") {
     "distiller_js_render_frame_observer.h",
     "distiller_native_javascript.cc",
     "distiller_native_javascript.h",
-    "distiller_page_notifier_service_impl.cc",
-    "distiller_page_notifier_service_impl.h",
   ]
 
   public_deps = [ "//components/dom_distiller/core/proto" ]

components/dom_distiller/content/renderer/distiller_js_render_frame_observer.cc

@@ -8,26 +8,18 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "components/dom_distiller/content/common/mojom/distiller_page_notifier_service.mojom.h"
-#include "components/dom_distiller/content/renderer/distiller_page_notifier_service_impl.h"
+#include "components/dom_distiller/core/url_utils.h"
 #include "content/public/renderer/render_frame.h"
-#include "mojo/public/cpp/bindings/self_owned_receiver.h"
-#include "services/service_manager/public/cpp/binder_registry.h"
 #include "v8/include/v8.h"
 
 namespace dom_distiller {
 
 DistillerJsRenderFrameObserver::DistillerJsRenderFrameObserver(
     content::RenderFrame* render_frame,
-    const int32_t distiller_isolated_world_id,
-    service_manager::BinderRegistry* registry)
+    const int32_t distiller_isolated_world_id)
     : RenderFrameObserver(render_frame),
       distiller_isolated_world_id_(distiller_isolated_world_id),
-      is_distiller_page_(false) {
-  registry->AddInterface(base::BindRepeating(
-      &DistillerJsRenderFrameObserver::CreateDistillerPageNotifierService,
-      weak_factory_.GetWeakPtr()));
-}
+      is_distiller_page_(false) {}
 
 DistillerJsRenderFrameObserver::~DistillerJsRenderFrameObserver() {}
 
@@ -35,6 +27,7 @@ void DistillerJsRenderFrameObserver::DidStartNavigation(
     const GURL& url,
     base::Optional<blink::WebNavigationType> navigation_type) {
   load_active_ = true;
+  is_distiller_page_ = url_utils::IsDistilledPage(url);
 }
 
 void DistillerJsRenderFrameObserver::DidFinishLoad() {
@@ -56,19 +49,6 @@ void DistillerJsRenderFrameObserver::DidCreateScriptContext(
   native_javascript_handle_->AddJavaScriptObjectToFrame(context);
 }
 
-void DistillerJsRenderFrameObserver::CreateDistillerPageNotifierService(
-    mojo::PendingReceiver<mojom::DistillerPageNotifierService> receiver) {
-  if (!load_active_)
-    return;
-  mojo::MakeSelfOwnedReceiver(
-      std::make_unique<DistillerPageNotifierServiceImpl>(this),
-      std::move(receiver));
-}
-
-void DistillerJsRenderFrameObserver::SetIsDistillerPage() {
-  is_distiller_page_ = true;
-}
-
 void DistillerJsRenderFrameObserver::OnDestruct() {
   delete this;
 }

components/dom_distiller/content/renderer/distiller_js_render_frame_observer.h

@@ -8,7 +8,6 @@
 #include "base/memory/weak_ptr.h"
 #include "components/dom_distiller/content/common/mojom/distiller_javascript_service.mojom.h"
 #include "components/dom_distiller/content/renderer/distiller_native_javascript.h"
-#include "components/dom_distiller/content/renderer/distiller_page_notifier_service_impl.h"
 #include "content/public/renderer/render_frame.h"
 #include "content/public/renderer/render_frame_observer.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
@@ -18,14 +17,11 @@
 namespace dom_distiller {
 
 // DistillerJsRenderFrame observer waits for a page to be loaded and then
-// tried to connect to a mojo service hosted in the browser process. The
-// service will tell this render process if the current page is a distiller
-// page.
+// adds the Javascript distiller object if it is a distilled page.
 class DistillerJsRenderFrameObserver : public content::RenderFrameObserver {
  public:
   DistillerJsRenderFrameObserver(content::RenderFrame* render_frame,
-                                 const int32_t distiller_isolated_world_id,
-                                 service_manager::BinderRegistry* registry);
+                                 const int32_t distiller_isolated_world_id);
   ~DistillerJsRenderFrameObserver() override;
 
   // RenderFrameObserver implementation.
@@ -36,12 +32,7 @@ class DistillerJsRenderFrameObserver : public content::RenderFrameObserver {
   void DidCreateScriptContext(v8::Local<v8::Context> context,
                               int32_t world_id) override;
 
-  // Flag the current page as a distiller page.
-  void SetIsDistillerPage();
-
  private:
-  void CreateDistillerPageNotifierService(
-      mojo::PendingReceiver<mojom::DistillerPageNotifierService> receiver);
 
   // RenderFrameObserver implementation.
   void OnDestruct() override;
@@ -58,7 +49,6 @@ class DistillerJsRenderFrameObserver : public content::RenderFrameObserver {
 
   // Handle to "distiller" JavaScript object functionality.
   std::unique_ptr<DistillerNativeJavaScript> native_javascript_handle_;
-  base::WeakPtrFactory<DistillerJsRenderFrameObserver> weak_factory_{this};
 };
 
 }  // namespace dom_distiller

components/dom_distiller/content/renderer/distiller_page_notifier_service_impl.cc

-// Copyright 2015 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 "components/dom_distiller/content/renderer/distiller_page_notifier_service_impl.h"
-
-#include <utility>
-
-#include "components/dom_distiller/content/renderer/distiller_js_render_frame_observer.h"
-#include "components/dom_distiller/content/renderer/distiller_native_javascript.h"
-#include "mojo/public/cpp/bindings/strong_binding.h"
-
-namespace dom_distiller {
-
-DistillerPageNotifierServiceImpl::DistillerPageNotifierServiceImpl(
-    DistillerJsRenderFrameObserver* observer)
-    : distiller_js_observer_(observer) {}
-
-void DistillerPageNotifierServiceImpl::NotifyIsDistillerPage() {
-  // TODO(mdjones): Send some form of unique ID so this call knows
-  // which tab it should respond to..
-  distiller_js_observer_->SetIsDistillerPage();
-}
-
-DistillerPageNotifierServiceImpl::~DistillerPageNotifierServiceImpl() {}
-
-}  // namespace dom_distiller

components/dom_distiller/content/renderer/distiller_page_notifier_service_impl.h

-// Copyright 2015 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.
-
-#ifndef COMPONENTS_DOM_DISTILLER_CONTENT_RENDERER_DISTILLER_PAGE_NOTIFIER_SERVICE_IMPL_H_
-#define COMPONENTS_DOM_DISTILLER_CONTENT_RENDERER_DISTILLER_PAGE_NOTIFIER_SERVICE_IMPL_H_
-
-#include "components/dom_distiller/content/common/mojom/distiller_page_notifier_service.mojom.h"
-#include "components/dom_distiller/content/renderer/distiller_js_render_frame_observer.h"
-#include "components/dom_distiller/content/renderer/distiller_native_javascript.h"
-
-namespace dom_distiller {
-
-class DistillerJsRenderFrameObserver;
-
-// mojom::DistillerPageNotifierService is responsible for listening to the
-// browser for
-// messages about if a page is a distiller page. No message is received if the
-// page is not a distiller page. This service should be removed from the
-// registry once the page is done loading.
-class DistillerPageNotifierServiceImpl
-    : public mojom::DistillerPageNotifierService {
- public:
-  explicit DistillerPageNotifierServiceImpl(
-      DistillerJsRenderFrameObserver* observer);
-  ~DistillerPageNotifierServiceImpl() override;
-
-  // Implementation of mojo interface DistillerPageNotifierService.
-  void NotifyIsDistillerPage() override;
-
- private:
-  DistillerJsRenderFrameObserver* distiller_js_observer_;
-};
-
-}  // namespace dom_distiller
-
-#endif  // COMPONENTS_DOM_DISTILLER_CONTENT_RENDERER_DISTILLER_PAGE_NOTIFIER_SERVICE_IMPL_H_