Rewards Extension: delay loading until enabled #3698

petemill · 2019-10-15T07:01:03Z

Replaces the rewards toolbar icon with a non-extension-related Button. When the button is clicked, the Rewards extension will be loaded. The extension browser action button will replace the previous 'stub' button and the popup will automatically open.

If the profile starts with rewards already enabled then the extension is immediately started.

Fix brave/brave-browser#3436

Delay extension loading until rewards is enabled
Implement 'stub' native button
Open popup when extension is installed after 'stub' button click
Fix extension data is out of sync because it doesn't 'sync' state at startup (see related issues).
Verify disable-rewards cli flag
Badge text '1' and image scaling
Rewards team to verify that panel does not need to 'sync' more data to state, and fix any impacted browser tests due to extension install delay.

Related but (probably) not blocking:

Rewards extension can't handle any gap in events. <-- should be fixed in this PR, please check @NejcZdovc @ryanml.
Rewards extension UI should get neccessary data (publisher, currency and settings for current tab) when panel opens, and not constantly in the background for every tab.
Rewards background script should only get data which is required for the badge (i.e. when a publisher has been found for a tab, or the notification count changes). The badge does not need to be updated every time active tab or window changes.
Rewards extension should not concern itself with active tab or active windows. It is unneccessary processing, code complexity, and can become out of sync.

Submitter Checklist:

Test Plan:

1. Panel works on fresh data directory

Fresh user data directory
Toolbar Rewards button and icon should be visible
Button should have badge with "1" text content (extension is not loaded)
Clicking button once should open Rewards extension panel (extension has loaded)
Can also verify extension loaded status in Brave Task Manager

2. Existing rewards profile

Existing data directory with 1 profile with rewards enabled
Toolbar Rewards icon should be visible
Toolbar Rewards icon should not have "1" in badge
Clicking rewards icon should open panel, which should show wallet and active tab publisher

Additional profiles in same data directory

Verify 1. and 2. but with new profiles in the same browser session. (App Menu -> Create New Profile). Each profile should have or not have its own copy of the brave rewards extension, and behave as per test 1. and 2.

cli flag to disable

Fresh profile
Launch with --disable-brave-rewards-extension
Rewards toolbar button is not visible
Go to brave://rewards
Enable rewards
Rewards toolbar button is still not visible

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.

petemill · 2019-10-15T17:14:15Z

browser/extensions/BUILD.gn

@@ -78,6 +78,7 @@ source_set("extensions") {

  if (brave_rewards_enabled) {
    deps += [
+      "//brave/components/brave_rewards/browser",


I believe this is an existing missing dep since brave_rewards_api.cc gets data from the rewards service. However, I also believe there's a circular dependency since there is a class within rewards which fires extension events.

petemill · 2019-10-16T20:57:25Z

browser/ui/views/brave_actions/brave_actions_container.cc

-  return rect.size();
+  // Width > Height to give space for a large bubble (especially for shields).
+  // TODO(petemill): Generate based on toolbar size.
+  return gfx::Size(34, 24);


The removed code was actually generating a very incorrect size (height() here is very small, likely because nothing is making the height of the container fill the height of the toolbar). This explicit size is moved from https://github.com/brave/brave-core/pull/3698/files#diff-469d5e5a21b50831d605cce6cae56765L160

petemill · 2019-10-16T20:59:31Z

browser/ui/views/brave_actions/brave_rewards_action_stub_view.cc

+
+namespace {
+  constexpr SkColor kRewardsBadgeBg = SkColorSetRGB(0xfb, 0x54, 0x2b);
+  const std::string kRewardsInitialBadgeText = "1";


Hard-coded string moved from the extension background script, which always ensured that it said "1" until the panel was opened.

Good change 👍

components/brave_rewards/resources/extension/brave_rewards/background/events/rewardsEvents.ts

...brave_rewards/resources/extension/brave_rewards/background/reducers/rewards_panel_reducer.ts

petemill · 2019-10-17T18:04:06Z

patches/chrome-browser-extensions-extension_action_manager.cc.patch

+         ExtensionAction::ActionIconSize(),
+         ExtensionAction::FallbackIcon().AsImageSkia(), nullptr));
+   }
+  BRAVE_GET_EXTENSION_ACTION


See chromium_src override. This is something that was technically 'broken' with the shields and rewards icons previously, but not noticeable. Now that the extension load happens sometimes after window creation and especially whilst the user is focusing on the button, the jump between image sizes was noticeable.

petemill · 2019-10-17T18:04:42Z

chromium_src/chrome/browser/extensions/extension_action_manager.cc

+        profile_, &extension, *action->default_icon(), \
+        brave_actions::kBraveActionGraphicSize, \
+        ExtensionAction::FallbackIcon().AsImageSkia(), nullptr)); \
+  }


See 84a4825#r336149378

See chromium_src override. This is something that was technically 'broken' with the shields and rewards icons previously, but not noticeable. Now that the extension load happens sometimes after window creation and especially whilst the user is focusing on the button, the jump between image sizes was noticeable.

bridiver · 2019-10-18T19:31:03Z

browser/extensions/brave_component_loader.cc

-    Add(IDR_BRAVE_REWARDS, brave_rewards_path);
-  }
+  // Enable rewards extension if already opted-in
+  HandleRewardsEnabledStatus();


nit OnBraveRewardsEnabled would be more conventional here

@bridiver even if that function handles both enabled and disabled?

bridiver

there are some issues with this approach (enables rewards extension when clicked even if rewards doesn't get enabled), but @petemill and I have discussed options moving forward so this is fine as a temporary fix

ryanml

Per comment above we should fetch recurring tip data after enabling, else it looks fine to me.

petemill · 2019-10-18T22:22:30Z

@ryanml please can you add that or point to the API

ryanml

I pushed the change which fetches recurring tips on enable, all looks good to me nice work!

NejcZdovc

A lot of browser tests are broken. https://staging.ci.brave.com/job/brave-browser-build-pr/job/rewards-delay-extension/5/execution/node/436/log/

12 tests failed:
02:59:58      BraveRewardsBrowserTest.AutoContribution (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:1715)
02:59:58      BraveRewardsBrowserTest.ClaimGrantViaPanel (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:1601)
02:59:58      BraveRewardsBrowserTest.ContributionWithLowAmount (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:1830)
02:59:58      BraveRewardsBrowserTest.InsufficientNotificationForInsufficientAmount (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2157)
02:59:58      BraveRewardsBrowserTest.InsufficientNotificationForVerifiedInsufficientAmount (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2198)
02:59:58      BraveRewardsBrowserTest.PendingContributionTip (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2027)
02:59:58      BraveRewardsBrowserTest.RecurringTipForUnverifiedPublisher (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:1809)
02:59:58      BraveRewardsBrowserTest.RecurringTipForVerifiedPublisher (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:1788)
02:59:58      BraveRewardsBrowserTest.TipConnectedPublisherAnon (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2408)
02:59:58      BraveRewardsBrowserTest.TipConnectedPublisherAnonAndConnected (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2428)
02:59:58      BraveRewardsBrowserTest.TipUnverifiedPublisher (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:1768)
02:59:58      BraveRewardsBrowserTest.TipVerifiedPublisher (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:1749)
02:59:58  3 tests timed out:
02:59:58      BraveRewardsBrowserTest.MultipleRecurringOverBudgetAndPartialAutoContribution (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2559)
02:59:58      BraveRewardsBrowserTest.ProcessPendingContributions (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2251)
02:59:58      BraveRewardsBrowserTest.RecurringAndPartialAutoContribution (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2512)

@ryanml can you please help @petemill with them. Thank you

NejcZdovc · 2019-10-19T06:41:58Z

@petemill should I still see extension in Task manager?

I just build the PR on clean profile and opened task manager

ryanml · 2019-10-19T09:39:45Z

Unable to reproduce test failures locally, @petemill are you?

NejcZdovc · 2019-10-19T10:25:53Z

@ryanml one thing could be that locally you are building as debug where CI is doing release which is a lot faster and you probably don't see this race condition on slower build

petemill · 2019-10-22T22:51:03Z

Fixed some of the panel issues via c8df68b

Still some test issues, especially wrt the grant-finished UI showing when the grant is solved elsewhere. Pinged @ryanml and @NejcZdovc about it.

…ound script This is now controlled by native 'stub' button

… after some time of unloadedness

The default extension browser action image was set to 16dp when brave browser actions currently uses 18dp. This was barely noticeable when the extensions were loaded on profile creation since only the first window would make this jump and it would happen when the window is opening whilst other paints are occurring. Now that the rewards extension is delay-loaded when the button is clicked, it is more noticeable.

Adds call to braveRewards.getGrants() if rewards is already enabled at extension startup. Puts get* calls after on* calls so that there is no possibility for items to be missed

ClaimGrant function was clicking on the Finish button. Occasionally this would work because the UI was still there, but since the 'finish' happens from C++, the UI proceeds on its own

petemill · 2019-10-23T06:28:41Z

@petemill should I still see extension in Task manager?

I just build the PR on clean profile and opened task manager

No, that's weird @NejcZdovc . Very weird. I cannot replicate. could you try again?

petemill · 2019-10-23T06:31:37Z

So I fixed the only remaining browser test by removing the forced button click to move from captcha to 'celebration' screen (or whatever you call that!). Not sure why clicking that button worked in master and not on this branch.
On this branch, clicking the button was actually dismissing the celebration screen!
221129c

petemill · 2019-10-23T17:24:16Z

Looks like all tests passed on this one now

00:02:49  SUCCESS: all tests passed.

Just got the normal lint failure.

NejcZdovc

rewards side looks good

petemill requested a review from NejcZdovc as a code owner October 15, 2019 07:01

petemill changed the title ~~Rewards delay extension~~ Rewards delay extension [WIP] Oct 15, 2019

petemill changed the title ~~Rewards delay extension [WIP]~~ Rewards Extension: delay loading until enabled [WIP] Oct 15, 2019

petemill self-assigned this Oct 15, 2019

NejcZdovc added the feature/rewards label Oct 15, 2019

petemill commented Oct 15, 2019

View reviewed changes

petemill force-pushed the rewards-delay-extension branch from 6611baa to 2cc0fa7 Compare October 16, 2019 05:23

petemill requested a review from bridiver as a code owner October 16, 2019 05:23

petemill force-pushed the rewards-delay-extension branch from 2cc0fa7 to 6b91526 Compare October 16, 2019 20:52

petemill commented Oct 16, 2019

View reviewed changes

components/brave_rewards/resources/extension/brave_rewards/background/events/rewardsEvents.ts Outdated Show resolved Hide resolved

petemill commented Oct 16, 2019

View reviewed changes

...brave_rewards/resources/extension/brave_rewards/background/reducers/rewards_panel_reducer.ts Show resolved Hide resolved

petemill changed the title ~~Rewards Extension: delay loading until enabled [WIP]~~ Rewards Extension: delay loading until enabled Oct 16, 2019

petemill force-pushed the rewards-delay-extension branch from 6b91526 to 84a4825 Compare October 17, 2019 17:58

petemill commented Oct 17, 2019

View reviewed changes

petemill requested review from mkarolin and ryanml October 17, 2019 18:06

bridiver reviewed Oct 18, 2019

View reviewed changes

bridiver approved these changes Oct 18, 2019

View reviewed changes

ryanml suggested changes Oct 18, 2019

View reviewed changes

ryanml self-requested a review October 19, 2019 00:07

ryanml approved these changes Oct 19, 2019

View reviewed changes

NejcZdovc suggested changes Oct 19, 2019

View reviewed changes

petemill force-pushed the rewards-delay-extension branch from c8b0db9 to c8df68b Compare October 22, 2019 22:45

petemill added 8 commits October 22, 2019 23:24

Missing deps: extensions -> rewards

cd68501

Rewards: only load extension when user enables rewards

ae37f0f

Rewards Extension Badge: do not controll pre-opt-in state from backgr…

6f1f4ed

…ound script This is now controlled by native 'stub' button

Rewards Extension: make sure state is up to date when extension loads…

b178a58

… after some time of unloadedness

Rewards browser tests wait for popup UI elements in DOM

85f466d

Rewards extension ensure grants fetched at startup

39f967c

Adds call to braveRewards.getGrants() if rewards is already enabled at extension startup. Puts get* calls after on* calls so that there is no possibility for items to be missed

Rewards fix browser test dismissing grant before reading final value

221129c

ClaimGrant function was clicking on the Finish button. Occasionally this would work because the UI was still there, but since the 'finish' happens from C++, the UI proceeds on its own

petemill force-pushed the rewards-delay-extension branch from c8df68b to 221129c Compare October 23, 2019 06:24

NejcZdovc approved these changes Oct 24, 2019

View reviewed changes

petemill merged commit 87e70e8 into master Oct 24, 2019

petemill deleted the rewards-delay-extension branch October 24, 2019 19:55

bughit mentioned this pull request Oct 24, 2019

don't load unused built-in extensions brave/brave-browser#4612

Closed

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

brave-builds pushed a commit that referenced this pull request Oct 28, 2019

Uplift of #3698 (squashed) to dev

bfc9708

This was referenced Oct 28, 2019

Rewards Extension: delay loading until enabled (uplift to 0.72.x) #3802

Merged

Rewards Extension: delay loading until enabled (uplift to 0.71.x) #3803

Merged

bsclifton pushed a commit that referenced this pull request Oct 29, 2019

Uplift of #3698 (squashed) to beta

f07fb58

petemill mentioned this pull request Oct 30, 2019

BAT icon in URL bar is misplaced when not rewards user and screen is 100% scaling brave/brave-browser#6663

Closed

LaurenWags mentioned this pull request Oct 30, 2019

Rewards extension process needs to be terminated when disabled, Follow up to #1475 brave/brave-browser#3436

Closed

This was referenced Oct 30, 2019

Rewards button icon pre-opt-in gets correct scaling #3830

Merged

Uplift to 0.72.x: Rewards button icon pre-opt-in gets correct scaling (#3830) #3834

Merged

Uplift to 0.71.x: Rewards button icon pre-opt-in gets correct scaling (#3830) #3835

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewards Extension: delay loading until enabled #3698

Rewards Extension: delay loading until enabled #3698

petemill commented Oct 15, 2019 •

edited

Loading

petemill Oct 15, 2019

petemill Oct 16, 2019

petemill Oct 16, 2019

ryanml Oct 18, 2019

petemill Oct 17, 2019 •

edited

Loading

petemill Oct 17, 2019

bridiver Oct 18, 2019

petemill Oct 24, 2019

bridiver left a comment

ryanml left a comment

petemill commented Oct 18, 2019

ryanml left a comment

NejcZdovc left a comment

NejcZdovc commented Oct 19, 2019

ryanml commented Oct 19, 2019

NejcZdovc commented Oct 19, 2019

petemill commented Oct 22, 2019

petemill commented Oct 23, 2019 •

edited

Loading

petemill commented Oct 23, 2019

petemill commented Oct 23, 2019

NejcZdovc left a comment

Rewards Extension: delay loading until enabled #3698

Rewards Extension: delay loading until enabled #3698

Conversation

petemill commented Oct 15, 2019 • edited Loading

Submitter Checklist:

Test Plan:

1. Panel works on fresh data directory

2. Existing rewards profile

Additional profiles in same data directory

cli flag to disable

Reviewer Checklist:

After-merge Checklist:

petemill Oct 15, 2019

Choose a reason for hiding this comment

petemill Oct 16, 2019

Choose a reason for hiding this comment

petemill Oct 16, 2019

Choose a reason for hiding this comment

ryanml Oct 18, 2019

Choose a reason for hiding this comment

petemill Oct 17, 2019 • edited Loading

Choose a reason for hiding this comment

petemill Oct 17, 2019

Choose a reason for hiding this comment

bridiver Oct 18, 2019

Choose a reason for hiding this comment

petemill Oct 24, 2019

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

ryanml left a comment

Choose a reason for hiding this comment

petemill commented Oct 18, 2019

ryanml left a comment

Choose a reason for hiding this comment

NejcZdovc left a comment

Choose a reason for hiding this comment

NejcZdovc commented Oct 19, 2019

ryanml commented Oct 19, 2019

NejcZdovc commented Oct 19, 2019

petemill commented Oct 22, 2019

petemill commented Oct 23, 2019 • edited Loading

petemill commented Oct 23, 2019

petemill commented Oct 23, 2019

NejcZdovc left a comment

Choose a reason for hiding this comment

petemill commented Oct 15, 2019 •

edited

Loading

petemill Oct 17, 2019 •

edited

Loading

petemill commented Oct 23, 2019 •

edited

Loading