Add SharedWorkerServiceImpl::CheckWorkerDependency().

Add SharedWorkerServiceImpl::CheckWorkerDependency().

This method checks the worker dependency of renderer processes and calls IncrementWorkerRefCount and DecrementWorkerRefCount of RenderProcessHostImpl on UI thread if necessary.

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

[horo, 2014-03-05 07:13:12]

kinuko@
Could you please review?

[kinuko, 2014-03-05 08:03:47]

looks good, is there a possibility to have some unittest for the core logic?

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

https://codereview.chromium.org/187533002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.h:95:
ScopedWorkerDependencyChecker(SharedWorkerServiceImpl* service_impl)
nit: explicit

https://codereview.chromium.org/187533002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.h:119: const
std::set<int> GetWorkerDependedRenderers();
GetRenderersWithWorkerDependency?

'worker-depended-renderers' is a bit hard to parse...

[horo, 2014-03-05 08:16:58]

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

https://codereview.chromium.org/187533002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.h:95:
ScopedWorkerDependencyChecker(SharedWorkerServiceImpl* service_impl)
On 2014/03/05 08:03:47, kinuko wrote:
> nit: explicit

Done.

https://codereview.chromium.org/187533002/diff/1/content/browser/shared_worke...
content/browser/shared_worker/shared_worker_service_impl.h:119: const
std::set<int> GetWorkerDependedRenderers();
On 2014/03/05 08:03:47, kinuko wrote:
> GetRenderersWithWorkerDependency?
> 
> 'worker-depended-renderers' is a bit hard to parse...

Done.

[kinuko, 2014-03-05 08:36:42]

Is there a possibility to have some unittest for the core logic?

[horo, 2014-03-06 13:34:24]

On 2014/03/05 08:36:42, kinuko wrote:
> Is there a possibility to have some unittest for the core logic?

Added.

[kinuko, 2014-03-10 04:25:43]

Thanks for adding tests.

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

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl.cc:26: const
std::vector<int> removed_ids) {
nit: const -> const&

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl.cc:320: return
dependend_renderers;
nit: probably you meant depended_ or dependent_ ?

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
File content/browser/shared_worker/shared_worker_service_impl.h (right):

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl.h:95: const
std::vector<int>);
const vector<int>& ?

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl.h:97: class
CONTENT_EXPORT ScopedWorkerDependencyChecker {
Does this class need to be exported?  In a quick look its definition can be
hidden in .cc.

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

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:28: class
NumberGenerator : public base::RefCountedThreadSafe<NumberGenerator> {
You can probably use base::AtomicSequenceNumber instead?

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:111: bool
Post(IPC::Message* message) {
MockRenderer -> MockRendererProcessHost
Post -> OnMessageReceived

feels more natural to me?

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:119: const
IPC::Message* responce_msg = queued_messages_.back();
responce -> response

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:130: int
QueuedMessageCount() { return queued_messages_.size(); }
nit: make it a const method

It'd be better to return size_t if there's no reason not to do so
(EXPECT_... lines will need 'u' suffix for literals if you do so)

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:132:
IPC::Message* PopMessage() {
Can this return scoped_ptr<Message> (and all Check*Msg method take
scoped_ptr<Message>)

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:183: int
route_id) {
nit: it'd read better if these params are prefixed with expected_ (i.e.
expected_url, expected_name, ...)

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:241:
g_worker_dependency_updates;
nit: I think just having two std::vector<int> would be more readable

Also since we're in a anon namespace naming this like a global variable may feel
a bit weird.
s_ prefix is probably better (though it's still less common) or you can just
make it a static fields of SharedWorkerServiceImplTest

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:269:
virtual void TearDown() {
OVERRIDE

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:280:
TEST_F(SharedWorkerServiceImplTest, SimpleTest) {
Can you add some more comments in the test code body (e.g. 'Simulate to create a
worker.', 'Simulate to send a message to a new worker' etc)?

This test does a lot, should all of these tested in one unit test or can this be
split into multiple tests?  I wouldn't call a test 'simple' if it's doing more
than just creating a worker in a single renderer.

[horo, 2014-03-10 11:48:31]

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

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl.cc:26: const
std::vector<int> removed_ids) {
On 2014/03/10 04:25:43, kinuko wrote:
> nit: const -> const&

Done.

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl.cc:320: return
dependend_renderers;
On 2014/03/10 04:25:43, kinuko wrote:
> nit: probably you meant depended_ or dependent_ ?

Done.

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
File content/browser/shared_worker/shared_worker_service_impl.h (right):

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl.h:95: const
std::vector<int>);
On 2014/03/10 04:25:43, kinuko wrote:
> const vector<int>& ?

Done.

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl.h:97: class
CONTENT_EXPORT ScopedWorkerDependencyChecker {
On 2014/03/10 04:25:43, kinuko wrote:
> Does this class need to be exported?  In a quick look its definition can be
> hidden in .cc.

Done.
Moved to .cc.

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

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:28: class
NumberGenerator : public base::RefCountedThreadSafe<NumberGenerator> {
On 2014/03/10 04:25:43, kinuko wrote:
> You can probably use base::AtomicSequenceNumber instead?

Done.

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:111: bool
Post(IPC::Message* message) {
On 2014/03/10 04:25:43, kinuko wrote:
> MockRenderer -> MockRendererProcessHost
> Post -> OnMessageReceived
> 
> feels more natural to me?

Done.

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:119: const
IPC::Message* responce_msg = queued_messages_.back();
On 2014/03/10 04:25:43, kinuko wrote:
> responce -> response

Done.

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:130: int
QueuedMessageCount() { return queued_messages_.size(); }
On 2014/03/10 04:25:43, kinuko wrote:
> nit: make it a const method
> 
> It'd be better to return size_t if there's no reason not to do so
> (EXPECT_... lines will need 'u' suffix for literals if you do so)

Done.

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:132:
IPC::Message* PopMessage() {
On 2014/03/10 04:25:43, kinuko wrote:
> Can this return scoped_ptr<Message> (and all Check*Msg method take
> scoped_ptr<Message>)

Done.

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:183: int
route_id) {
On 2014/03/10 04:25:43, kinuko wrote:
> nit: it'd read better if these params are prefixed with expected_ (i.e.
> expected_url, expected_name, ...)

Done.

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:241:
g_worker_dependency_updates;
On 2014/03/10 04:25:43, kinuko wrote:
> nit: I think just having two std::vector<int> would be more readable
> 
> Also since we're in a anon namespace naming this like a global variable may
feel
> a bit weird.
> s_ prefix is probably better (though it's still less common) or you can just
> make it a static fields of SharedWorkerServiceImplTest

Done.

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:269:
virtual void TearDown() {
On 2014/03/10 04:25:43, kinuko wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/187533002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:280:
TEST_F(SharedWorkerServiceImplTest, SimpleTest) {
On 2014/03/10 04:25:43, kinuko wrote:
> Can you add some more comments in the test code body (e.g. 'Simulate to create
a
> worker.', 'Simulate to send a message to a new worker' etc)?
> 
> This test does a lot, should all of these tested in one unit test or can this
be
> split into multiple tests?  I wouldn't call a test 'simple' if it's doing more
> than just creating a worker in a single renderer.

Done.

[kinuko, 2014-03-11 05:35:41]

lgtm, + some more nit comments

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

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:82:
MockRendererProcessHost(int process_no,
nit: process_no -> process_id to match the terms we actually use in production
code

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:330:
TEST_F(SharedWorkerServiceImplTest, SimpleTest) {
Um... Simple -> Basic maybe? (Still doesn't look simple)

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:339: //
ViewHostMsg_CreateWorker_Params.
Thanks for adding very descriptive comments, but I think you can relax a bit
more on commenting next time (it's very subjective thing and everyone could say
different things, but it feels a bit excessive to me. Say, I'd probably just put
one comment 'Create a pair of message ports and send CreateWorker msg' etc for
line 340-350)

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:342: //
WorkerProcessMsg_CreateWorker shoud be sent to the renderer in which
shoud -> should (here and below)

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:361: //
When SharedWorkerConnector recieves ViewMsg_WorkerCreated(1), it sends
recieve -> receive (here and below)

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:394: //
SharedWorker side shuold recieve MessagePortMsg_Message.
shuold -> should (here and below)

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:416: // The
first rendere host.
rendere -> renderer

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:438:
connector1->route_id());
Some of these common code blocks could be probably factored out to a subroutine?

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:500: // The
second rendere host.
rendere -> renderer

[horo, 2014-03-11 06:55:56]

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

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:82:
MockRendererProcessHost(int process_no,
On 2014/03/11 05:35:41, kinuko wrote:
> nit: process_no -> process_id to match the terms we actually use in production
> code

Done.

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:330:
TEST_F(SharedWorkerServiceImplTest, SimpleTest) {
On 2014/03/11 05:35:41, kinuko wrote:
> Um... Simple -> Basic maybe? (Still doesn't look simple)

Done.

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:342: //
WorkerProcessMsg_CreateWorker shoud be sent to the renderer in which
On 2014/03/11 05:35:41, kinuko wrote:
> shoud -> should (here and below)

Done.

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:361: //
When SharedWorkerConnector recieves ViewMsg_WorkerCreated(1), it sends
On 2014/03/11 05:35:41, kinuko wrote:
> recieve -> receive (here and below)

Done.

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:394: //
SharedWorker side shuold recieve MessagePortMsg_Message.
On 2014/03/11 05:35:41, kinuko wrote:
> shuold -> should (here and below)

Done.

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:416: // The
first rendere host.
On 2014/03/11 05:35:41, kinuko wrote:
> rendere -> renderer

Done.

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:438:
connector1->route_id());
On 2014/03/11 05:35:41, kinuko wrote:
> Some of these common code blocks could be probably factored out to a
subroutine?

I moved PopMessage() calls to Check**() methods to improve the readability.

https://codereview.chromium.org/187533002/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:500: // The
second rendere host.
On 2014/03/11 05:35:41, kinuko wrote:
> rendere -> renderer

Done.

[horo, 2014-03-11 08:07:37]

jochen@
Could you please review content/browser/message_port_message_filter.h in this
cl?
Thank you.

[jochen (gone - plz use gerrit), 2014-03-11 14:31:06]

lgtm

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

https://codereview.chromium.org/187533002/diff/160001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl.cc:25: class
CONTENT_EXPORT ScopedWorkerDependencyChecker {
why do you need to content export this class?

[horo, 2014-03-11 16:15:05]

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

https://codereview.chromium.org/187533002/diff/160001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl.cc:25: class
CONTENT_EXPORT ScopedWorkerDependencyChecker {
On 2014/03/11 14:31:06, jochen wrote:
> why do you need to content export this class?

Done.
Deleted.

[horo, 2014-03-11 16:15:13]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-03-11 16:16:18]

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

[commit-bot: I haz the power, 2014-03-11 20:45:34]

Change committed as 256305