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

Issue 992353003: Decouple Cache Storage messaging from Service Worker/Embedded Worker (Closed)

Created:
5 years, 9 months ago by jsbell
Modified:
5 years, 9 months ago
CC:
chromium-reviews, jsbell+serviceworker_chromium.org, serviceworker-reviews, creis+watch_chromium.org, mlamouri+watch-content_chromium.org, tzik, nasko+codewatch_chromium.org, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, kinuko+serviceworker, markusheintz_, kinuko+watch
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Decouple Cache Storage messaging from Service Worker/Embedded Worker We plan to expose the Cache Storage API independent of Service Worker. Tease the IPC message routing off of Service Worker and Embedded Worker, mirroring the other context-independent storage APIs. With this CL the browser side is independent but the renderer side entry point is still only available to the SW's context. Files have been left in the content/*/service_worker/ directories for now; follow-ups will shorten the names and move them to dedicated directories. BUG=439389 Committed: https://crrev.com/abadb9bf3f02e61f3543ea62ade29a009b18e654 Cr-Commit-Position: refs/heads/master@{#321847}

Patch Set 1 #

Patch Set 2 : Revert whitespace-only change #

Patch Set 3 : Revert whitespace-only changes #

Patch Set 4 : Comments, include tidying #

Patch Set 5 : Address TODOs, dead code removal #

Patch Set 6 : More TODOs resolved, further simplifications #

Patch Set 7 : Remove unused public stubs #

Total comments: 11

Patch Set 8 : Review feedback; no more content/public changes #

Patch Set 9 : Rebased #

Patch Set 10 : Add missing forward declaration #

Total comments: 3

Patch Set 11 : Added DCHECK per review #

Patch Set 12 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1141 lines, -483 lines) Patch
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +7 lines, -0 lines 0 comments Download
A content/browser/service_worker/cache_storage_context_impl.h View 1 2 3 4 5 6 7 1 chunk +87 lines, -0 lines 0 comments Download
A content/browser/service_worker/cache_storage_context_impl.cc View 1 2 3 4 5 1 chunk +105 lines, -0 lines 0 comments Download
A content/browser/service_worker/cache_storage_dispatcher_host.h View 1 2 3 4 5 1 chunk +50 lines, -0 lines 0 comments Download
A content/browser/service_worker/cache_storage_dispatcher_host.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +57 lines, -0 lines 0 comments Download
M content/browser/service_worker/embedded_worker_test_helper.cc View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/service_worker/service_worker_cache_listener.h View 1 2 3 4 5 5 chunks +58 lines, -30 lines 0 comments Download
M content/browser/service_worker/service_worker_cache_listener.cc View 1 2 3 4 5 6 7 8 9 10 11 chunks +104 lines, -95 lines 0 comments Download
M content/browser/service_worker/service_worker_context_core.h View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +0 lines, -14 lines 0 comments Download
M content/browser/service_worker/service_worker_context_core.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +0 lines, -17 lines 0 comments Download
M content/browser/service_worker/service_worker_context_wrapper.h View 1 2 3 4 4 chunks +0 lines, -16 lines 0 comments Download
M content/browser/service_worker/service_worker_context_wrapper.cc View 1 2 3 4 6 chunks +0 lines, -23 lines 0 comments Download
M content/browser/service_worker/service_worker_registration_unittest.cc View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/service_worker/service_worker_storage_unittest.cc View 1 2 3 4 2 chunks +0 lines, -2 lines 0 comments Download
M content/browser/service_worker/service_worker_version.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +0 lines, -2 lines 0 comments Download
M content/browser/service_worker/service_worker_version.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +0 lines, -8 lines 0 comments Download
M content/browser/service_worker/service_worker_version_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -26 lines 0 comments Download
M content/browser/storage_partition_impl.h View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +5 lines, -0 lines 0 comments Download
M content/browser/storage_partition_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +20 lines, -5 lines 0 comments Download
M content/browser/storage_partition_impl_map.cc View 1 chunk +3 lines, -4 lines 0 comments Download
M content/common/content_message_generator.h View 1 chunk +1 line, -0 lines 0 comments Download
A content/common/service_worker/cache_storage_messages.h View 1 2 3 1 chunk +197 lines, -0 lines 0 comments Download
M content/common/service_worker/service_worker_messages.h View 1 2 3 4 5 6 7 8 5 chunks +0 lines, -133 lines 0 comments Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -0 lines 0 comments Download
M content/content_common.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M content/content_renderer.gypi View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +4 lines, -0 lines 0 comments Download
M content/renderer/render_thread_impl.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +4 lines, -1 line 0 comments Download
M content/renderer/render_thread_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +6 lines, -0 lines 0 comments Download
A content/renderer/service_worker/cache_storage_message_filter.h View 1 2 3 1 chunk +32 lines, -0 lines 0 comments Download
A content/renderer/service_worker/cache_storage_message_filter.cc View 1 chunk +38 lines, -0 lines 0 comments Download
M content/renderer/service_worker/embedded_worker_context_client.h View 1 2 3 4 5 6 7 8 9 11 2 chunks +6 lines, -0 lines 0 comments Download
M content/renderer/service_worker/service_worker_cache_storage_dispatcher.h View 1 2 3 5 chunks +85 lines, -45 lines 0 comments Download
M content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc View 1 2 3 20 chunks +133 lines, -49 lines 0 comments Download
M content/renderer/service_worker/service_worker_script_context.h View 1 2 3 4 5 6 7 8 3 chunks +7 lines, -4 lines 0 comments Download
M content/renderer/service_worker/service_worker_script_context.cc View 1 2 3 4 5 6 7 8 11 3 chunks +5 lines, -7 lines 0 comments Download
A content/renderer/service_worker/webserviceworkercachestorage_impl.h View 1 2 3 1 chunk +58 lines, -0 lines 0 comments Download
A content/renderer/service_worker/webserviceworkercachestorage_impl.cc View 1 chunk +63 lines, -0 lines 0 comments Download
M ipc/ipc_message_start.h View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 41 (6 generated)
jsbell
NOT ready for landing - still some TODOs that need addressing, object ownership to analyze, ...
5 years, 9 months ago (2015-03-11 00:07:32 UTC) #2
jsbell
And although this isn't ready for landing and full of TODOs, any early feedback would ...
5 years, 9 months ago (2015-03-11 16:35:38 UTC) #3
jsbell
Big remaining TODO at this point is the origin deletion hookup (for browsing data/quota). I'm ...
5 years, 9 months ago (2015-03-11 20:49:15 UTC) #4
michaeln
ok, i'll take a look this afternoon
5 years, 9 months ago (2015-03-11 20:56:13 UTC) #5
jsbell
On 2015/03/11 20:49:15, jsbell wrote: > Big remaining TODO at this point is the origin ...
5 years, 9 months ago (2015-03-11 22:02:43 UTC) #6
jsbell
Ready for review gavinp, jkarlin, michaeln, kinuko - please take a look?
5 years, 9 months ago (2015-03-11 23:45:22 UTC) #7
jsbell
https://codereview.chromium.org/992353003/diff/110001/content/public/browser/storage_partition.h File content/public/browser/storage_partition.h (right): https://codereview.chromium.org/992353003/diff/110001/content/public/browser/storage_partition.h#newcode43 content/public/browser/storage_partition.h:43: class CacheStorageContext; Oops - I can revert this change ...
5 years, 9 months ago (2015-03-11 23:47:26 UTC) #8
michaeln
This is looking pretty good to me. https://codereview.chromium.org/992353003/diff/110001/content/browser/service_worker/cache_storage_context_impl.cc File content/browser/service_worker/cache_storage_context_impl.cc (right): https://codereview.chromium.org/992353003/diff/110001/content/browser/service_worker/cache_storage_context_impl.cc#newcode45 content/browser/service_worker/cache_storage_context_impl.cc:45: // TODO: ...
5 years, 9 months ago (2015-03-12 02:03:43 UTC) #9
kinuko
It's big, I may be missing some details but I think this lgtm https://codereview.chromium.org/992353003/diff/110001/content/browser/service_worker/cache_storage_context_impl.cc File ...
5 years, 9 months ago (2015-03-12 10:57:26 UTC) #10
jsbell
https://codereview.chromium.org/992353003/diff/110001/content/browser/service_worker/cache_storage_dispatcher_host.cc File content/browser/service_worker/cache_storage_dispatcher_host.cc (right): https://codereview.chromium.org/992353003/diff/110001/content/browser/service_worker/cache_storage_dispatcher_host.cc#newcode18 content/browser/service_worker/cache_storage_dispatcher_host.cc:18: CacheStorageMsgStart, On 2015/03/12 02:03:43, michaeln wrote: > The const ...
5 years, 9 months ago (2015-03-12 15:56:47 UTC) #11
jsbell
+tsepez@ - can you please review the IPC messaging changes? +jam@ - can you please ...
5 years, 9 months ago (2015-03-12 15:59:46 UTC) #13
gavinp
On 2015/03/11 23:45:22, jsbell wrote: > Ready for review > > gavinp, jkarlin, michaeln, kinuko ...
5 years, 9 months ago (2015-03-12 16:28:50 UTC) #14
Tom Sepez
Looks like you're now passing origin information back from the renderer to the browser. What's ...
5 years, 9 months ago (2015-03-12 17:58:31 UTC) #15
jsbell
On 2015/03/12 17:58:31, Tom Sepez wrote: > Looks like you're now passing origin information back ...
5 years, 9 months ago (2015-03-12 18:26:00 UTC) #16
michaeln
> > Looks like you're now passing origin information back from the renderer to the ...
5 years, 9 months ago (2015-03-12 19:01:23 UTC) #17
jsbell
On 2015/03/12 19:01:23, michaeln wrote: > Interesting, like a process specific map of guids to ...
5 years, 9 months ago (2015-03-12 19:22:26 UTC) #18
michaeln
On 2015/03/12 19:22:26, jsbell wrote: > On 2015/03/12 19:01:23, michaeln wrote: > > Interesting, like ...
5 years, 9 months ago (2015-03-12 20:00:43 UTC) #19
kinuko
Yeah it's a really good point- On 2015/03/12 20:00:43, michaeln wrote: > On 2015/03/12 19:22:26, ...
5 years, 9 months ago (2015-03-13 04:19:07 UTC) #20
jsbell
So... given that this is not new behavior for storage APIs, do we want to ...
5 years, 9 months ago (2015-03-13 15:45:56 UTC) #21
jam
On 2015/03/12 18:26:00, jsbell wrote: > On 2015/03/12 17:58:31, Tom Sepez wrote: > > Looks ...
5 years, 9 months ago (2015-03-13 16:36:15 UTC) #22
jsbell
On 2015/03/13 16:36:15, jam wrote: > I defer to security folks ultimately, but it seems ...
5 years, 9 months ago (2015-03-13 16:43:13 UTC) #23
Tom Sepez
On 2015/03/13 16:43:13, jsbell wrote: > On 2015/03/13 16:36:15, jam wrote: > > I defer ...
5 years, 9 months ago (2015-03-13 19:32:33 UTC) #24
jsbell
On 2015/03/13 19:32:33, Tom Sepez wrote: > Ok. File a follow-up bug to address the ...
5 years, 9 months ago (2015-03-13 20:16:38 UTC) #25
Tom Sepez
LGTM as promised.
5 years, 9 months ago (2015-03-13 20:43:26 UTC) #26
jsbell
jam@, michaeln@, gavin@, jkarlin@ - any (more) feedback?
5 years, 9 months ago (2015-03-16 15:54:44 UTC) #27
jkarlin
lgtm https://codereview.chromium.org/992353003/diff/170001/content/browser/service_worker/cache_storage_dispatcher_host.cc File content/browser/service_worker/cache_storage_dispatcher_host.cc (right): https://codereview.chromium.org/992353003/diff/170001/content/browser/service_worker/cache_storage_dispatcher_host.cc#newcode43 content/browser/service_worker/cache_storage_dispatcher_host.cc:43: DCHECK_CURRENTLY_ON(BrowserThread::IO); DCHECK(cache_listener_) so that we know that Init ...
5 years, 9 months ago (2015-03-17 19:55:29 UTC) #28
jsbell
Thanks! FYI (everyone): now that I'm walking through the dependent patches I may tweak this ...
5 years, 9 months ago (2015-03-17 21:38:13 UTC) #29
jsbell
https://codereview.chromium.org/992353003/diff/170001/content/browser/service_worker/cache_storage_dispatcher_host.cc File content/browser/service_worker/cache_storage_dispatcher_host.cc (right): https://codereview.chromium.org/992353003/diff/170001/content/browser/service_worker/cache_storage_dispatcher_host.cc#newcode43 content/browser/service_worker/cache_storage_dispatcher_host.cc:43: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2015/03/17 21:38:13, jsbell wrote: > On 2015/03/17 ...
5 years, 9 months ago (2015-03-18 00:00:10 UTC) #30
michaeln
> jam@, michaeln@, gavin@, jkarlin@ - any (more) feedback? nope, this lgtm
5 years, 9 months ago (2015-03-18 22:30:15 UTC) #31
jsbell
jam@ - OWNERS review of the content/ stuff outside of service_worker?
5 years, 9 months ago (2015-03-19 16:27:36 UTC) #32
jsbell
Oops, - jam is OOO avi@ - can you please review the content/browser/render_process_host_impl*, content/browser/storage_partition_impl* and ...
5 years, 9 months ago (2015-03-20 19:05:23 UTC) #35
Avi (use Gerrit)
lgtm
5 years, 9 months ago (2015-03-20 19:45:31 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/992353003/210001
5 years, 9 months ago (2015-03-23 16:58:16 UTC) #39
commit-bot: I haz the power
Committed patchset #12 (id:210001)
5 years, 9 months ago (2015-03-23 21:04:23 UTC) #40
commit-bot: I haz the power
5 years, 9 months ago (2015-03-23 21:05:07 UTC) #41
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/abadb9bf3f02e61f3543ea62ade29a009b18e654
Cr-Commit-Position: refs/heads/master@{#321847}

Powered by Google App Engine
This is Rietveld 408576698