Implement some mothods in SharedWorkerHost and SharedWorkerServiceImpl

Implement some mothods in SharedWorkerHost and SharedWorkerServiceImpl

BUG=327256
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254130

[horo, 2014-02-27 06:56:29]

kinuko@
Could you please review?

[kinuko, 2014-02-27 11:34:44]

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
File content/browser/shared_worker/shared_worker_host.cc (right):

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_host.cc:60:
SharedWorkerMessageFilter* filter) {
nit: indent

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
File content/browser/shared_worker/shared_worker_host.h (right):

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_host.h:30: // Sends the message to
the SharedWorker in a renderer process.
nit: message -> |message| for parameters

here and below, just for better (chrome-preferred-style) commenting

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_host.h:73: // Relays a message to
the given endpoint.  Takes care of parsing the message
'given endpoint' is not obvious from the signature...

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_host.h:79:
SharedWorkerMessageFilter* parent_render_filter_;
Is this filter to talk to a shared worker, but not to the parent document? If so
this name feels a bit confusing.

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
File content/browser/shared_worker/shared_worker_service_impl.cc (right):

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.cc:132: }
nit: no need of { } for one-line (unless you're writing goto fail in SSL code!)

here and below

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.cc:248: }
Since we often call this (and also sometimes walk through the vector again to
remove the returned one) does it make sense to consider using
base::ScopedPtrHashMap (with pair<filter, route_id> as its key) instead of
base::ScopedVector?

[horo, 2014-02-28 04:59:14]

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
File content/browser/shared_worker/shared_worker_host.cc (right):

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_host.cc:60:
SharedWorkerMessageFilter* filter) {
On 2014/02/27 11:34:44, kinuko wrote:
> nit: indent

Done.

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
File content/browser/shared_worker/shared_worker_host.h (right):

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_host.h:30: // Sends the message to
the SharedWorker in a renderer process.
On 2014/02/27 11:34:44, kinuko wrote:
> nit: message -> |message| for parameters
> 
> here and below, just for better (chrome-preferred-style) commenting

Done.

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_host.h:73: // Relays a message to
the given endpoint.  Takes care of parsing the message
On 2014/02/27 11:34:44, kinuko wrote:
> 'given endpoint' is not obvious from the signature...

Done.

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_host.h:79:
SharedWorkerMessageFilter* parent_render_filter_;
On 2014/02/27 11:34:44, kinuko wrote:
> Is this filter to talk to a shared worker, but not to the parent document? If
so
> this name feels a bit confusing.

Done.

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
File content/browser/shared_worker/shared_worker_service_impl.cc (right):

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.cc:132: }
On 2014/02/27 11:34:44, kinuko wrote:
> nit: no need of { } for one-line (unless you're writing goto fail in SSL
code!)
> 
> here and below

Done.

https://codereview.chromium.org/182693002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.cc:248: }
On 2014/02/27 11:34:44, kinuko wrote:
> Since we often call this (and also sometimes walk through the vector again to
> remove the returned one) does it make sense to consider using
> base::ScopedPtrHashMap (with pair<filter, route_id> as its key) instead of
> base::ScopedVector?

Changed to use base::ScopedPtrHashMap with <ProcessID,WorkerRouteId> pair.

[kinuko, 2014-02-28 06:52:19]

https://codereview.chromium.org/182693002/diff/100001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_service_impl.cc (right):

https://codereview.chromium.org/182693002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl.cc:158: if (!host)
if (!host || !host->instance()) ?

https://codereview.chromium.org/182693002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl.cc:220:
worker_route_id));
In the previous implementation we also used to check if (*iter)->instance() is
non-null. Is it safe to remove the condition?

[horo, 2014-02-28 07:16:16]

https://codereview.chromium.org/182693002/diff/100001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_service_impl.cc (right):

https://codereview.chromium.org/182693002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl.cc:158: if (!host)
On 2014/02/28 06:52:20, kinuko wrote:
> if (!host || !host->instance()) ?

Added null check of instance in SharedWorkerHost.

https://codereview.chromium.org/182693002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl.cc:220:
worker_route_id));
On 2014/02/28 06:52:20, kinuko wrote:
> In the previous implementation we also used to check if (*iter)->instance() is
> non-null. Is it safe to remove the condition?
Added null check of instance in SharedWorkerHost.

[kinuko, 2014-02-28 07:23:06]

lgtm

[horo, 2014-02-28 07:24:08]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-02-28 07:24:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/182693002/140001

[commit-bot: I haz the power, 2014-02-28 15:36:26]

Change committed as 254130