Speedreader communicate result to tab_helper #9659

kkuehlz · 2021-08-05T00:03:00Z

Resolves brave/brave-browser#17355

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

goodov · 2021-08-05T06:35:09Z

browser/speedreader/speedreader_extended_info_handler.cc

+  std::string value;
+};
+
+}  // anonymous namespace


just // namespace

goodov · 2021-08-05T06:51:12Z

components/speedreader/speedreader_throttle.cc


+}  // anonymous namespace


just // namespace

goodov · 2021-08-05T06:58:08Z

patches/chrome-browser-chrome_browser_main.cc.patch

+   offline_pages::OfflinePageInfoHandler::Register();
+ #endif
+
+  RegisterBraveExtendedInfoHandlers();


Can we put this into BraveBrowserMainParts::PreMainMessageLoopRun and don't introduce this patch? ChromeBrowserMainParts::PreMainMessageLoopRun is virtual.

I tried this first.

If I register before, then I miss the call to sessions::ContentSerializedNavigationDriver::SetInstance(..). Happens here.

If I register after calling ChromeBrowserMainParts::PreMainMessageLoopRun, it introduces a very subtle bug where the tabs are restored before we register our handler. So no persisted data ever gets written to the NavigationEntry

What about putting it into PreBrowserStart? It's very close to the current position.

that worked. patch + chromium_src deleted. thanks :)

Would be good if we can DCHECK ensuring Chromium doesn't move this logic around. If the registration happens after any serialized navigation entries are loaded, then it's a no-op. Could introduce a silent bug.

iefremov · 2021-08-18T14:11:03Z

browser/speedreader/speedreader_extended_info_handler.cc

+void SpeedreaderExtendedInfoHandler::Register() {
+  auto* handler = new SpeedreaderExtendedInfoHandler();
+  sessions::ContentSerializedNavigationDriver::GetInstance()
+      ->RegisterExtendedInfoHandler(kSpeedreaderKey, base::WrapUnique(handler));


just use make_unique to create the unique_ptr and then std::move to pass it

iefremov · 2021-08-18T14:53:10Z

browser/speedreader/speedreader_extended_info_handler.h

+
+// This class is meant to persist data to a content::NavigationEntry so that
+// distilled pages will be recognized on a restored session.
+class SpeedreaderExtendedInfoHandler : public sessions::ExtendedInfoHandler {


why don't you put this class to components/speedreader?

good idea. moved it there, along with the speedreader::DistillState enum. This will be helpful later when we talk to renderer

iefremov · 2021-08-19T15:03:20Z

browser/speedreader/speedreader_extended_info_handler.h

+  SpeedreaderExtendedInfoHandler() = default;
+  ~SpeedreaderExtendedInfoHandler() override = default;
+
+  std::string GetExtendedInfo(content::NavigationEntry* entry) const override;


pls add // sessions::ExtendedInfoHandler:

iefremov · 2021-08-19T15:03:46Z

browser/speedreader/speedreader_extended_info_handler.h

+  SpeedreaderExtendedInfoHandler& operator=(
+      const SpeedreaderExtendedInfoHandler&) = delete;
+
+  SpeedreaderExtendedInfoHandler() = default;


commonly, this constructor should be above copy/assignment

iefremov · 2021-08-19T15:08:33Z

browser/speedreader/speedreader_extended_info_handler.cc

+}
+
+// static
+bool SpeedreaderExtendedInfoHandler::IsCachedReaderMode(


i would rather return some sort of enum instead of making identical methods

fixed. good suggestion. this cleaned up a lot of the logic

iefremov · 2021-09-01T16:42:23Z

components/speedreader/speedreader_url_loader.cc

+      // When distillation fails we return the original data. Causes this
+      // throttle to get deleted.
+      //
+      // TODO(iefremov): Is it okay to rely on just this, or should the distill


I'm not sure, can you please point out when it gets deleted? Probably ok, but I can't quickly find a reference

i verified heuristically, which is why i was asking. if the throttle returned the original data (distill failed) that function is never reached.

iefremov · 2021-09-01T16:45:28Z

components/speedreader/speedreader_result_delegate.h

+#ifndef BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_RESULT_DELEGATE_H_
+#define BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_RESULT_DELEGATE_H_
+
+#include "content/public/browser/web_contents_user_data.h"


iefremov · 2021-09-01T16:45:34Z

components/speedreader/speedreader_result_delegate.h

+
+#include "content/public/browser/web_contents_user_data.h"
+
+namespace content {


iefremov · 2021-09-01T16:55:05Z

browser/brave_content_browser_client.cc

      std::unique_ptr<speedreader::SpeedReaderThrottle> throttle =
          speedreader::SpeedReaderThrottle::MaybeCreateThrottleFor(
              g_brave_browser_process->speedreader_rewriter_service(),
              HostContentSettingsMapFactory::GetForProfile(
                  Profile::FromBrowserContext(browser_context)),
-              request.url, state == DistillState::kSpeedreaderMode,
+              contents, tab_helper, request.url, check_disabled_sites,


I think we can pass wc_getter and avoid adding the lifetime helper

iefremov · 2021-09-01T19:58:22Z

browser/speedreader/speedreader_tab_helper.cc

+void SpeedreaderTabHelper::OnDistillComplete() {
+  // Perform a state transition
+  auto* entry = web_contents()->GetController().GetLastCommittedEntry();
+  DCHECK(entry);


this is a discouraged pattern, see https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#check_dcheck_and-notreached

gotcha, fixed

iefremov · 2021-09-01T20:34:16Z

components/speedreader/speedreader_extended_info_handler.cc

+#include <utility>
+
+#include "base/supports_user_data.h"
+#include "components/sessions/content/content_serialized_navigation_driver.h"


this should be added to DEPS

browser/brave_content_browser_client.cc

browser/speedreader/speedreader_browsertest.cc

iefremov · 2021-09-02T14:14:08Z

browser/speedreader/speedreader_tab_helper.cc

+    distill_state_ = DistillState::kNone;
+  }
+  auto* entry = web_contents()->GetController().GetLastCommittedEntry();
+  DCHECK(entry);


I checked other code and it seems we'd better have if

fixed and also moved to NavigationEntryCommitted

Uses the webcontents to deliver the result to the correct tab helper. We might want to follow this up by having the distiller thread return tuple: struct DistillResult { bool success; std::string buffered_body; }; If success is ever false we can abort the loader.

Resolves brave/brave-browser#17355

* Use the DistillState enum in SpeedreaderExtendedInfoHandler to de-duplicate logic. * Move DistillState to speedreader component * Move SpeedreaderExtendedInfoHandler to speedreader component * Pass wc_getter to SpeedreaderThrottle instead of creating the lifetime helper

Previously we were persisting the SpeedreaderExtendedInfoHandler data to the NavigationEntry in OnDistillComplete(). Since OnDistillComplete() is called by the throttle, the navigation entry might not be persisted yet, so the behavior is not defined. On backward navigations we would sometimes persist to the previous navigation entry, causing bugs where pages would incorrectly show up as readable. Since we wait for DidFinishNavigation, we can safely get the last committed navgiation entry.

We add two browser test cases for Speedreader. In these tests Speedreader is enabled. (1) Run a readable page through Speedreader. Close the browser and open it back up. The page should restore as readable. (2) Navigate to a non-readable page, then to a readable page, then do a back navigation. The readable state should not "stick".

components/speedreader/speedreader_throttle_unittest.cc

kkuehlz · 2021-09-02T21:43:15Z

CodeQL failure unrelated to the C++ code. It's an HTML blob from the guardian (for testing only) that was already vendored before these changes. I moved the directories around and CI scanned it. Safe to ignore that.

iefremov · 2021-09-03T06:02:00Z

components/speedreader/speedreader_throttle_unittest.cc

@@ -27,6 +28,11 @@ namespace {
 constexpr char kTestProfileName[] = "TestProfile";
 }  // anonymous namespace

+class TestSpeedreaderResultDelegate : public SpeedreaderResultDelegate {


also it can go the anonymous namespace above

iefremov · 2021-09-03T06:05:39Z

browser/speedreader/speedreader_tab_helper.h

  ~SpeedreaderTabHelper() override;

  SpeedreaderTabHelper(const SpeedreaderTabHelper&) = delete;
  SpeedreaderTabHelper& operator=(SpeedreaderTabHelper&) = delete;

+  // Helper function to return a weak pointer


IMO the method purpose is pretty obvious, not sure if a comment like this is useful

sure thing. removed it.

* Use NavigationEntryCommitted() instead of DidNavigationFinish(). We have no guarantees about the next DidNavigationStart() being called first. * Fix throttle unit test * Make several review requests to SpeedreaderBrowserTest

kkuehlz · 2021-09-03T20:17:05Z

AdBlockServiceTest.SubscribeToCustomSubscription failed on Windows but that is a known issue. Merging.

…esult" This reverts commit c16968e, reversing changes made to 5ac6c3b.

kkuehlz requested review from iefremov and a team as code owners August 5, 2021 00:03

github-actions bot assigned kkuehlz Aug 5, 2021

kkuehlz force-pushed the speedreader_communicate_result branch from c0c5559 to 0730123 Compare August 5, 2021 06:08

goodov reviewed Aug 5, 2021

View reviewed changes

kkuehlz force-pushed the speedreader_communicate_result branch 2 times, most recently from a2f4d69 to 49d5cdf Compare August 5, 2021 17:16

kkuehlz requested a review from goodov August 5, 2021 17:19

kkuehlz force-pushed the speedreader_communicate_result branch 2 times, most recently from 2bb9213 to 47b9a4d Compare August 5, 2021 22:00

iefremov reviewed Aug 18, 2021

View reviewed changes

iefremov reviewed Aug 19, 2021

View reviewed changes

kkuehlz force-pushed the speedreader_communicate_result branch from 47b9a4d to 235a851 Compare August 27, 2021 18:46

kkuehlz requested a review from iefremov August 27, 2021 18:46

kkuehlz force-pushed the speedreader_communicate_result branch 3 times, most recently from 0f7c734 to 21724ae Compare September 1, 2021 05:34

iefremov reviewed Sep 1, 2021

View reviewed changes

kkuehlz force-pushed the speedreader_communicate_result branch from 21724ae to c548ea0 Compare September 1, 2021 19:14

kkuehlz requested a review from iefremov September 1, 2021 19:20

iefremov reviewed Sep 1, 2021

View reviewed changes

browser/brave_content_browser_client.cc Outdated Show resolved Hide resolved