Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

speedreader icon not showing up on CR94 #18151

Closed
kkuehlz opened this issue Sep 16, 2021 · 2 comments · Fixed by brave/brave-core#10118
Closed

speedreader icon not showing up on CR94 #18151

kkuehlz opened this issue Sep 16, 2021 · 2 comments · Fixed by brave/brave-core#10118
Assignees
@kkuehlz
Labels
bug feature/speedreader OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass-macOS QA/Yes release-notes/exclude
Milestone
1.31.x - Release

Comments

@kkuehlz
Copy link
Contributor

kkuehlz commented Sep 16, 2021 •
edited
Loading

brave/brave-core#9659 broke on CR94. The lifetime of the throttle_ cannot be assumed to outlive the loader.
@kkuehlz kkuehlz self-assigned this Sep 16, 2021
@kkuehlz kkuehlz changed the title speedreader result not showing up on CR94 speedreader icon not showing up on CR94 Sep 16, 2021
@kkuehlz kkuehlz added bug OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. feature/speedreader QA/Yes release-notes/exclude labels Sep 16, 2021
@kkuehlz kkuehlz mentioned this issue Sep 16, 2021
Closed
24 tasks
@kkuehlz
Copy link
Contributor Author

kkuehlz commented Sep 16, 2021 •
edited
Loading

Test Plan

Go to nytimes and click a bunch of articles on the front page. Every article that is distilled should also have the Speedreader icon present. NOTE: May require a hard refresh if you used the affected nightly and load a cached page.

kkuehlz pushed a commit to brave/brave-core that referenced this issue Sep 16, 2021
speedreader: Pass result correctly for CR94
ce90b1e 
Prior to CR94 we were able to assume that the URLLoaderThrottle would
outlive the URLLoader. This is no longer the case, so we pass the
result_delegate_ to the loader.

Resolves brave/brave-browser#18151
@kkuehlz kkuehlz mentioned this issue Sep 16, 2021
Merged
24 tasks
@mkarolin mkarolin mentioned this issue Sep 16, 2021
Merged
24 tasks
@kkuehlz kkuehlz added this to the 1.31.x - Beta milestone Sep 17, 2021
@kkuehlz kkuehlz mentioned this issue Sep 20, 2021
Open
@stephendonner stephendonner mentioned this issue Sep 21, 2021
Closed
@stephendonner
Copy link

Verified PASSED using

Brave 1.31.58 Chromium: 94.0.4606.54 (Official Build) beta (x86_64)
Revision c8191a1d5cccbf64e8fe7269043f8ace8d74dd05-refs/branch-heads/4606@{#1130}
OS macOS Version 11.6 (Build 20G165)

Steps:

  1. new profile
  2. launched Brave
  3. loaded https://www.nytimes.com/
  4. clicked on Travel, and then loaded https://www.nytimes.com/2021/09/22/travel/barcelona-airbnb.html
  5. clicked on the Reader Mode icon
  6. clicked on the Turn on Speedreader button
example example example example example
Screen Shot 2021-09-22 at 6 37 49 PM Screen Shot 2021-09-22 at 6 37 52 PM Screen Shot 2021-09-22 at 6 34 32 PM Screen Shot 2021-09-22 at 6 34 37 PM Screen Shot 2021-09-22 at 6 35 03 PM

Note: This issue covers the initial Speedreader icon being enabled; enabled/disabled state is being improved further, in:

  1. eWEEK news page is Speedreader-enabled but probably shouldn't be #18231
  2. Speedreader icon disappears after toggling on/off a few times #18234

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kkuehlz kkuehlz

Labels
bug feature/speedreader OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass-macOS QA/Yes release-notes/exclude
Projects
None yet
Milestone
1.31.x - Release
2 participants
@stephendonner @kkuehlz