Service Worker: Expose Get/Store/ClearUserData to content layer.

Service Worker: Expose Get/Store/ClearUserData to content layer.

This will be used by chrome/browser/services/gcm/push_messaging_service_impl.cc
to store data obtained in and used by the chrome layer, that is associated with
Service Workers.

If we didn't expose these generic methods, we'd have to make an extra round-trip
to the content layer, and expose single-purpose methods somewhere whose only
purpose would be to call these methods on behalf of PushMessagingServiceImpl.

BUG=437277

[johnme, 2015-01-23 21:55:50]

johnme@chromium.org changed reviewers:
+ nhiroki@chromium.org

[nhiroki, 2015-01-26 03:25:27]

Looks good other than thread-hopping.

https://codereview.chromium.org/875573002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/875573002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:230:
base::Bind(callback, data, success, not_found));
I'd prefer to run the callback on the IO thread because
{Get,Store,Clear}UserData() is assumed to be called on the IO thread. IMO,
function and its callback should be called on the same thread.

(Although other functions are doing the similar thing (running the callback on
the UI thread), probably they need to be fixed in a separate CL)

[johnme, 2015-01-26 11:02:58]

https://codereview.chromium.org/875573002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/875573002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:230:
base::Bind(callback, data, success, not_found));
On 2015/01/26 03:25:26, nhiroki (slow) wrote:
> I'd prefer to run the callback on the IO thread because
> {Get,Store,Clear}UserData() is assumed to be called on the IO thread. IMO,
> function and its callback should be called on the same thread.
> 
> (Although other functions are doing the similar thing (running the callback on
> the UI thread), probably they need to be fixed in a separate CL)

Done (oops, that was actually my intention, but I had a typo).

[nhiroki, 2015-01-26 11:52:30]

https://codereview.chromium.org/875573002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/875573002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:230:
base::Bind(callback, data, success, not_found));
You might want to directly call the callback because we're already on the IO
thread:

  callback.Run(data, success, not_found);

https://codereview.chromium.org/875573002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:239:
base::Bind(callback, success));
ditto.

https://codereview.chromium.org/875573002/diff/40001/content/public/browser/s...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/875573002/diff/40001/content/public/browser/s...
content/public/browser/service_worker_context.h:23: class BrowserContext;
nit: not necessary?

[johnme, 2015-01-26 11:58:54]

Thanks for the review :)

https://codereview.chromium.org/875573002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/875573002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:230:
base::Bind(callback, data, success, not_found));
On 2015/01/26 11:52:30, nhiroki (slow) wrote:
> You might want to directly call the callback because we're already on the IO
> thread:
> 
>   callback.Run(data, success, not_found);

Done (good point!).

https://codereview.chromium.org/875573002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:239:
base::Bind(callback, success));
On 2015/01/26 11:52:30, nhiroki (slow) wrote:
> ditto.

Done.

https://codereview.chromium.org/875573002/diff/40001/content/public/browser/s...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/875573002/diff/40001/content/public/browser/s...
content/public/browser/service_worker_context.h:23: class BrowserContext;
On 2015/01/26 11:52:30, nhiroki (slow) wrote:
> nit: not necessary?

Done.

[nhiroki, 2015-01-26 12:00:31]

LGTM!

[johnme, 2015-01-26 12:08:55]

johnme@chromium.org changed reviewers:
+ jochen@chromium.org

[johnme, 2015-01-26 12:08:55]

jochen@chromium.org: Please review changes in
content/public/browser/service_worker_context.h

Thanks!

[jochen (gone - plz use gerrit), 2015-01-26 15:55:40]

i don't get from the referenced bug or the CL description what this is used for.

I don't think we should add a seemingly random storage mechanism to content for
data used only in chrome (not lgtm)

[johnme, 2015-01-27 16:13:58]

On 2015/01/26 15:55:40, jochen (slow) wrote:
> i don't get from the referenced bug or the CL description what this is used
for.
> 
> I don't think we should add a seemingly random storage mechanism to content
for
> data used only in chrome (not lgtm)

This is intended to be used by https://codereview.chromium.org/866443003 to
store counts of how many notifications have been shown by the recent push
messages (part of the heuristics in https://crbug.com/437277, specifically the
ratio).

Storing the data in the Service Worker database (alongside the push registration
id, etc stored there by the content layer) has several benefits, for example by
storing it per SW instead of per origin, one rogue SW won't affect push
notifications in well-behaved SWs on the same origin (e.g. google.com/mail/ and
google.com/calendar/ wouldn't affect each other), and the data will be
automatically cleared if the Service Worker is unregistered, or cookies are
cleared, etc.

[michaeln, 2015-01-27 21:00:23]

The storage and retrieval of particular values related to push messaging could
be a function of the public content::PushMessagingService (which could use the
storage mechanims provided by the ServiceWorkerStorage class).

[jochen (gone - plz use gerrit), 2015-01-29 14:44:12]

On 2015/01/27 at 21:00:23, michaeln wrote:
> The storage and retrieval of particular values related to push messaging could
be a function of the public content::PushMessagingService (which could use the
storage mechanims provided by the ServiceWorkerStorage class).

yeah, either storing it in chrome/ or making sure that the API is somewhat
restricted to the intended use cases

[michaeln, 2015-01-29 21:14:12]

I see that a concrete impl of the PushMessagingService interface is provided by
the embedder for use by the content lib. So adding virtual methods to that
doesn't work, but a few static class methods the content lib provides to the
embedded could work.

[johnme, 2015-02-04 16:42:56]

Sounds reasonable. Abandoning this patch, and instead I've incorporated such
static methods into https://codereview.chromium.org/883743002