Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(19)

Issue 955673004: Move gcm-independent parts of push messaging out of gcm namespace and directory (Closed)

Created:
5 years, 10 months ago by kbalazs
Modified:
5 years, 9 months ago
Reviewers:
Peter Beverloo, johnme, Nico
CC:
chromium-reviews, zea+watch_chromium.org, Miguel Garcia, Michael van Ouwerkerk, fgorski, jianli
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Move push messaging out of services/gcm into it's own place Push messaging is a client of gcm and not a part of it. To improve layering it should live in a seperate directory. This CL moves files matching chrome/browser/services/gcm/push_messaging_* into chrome/browser/push_messaging/, move the classes out of the gcm namespace. It also restrains GCMProfileService to be the owner of PushMessagingServiceImpl. Instead it became a KeyedService. By making it such it became easy to clean up the dependencies and ownership concerns between push messaging and gcm. BUG=402486 Committed: https://crrev.com/4596b516cf074ad33f7148d4a069b864487e9ff7 Cr-Commit-Position: refs/heads/master@{#319089}

Patch Set 1 #

Total comments: 3

Patch Set 2 : moving all of push_messaging out #

Total comments: 18

Patch Set 3 : comments and test #

Patch Set 4 : rebase #

Patch Set 5 : buildfix #

Patch Set 6 : buildfix push_messaging_application_id_unittest #

Total comments: 4

Patch Set 7 : test++ #

Total comments: 2

Patch Set 8 : remove redundant cast #

Unified diffs Side-by-side diffs Delta from patch set Stats (+236 lines, -3000 lines) Patch
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/notifications/platform_notification_service_impl.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/prefs/browser_prefs.cc View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/profiles/profile_impl.cc View 1 2 3 4 5 6 3 chunks +4 lines, -5 lines 0 comments Download
A + chrome/browser/push_messaging/OWNERS View 1 0 chunks +-1 lines, --1 lines 0 comments Download
A + chrome/browser/push_messaging/push_messaging_application_id.h View 1 3 chunks +3 lines, -7 lines 0 comments Download
A + chrome/browser/push_messaging/push_messaging_application_id.cc View 1 2 3 3 chunks +1 line, -5 lines 0 comments Download
A + chrome/browser/push_messaging/push_messaging_application_id_unittest.cc View 1 2 3 4 5 3 chunks +4 lines, -8 lines 0 comments Download
A + chrome/browser/push_messaging/push_messaging_browsertest.cc View 1 2 3 4 5 6 14 chunks +20 lines, -22 lines 0 comments Download
A + chrome/browser/push_messaging/push_messaging_constants.h View 1 2 chunks +3 lines, -7 lines 0 comments Download
A + chrome/browser/push_messaging/push_messaging_constants.cc View 1 1 chunk +1 line, -5 lines 0 comments Download
A + chrome/browser/push_messaging/push_messaging_permission_context.h View 1 2 3 4 5 6 3 chunks +4 lines, -7 lines 0 comments Download
A + chrome/browser/push_messaging/push_messaging_permission_context.cc View 1 2 3 4 5 6 3 chunks +1 line, -5 lines 0 comments Download
A + chrome/browser/push_messaging/push_messaging_permission_context_factory.h View 1 2 2 chunks +3 lines, -6 lines 0 comments Download
A + chrome/browser/push_messaging/push_messaging_permission_context_factory.cc View 1 2 chunks +2 lines, -6 lines 0 comments Download
A + chrome/browser/push_messaging/push_messaging_permission_context_unittest.cc View 1 2 3 4 5 6 3 chunks +1 line, -5 lines 0 comments Download
A chrome/browser/push_messaging/push_messaging_service_factory.h View 1 2 1 chunk +35 lines, -0 lines 0 comments Download
A chrome/browser/push_messaging/push_messaging_service_factory.cc View 1 2 1 chunk +51 lines, -0 lines 0 comments Download
A + chrome/browser/push_messaging/push_messaging_service_impl.h View 1 2 3 4 5 6 9 chunks +21 lines, -20 lines 0 comments Download
A + chrome/browser/push_messaging/push_messaging_service_impl.cc View 1 2 3 4 5 6 7 24 chunks +59 lines, -81 lines 0 comments Download
M chrome/browser/services/gcm/OWNERS View 1 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/services/gcm/fake_gcm_profile_service.cc View 1 5 6 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/services/gcm/gcm_profile_service.h View 1 2 3 4 5 6 4 chunks +0 lines, -11 lines 0 comments Download
M chrome/browser/services/gcm/gcm_profile_service.cc View 1 2 3 4 5 6 3 chunks +3 lines, -12 lines 0 comments Download
D chrome/browser/services/gcm/push_messaging_application_id.h View 1 chunk +0 lines, -85 lines 0 comments Download
D chrome/browser/services/gcm/push_messaging_application_id.cc View 1 2 3 1 chunk +0 lines, -172 lines 0 comments Download
D chrome/browser/services/gcm/push_messaging_application_id_unittest.cc View 1 chunk +0 lines, -41 lines 0 comments Download
D chrome/browser/services/gcm/push_messaging_browsertest.cc View 1 1 chunk +0 lines, -1013 lines 0 comments Download
D chrome/browser/services/gcm/push_messaging_constants.h View 1 1 chunk +0 lines, -18 lines 0 comments Download
D chrome/browser/services/gcm/push_messaging_constants.cc View 1 1 chunk +0 lines, -14 lines 0 comments Download
D chrome/browser/services/gcm/push_messaging_permission_context.h View 1 2 3 4 5 6 1 chunk +0 lines, -62 lines 0 comments Download
D chrome/browser/services/gcm/push_messaging_permission_context.cc View 1 2 3 4 5 6 1 chunk +0 lines, -142 lines 0 comments Download
D chrome/browser/services/gcm/push_messaging_permission_context_factory.h View 1 chunk +0 lines, -39 lines 0 comments Download
D chrome/browser/services/gcm/push_messaging_permission_context_factory.cc View 1 chunk +0 lines, -49 lines 0 comments Download
D chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc View 1 2 3 4 5 6 1 chunk +0 lines, -166 lines 0 comments Download
D chrome/browser/services/gcm/push_messaging_service_impl.h View 1 2 3 4 5 6 1 chunk +0 lines, -172 lines 0 comments Download
D chrome/browser/services/gcm/push_messaging_service_impl.cc View 1 2 3 4 5 6 1 chunk +0 lines, -791 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 2 chunks +12 lines, -10 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 2 chunks +1 line, -1 line 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 34 (7 generated)
kbalazs
johnme@chromium.org: Please review changes in push_messaging* peter@chromium.org: Please review changes in push_messaging* thakis@chromium.org: Please review ...
5 years, 10 months ago (2015-02-24 16:14:21 UTC) #2
Nico
lgtm once peter and john are happy
5 years, 10 months ago (2015-02-24 17:09:40 UTC) #3
Peter Beverloo
+Michael, Miguel, Filip, Jian: FYI Hi Balazs, thank you for the patch! Could you please ...
5 years, 10 months ago (2015-02-24 18:08:38 UTC) #4
kbalazs
On 2015/02/24 18:08:38, Peter Beverloo wrote: > +Michael, Miguel, Filip, Jian: FYI > > Hi ...
5 years, 10 months ago (2015-02-24 18:33:25 UTC) #5
Peter Beverloo
On 2015/02/24 18:33:25, kbalazs wrote: > On 2015/02/24 18:08:38, Peter Beverloo wrote: > > +Michael, ...
5 years, 10 months ago (2015-02-24 18:40:27 UTC) #6
kbalazs
There is one complication of moving all push files. Currently the PushMessagingServiecImpl is owned by ...
5 years, 10 months ago (2015-02-25 00:14:51 UTC) #7
Peter Beverloo
On 2015/02/25 00:14:51, kbalazs wrote: > There is one complication of moving all push files. ...
5 years, 10 months ago (2015-02-25 00:23:59 UTC) #8
kbalazs
How do you guys test this code? Is there some public app service that can ...
5 years, 10 months ago (2015-02-25 20:53:02 UTC) #9
Peter Beverloo
On 2015/02/25 20:53:02, kbalazs wrote: > How do you guys test this code? Is there ...
5 years, 10 months ago (2015-02-25 22:24:21 UTC) #10
kbalazs
On 2015/02/25 22:24:21, Peter Beverloo wrote: > On 2015/02/25 20:53:02, kbalazs wrote: > > How ...
5 years, 10 months ago (2015-02-25 23:27:51 UTC) #11
Peter Beverloo
I like this a lot Balazs, thank you so much for doing this!! The explicit ...
5 years, 10 months ago (2015-02-25 23:55:05 UTC) #12
kbalazs
https://codereview.chromium.org/955673004/diff/20001/chrome/browser/profiles/profile_impl.cc File chrome/browser/profiles/profile_impl.cc (left): https://codereview.chromium.org/955673004/diff/20001/chrome/browser/profiles/profile_impl.cc#oldcode694 chrome/browser/profiles/profile_impl.cc:694: gcm::PushMessagingServiceImpl::InitializeForProfile(this); On 2015/02/25 23:55:05, Peter Beverloo wrote: > I ...
5 years, 9 months ago (2015-02-27 22:40:36 UTC) #13
kbalazs
The test was a bit tricky. Previously SetTestingFactoryAndUse was used to override the gcm profile ...
5 years, 9 months ago (2015-02-27 22:56:05 UTC) #14
kbalazs
The test needs more work. Problem is that some tests use KeyedService::SetTestingFactory which shuts down ...
5 years, 9 months ago (2015-03-03 01:10:10 UTC) #15
Peter Beverloo
Perhaps it's OK to only emit a warning, mentioning that behavior in tests is expected ...
5 years, 9 months ago (2015-03-03 15:54:18 UTC) #16
johnme
On 2015/03/03 01:10:10, kbalazs wrote: > The test needs more work. Problem is that some ...
5 years, 9 months ago (2015-03-03 17:59:19 UTC) #17
kbalazs
On 2015/03/03 17:59:19, johnme wrote: > On 2015/03/03 01:10:10, kbalazs wrote: > > The test ...
5 years, 9 months ago (2015-03-03 22:45:11 UTC) #18
kbalazs
On 2015/03/03 15:54:18, Peter Beverloo wrote: > Perhaps it's OK to only emit a warning, ...
5 years, 9 months ago (2015-03-03 22:54:30 UTC) #19
Peter Beverloo
Awesome! Thanks for doing this! :-) lgtm https://codereview.chromium.org/955673004/diff/120001/chrome/browser/push_messaging/push_messaging_service_impl.cc File chrome/browser/push_messaging/push_messaging_service_impl.cc (right): https://codereview.chromium.org/955673004/diff/120001/chrome/browser/push_messaging/push_messaging_service_impl.cc#newcode116 chrome/browser/push_messaging/push_messaging_service_impl.cc:116: PushMessagingServiceFactory::GetForProfile(profile)); PushMessagingServiceFactory::GetForProfile() ...
5 years, 9 months ago (2015-03-03 23:04:57 UTC) #20
kbalazs
https://codereview.chromium.org/955673004/diff/120001/chrome/browser/push_messaging/push_messaging_service_impl.cc File chrome/browser/push_messaging/push_messaging_service_impl.cc (right): https://codereview.chromium.org/955673004/diff/120001/chrome/browser/push_messaging/push_messaging_service_impl.cc#newcode116 chrome/browser/push_messaging/push_messaging_service_impl.cc:116: PushMessagingServiceFactory::GetForProfile(profile)); On 2015/03/03 23:04:56, Peter Beverloo wrote: > PushMessagingServiceFactory::GetForProfile() ...
5 years, 9 months ago (2015-03-03 23:57:26 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/955673004/140001
5 years, 9 months ago (2015-03-04 16:12:01 UTC) #24
commit-bot: I haz the power
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ninja/builds/44505)
5 years, 9 months ago (2015-03-04 16:22:41 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/955673004/140001
5 years, 9 months ago (2015-03-04 16:25:28 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/2027)
5 years, 9 months ago (2015-03-04 16:32:42 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/955673004/140001
5 years, 9 months ago (2015-03-04 17:45:11 UTC) #32
commit-bot: I haz the power
Committed patchset #8 (id:140001)
5 years, 9 months ago (2015-03-04 18:43:36 UTC) #33
commit-bot: I haz the power
5 years, 9 months ago (2015-03-04 18:44:32 UTC) #34
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/4596b516cf074ad33f7148d4a069b864487e9ff7
Cr-Commit-Position: refs/heads/master@{#319089}

Powered by Google App Engine
This is Rietveld 408576698