Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2473483012:
Move content/child/background_sync to Blink. (Closed)
|
Created:
4 years, 1 month ago by adithyas Modified:
4 years, 1 month ago CC:
blink-reviews, blink-reviews-api_chromium.org, blink-worker-reviews_chromium.org, chasej+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, haraken, horo+watch_chromium.org, iclelland+watch_chromium.org, jam, jbroman, jkarlin+watch_chromium.org, jsbell+serviceworker_chromium.org, kinuko+serviceworker, kinuko+watch, michaeln, mlamouri+watch-content_chromium.org, nhiroki, Peter Beverloo, serviceworker-reviews, shimazu+serviceworker_chromium.org, tzik Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove content/child/background_sync to Blink.
- Moves background_sync_provider to third_party/WebKit/Source/modules/background_sync
- Renames background_sync_provider.(h|cc) to BackgroundSyncProvider.(h|cpp)
- Removes WebSyncProvider.h
- Removes WebSyncRegistration.h
- Changes all files to directly use SyncRegistration instead
- Removes unneeded type converters
- Remove unneeded tests in background_sync_type_converters_unittest and
add 2 new tests
NOTE: content/child/background_sync isn't completely removed as background_sync_type_converters.h
is still used in content/renderer
BUG=662134
Committed: https://crrev.com/c5b9f3648b7b5008dfadd7929d84e2e8c329c214
Cr-Commit-Position: refs/heads/master@{#433109}
Patch Set 1 #Patch Set 2 : Some fixes. #Patch Set 3 : Update OWNERS to make presubmit happy #
Total comments: 50
Patch Set 4 : Code review changes #
Total comments: 28
Patch Set 5 : Fix nits #Patch Set 6 : Remove background_sync_type_converters_unittest #
Total comments: 15
Patch Set 7 : Service worker nit #
Total comments: 2
Patch Set 8 : Fix nits #Messages
Total messages: 73 (45 generated)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Move content/child/background_sync to Blink.
- Moves background_sync_provider to
third_party/WebKit/Source/modules/background_sync
- Renames background_sync_provider.(h|cc) to BackgroundSyncProvider.(h|cpp)
- Removes WebSyncProvider.h
- Removes WebSyncRegistration.h
- Changes all files to directly use SyncRegistration instead
- Removes unneeded type converters
- Remove unneeded tests in background_sync_type_converters_unittest and
add 2 new tests
NOTE: content/child/background_sync isn't completely removed as
background_sync_type_converters.h
is still used in content/renderer
BUG=662134
==========
to
==========
Move content/child/background_sync to Blink.
- Moves background_sync_provider to
third_party/WebKit/Source/modules/background_sync
- Renames background_sync_provider.(h|cc) to BackgroundSyncProvider.(h|cpp)
- Removes WebSyncProvider.h
- Removes WebSyncRegistration.h
- Changes all files to directly use SyncRegistration instead
- Removes unneeded type converters
- Remove unneeded tests in background_sync_type_converters_unittest and
add 2 new tests
NOTE: content/child/background_sync isn't completely removed as
background_sync_type_converters.h
is still used in content/renderer
BUG=662134
==========
adithyas@chromium.org changed reviewers: + iclelland@chromium.org, jbroman@chromium.org
adithyas@chromium.org changed required reviewers: + iclelland@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp (right): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:39: // will wipe its callbacks) before 'this' is deleted. It may be safe but is not nice. See the below comment. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:43: WTF::unretained(this), We normally make the object GarbageCollected, but use wrapPersistent(this) to keep the object alive until the callback is complete. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:62: WTF::unretained(this), WTF::passed(std::move(callbacksPtr))))); Ditto. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:95: break; Shall we add: default: NOTREACHED(); ? https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:127: break; Ditto. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:136: Platform::current()->mainThread()->getWebTaskRunner()->postTask( Why do we need to post a task? https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h (right): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/SyncManager.cpp (right): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/SyncManager.cpp:25: new ThreadSpecific<BackgroundSyncProvider>); Is there any per-worker object with which BackgroundSyncProvider should be associated? In general, we want to avoid using ThreadSpecific to store per-worker things.
Thanks for taking this on; this is already looking pretty good. A few comments (mostly style nits), and a few suggestions for further simplifications in a followup CL. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp (right): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:21: DCHECK(Platform::current()); No need to check that Platform exists. If it doesn't, something went very bad a long time ago already (and besides, we'll already get a crash on the next line anyhow). https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:22: Platform::current()->interfaceProvider()->getInterface(std::move(request)); public/platform/InterfaceProvider.h promises to be thread-safe already (in fact, it does a similar post to the main thread), so you should be able to just use this directly in GetBackgroundSyncServicePtr https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:28: mojom::blink::SyncRegistrationPtr& options, This is a reference-to-pointer; you should just use mojom::blink::SyncRegistrationPtr. (You may need to add std::move to the call site in order to do this). https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:29: WebServiceWorkerRegistration* serviceWorkerRegistration, For legacy reasons, owning pointers were sometimes passed as raw pointers. This parameter should probably be a unique_ptr (this will remove the need to create a unique_ptr a few lines below). Similar elsewhere. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:43: WTF::unretained(this), On 2016/11/05 at 13:00:19, haraken wrote: > We normally make the object GarbageCollected, but use wrapPersistent(this) to keep the object alive until the callback is complete. Here and below, it doesn't look like any members are actually used, in which case the methods could be made static (or file-local), and the |this| parameter wouldn't be needed at all. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:65: void BackgroundSyncProvider::RegisterCallback( (here and elsewhere) Blink method names should begin with a lowercase letter (e.g. "registerCallback"). This may get rewritten in the future, but for now camelCase is preferred for consistency. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:95: break; On 2016/11/05 at 13:00:19, haraken wrote: > Shall we add: > > default: > NOTREACHED(); > > ? Doesn't doing so turn a compile-time error (not exhaustively covering the enum) into a runtime error? (here and below) https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:106: WebVector<mojom::blink::SyncRegistration*> results(registrations.size()); In a followup CL, you can switch from WebVector to Vector<mojom::blink::SyncRegistrationPtr>, which is what WTFArray is backed by anyhow (registrations.storage() will give you a const&, or you can pass ownership out of the WTFArray if needed). https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:121: syncGetRegistrationCallbacks->onError(blink::WebSyncError( Similarly, it looks like WebSyncError can move to modules/background_sync/ in a followup CL (and switch from WebString to String). https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:136: Platform::current()->mainThread()->getWebTaskRunner()->postTask( On 2016/11/05 at 13:00:18, haraken wrote: > Why do we need to post a task? I commented on this above. The content-side equivalent of this class isn't thread-safe, but the Blink-side one is (and in fact, is implemented with a post-to-main). https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h (right): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:27: explicit BackgroundSyncProvider(){}; You can remove this constructor; the default one will do the same thing. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:29: // blink::WebSyncProvider implementation Remove this comment, as blink::WebSyncProvider is gone. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:49: mojom::blink::BackgroundSyncServicePtr backgroundSyncService; Data members in blink should be named beginning with m_ (in this case, m_backgroundSyncService). https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:51: DISALLOW_COPY_AND_ASSIGN(BackgroundSyncProvider); In Blink, use WTF_MAKE_NONCOPYABLE instead of DISALLOW_COPY_AND_ASSIGN (and thus wtf/Noncopyable.h instead of base/macros.h). Conventionally this appears at the beginning of the class, before public:. (Yes, there are a lot of these arbitrary differences.) https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:54: } // namespace content namespace blink https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:56: #endif // MODULES_BACKGROUND_SYNC_BACKGROUND_SYNC_PROVIDER_H_ BackgroundSyncProvider_h https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/SyncCallbacks.h (right): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/SyncCallbacks.h:27: class SyncRegistrationCallbacks final It should probably be a followup CL, but this can be rewritten to not need to use Blink API types (WebCallbacks, WebVector, etc.) later on. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/SyncManager.cpp (right): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/SyncManager.cpp:24: sync_provider, nit: no underscores in variable names in Blink (for m_, s_, etc. prefixes) https://www.chromium.org/blink/coding-style#TOC-Names Going back and forth between Blink and the rest of Chromium, this can be annoying to keep straight. People are working on making this consistent in the future. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/SyncManager.cpp:25: new ThreadSpecific<BackgroundSyncProvider>); On 2016/11/05 at 13:00:19, haraken wrote: > Is there any per-worker object with which BackgroundSyncProvider should be associated? In general, we want to avoid using ThreadSpecific to store per-worker things. My two cents: a thread-specific object is how this currently works, so it might be worth maintaining that to avoid mixing too many changes into this one CL. Ultimately, it looks to me (iclelland might know better) that this class can ultimately be merged into SyncManager, which has a more typical lifetime (it's a garbage-collected supplement to the ServiceWorkerRegistration). https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/SyncManager.h (right): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/SyncManager.h:36: enum { UNREGISTERED_SYNC_ID = -1 }; nit: kUnregisteredSyncID (Blink name conventions) https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/background_sync/WebSyncProvider.h (left): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/background_sync/WebSyncProvider.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. third_party/WebKit/public/BUILD.gn should be updated for these file deletions https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameOwnerProperties.h (right): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameOwnerProperties.h:11: This change looks unrelated to this CL; revert?
https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp (right): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:21: DCHECK(Platform::current()); On 2016/11/05 22:51:09, jbroman wrote: > No need to check that Platform exists. If it doesn't, something went very bad a > long time ago already (and besides, we'll already get a crash on the next line > anyhow). Done. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:22: Platform::current()->interfaceProvider()->getInterface(std::move(request)); On 2016/11/05 22:51:09, jbroman wrote: > public/platform/InterfaceProvider.h promises to be thread-safe already (in fact, > it does a similar post to the main thread), so you should be able to just use > this directly in GetBackgroundSyncServicePtr Ah I see, did not know that, thanks! https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:28: mojom::blink::SyncRegistrationPtr& options, On 2016/11/05 22:51:09, jbroman wrote: > This is a reference-to-pointer; you should just use > mojom::blink::SyncRegistrationPtr. (You may need to add std::move to the call > site in order to do this). Done. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:29: WebServiceWorkerRegistration* serviceWorkerRegistration, On 2016/11/05 22:51:08, jbroman wrote: > For legacy reasons, owning pointers were sometimes passed as raw pointers. This > parameter should probably be a unique_ptr (this will remove the need to create a > unique_ptr a few lines below). Similar elsewhere. Done. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:39: // will wipe its callbacks) before 'this' is deleted. On 2016/11/05 13:00:18, haraken wrote: > > It may be safe but is not nice. See the below comment. Acknowledged. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:43: WTF::unretained(this), On 2016/11/05 22:51:09, jbroman wrote: > On 2016/11/05 at 13:00:19, haraken wrote: > > We normally make the object GarbageCollected, but use wrapPersistent(this) to > keep the object alive until the callback is complete. > > Here and below, it doesn't look like any members are actually used, in which > case the methods could be made static (or file-local), and the |this| parameter > wouldn't be needed at all. Done. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:65: void BackgroundSyncProvider::RegisterCallback( On 2016/11/05 22:51:09, jbroman wrote: > (here and elsewhere) Blink method names should begin with a lowercase letter > (e.g. "registerCallback"). This may get rewritten in the future, but for now > camelCase is preferred for consistency. Done. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:106: WebVector<mojom::blink::SyncRegistration*> results(registrations.size()); On 2016/11/05 22:51:08, jbroman wrote: > In a followup CL, you can switch from WebVector to > Vector<mojom::blink::SyncRegistrationPtr>, which is what WTFArray is backed by > anyhow (registrations.storage() will give you a const&, or you can pass > ownership out of the WTFArray if needed). Acknowledged. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h (right): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/11/05 13:00:19, haraken wrote: > > 2016 Done. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:27: explicit BackgroundSyncProvider(){}; On 2016/11/05 22:51:09, jbroman wrote: > You can remove this constructor; the default one will do the same thing. Omitting this line seems to generate a compiler error with ThreadSpecific being unable to find a default constructor (I think it's because using WTF_MAKE_NONCOPYABLE prevents the class from having an implicitly declared default constructor?). https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:29: // blink::WebSyncProvider implementation On 2016/11/05 22:51:09, jbroman wrote: > Remove this comment, as blink::WebSyncProvider is gone. Done. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:49: mojom::blink::BackgroundSyncServicePtr backgroundSyncService; On 2016/11/05 22:51:09, jbroman wrote: > Data members in blink should be named beginning with m_ (in this case, > m_backgroundSyncService). Done. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:51: DISALLOW_COPY_AND_ASSIGN(BackgroundSyncProvider); On 2016/11/05 22:51:09, jbroman wrote: > In Blink, use WTF_MAKE_NONCOPYABLE instead of DISALLOW_COPY_AND_ASSIGN (and thus > wtf/Noncopyable.h instead of base/macros.h). Conventionally this appears at the > beginning of the class, before public:. (Yes, there are a lot of these arbitrary > differences.) Done. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:54: } // namespace content On 2016/11/05 22:51:09, jbroman wrote: > namespace blink Done. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:56: #endif // MODULES_BACKGROUND_SYNC_BACKGROUND_SYNC_PROVIDER_H_ On 2016/11/05 22:51:09, jbroman wrote: > BackgroundSyncProvider_h Done. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/SyncCallbacks.h (right): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/SyncCallbacks.h:27: class SyncRegistrationCallbacks final On 2016/11/05 22:51:09, jbroman wrote: > It should probably be a followup CL, but this can be rewritten to not need to > use Blink API types (WebCallbacks, WebVector, etc.) later on. Acknowledged. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/SyncManager.cpp (right): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/SyncManager.cpp:24: sync_provider, On 2016/11/05 22:51:09, jbroman wrote: > nit: no underscores in variable names in Blink (for m_, s_, etc. prefixes) > https://www.chromium.org/blink/coding-style#TOC-Names > > Going back and forth between Blink and the rest of Chromium, this can be > annoying to keep straight. People are working on making this consistent in the > future. Done. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/SyncManager.h (right): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/SyncManager.h:36: enum { UNREGISTERED_SYNC_ID = -1 }; On 2016/11/05 22:51:09, jbroman wrote: > nit: kUnregisteredSyncID (Blink name conventions) Done. https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/background_sync/WebSyncProvider.h (left): https://codereview.chromium.org/2473483012/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/background_sync/WebSyncProvider.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/11/05 22:51:08, jbroman wrote: > third_party/WebKit/public/BUILD.gn should be updated for these file deletions Done.
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm (with only the most minor of nits) once haraken and iclelland approve Great to see the net -350 LOC in this change. :D https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:7: #include "platform/WebTaskRunner.h" super-nit: This include seems unused now that the post-to-main-thread is gone. https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/SyncManager.cpp (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/SyncManager.cpp:58: std::unique_ptr<SyncRegistrationCallbacks>( super-nit: You might consider using makeUnique (from wtf/PtrUtil.h) for cases like this to avoid having to write the type name twice. I don't feel strongly one way or the other, though. https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/SyncManager.h (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/SyncManager.h:9: #include "modules/background_sync/BackgroundSyncProvider.h" super-nit: if you only need to refer to the pointer (like here), you can use a forward-declaration instead of a #include (and put the #include in the source file) to save compile time.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
haraken@chromium.org changed reviewers: + nhiroki@chromium.org
LGTM with comments. https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:31: WTF::passed(std::move(callbacks))))); Why do we need WTF::passed? It looks strange that we need both std::move and WTF::passed. https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:46: WTF::passed(std::move(callbacks))))); Ditto. https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:96: syncGetRegistrationCallbacks->onSuccess(results); Why can't we directly pass in registrations? https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/SyncManager.cpp (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/SyncManager.cpp:23: DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<BackgroundSyncProvider>, nhiroki@: We might want to consider introducing a PerThreadData class to avoid defining per-thread classes like this with DEFINE_THREAD_SAFE_STATIC_LOCAL. Ideally, the caller should be able to get the BackgroundSyncProvider by PerThreadData::current()->backgroundSyncProvider().
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
(drive-by) also mojo changes lgtm https://codereview.chromium.org/2473483012/diff/60001/content/child/blink_pla... File content/child/blink_platform_impl.h (right): https://codereview.chromium.org/2473483012/diff/60001/content/child/blink_pla... content/child/blink_platform_impl.h:47: class FlingCurveConfiguration; Introduced accidentally by a rebase?
https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:31: WTF::passed(std::move(callbacks))))); On 2016/11/08 07:11:36, haraken wrote: > > Why do we need WTF::passed? It looks strange that we need both std::move and > WTF::passed. > WTF::passed() only binds to rvalues, so this needs to use std::move() to turn callbacks into an rvalue. In base, we provide base::Passed(T*) as a short form of this, so it's possible to write: base::Passed(&callbacks) instead of base::Passed(std::move(callbacks)) (The reason it takes a pointer parameter rather than a mutable reference is because Chromium doesn't allow mutable references. If we introduced a similar construct in WTF, I'm not sure if we'd want to follow the Chromium model or not: I can see an argument either way)
peter@chromium.org changed reviewers: + peter@chromium.org
Thanks for doing this! (Reviewing from the background sync POV) https://codereview.chromium.org/2473483012/diff/60001/content/child/backgroun... File content/child/background_sync/background_sync_type_converters_unittest.cc (right): https://codereview.chromium.org/2473483012/diff/60001/content/child/backgroun... content/child/background_sync/background_sync_type_converters_unittest.cc:1: I'd drop this entire test. The type converter itself has COMPILE_ASSERT_MATCHING_ENUM statements to make sure the mappings are correct, so these are effectively testing static_cast<>. https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:10: #include "public/platform/modules/serviceworker/WebServiceWorkerRegistration.h" micro nit: you can forward declare WebServiceWorkerRegistration instead. https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:14: #include <string> nit: these three includes are unused. https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:25: class BackgroundSyncProvider { BackgroundSyncProvider used to observe the WorkerThread (if any) to trigger its own deletion, and use the lifetime of the BlinkPlatformImpl (tied to the RenderThreadImpl) for instance on the main thread. I can't quite figure out the new story. Since the instances are thread locals, do we keep them around forever -- at least until the next navigation? Does that mean that we may end up with unused Mojo handles for workers that no longer exist? (The BackgroundSyncContext keeps around instances until either the entire context goes (i.e. the profile), or the service had a connection error.) Other features, e.g. Web Payments and Notifications, make sure that the class owning the connection can be garbage collected when the context goes away. Is there a reason not to use that same pattern here? https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerRegistration.h (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerRegistration.h:70: virtual int64_t registrationId() const = 0; micro nit: maybe put this next to scope(), since they're semantically similar https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerRegistration.h:70: virtual int64_t registrationId() const = 0; nit: everything else in this class has empty implementations. I don't know the reason, but why not stay consistent?
https://codereview.chromium.org/2473483012/diff/60001/content/child/blink_pla... File content/child/blink_platform_impl.h (right): https://codereview.chromium.org/2473483012/diff/60001/content/child/blink_pla... content/child/blink_platform_impl.h:47: class FlingCurveConfiguration; On 2016/11/08 07:23:12, dcheng wrote: > Introduced accidentally by a rebase? Yup, removed it :) https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:7: #include "platform/WebTaskRunner.h" On 2016/11/07 19:48:10, jbroman wrote: > super-nit: This include seems unused now that the post-to-main-thread is gone. Done. https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:96: syncGetRegistrationCallbacks->onSuccess(results); On 2016/11/08 07:11:36, haraken wrote: > > Why can't we directly pass in registrations? We can! I'll be fixing this in a follow-up CL (http://crrev.com/2481393002). https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/SyncManager.cpp (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/SyncManager.cpp:58: std::unique_ptr<SyncRegistrationCallbacks>( On 2016/11/07 19:48:10, jbroman wrote: > super-nit: You might consider using makeUnique (from wtf/PtrUtil.h) for cases > like this to avoid having to write the type name twice. I don't feel strongly > one way or the other, though. Done! I like makeUnique better in this case :) https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/SyncManager.h (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/SyncManager.h:9: #include "modules/background_sync/BackgroundSyncProvider.h" On 2016/11/07 19:48:10, jbroman wrote: > super-nit: if you only need to refer to the pointer (like here), you can use a > forward-declaration instead of a #include (and put the #include in the source > file) to save compile time. Done.
(just re-iterating my POV on the lifetime issue) https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:25: class BackgroundSyncProvider { On 2016/11/08 at 16:45:11, Peter Beverloo wrote: > BackgroundSyncProvider used to observe the WorkerThread (if any) to trigger its own deletion, and use the lifetime of the BlinkPlatformImpl (tied to the RenderThreadImpl) for instance on the main thread. > > I can't quite figure out the new story. Since the instances are thread locals, do we keep them around forever -- at least until the next navigation? Does that mean that we may end up with unused Mojo handles for workers that no longer exist? WTF::ThreadSpecific dies when the thread dies. > (The BackgroundSyncContext keeps around instances until either the entire context goes (i.e. the profile), or the service had a connection error.) > > Other features, e.g. Web Payments and Notifications, make sure that the class owning the connection can be garbage collected when the context goes away. Is there a reason not to use that same pattern here? That said, I suggested in a previous comment that this could be merged into the GC-ed SyncManager in a followup CL (along with SyncCallbacks, since the implementations that pass to ScriptPromiseResolver could be in that source file, too).
https://codereview.chromium.org/2473483012/diff/60001/content/child/backgroun... File content/child/background_sync/background_sync_type_converters_unittest.cc (right): https://codereview.chromium.org/2473483012/diff/60001/content/child/backgroun... content/child/background_sync/background_sync_type_converters_unittest.cc:1: On 2016/11/08 16:45:11, Peter Beverloo wrote: > I'd drop this entire test. > > The type converter itself has COMPILE_ASSERT_MATCHING_ENUM statements to make > sure the mappings are correct, so these are effectively testing static_cast<>. Done. https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:10: #include "public/platform/modules/serviceworker/WebServiceWorkerRegistration.h" On 2016/11/08 16:45:11, Peter Beverloo wrote: > micro nit: you can forward declare WebServiceWorkerRegistration instead. Done. https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:14: #include <string> On 2016/11/08 16:45:11, Peter Beverloo wrote: > nit: these three includes are unused. Done. https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:25: class BackgroundSyncProvider { On 2016/11/08 17:39:32, jbroman wrote: > That said, I suggested in a previous comment that this could be merged into the > GC-ed SyncManager in a followup CL (along with SyncCallbacks, since the > implementations that pass to ScriptPromiseResolver could be in that source file, > too). My initial implementation did have one instance of BackgroundSyncProvider per SyncManager instance (which would mean BackgroundSyncProvider got GCed along with SyncManager) but I wasn't sure if it was semantically the same thing as the current implementation (there could be multiple SyncManagers per thread?) https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerRegistration.h (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerRegistration.h:70: virtual int64_t registrationId() const = 0; On 2016/11/08 16:45:11, Peter Beverloo wrote: > nit: everything else in this class has empty implementations. I don't know the > reason, but why not stay consistent? I don't really have a good default value; I would say -1 based on this: https://cs.chromium.org/chromium/src/content/common/service_worker/service_wo..., but I can't include that header file. https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerRegistration.h:70: virtual int64_t registrationId() const = 0; On 2016/11/08 16:45:11, Peter Beverloo wrote: > nit: everything else in this class has empty implementations. I don't know the > reason, but why not stay consistent? Done.
https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/background_sync/SyncManager.cpp (right): https://codereview.chromium.org/2473483012/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/background_sync/SyncManager.cpp:23: DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<BackgroundSyncProvider>, On 2016/11/08 07:11:36, haraken wrote: > > nhiroki@: We might want to consider introducing a PerThreadData class to avoid > defining per-thread classes like this with DEFINE_THREAD_SAFE_STATIC_LOCAL. > > Ideally, the caller should be able to get the BackgroundSyncProvider by > PerThreadData::current()->backgroundSyncProvider(). Filed an issue: https://bugs.chromium.org/p/chromium/issues/detail?id=663632 Let's discuss there :)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nhiroki@chromium.org changed reviewers: - nhiroki@chromium.org
nhiroki@chromium.org changed reviewers: + nhiroki@chromium.org
ServiceWorker changes LGTM
falken@chromium.org changed reviewers: + falken@chromium.org
https://codereview.chromium.org/2473483012/diff/100001/content/child/service_... File content/child/service_worker/web_service_worker_registration_impl.cc (right): https://codereview.chromium.org/2473483012/diff/100001/content/child/service_... content/child/service_worker/web_service_worker_registration_impl.cc:196: } For code health, can we remove registration_id() and just use registrationId()? https://codereview.chromium.org/2473483012/diff/100001/content/child/service_... File content/child/service_worker/web_service_worker_registration_impl.h (right): https://codereview.chromium.org/2473483012/diff/100001/content/child/service_... content/child/service_worker/web_service_worker_registration_impl.h:70: int64_t registrationId() const override; nit: remove the blank line above (we don't add empty lines between overridden methods)
https://codereview.chromium.org/2473483012/diff/100001/content/child/service_... File content/child/service_worker/web_service_worker_registration_impl.cc (right): https://codereview.chromium.org/2473483012/diff/100001/content/child/service_... content/child/service_worker/web_service_worker_registration_impl.cc:196: } On 2016/11/11 07:59:17, falken wrote: > For code health, can we remove registration_id() and just use registrationId()? (it's OK to do this as a followup patch, perhaps just add a TODO in the header file)
https://codereview.chromium.org/2473483012/diff/100001/content/child/service_... File content/child/service_worker/web_service_worker_registration_impl.cc (right): https://codereview.chromium.org/2473483012/diff/100001/content/child/service_... content/child/service_worker/web_service_worker_registration_impl.cc:196: } On 2016/11/11 at 08:08:07, falken wrote: > On 2016/11/11 07:59:17, falken wrote: > > For code health, can we remove registration_id() and just use registrationId()? > > (it's OK to do this as a followup patch, perhaps just add a TODO in the header file) The function is used in 6 other files, so I'll make the changes in another patch. Adding a TODO for now. https://codereview.chromium.org/2473483012/diff/100001/content/child/service_... File content/child/service_worker/web_service_worker_registration_impl.h (right): https://codereview.chromium.org/2473483012/diff/100001/content/child/service_... content/child/service_worker/web_service_worker_registration_impl.h:70: int64_t registrationId() const override; On 2016/11/11 at 07:59:17, falken wrote: > nit: remove the blank line above (we don't add empty lines between overridden methods) Done.
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This is great, thanks! LGTM with nits https://codereview.chromium.org/2473483012/diff/100001/content/child/service_... File content/child/service_worker/web_service_worker_registration_impl.h (right): https://codereview.chromium.org/2473483012/diff/100001/content/child/service_... content/child/service_worker/web_service_worker_registration_impl.h:70: int64_t registrationId() const override; Why the duplication here? Can we remove one or the other instead? (I'm not sure if this is just a style issue, but it looks redundant) https://codereview.chromium.org/2473483012/diff/100001/content/renderer/backg... File content/renderer/background_sync/background_sync_client_impl.cc (left): https://codereview.chromium.org/2473483012/diff/100001/content/renderer/backg... content/renderer/background_sync/background_sync_client_impl.cc:10: #include "content/child/background_sync/background_sync_provider.h" Thanks for cleaning those up. https://codereview.chromium.org/2473483012/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h (right): https://codereview.chromium.org/2473483012/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:22: // object. Nit: re-wrap the paragraph https://codereview.chromium.org/2473483012/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:31: std::unique_ptr<SyncRegistrationCallbacks>); I think you need to #include <memory> to get std::unique_ptr (you're probably including it transitively, but it's not obvious from where) https://codereview.chromium.org/2473483012/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:46: // Helper method that returns an initialized BackgroundSyncServicePtr. Can you mention here that it will connect to the browser's BackgroundSyncService if necessary? (i.e., it's not just a simple getter for m_backgroundSyncService) https://codereview.chromium.org/2473483012/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerRegistration.h (right): https://codereview.chromium.org/2473483012/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerRegistration.h:53: virtual int64_t registrationId() const = 0; This makes the class abstract; there are no other pure virtual methods here. But if the serviceworker folks are okay with it, then it must be okay :) https://codereview.chromium.org/2473483012/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/background_sync/SyncManager.cpp (right): https://codereview.chromium.org/2473483012/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/background_sync/SyncManager.cpp:28: DCHECK(syncProvider); I don't think the DCHECK is necessary here
https://codereview.chromium.org/2473483012/diff/100001/content/child/service_... File content/child/service_worker/web_service_worker_registration_impl.h (right): https://codereview.chromium.org/2473483012/diff/100001/content/child/service_... content/child/service_worker/web_service_worker_registration_impl.h:70: int64_t registrationId() const override; On 2016/11/14 18:43:07, iclelland wrote: > Why the duplication here? Can we remove one or the other instead? (I'm not sure > if this is just a style issue, but it looks redundant) nm; Now I see the comment in the most recent patch. Thanks!
https://codereview.chromium.org/2473483012/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h (right): https://codereview.chromium.org/2473483012/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:22: // object. On 2016/11/14 at 18:43:07, iclelland wrote: > Nit: re-wrap the paragraph Done. https://codereview.chromium.org/2473483012/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:31: std::unique_ptr<SyncRegistrationCallbacks>); On 2016/11/14 at 18:43:07, iclelland wrote: > I think you need to #include <memory> to get std::unique_ptr (you're probably including it transitively, but it's not obvious from where) Done. https://codereview.chromium.org/2473483012/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:46: // Helper method that returns an initialized BackgroundSyncServicePtr. On 2016/11/14 at 18:43:07, iclelland wrote: > Can you mention here that it will connect to the browser's BackgroundSyncService if necessary? (i.e., it's not just a simple getter for m_backgroundSyncService) Done. https://codereview.chromium.org/2473483012/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/background_sync/SyncManager.cpp (right): https://codereview.chromium.org/2473483012/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/background_sync/SyncManager.cpp:28: DCHECK(syncProvider); On 2016/11/14 at 18:43:07, iclelland wrote: > I don't think the DCHECK is necessary here Removed, I had that in for debugging :)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
adithyas@chromium.org changed reviewers: + esprehn@chromium.org
Adding @esprehn to review content/child/blink_platform_impl.* changes
service worker lgtm/2
lgtm
The CQ bit was checked by adithyas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dcheng@chromium.org, jbroman@chromium.org, nhiroki@chromium.org, iclelland@chromium.org Link to the patchset: https://codereview.chromium.org/2473483012/#ps140001 (title: "Fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jbroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Move content/child/background_sync to Blink.
- Moves background_sync_provider to
third_party/WebKit/Source/modules/background_sync
- Renames background_sync_provider.(h|cc) to BackgroundSyncProvider.(h|cpp)
- Removes WebSyncProvider.h
- Removes WebSyncRegistration.h
- Changes all files to directly use SyncRegistration instead
- Removes unneeded type converters
- Remove unneeded tests in background_sync_type_converters_unittest and
add 2 new tests
NOTE: content/child/background_sync isn't completely removed as
background_sync_type_converters.h
is still used in content/renderer
BUG=662134
==========
to
==========
Move content/child/background_sync to Blink.
- Moves background_sync_provider to
third_party/WebKit/Source/modules/background_sync
- Renames background_sync_provider.(h|cc) to BackgroundSyncProvider.(h|cpp)
- Removes WebSyncProvider.h
- Removes WebSyncRegistration.h
- Changes all files to directly use SyncRegistration instead
- Removes unneeded type converters
- Remove unneeded tests in background_sync_type_converters_unittest and
add 2 new tests
NOTE: content/child/background_sync isn't completely removed as
background_sync_type_converters.h
is still used in content/renderer
BUG=662134
==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from
==========
Move content/child/background_sync to Blink.
- Moves background_sync_provider to
third_party/WebKit/Source/modules/background_sync
- Renames background_sync_provider.(h|cc) to BackgroundSyncProvider.(h|cpp)
- Removes WebSyncProvider.h
- Removes WebSyncRegistration.h
- Changes all files to directly use SyncRegistration instead
- Removes unneeded type converters
- Remove unneeded tests in background_sync_type_converters_unittest and
add 2 new tests
NOTE: content/child/background_sync isn't completely removed as
background_sync_type_converters.h
is still used in content/renderer
BUG=662134
==========
to
==========
Move content/child/background_sync to Blink.
- Moves background_sync_provider to
third_party/WebKit/Source/modules/background_sync
- Renames background_sync_provider.(h|cc) to BackgroundSyncProvider.(h|cpp)
- Removes WebSyncProvider.h
- Removes WebSyncRegistration.h
- Changes all files to directly use SyncRegistration instead
- Removes unneeded type converters
- Remove unneeded tests in background_sync_type_converters_unittest and
add 2 new tests
NOTE: content/child/background_sync isn't completely removed as
background_sync_type_converters.h
is still used in content/renderer
BUG=662134
Committed: https://crrev.com/c5b9f3648b7b5008dfadd7929d84e2e8c329c214
Cr-Commit-Position: refs/heads/master@{#433109}
==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c5b9f3648b7b5008dfadd7929d84e2e8c329c214 Cr-Commit-Position: refs/heads/master@{#433109} |
