Issue 2301: Enable push messaging #3390

jumde · 2019-09-09T22:49:49Z

fix brave/brave-browser#2301

Submitter Checklist:

Test Plan:

Navigate to brave://settings/privacy
Enable Use Google Services for Push Messaging
Relaunch
Navigate to:

and verify push messaging works.

Navigate to brave://settings/privacy
Disable Use Google Services for Push Messaging and relaunch
Verify that push notifications don't work after push messaging is disabled.

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.

jumde · 2019-09-09T22:51:35Z

patches/chrome-browser-push_messaging-push_messaging_service_impl.cc.patch

+   PushMessagingServiceImpl* push_service =
+       PushMessagingServiceFactory::GetForProfile(profile);
+-  push_service->IncreasePushSubscriptionCount(count, false /* is_pending */);
+  if (push_service) { push_service->IncreasePushSubscriptionCount(count, false /* is_pending */); }


GetForProfile can return NULL, upstream issue here: https://bugs.chromium.org/p/chromium/issues/detail?id=999665

Is this the only place where the check is needed? I'm concerned about https://bugs.chromium.org/p/chromium/issues/detail?id=999665#c2

I'm ok with this patch, given that this check is enough. I'm just not sure if we will not hit any other null push_service pointers.

cc: @bridiver

Looking at the chromium code looks like other push_service pointers are covered -

https://cs.chromium.org/chromium/src/content/browser/push_messaging/push_messaging_context.cc?l=32
https://cs.chromium.org/chromium/src/content/browser/push_messaging/push_messaging_context.cc?l=34
https://cs.chromium.org/chromium/src/content/browser/push_messaging/push_messaging_context.cc?l=43
https://cs.chromium.org/chromium/src/content/browser/push_messaging/push_messaging_manager.cc?l=423

iefremov · 2019-09-10T05:38:31Z

browser/ui/webui/settings/brave_privacy_handler.h

@@ -7,12 +7,18 @@

 #include "chrome/browser/ui/webui/settings/settings_page_ui_handler.h"

+namespace content {
+  class WebUIDataSource;


spacing is not needed

iefremov · 2019-09-10T06:28:15Z

browser/net/brave_static_redirect_network_delegate_helper.cc

@@ -30,6 +30,7 @@ int OnBeforeURLRequest_StaticRedirectWork(
 int OnBeforeURLRequest_StaticRedirectWorkForGURL(
    const GURL& request_url,
    GURL* new_url) {
+  LOG(INFO) << "URL: " << request_url.spec();


this is too much for production, I hope this is something temporary :)

jumde · 2019-09-24T02:41:16Z

patches/components-gcm_driver-instance_id-instance_id_driver.cc.patch

+   std::string group_name =
+       base::FieldTrialList::FindFullName(kInstanceIDFieldTrialName);
+-  return !base::StartsWith(group_name, kInstanceIDFieldTrialDisabledGroupPrefix,
+  return true || !base::StartsWith(group_name, kInstanceIDFieldTrialDisabledGroupPrefix,


To deal with NULL deref: https://bugs.chromium.org/p/chromium/issues/detail?id=999665

I don't understand, either we're not setting this field trial in which case it won't start with "Disabled" or we should set it to something that doesn't start with "Disabled" if we always want it to return true. If we did need to patch it should return true; on its own line before the original return, but it doesn't look to me like we should have to.

This is my bad, was trying to refactor the original patch. Will fix. Sorry about this.

bsclifton · 2019-09-24T06:27:11Z

If a relaunch is required (seems to be, from my testing) should this present a similar banner to what was done for Chromecast (Media Router)? (cc: @rebron)

It does have the Relaunch button, which is a pretty nice visual indicator 😄

bsclifton · 2019-09-24T09:30:58Z

app/brave_generated_resources.grd

@@ -389,6 +389,10 @@ By installing this extension, you are agreeing to the Google Widevine Terms of U
      <message name="IDS_SETTINGS_WEBRTC_POLICY_DISABLE_NON_PROXIED_UDP" desc="Select value">
        Disable non-proxied UDP
      </message>
+      <!-- Push Messaging -->
+      <message name="IDS_SETTINGS_PUSH_MESSAGING" desc="Select value">
+        Use GCM/FCM for Push Messaging


@rebron what do you think of this text? Is this good enough for users? or should this be simplified a bit?

Change to "Use Google Services for Push Messaging"

bsclifton

Ran through test plan - works great 😄 Code changes look good too, pending review feedback from @bridiver and @rebron (some comments left)

Great job on this 😄👍

jumde · 2019-09-24T18:06:45Z

If a relaunch is required (seems to be, from my testing) should this present a similar banner to what was done for Chromecast (Media Router)? (cc: @rebron)

It does have the Relaunch button, which is a pretty nice visual indicator 😄

cc: @petemill

rebron · 2019-09-24T20:04:21Z

If a relaunch is required (seems to be, from my testing) should this present a similar banner to what was done for Chromecast (Media Router)? (cc: @rebron)

It does have the Relaunch button, which is a pretty nice visual indicator 😄

Yes. We should do the same UE as Chromecast (Media Router). Sounds like that's already the case.

diracdeltas · 2019-09-24T20:32:58Z

sec review: lgtm with the current text and the switch off by default

bridiver · 2019-09-30T18:44:17Z

browser/ui/BUILD.gn

@@ -161,6 +163,8 @@ source_set("ui") {
    "//chrome/app:command_ids",
    "//chrome/common",
    "//components/prefs",
+    "//components/gcm_driver:gcm_driver",


this doesn't need gcm_driver anymore, but brave_gcm_driver target does

bridiver · 2019-09-30T18:44:37Z

browser/ui/BUILD.gn

@@ -153,6 +154,7 @@ source_set("ui") {
    "//brave/browser/tor",
    "//brave/common",
    "//brave/components/brave_adblock_ui:generated_resources",
+    "//brave/components/brave_gcm_driver:brave_gcm_driver",


:brave_gcm_driver is redundant. //brave/components/brave_gcm_driver is all you need

bridiver · 2019-10-01T18:59:28Z

components/brave_gcm_driver/brave_gcm_channel_status_syncer.cc

+
+namespace gcm {
+
+static bool gcm_channel_enabled_ = false;


already discussed in DM, but can't have a static var for a per-profile setting

jumde · 2019-10-02T16:29:25Z

How does it differ from the original test? I'm not sure if copypasting it is better than patching the original

@iefremov - One line change to enable the pushMessaging pref. I try to avoid patching as much as possible.

bridiver · 2019-10-02T18:54:13Z

components/brave_gcm_driver/brave_gcm_channel_status_syncer.cc

+#include "brave/components/brave_gcm_driver/brave_gcm_channel_status_syncer.h"
+
+#include "base/memory/ptr_util.h"
+#include "chrome/browser/profiles/profile.h"


you can't have dependencies on chrome/browser inside a component. This class should be moved to brave/browser/gcm_driver/

actually that's a problem because of this https://github.com/brave/brave-core/pull/3390/files#diff-b7494bf5a7e56428c188f4349d20eb49R10

do we still need ^^ ?

yes we are capturing the initial pref status using the subclass BraveGCMChannelStatusSyncer

you can't have dependencies on chrome/browser inside a component. This class should be moved to brave/browser/gcm_driver/

will move.

it seems like we don't need the BraveGCMChannelStatusSyncer class at all anymore and only need BraveGCMChannelStatus?

bridiver · 2019-10-02T19:02:41Z

components/brave_gcm_driver/brave_gcm_channel_status_syncer.cc

+                             user_agent,
+                             url_loader_factory) {
+  // At destruction all the objects will be cleaned up by SupportsUserData
+  BraveGCMChannelStatusSyncer::status =


This is not correct, you've created a static BraveGCMChannelStatus* here. You should be creating a unique_ptr in GetForProfile if profile->GetUserData(kBraveGCMStatusKey) is null

bridiver · 2019-10-02T20:15:58Z

browser/gcm_driver/BUILD.gn

+import("//build/buildflag_header.gni")
+import("//components/gcm_driver/config.gni")
+
+static_library("gcm_driver") {


why a static_library instead of source_set?

bridiver · 2019-10-02T20:17:23Z

browser/ui/webui/settings/brave_privacy_handler.cc

+      gcm::BraveGCMChannelStatus::GetForProfile(profile);
+
+  DCHECK(gcm_channel_status);
+  data_source->AddBoolean("pushMessagingEnabledAtStartup",


should this just be true if USE_GCM_FROM_PLATFORM?

jumde · 2019-10-04T05:23:56Z

https://bugs.chromium.org/p/chromium/issues/detail?id=1011244 - Debug failure on receiving push notifications on some site.

jumde requested review from bridiver and iefremov as code owners September 9, 2019 22:49

jumde commented Sep 9, 2019

View reviewed changes

jumde force-pushed the push_messaging branch from 10c90db to de56006 Compare September 9, 2019 23:07

iefremov reviewed Sep 10, 2019

View reviewed changes

jumde force-pushed the push_messaging branch 3 times, most recently from 07acad0 to fae9111 Compare September 19, 2019 11:12

jumde self-assigned this Sep 21, 2019

jumde requested a review from bsclifton September 21, 2019 03:12

jumde force-pushed the push_messaging branch from fae9111 to 0b45159 Compare September 21, 2019 03:16

capouch mentioned this pull request Sep 21, 2019

Push notifications broken in brave-core brave/brave-browser#2301

Closed

jumde changed the title ~~[WIP] Issue 2301: Enable push messaging~~ Issue 2301: Enable push messaging Sep 21, 2019

jumde force-pushed the push_messaging branch 3 times, most recently from 3050dd0 to e138d62 Compare September 23, 2019 22:34

jumde commented Sep 24, 2019

View reviewed changes

bsclifton reviewed Sep 24, 2019

View reviewed changes

bsclifton requested a review from rebron September 24, 2019 09:31

bsclifton previously approved these changes Sep 24, 2019

View reviewed changes

jumde dismissed bsclifton’s stale review via 5d77c6f September 24, 2019 18:30

jumde force-pushed the push_messaging branch 4 times, most recently from cf9e3ca to 3351a08 Compare September 24, 2019 19:58

jumde force-pushed the push_messaging branch from 9e2f818 to 92b1bf5 Compare September 30, 2019 16:10

iefremov self-requested a review September 30, 2019 17:11

iefremov previously approved these changes Sep 30, 2019

View reviewed changes

bridiver reviewed Sep 30, 2019

View reviewed changes

bridiver reviewed Oct 1, 2019

View reviewed changes

jumde dismissed iefremov’s stale review via 55447ab October 2, 2019 00:06

jumde force-pushed the push_messaging branch 4 times, most recently from 346eb40 to 5a6a017 Compare October 2, 2019 07:33

bridiver reviewed Oct 2, 2019

View reviewed changes

jumde force-pushed the push_messaging branch 2 times, most recently from 4bf7dcf to b475918 Compare October 2, 2019 20:06

bridiver reviewed Oct 2, 2019

View reviewed changes

jumde force-pushed the push_messaging branch 2 times, most recently from efeffcd to 8fd3083 Compare October 3, 2019 21:00

bridiver previously approved these changes Oct 3, 2019

View reviewed changes

jumde added 2 commits October 3, 2019 17:22

Issue 2301: Enable push messaging

8ee11cb

Issue 2301: Enable push messaging

73c132d

jumde dismissed bridiver’s stale review via 73c132d October 4, 2019 00:24

jumde force-pushed the push_messaging branch from d1751e8 to 73c132d Compare October 4, 2019 00:24

bsclifton merged commit cd3df93 into master Oct 4, 2019

bsclifton deleted the push_messaging branch October 4, 2019 17:21

jumde added this to the 0.72.x - Nightly milestone Oct 4, 2019

LesterCovax mentioned this pull request Oct 28, 2019

"Join" extension won't function in Brave brave/brave-browser#2408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 2301: Enable push messaging #3390

Issue 2301: Enable push messaging #3390

jumde commented Sep 9, 2019 •

edited

jumde Sep 9, 2019

iefremov Sep 10, 2019

iefremov Sep 17, 2019

jumde Sep 24, 2019

iefremov Sep 10, 2019

iefremov Sep 10, 2019

jumde Sep 24, 2019

bridiver Sep 24, 2019

jumde Sep 24, 2019

bsclifton commented Sep 24, 2019 •

edited

bsclifton Sep 24, 2019

rebron Sep 24, 2019

bsclifton left a comment

jumde commented Sep 24, 2019

rebron commented Sep 24, 2019 •

edited

diracdeltas commented Sep 24, 2019

bridiver Sep 30, 2019

bridiver Sep 30, 2019

bridiver Oct 1, 2019

jumde commented Oct 2, 2019 •

edited

bridiver Oct 2, 2019

bridiver Oct 2, 2019

bridiver Oct 2, 2019 •

edited

jumde Oct 2, 2019

jumde Oct 2, 2019

bridiver Oct 2, 2019

bridiver Oct 2, 2019

bridiver Oct 2, 2019

bridiver Oct 2, 2019

jumde Oct 3, 2019

jumde commented Oct 4, 2019

Issue 2301: Enable push messaging #3390

Issue 2301: Enable push messaging #3390

Conversation

jumde commented Sep 9, 2019 • edited

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

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

bsclifton commented Sep 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

jumde commented Sep 24, 2019

rebron commented Sep 24, 2019 • edited

diracdeltas commented Sep 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jumde commented Oct 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Oct 2, 2019 • edited

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

jumde commented Oct 4, 2019

jumde commented Sep 9, 2019 •

edited

bsclifton commented Sep 24, 2019 •

edited

rebron commented Sep 24, 2019 •

edited

jumde commented Oct 2, 2019 •

edited

bridiver Oct 2, 2019 •

edited