Implement SharedWorkerServiceImpl::CreateWorker and SharedWorkerInstance and SharedWorkerHost

Implement SharedWorkerServiceImpl::CreateWorker and SharedWorkerInstance and SharedWorkerHost.

SharedWorkerInstance is almost same as WorkerProcessHost::WorkerInstance in content/browser/worker_host/worker_process_host.h.

In the next CL, I will implement SharedWorkerHost::Init() which sends CreateWorker message to the renderer.

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

[horo, 2014-02-26 07:37:42]

kinuko@
Could you please review this CL?

[kinuko, 2014-02-26 08:51:17]

Looks good.

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

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_host.h:16: class SharedWorkerHost {
Would be nice to have a brief class-level comment.

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_host.h:18:
SharedWorkerHost(SharedWorkerInstance* instance);
explicit

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_host.h:19: virtual
~SharedWorkerHost();
nit: probably no need to be virtual?

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

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_instance.cc:50: // b) the names are
both empty, and the urls are equal
This comments can be probably moved to .h

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

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_instance.h:44: };
nit: can we insert one-line space before line 30 and here?

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_instance.h:52: typedef
std::list<FilterInfo> FilterList;
style-nit: basically we prefer having all typedefs (and inner class definitions)
before any methods.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_instance.h:57: // applies to shared
workers.
The last line (only applies to shared workers) looks unnecessary here

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_instance.h:64: // Accessors
nit: end comment with period

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_instance.h:73: return
security_policy_type_;
nit: indent (line 70 and 73)

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

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.cc:91: SharedWorkerHost
*worker(new SharedWorkerHost(instance.release()));
nit: can we create this as scoped_ptr<SharedWorkerHost> and then release on line
94?

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.cc:92:
worker->Init(filter);
We don't need to check the return value of Init() here?

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

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.h:100:
ScopedVector<SharedWorkerHost> worker_hosts_;
question: For my better understanding can you tell me how we were managing the
lifetime of WorkerProcessHost in the original code?

[horo, 2014-02-26 10:36:18]

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

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_host.h:16: class SharedWorkerHost {
On 2014/02/26 08:51:17, kinuko wrote:
> Would be nice to have a brief class-level comment.

Done.

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_host.h:18:
SharedWorkerHost(SharedWorkerInstance* instance);
On 2014/02/26 08:51:17, kinuko wrote:
> explicit

Done.

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_host.h:19: virtual
~SharedWorkerHost();
On 2014/02/26 08:51:17, kinuko wrote:
> nit: probably no need to be virtual?

Done.

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

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_instance.cc:50: // b) the names are
both empty, and the urls are equal
On 2014/02/26 08:51:17, kinuko wrote:
> This comments can be probably moved to .h

Done.

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

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_instance.h:44: };
On 2014/02/26 08:51:17, kinuko wrote:
> nit: can we insert one-line space before line 30 and here?

Done.

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_instance.h:52: typedef
std::list<FilterInfo> FilterList;
On 2014/02/26 08:51:17, kinuko wrote:
> style-nit: basically we prefer having all typedefs (and inner class
definitions)
> before any methods.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Done.

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_instance.h:57: // applies to shared
workers.
On 2014/02/26 08:51:17, kinuko wrote:
> The last line (only applies to shared workers) looks unnecessary here

Done.

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_instance.h:64: // Accessors
On 2014/02/26 08:51:17, kinuko wrote:
> nit: end comment with period

Done.

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_instance.h:73: return
security_policy_type_;
On 2014/02/26 08:51:17, kinuko wrote:
> nit: indent (line 70 and 73)

Done.

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

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.cc:91: SharedWorkerHost
*worker(new SharedWorkerHost(instance.release()));
On 2014/02/26 08:51:17, kinuko wrote:
> nit: can we create this as scoped_ptr<SharedWorkerHost> and then release on
line
> 94?

Done.

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.cc:92:
worker->Init(filter);
On 2014/02/26 08:51:17, kinuko wrote:
> We don't need to check the return value of Init() here?

I changed the return value type of SharedWorkerHost::Init() to void.

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

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.h:100:
ScopedVector<SharedWorkerHost> worker_hosts_;
On 2014/02/26 08:51:17, kinuko wrote:
> question: For my better understanding can you tell me how we were managing
the
> lifetime of WorkerProcessHost in the original code?

The lifetime of WorkerProcessHost is managed by BrowserChildProcessHostImpl.

WorkerProcessHost is deleted in
BrowserChildProcessHostImpl::OnChildDisconnected().

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

[kinuko, 2014-02-26 11:01:59]

lgtm... modulo: if these were new files I'd probably requested some tests. 
Current code doesn't have much logic to be tested but might be nicer to have
some small ones, e.g. for SharedWorkerInstance::Matches.

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

https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.h:100:
ScopedVector<SharedWorkerHost> worker_hosts_;
On 2014/02/26 10:36:19, horo wrote:
> On 2014/02/26 08:51:17, kinuko wrote:
> > question: For my better understanding can you tell me how we were managing
the
> > lifetime of WorkerProcessHost in the original code?
> 
> The lifetime of WorkerProcessHost is managed by BrowserChildProcessHostImpl.
> 
> WorkerProcessHost is deleted in
> BrowserChildProcessHostImpl::OnChildDisconnected().
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

I see- thanks!

[horo, 2014-02-26 14:23:28]

On 2014/02/26 11:01:59, kinuko wrote:
> lgtm... modulo: if these were new files I'd probably requested some tests. 
> Current code doesn't have much logic to be tested but might be nicer to have
> some small ones, e.g. for SharedWorkerInstance::Matches.
> 
>
https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
> File content/browser/shared_worker/shared_worker_service_impl.h (right):
> 
>
https://codereview.chromium.org/177043003/diff/1/content/browser/shared_worke...
> content/browser/shared_worker/shared_worker_service_impl.h:100:
> ScopedVector<SharedWorkerHost> worker_hosts_;
> On 2014/02/26 10:36:19, horo wrote:
> > On 2014/02/26 08:51:17, kinuko wrote:
> > > question: For my better understanding can you tell me how we were managing
> the
> > > lifetime of WorkerProcessHost in the original code?
> > 
> > The lifetime of WorkerProcessHost is managed by BrowserChildProcessHostImpl.
> > 
> > WorkerProcessHost is deleted in
> > BrowserChildProcessHostImpl::OnChildDisconnected().
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...
> 
> I see- thanks!

Added tests for SharedWorkerInstance.

[kinuko, 2014-02-26 14:33:24]

still lgtm, thanks!

https://codereview.chromium.org/177043003/diff/60001/content/browser/shared_w...
File content/browser/shared_worker/shared_worker_instance_unittest.cc (right):

https://codereview.chromium.org/177043003/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_instance_unittest.cc:25: const
wchar_t* name) {
nit: Can these just take const std::string& and const std::string16&?

https://codereview.chromium.org/177043003/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_instance_unittest.cc:39:
SharedWorkerInstance instance1(GURL(L"http://example.com/w.js"),
I don't think we need L"" literals for GURL (it just takes std::string)

[horo, 2014-02-26 16:31:21]

https://codereview.chromium.org/177043003/diff/60001/content/browser/shared_w...
File content/browser/shared_worker/shared_worker_instance_unittest.cc (right):

https://codereview.chromium.org/177043003/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_instance_unittest.cc:25: const
wchar_t* name) {
On 2014/02/26 14:33:25, kinuko wrote:
> nit: Can these just take const std::string& and const std::string16&? 

Changed to use std::string and base::StringPiece

https://codereview.chromium.org/177043003/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_instance_unittest.cc:39:
SharedWorkerInstance instance1(GURL(L"http://example.com/w.js"),
On 2014/02/26 14:33:25, kinuko wrote:
> I don't think we need L"" literals for GURL (it just takes std::string)

Done.

[horo, 2014-02-26 16:55:07]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-02-26 16:56:08]

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

[commit-bot: I haz the power, 2014-02-26 23:13:54]

Change committed as 253620