Issue 8416: Introduce speedreader button. #5085

iefremov · 2020-03-27T19:28:45Z

brave/brave-browser#8416

Submitter Checklist:

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

iefremov · 2020-03-27T19:30:58Z

The button is not colorful yet - have to do some manual adjustments in the .icon files :-/

simonhong · 2020-03-30T08:51:24Z

I'll take a look tomorrow :)

AndriusA · 2020-03-30T18:04:42Z

components/speedreader/buildflags.gni

@@ -1,5 +1,5 @@
 import("//build/config/features.gni")

 declare_args() {
- enable_speedreader = !is_ios
+ enable_speedreader = !is_android


don't we want to disable it for both ios and android currently?

i believe ios is based on a different codebase...

but we do use the flag in various other components, and the CI seems to build some libraries used by ios - there still doesn't seem to be a need to build it for ios, so should be disabled?

I think components for ios are manually selected, so we shouldn't explicitly disable - it is never reached. @bsclifton please correct me if I'm wrong?

AndriusA · 2020-03-30T18:23:08Z

browser/speedreader/speedreader_tab_helper.cc

+ SpeedreaderButton* button =
+ static_cast<BraveToolbarView*>(view->toolbar())->speedreader_button();
+ if (button) {
+ button->SetActive(active_);


just checking: the current interaction here will show SpeedReader active icon even on tabs that have not been reloaded since SpeedReader was enabled?

yeah, actually this is a bug. The user could 1) enable speedreader, load a distilled page 2) open another tab 3) turn off speedreader 4) come back to tab#1 5) navigate away to e.g. blank page 6) open tab#2, enable speedreader 7) open tab#1 - speedreader icon is active

AndriusA · 2020-03-30T18:25:50Z

browser/speedreader/speedreader_browsertest.cc

@@ -47,6 +49,7 @@ class SpeedReaderBrowserTest : public InProcessBrowserTest {
 };

 IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, SmokeTest) {


more than a smoke test would be good

I think I'll add more test when a component updater stuff is in place

simonhong · 2020-03-31T03:46:59Z

browser/speedreader/speedreader_service_factory.h

+
+class SpeedreaderService;
+
+class SpeedreaderServiceFactory : public BrowserContextKeyedServiceFactory {


I think this service & factory aren't necessary.
If this is only used for get/set related prefs value, I think just using simple apis seems fine. WDYT?

Yeah, I agree, though I decided to keep it in case we would add a user preference (and would need to keep a pref registrar), or something like this.

Ah, ok. if service will have registrar in the future, having service seems reasonable.

simonhong · 2020-03-31T03:52:04Z

browser/speedreader/speedreader_tab_helper.h

+namespace speedreader {
+
+// Updates the speedreader toolbar button state when a tab visibility changes.
+class SpeedreaderTabHelper


Whenever browser ui change needs, BraveToolBarView::Update(content::WebContents*) is called.
I think we can manage speedreader button's state at there w/o this tab helper.
Also, using views api outside of views modules is layer violation.

I don't think the toolbar should be aware of speedreader button activation logic, so I propose to keep the helper.
Agree that there is a violation, I'm open for suggestions :) We probably should involve the Browser object, as bookmarks do

I mean we can call SpeedReaderButton::Update from ToolBarView.
Maybe speedreader button can have the logic that tab helper has?

With tab helper, I think it's hard to remove this layer violation.

The main problem is that the "active" state is determined in BraveContentBrowserClient - we should pass it to the toolbar/button somehow. So it seems the only way is to user Browser - i.e. subclass it, add a method and override it in BraveBrowserView

Ok - I see. How about this?
Setting active state to tab helper from here and checking that state from SpeedReaderButton::Update()?
With this, I think we don't need to introduce additional api to BrowserView.
I think SpeedReaderButton::Update() can be called whenever toolbar state should be updated like tab changing, loading and etc..

I did a small refactor, so now Tab Helper determines the active state by itself and the toolbar actively queries this state - PTAL :)

iefremov · 2020-03-31T16:07:04Z

@bridiver Please take a look at a tiny patch :)

simonhong · 2020-04-06T08:17:15Z

browser/speedreader/speedreader_tab_helper.cc

+ SpeedreaderServiceFactory::GetForProfile(profile)->IsEnabled();
+
+ if (!enabled) {
+ active_ = false;


maybe we should add return false; here?
I assume active state is always false when preference is off. isn't it?

actually it was caught by the test :D

simonhong · 2020-04-06T09:23:36Z

browser/speedreader/speedreader_tab_helper.h

+ SpeedreaderTabHelper(const SpeedreaderTabHelper&) = delete;
+ SpeedreaderTabHelper& operator=(SpeedreaderTabHelper&) = delete;
+
+ void UpdateActiveState(content::NavigationHandle* handle);


This could be private method.

simonhong · 2020-04-06T10:46:26Z

browser/ui/views/toolbar/brave_toolbar_view.cc

@@ -182,14 +206,25 @@ void BraveToolbarView::Update(content::WebContents* tab) {
 bookmark_->SetVisible(browser_defaults::bookmarks_enabled &&
 edit_bookmarks_enabled_.GetValue());
 }
+ if (speedreader_) {
+ speedreader_->SetVisible(true);


View is visible by default.
I think this button's visibility is determined by SpeedreaderService::IsEnabled().

just removed that line, if it exists it is visible

simonhong · 2020-04-06T10:47:38Z

browser/ui/views/toolbar/brave_toolbar_view.cc

+ auto* active_contents = GetWebContents();
+ if (active_contents) {
+ auto* tab_helper =
+ speedreader::SpeedreaderTabHelper::FromWebContents(active_contents);
+ if (tab_helper) {
+ speedreader_->SetActive(tab_helper->IsActiveForMainFrame());
+ }
+ }
+ }


I think this logic could be moved into SpeedreaderButton::Update(content::WebContents* web_contents).

simonhong · 2020-04-06T10:50:14Z

browser/ui/browser_commands.cc

+ // TODO(iefremov): We probably should do it with lesser amount of hacks.
+ BrowserView* view = static_cast<BrowserView*>(browser->window());
+ SpeedreaderButton* button =
+ static_cast<BraveToolbarView*>(view->toolbar())->speedreader_button();
+ if (button) {
+ button->Toggle();
+ }


How about control button's toggle status by button itself by overriding OnMouseRelease()?

The command may be executed not only by mouse, so I think it's better as is

Yeah, accelerator could run it. I'm still thinking about how to delete this code...
Ok - how about update button's on/off state by checking kSpeedreaderEnabled value in Button::Update()?
Or monitoring that prefs in the button?

I'm not in favor of splitting this logic... Probably we should just patch or inherit Browser. I've placed a TODO here, mb we can think about it in follow-ups?

For me, most important thing here is avoiding layer violation. I'm fine both adding new method to BraveBrowserView or letting button handle its on/off status by itself.
If you want to fix this layer violation later, it's fine for me. but why not now?

Ah, my bad, I didn't notice this is a layering violation as well. OK, I'll push a fix

iefremov · 2020-04-07T14:29:49Z

@simonhong ptal

simonhong

LGTM 👍

simonhong · 2020-04-09T01:00:41Z

patches/chrome-browser-ui-view_ids.h.patch

+ // IDs for the WebUI-based tab strip. See https://crbug.com/989131.
+ VIEW_ID_WEBUI_TAB_STRIP_TAB_COUNTER,
+ VIEW_ID_WEBUI_TAB_STRIP_NEW_TAB_BUTTON,
+


nit: maybe we don't want blank line to patch file.

simonhong · 2020-04-10T00:14:55Z

Yeah, I'm fine with using our new view id file. I think you forgot to add that file in this PR :)

iefremov self-assigned this Mar 27, 2020

iefremov requested review from mkarolin, simonhong, petemill, AndriusA and bsclifton March 27, 2020 19:29

iefremov requested a review from bridiver as a code owner March 30, 2020 05:59

AndriusA approved these changes Mar 30, 2020

View reviewed changes

simonhong reviewed Mar 31, 2020

View reviewed changes

simonhong reviewed Apr 6, 2020

View reviewed changes

AndriusA mentioned this pull request Apr 7, 2020

Speedreader repository moved into brave core #5142

Merged

32 tasks

simonhong approved these changes Apr 9, 2020

View reviewed changes

iefremov force-pushed the ie_speedreader_button_2 branch from 691f18d to f7227f0 Compare April 9, 2020 11:49

iefremov force-pushed the ie_speedreader_button_2 branch 3 times, most recently from c1eb716 to bb5d7c2 Compare April 10, 2020 15:02

Ivan Efremov added 7 commits April 13, 2020 12:33

Issue 8416: Introduce speedreader button.

1617f66

Issue 8416: Restore missing patch.

51f575d

Fix speedreader icon colors.

ee861c4

Minor cleanup.

c3698bc

Issue 8416: Teach speedreader browsertest about the Button.

d93407e

Disabled speedreader on android.

beda457

Whitespace fix.

66d05f6

Ivan Efremov and others added 7 commits April 13, 2020 12:33

Issue 8416: Fix deps.

5c1b806

Trying to fix android build.

be9b0fd

Issue 8416: Move speedreader activation to the tab helper.

1503aab

Issue 8416: Minor improvements.

c835a62

Issue 8416: Untie speedrader command from ui/views.

91894ec

Get rid of the patch.

d2495b0

Fix android build.

9b98691

iefremov force-pushed the ie_speedreader_button_2 branch from bb5d7c2 to 9b98691 Compare April 13, 2020 05:35

iefremov merged commit 5cfd089 into master Apr 14, 2020

iefremov deleted the ie_speedreader_button_2 branch April 14, 2020 13:25

bbondy added this to the 1.9.x - Release milestone Jun 10, 2020

		@@ -47,6 +49,7 @@ class SpeedReaderBrowserTest : public InProcessBrowserTest {
		};

		IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, SmokeTest) {


		class SpeedreaderService;

		class SpeedreaderServiceFactory : public BrowserContextKeyedServiceFactory {

Issue 8416: Introduce speedreader button. #5085

Issue 8416: Introduce speedreader button. #5085

Conversation

iefremov commented Mar 27, 2020 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

iefremov commented Mar 27, 2020

simonhong commented Mar 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iefremov commented Mar 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iefremov commented Apr 7, 2020

simonhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Apr 10, 2020

iefremov commented Mar 27, 2020 •

edited

Loading

simonhong Apr 2, 2020 •

edited

Loading