Move Greaselion handling into Brave extension #6978

emerick · 2020-10-28T17:23:45Z

Resolves brave/brave-browser#12307
Resolves brave/brave-browser#6462

Related Greaselion PR: brave/brave-site-specific-scripts#25

Submitter Checklist:

Test Plan:

Rewards panel test

Clean profile
Start the browser
Verify that you have the latest Greaselion scripts installed (brave://components)
Visit a publisher (e.g., http://twitter.com/emerick)
Verify that Rewards panel shows expected publisher info
Verify that tipping from panel works

Tipping test

Clean profile
Start the browser
Verify that you have the latest Greaselion scripts installed (brave://components)
Visit a publisher (e.g., http://twitter.com/emerick)
Verify that inline tipping works as expected

Extension test

Clean profile
Start the browser
Verify that only Brave extension is running (brave://inspect -> Extensions)
Visit a publisher (e.g., http://twitter.com/emerick)
Still only Brave extension running
Verify that showing panel or clicking inline tip starts the Rewards extension

Ideally, this should be tested for all of our supported publishers. In addition, we should verify that this works with YouTube on 1.16.

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.

NejcZdovc

inline tipping is not shown

clean profile
go to https://twitter.com/emerick
no inline tipping button

emerick · 2020-11-03T15:26:09Z

@NejcZdovc Fixed problem with Twitter inline tips not showing up (scripts need to handle readystatechange event, probably an oversight from splitting them out). Should be good for review again.

emerick · 2020-11-06T23:25:01Z

components/brave_rewards/browser/rewards_service_impl.cc

@@ -400,8 +400,6 @@ void RewardsServiceImpl::OnPreferenceChanged(const std::string& key) {
    return;
  }

-  EnableGreaseLion();


It would be dangerous to do this in response to a preference change, as this currently ends up unloading and reinstalling the Greaselion extension yet leaves the related content script injected and in a non-functional state. It seemed odd that this preference callback was being called at all, but it's due to StateMigration::FreshInstall which sets the three inline tip preferences to true.

I think not calling this here is the safest way forward until/unless Greaselion is changed to be a bit smarter about how it handles this scenario. For the time being, we may want to suggest to the user that a browser restart is required when updating these settings.

emerick · 2020-11-06T23:28:48Z

@NejcZdovc Should be ready for review again. I left a comment regarding the fix I made. Open to other suggestions too, of course; it just seemed like the most expedient approach for 1.18.

emerick · 2020-11-10T17:43:58Z

components/brave_extension/extension/brave_extension/manifest.json

+    "ids": [
+      "*"
+    ],


If we don't specify this, no extensions/apps will be able to communicate with the Brave extension per https://developer.chrome.com/extensions/manifest/externally_connectable

zenparsing · 2020-11-10T19:46:19Z

components/brave_extension/extension/brave_extension/background/greaselion.ts

+
+chrome.runtime.onMessageExternal.addListener(
+  function (msg: any, sender: chrome.runtime.MessageSender, sendResponse: any) {
+    if (!msg) {


Do we need to filter extensions here (e.g. filter for rewards extension or greaselion extensions), for security reasons?

Do you mean similar to what we do in the onConnectExternal listener?

Right - given that we have externally_connectable opened up, is there any way that some random extension could send a message through this listener?

Will enter a follow-up issue to address this.

This will absolutely be required and needs to be uplifted to wherever this PR is ASAP

Agreed, will get it in tomorrow.

emerick · 2020-11-11T00:40:50Z

CI had expected test failures on Mac and passed on all other platforms but Windows. Will restart for Windows.

emerick · 2020-11-11T04:04:38Z

Windows CI passed.

petemill · 2020-11-11T04:12:28Z

components/brave_extension/extension/brave_extension/manifest.json

@@ -19,9 +19,13 @@
  "web_accessible_resources": [
  ],
  "background": {
-    "scripts": ["out/brave_extension_background.bundle.js"]
+    "scripts": ["out/brave_extension_background.bundle.js"],
+    "persistent": true


Why do we need this? If it uses an API that needs the background script to be alive won't that just happen automatically?

It's needed by the webRequest API. I'm not sure if it will stay alive automatically, it's called out in the docs to make it persisent here: https://developers.chrome.com/extensions/background_pages ("The webRequest API is incompatible with non-persistent background pages.")

emerick added the feature/rewards label Oct 28, 2020

emerick self-assigned this Oct 28, 2020

emerick force-pushed the rewards-greaselion-brave-extension branch 3 times, most recently from 057ec27 to 43d7afe Compare October 29, 2020 21:10

emerick marked this pull request as ready for review October 30, 2020 12:26

emerick requested a review from a team as a code owner October 30, 2020 12:26

emerick requested review from zenparsing and NejcZdovc October 30, 2020 12:26

emerick force-pushed the rewards-greaselion-brave-extension branch 5 times, most recently from e0e044c to a35628d Compare November 2, 2020 22:47

NejcZdovc suggested changes Nov 3, 2020

View reviewed changes

emerick requested a review from NejcZdovc November 3, 2020 15:24

emerick force-pushed the rewards-greaselion-brave-extension branch 2 times, most recently from 86a8ee3 to cbb4d99 Compare November 6, 2020 19:52

emerick commented Nov 6, 2020

View reviewed changes

emerick force-pushed the rewards-greaselion-brave-extension branch 3 times, most recently from 19a2739 to 220739b Compare November 10, 2020 17:42

emerick commented Nov 10, 2020

View reviewed changes

NejcZdovc approved these changes Nov 10, 2020

View reviewed changes

zenparsing approved these changes Nov 10, 2020

View reviewed changes

emerick force-pushed the rewards-greaselion-brave-extension branch from 220739b to 7755775 Compare November 10, 2020 19:46

zenparsing reviewed Nov 10, 2020

View reviewed changes

emerick force-pushed the rewards-greaselion-brave-extension branch from 3ca3764 to dbd5a2d Compare November 10, 2020 23:06

emerick added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Nov 11, 2020

emerick added 2 commits November 10, 2020 19:41

Move Greaselion handling into Brave extension

51e25b3

Adjust Greaselion ScriptInjection test to account for need to restart

8c8562c

emerick force-pushed the rewards-greaselion-brave-extension branch from dbd5a2d to 8c8562c Compare November 11, 2020 00:41

emerick merged commit 6a02303 into master Nov 11, 2020

emerick deleted the rewards-greaselion-brave-extension branch November 11, 2020 04:04

emerick added this to the 1.18.x - Nightly milestone Nov 11, 2020

emerick removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Nov 11, 2020

petemill reviewed Nov 11, 2020

View reviewed changes

emerick mentioned this pull request Nov 11, 2020

Only allow known extensions to communicate with Brave extension brave/brave-browser#12631

Closed

This was referenced Nov 23, 2020

Make Brave Rewards extension non-persistent brave/brave-browser#6462

Closed

Inline tip button is shown in Reddit/Twitter/GH even though tipping is disabled in Tips settings brave/brave-browser#12870

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Greaselion handling into Brave extension #6978

Move Greaselion handling into Brave extension #6978

emerick commented Oct 28, 2020 •

edited

Loading

NejcZdovc left a comment

emerick commented Nov 3, 2020

emerick Nov 6, 2020

emerick commented Nov 6, 2020

emerick Nov 10, 2020

zenparsing Nov 10, 2020

emerick Nov 10, 2020

zenparsing Nov 10, 2020

emerick Nov 10, 2020

petemill Nov 11, 2020

emerick Nov 11, 2020

emerick commented Nov 11, 2020

emerick commented Nov 11, 2020

petemill Nov 11, 2020

emerick Nov 11, 2020

Move Greaselion handling into Brave extension #6978

Move Greaselion handling into Brave extension #6978

Conversation

emerick commented Oct 28, 2020 • edited Loading

Submitter Checklist:

Test Plan:

Rewards panel test

Tipping test

Extension test

Reviewer Checklist:

After-merge Checklist:

NejcZdovc left a comment

Choose a reason for hiding this comment

emerick commented Nov 3, 2020

Choose a reason for hiding this comment

emerick commented Nov 6, 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

emerick commented Nov 11, 2020

emerick commented Nov 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emerick commented Oct 28, 2020 •

edited

Loading