API to open Shields and Rewards dialog UI #3664

petemill · 2019-10-10T22:06:56Z

Any uplifts should also include:

Master debug build failure - unimplemented pure virtual method 'DumpContextDependencies' in 'BraveActionAPIDependencyManager' brave-browser#6628

Introduces chrome.{ braveRewards, braveShields }.openBrowserActionUI(optional windowId, optional relativePath).

Several upcoming features require being able to summon a dialog attached to Shields or Rewards icons from some other user action that is not clicking on the icon buttons (the only current way to open the dialogs). These features include:

• Offering to bypass paywalls (Publisher Access Pass)
• Offering to turn shields off (Webcompat detection)
• Offering to opt-in to Rewards or complete a Rewards captcha

The API allows a custom path to be sent to open a specific HTML page other than the one configured for the active tab, e.g. offer-paywall.html, solve-captcha.html etc

The API will default to the current window for the profile, which only works when called from a WebUI page. When called from a background script, a windowId will need to be provided so that the popup is shown on a compatible active or recently-active window (and not, e.g., a devtools window).

This was prototyped for the Publisher Access Pass concept at #3565 and modified for this PR to use the Observer pattern.

If there is a webui page which has access to chrome.braveShields or chrome.braveRewards API, then simply call chrome.brave{Shields | Rewards}.openBrowserActionUI() and chrome.brave{Shields | Rewards}.openBrowserActionUI('fake-page.html) to test. A popup should open on the current window

Security / Permissions

The use of the target extension ID coming from the ExtensionFunction means that only contexts which are granted access to an extension's overall API will have permission to open that extension's popup from code.

Test Plan

Open chrome://inspect, switch to the Extensions tab, and inspect either the Rewards or Brave extension. Then use the following API calls, one at a time (uncomment the line you wish to run):

chrome.windows.getAll(wins => {
  // test that we can open in a specified window
  // chrome.braveShields.openBrowserActionUI(wins[0].id)
  // test that we can specify a page (should be 404 though!)
  // chrome.braveShields.openBrowserActionUI(wins[0].id, 'another.html')
})
// Test that we can call in the 'active' window (e.g. if the call is made from NTP, which is the intention for rewards). Run this command then switch focus to an actual browser window for the profile.
// setTimeout(() => chrome.braveShields.openBrowserActionUI('popup.html?grant-id=42'), 2000)
// And with the default popup html
// setTimeout(() => chrome.braveShields.openBrowserActionUI(), 2000)

Then create or open another profile at the same time, and verify that calling the API from one profile cannot open a popup in a window id from another profile.
Also check that the API works for the other profile.

Fix brave/brave-browser#6419

Submitter Checklist:

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.

ryanml

Please check CI for build/browser test errors

petemill · 2019-10-11T05:53:10Z

I think the problem here is a dependency violation in calling code in brave/browser/ui from brave/browser/extensions.
We should probably turn the dependency around and have an observer or delegate in brave/browser/extensions that the browser/ui/BraveActionViewController calls.

simonhong · 2019-10-11T06:15:34Z

browser/extensions/brave_browser_action_util.cc

+#include "brave/browser/ui/views/brave_actions/brave_actions_container.h"
+#include "brave/browser/ui/views/location_bar/brave_location_bar_view.h"
+#include "brave/browser/ui/views/toolbar/brave_toolbar_view.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"


As discussed in DM, these views header files should not be included here.

Changed, now using an obserer pattern

petemill · 2019-10-18T05:15:29Z

This is ready for review. Now using the Observer pattern, modeled on ExtensionActionAPI and ExtensionActionAPI::Observer. Interestingly enough there already is an API to open popups from an extension itself (we're not using that here since we want to open the popup from other contexts, specifying which extension to open). The pattern for that API is to store a reference on the BrowserWindow to the UI model which contains the ExtensionActions, and for the ExtensionActionAPI to go through the browser UI layer and call ExecuteAction directly. I much prefer the observer pattern here so that the UI project looks at the Extension project. Plus, it doesn't need a new patch or chromium_src override, which adding a property to BrowserWindow would require.

petemill · 2019-10-18T05:28:55Z

One question concerning the BrowserContextKeyedAPIFactory, which I am using to get a singleton-per-profile instance. I have exposed one but haven't added a call inside browser_context_keyed_service_factories.cc. Is this neccessary since the service doesn't need to do anything outside of having a member function being explicitly called.

Likewise I haven't called DependsOn( anywhere for the factory instance, and not sure if that's neccessary.

Let me know if you have any thoughts @bridiver @simonhong.

ryanml · 2019-10-18T05:37:56Z

browser/extensions/api/brave_rewards_api.cc

@@ -53,6 +56,26 @@ ExtensionFunction::ResponseAction BraveRewardsCreateWalletFunction::Run() {
  return RespondNow(NoArguments());
 }

+BraveRewardsOpenBrowserActionUIFunction::


Nit: Not sure if this effects lint, but can you format definitions that need to break like: https://github.com/brave/brave-core/blob/9723cdad5bb75b84d6ccdaac55fbc1d137181fd7/browser/extensions/api/brave_rewards_api.cc#L955

ryanml · 2019-10-18T05:38:39Z

browser/extensions/api/brave_rewards_api.cc

+    ) {
+}
+
+ExtensionFunction::ResponseAction BraveRewardsOpenBrowserActionUIFunction::Run(


nit: same as above, see: https://github.com/brave/brave-core/blob/9723cdad5bb75b84d6ccdaac55fbc1d137181fd7/browser/extensions/api/brave_rewards_api.cc#L955

ryanml · 2019-10-18T05:40:00Z

browser/ui/views/brave_actions/brave_actions_container.cc

+    return;
+  }
+  if (actions_[extension_id].view_controller_) {
+    // auto view_controller = static_cast<BraveActionViewController*>(


Can this comment be removed or does it indicate needed code?

Thanks for the spot - removed

simonhong · 2019-10-18T08:49:10Z

browser/extensions/api/brave_action_api.cc

+    }
+  }
+  // Don't support showing action popups in a popup window.
+  if (!browser->SupportsWindowFeature(Browser::FEATURE_TOOLBAR)) {


I think using BrowserWindow::IsToolbarVisible() seems more robust.

Actually I think I'll remove this part. It was there from the first implementation, where we called methods directly on the toolbar. Now, the subscriber for that Browser will either be there or it won't. Plus, the subscriber can decide to still show the popup even if the toolbar is hidden, like what happens in full screen windows when permission bubbles appear.

simonhong · 2019-10-18T11:07:24Z

I think we don't need to have BraveActionAPI instance per profile and I can't find any reason why BraveActionAPI should be tied with Profile.
IMO, just one class that manage observer list seems sufficient.
and I think BraveBrowserProcessImpl seems fine as a owner of it because just one instance is fine for whole browser. WDYT? @petemill

simonhong · 2019-10-18T11:47:35Z

browser/extensions/api/brave_action_api.cc

+    return false;
+  }
+  // Pass event to correct observer
+  for (auto& observer : observers_) {


How about using Browser* and observers map?
I think it should be a little bit efficient than iterating all every time.

bridiver · 2019-10-18T19:51:56Z

common/extensions/api/brave_rewards.json

@@ -379,6 +379,24 @@
      }
    ],
    "functions": [
+      {


if this is a utility function that can be used from any internal extension it should go in it's own api config instead of being duplicated in rewards and shields

as discussed, it's specific to each API (each API opens the UI related to that API)

petemill · 2019-10-21T18:20:39Z

As discussed with @bridiver, we'll attempt to make a KeyedServiceFactory where the key is a Browser instance. That'll solve both of @simonhong 's valid points.

petemill · 2019-10-24T02:47:00Z

This is ready for re-review @simonhong @bridiver now that I've converted from BrowserContextKeyedService to a Browser-keyed-KeyedService. Changes are here: https://github.com/brave/brave-core/compare/bdbf9a7e6d6131c2b5ab12f80acb71c67e3dceba..trigger-internal-extension-popup

bridiver · 2019-10-24T18:01:04Z

browser/ui/views/brave_actions/brave_actions_container.h

@@ -10,6 +10,7 @@
 #include <memory>
 #include <string>

+#include "brave/browser/extensions/api/brave_action_api.h"


I think you might be missing a dep for this

made changes requested

Introduces `chrome.{ braveRewards, braveShields }.openBrowserActionUI(optional windowId, optional relativePath)`. Several upcoming features require being able to summon a dialog attached to Shields or Rewards icons from some other user action that is not clicking on the icon buttons (the only current way to open the dialogs). These features include: • Offering to bypass paywalls (Publisher Access Pass) • Offering to turn shields off (Webcompat detection) • Offering to opt-in to Rewards or complete a Rewards captcha The API allows a custom path to be sent to open a specific HTML page other than the one configured for the active tab, e.g. offer-paywall.html, solve-captcha.html etc The API will default to the current window for the profile, which only works when called from a WebUI page. When called from a background script, a windowId will need to be provided so that the popup is shown on a compatible active or recently-active window (and not, e.g., a devtools window). This was prototyped for the Publisher Access Pass concept at #3565 and modified for this PR to use the Observer pattern. If there is a webui page which has access to chrome.braveShields or chrome.braveRewards API, then simply call `chrome.brave{Shields | Rewards}.openBrowserActionUI()` and `chrome.brave{Shields | Rewards}.openBrowserActionUI('fake-page.html)` to test. A popup should open on the current window Test Plan: Open chrome://inspect, switch to the Extensions tab, and inspect either the Rewards or Brave extension. Then use the following API calls, one at a time (uncomment the line you wish to run): ``` chrome.windows.getAll(wins => { // test that we can open in a specified window // chrome.braveShields.openBrowserActionUI(wins[0].id) // test that we can specify a page (should be 404 though!) // chrome.braveShields.openBrowserActionUI(wins[0].id, 'another.html) }) // Test that we can call in the 'active' window (e.g. if the call is made from NTP, which is the intention for rewards). Run this command then switch focus to an actual browser window for the profile. // setTimeout(() => chrome.braveShields.openBrowserActionUI('popup.html?grant-id=42'), 2000) // And with the default popup html // setTimeout(() => chrome.braveShields.openBrowserActionUI(), 2000) ``` Then create or open another profile at the same time, and verify that calling the API from one profile cannot open a popup in a window id from another profile. Also check that the API works for the other profile.

NejcZdovc

rewards code looks good

petemill · 2019-10-25T16:40:11Z

CI still shows all tests passed even though ultimate status is the grey circle. Merging.

petemill requested review from bsclifton, ryanml, NejcZdovc and mkarolin October 10, 2019 22:06

petemill self-assigned this Oct 10, 2019

petemill force-pushed the trigger-internal-extension-popup branch from 2111317 to 1ad4d45 Compare October 10, 2019 22:08

ryanml approved these changes Oct 11, 2019

View reviewed changes

ryanml previously requested changes Oct 11, 2019

View reviewed changes

NejcZdovc added the feature/rewards label Oct 11, 2019

simonhong reviewed Oct 11, 2019

View reviewed changes

petemill removed request for bsclifton, NejcZdovc and mkarolin October 11, 2019 17:31

ryanml mentioned this pull request Oct 15, 2019

Adding Rewards Widget for New Tab Page #3624

Merged

32 tasks

petemill mentioned this pull request Oct 17, 2019

Add webcompat issue reporting dialog #3420

Merged

32 tasks

petemill force-pushed the trigger-internal-extension-popup branch from 1ad4d45 to 1fc2597 Compare October 18, 2019 05:08

petemill requested review from simonhong, ryanml, mkarolin, NejcZdovc and darkdh October 18, 2019 05:15

petemill force-pushed the trigger-internal-extension-popup branch from 1fc2597 to 9723cda Compare October 18, 2019 05:17

petemill requested a review from bridiver October 18, 2019 05:17

ryanml reviewed Oct 18, 2019

View reviewed changes

petemill force-pushed the trigger-internal-extension-popup branch from 9723cda to 31ba47a Compare October 18, 2019 06:04

simonhong reviewed Oct 18, 2019

View reviewed changes

bridiver reviewed Oct 18, 2019

View reviewed changes

petemill force-pushed the trigger-internal-extension-popup branch 2 times, most recently from bdbf9a7 to c8ecffa Compare October 24, 2019 02:40

petemill force-pushed the trigger-internal-extension-popup branch from c8ecffa to c6921d9 Compare October 24, 2019 02:50

bridiver reviewed Oct 24, 2019

View reviewed changes

petemill force-pushed the trigger-internal-extension-popup branch 2 times, most recently from 463fb09 to 76d045e Compare October 24, 2019 18:44

bridiver approved these changes Oct 24, 2019

View reviewed changes

petemill force-pushed the trigger-internal-extension-popup branch 3 times, most recently from 202f3f3 to 194a65f Compare October 24, 2019 20:02

petemill requested a review from ryanml October 24, 2019 20:21

petemill force-pushed the trigger-internal-extension-popup branch from 194a65f to 73e30e9 Compare October 24, 2019 21:18

NejcZdovc approved these changes Oct 25, 2019

View reviewed changes

petemill merged commit cff1e51 into master Oct 25, 2019

petemill deleted the trigger-internal-extension-popup branch October 25, 2019 16:40

bsclifton mentioned this pull request Oct 28, 2019

Master debug build failure - unimplemented pure virtual method 'DumpContextDependencies' in 'BraveActionAPIDependencyManager' brave/brave-browser#6628

Closed

NejcZdovc added this to the 0.73.x - Nightly milestone Oct 28, 2019

brave-builds pushed a commit that referenced this pull request Nov 4, 2019

Uplift of #3664 (squashed) to dev

d909599

brave-builds pushed a commit that referenced this pull request Nov 4, 2019

Uplift of #3664 (squashed) to beta

cdc503b

This was referenced Nov 4, 2019

API to open Shields and Rewards dialog UI (uplift to 0.72.x) #3871

Merged

API to open Shields and Rewards dialog UI (uplift to 0.71.x) #3872

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API to open Shields and Rewards dialog UI #3664

API to open Shields and Rewards dialog UI #3664

petemill commented Oct 10, 2019 •

edited

Loading

ryanml left a comment

petemill commented Oct 11, 2019

simonhong Oct 11, 2019

petemill Oct 18, 2019

petemill commented Oct 18, 2019 •

edited

Loading

petemill commented Oct 18, 2019

ryanml Oct 18, 2019

petemill Oct 24, 2019

ryanml Oct 18, 2019

petemill Oct 24, 2019

ryanml Oct 18, 2019

petemill Oct 18, 2019

simonhong Oct 18, 2019

petemill Oct 24, 2019

simonhong commented Oct 18, 2019 •

edited

Loading

simonhong Oct 18, 2019

bridiver Oct 18, 2019

petemill Oct 21, 2019

petemill commented Oct 21, 2019

petemill commented Oct 24, 2019 •

edited

Loading

bridiver Oct 24, 2019

petemill Oct 24, 2019

NejcZdovc left a comment

petemill commented Oct 25, 2019

API to open Shields and Rewards dialog UI #3664

API to open Shields and Rewards dialog UI #3664

Conversation

petemill commented Oct 10, 2019 • edited Loading

Any uplifts should also include:

Security / Permissions

Test Plan

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

ryanml left a comment

Choose a reason for hiding this comment

petemill commented Oct 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill commented Oct 18, 2019 • edited Loading

petemill commented Oct 18, 2019

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 commented Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill commented Oct 21, 2019

petemill commented Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc left a comment

Choose a reason for hiding this comment

petemill commented Oct 25, 2019

petemill commented Oct 10, 2019 •

edited

Loading

petemill commented Oct 18, 2019 •

edited

Loading

simonhong commented Oct 18, 2019 •

edited

Loading

petemill commented Oct 24, 2019 •

edited

Loading