Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2627123003:
SharedWorker: Simplify connection sequence (Closed)
|
Created:
3 years, 11 months ago by nhiroki Modified:
3 years, 11 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, dglazkov+blink, darin-cc_chromium.org, blink-reviews, kinuko+watch, blink-reviews-api_chromium.org, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSharedWorker: Simplify connection sequence
This is a cleanup CL and should not change behavior.
Before this CL, shared worker connection sequence consists of two pathes:
1) SharedWorkerRepositoryClientImpl::conenct (in blink)
- SharedWorkerRepository::createSharedWorkerConnector (in chromium)
* sends a worker creation message to the browser
* returns an instance of WebSharedWorkerProxy
2) SharedWorkerRepositoryClientImpl::connect (in blink)
- SharedWorkerConnector::connect (in blink)
- WebSharedWorkerProxy::connect (in chromium)
* sends a connection request to the browser
After this CL, they are merged into one path like this:
1) SharedWorkerRepositoryClientImpl::connect (in blink)
- SharedWorkerRepository::connect (in chromium)
* creates an instance of WebSharedWorkerProxy
- WebSharedWorkerProxy::connect (in chromium)
* sends a worker creation message to the browser
* sends a connection request to the browser
BUG=612308
Review-Url: https://codereview.chromium.org/2627123003
Cr-Commit-Position: refs/heads/master@{#444557}
Committed: https://chromium.googlesource.com/chromium/src/+/b28a77ff0b6d09ba31cafb8029eda163accae3c8
Patch Set 1 #
Total comments: 8
Patch Set 2 : address review comments #Patch Set 3 : rebase #Patch Set 4 : maybe fix win compile failures #
Total comments: 2
Patch Set 5 : call connect() in the ctor #Patch Set 6 : remove unnecessary forward declaration #Messages
Total messages: 56 (41 generated)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
SharedWorker: Simplify connection sequence
BUG=
==========
to
==========
SharedWorker: Simplify connection sequence
This is a cleanup CL and should not change behavior.
Before this CL, shared worker connection sequence consists of two pathes:
1) SharedWorkerRepositoryClientImpl::conenct (in blink)
- SharedWorkerRepository::createSharedWorkerConnector (in chromium)
* sends a worker creation message to the browser
* returns an instance of WebSharedWorkerProxy
2) SharedWorkerRepositoryClientImpl::connect (in blink)
- SharedWorkerConnector::connect (in blink)
- WebSharedWorkerProxy::connect (in chromium)
* sends a connection request to the browser
After this CL, they are merged into one path like this:
1) SharedWorkerRepositoryClientImpl::connect (in blink)
- SharedWorkerRepository::connect (in chromium)
* creates an instance of WebSharedWorkerProxy
- WebSharedWorkerProxy::connect (in chromium)
* sends a worker creation message to the browser
* sends a connection request to the browser
BUG=612308
==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from
==========
SharedWorker: Simplify connection sequence
This is a cleanup CL and should not change behavior.
Before this CL, shared worker connection sequence consists of two pathes:
1) SharedWorkerRepositoryClientImpl::conenct (in blink)
- SharedWorkerRepository::createSharedWorkerConnector (in chromium)
* sends a worker creation message to the browser
* returns an instance of WebSharedWorkerProxy
2) SharedWorkerRepositoryClientImpl::connect (in blink)
- SharedWorkerConnector::connect (in blink)
- WebSharedWorkerProxy::connect (in chromium)
* sends a connection request to the browser
After this CL, they are merged into one path like this:
1) SharedWorkerRepositoryClientImpl::connect (in blink)
- SharedWorkerRepository::connect (in chromium)
* creates an instance of WebSharedWorkerProxy
- WebSharedWorkerProxy::connect (in chromium)
* sends a worker creation message to the browser
* sends a connection request to the browser
BUG=612308
==========
to
==========
SharedWorker: Simplify connection sequence
This is a cleanup CL and should not change behavior.
Before this CL, shared worker connection sequence consists of two pathes:
1) SharedWorkerRepositoryClientImpl::conenct (in blink)
- SharedWorkerRepository::createSharedWorkerConnector (in chromium)
* sends a worker creation message to the browser
* returns an instance of WebSharedWorkerProxy
2) SharedWorkerRepositoryClientImpl::connect (in blink)
- SharedWorkerConnector::connect (in blink)
- WebSharedWorkerProxy::connect (in chromium)
* sends a connection request to the browser
After this CL, they are merged into one path like this:
1) SharedWorkerRepositoryClientImpl::connect (in blink)
- SharedWorkerRepository::connect (in chromium)
* creates an instance of WebSharedWorkerProxy
- WebSharedWorkerProxy::connect (in chromium)
* sends a worker creation message to the browser
* sends a connection request to the browser
BUG=612308
==========
nhiroki@chromium.org changed reviewers: + horo@chromium.org, kinuko@chromium.org
PTAL, thanks! https://codereview.chromium.org/2627123003/diff/120001/content/renderer/share... File content/renderer/shared_worker/websharedworker_proxy.cc (left): https://codereview.chromium.org/2627123003/diff/120001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.cc:30: void WebSharedWorkerProxy::connect(blink::WebMessagePortChannel* channel, Question: Who owns this |channel|? AFAICS, this is passed from SharedWorkerConnector and no one manages it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Thanks for making this patch, https://codereview.chromium.org/2627123003/diff/120001/content/renderer/share... File content/renderer/shared_worker/websharedworker_proxy.cc (left): https://codereview.chromium.org/2627123003/diff/120001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.cc:30: void WebSharedWorkerProxy::connect(blink::WebMessagePortChannel* channel, On 2017/01/13 05:28:53, nhiroki wrote: > Question: Who owns this |channel|? AFAICS, this is passed from > SharedWorkerConnector and no one manages it. The comment says: "The object owns itself and is signalled that its not needed anymore with the destroy() call." but looks like we may not be calling destroy() https://codereview.chromium.org/2627123003/diff/120001/content/renderer/share... File content/renderer/shared_worker/websharedworker_proxy.cc (right): https://codereview.chromium.org/2627123003/diff/120001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.cc:46: listener_->connecting(); Why do we need this method separately from workerCreated? https://codereview.chromium.org/2627123003/diff/120001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.cc:71: delete this; would have been nice if we could remove this... but we're replacing this class soon, so maybe ok
https://codereview.chromium.org/2627123003/diff/120001/content/renderer/share... File content/renderer/shared_worker/websharedworker_proxy.cc (left): https://codereview.chromium.org/2627123003/diff/120001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.cc:30: void WebSharedWorkerProxy::connect(blink::WebMessagePortChannel* channel, On 2017/01/13 07:31:34, kinuko wrote: > On 2017/01/13 05:28:53, nhiroki wrote: > > Question: Who owns this |channel|? AFAICS, this is passed from > > SharedWorkerConnector and no one manages it. > > The comment says: > "The object owns itself and is signalled that its not needed anymore with the > destroy() call." > > but looks like we may not be calling destroy() Humm... It looks leaking after this change. https://codereview.chromium.org/2043583002/diff/1/third_party/WebKit/Source/w...
On 2017/01/16 07:11:03, horo wrote: > Humm... > It looks leaking after this change. > https://codereview.chromium.org/2043583002/diff/1/third_party/WebKit/Source/w... Hm? My change should not make a difference regarding the lifetime of pointers (release() works just like leakPtr()).
On 2017/01/16 09:16:12, Yuta Kitamura wrote: > On 2017/01/16 07:11:03, horo wrote: > > Humm... > > It looks leaking after this change. > > > https://codereview.chromium.org/2043583002/diff/1/third_party/WebKit/Source/w... > > Hm? My change should not make a difference regarding the lifetime > of pointers (release() works just like leakPtr()). Ah, sorry. I completely misunderstood. The channel is released in WebMessagePortChannelImpl::OnMessagesQueued(). https://chromium.googlesource.com/chromium/src/+/414c42a/content/child/webmes...
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for your comments! Updated. https://codereview.chromium.org/2627123003/diff/120001/content/renderer/share... File content/renderer/shared_worker/websharedworker_proxy.cc (left): https://codereview.chromium.org/2627123003/diff/120001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.cc:30: void WebSharedWorkerProxy::connect(blink::WebMessagePortChannel* channel, On 2017/01/16 07:11:03, horo wrote: > On 2017/01/13 07:31:34, kinuko wrote: > > On 2017/01/13 05:28:53, nhiroki wrote: > > > Question: Who owns this |channel|? AFAICS, this is passed from > > > SharedWorkerConnector and no one manages it. > > > > The comment says: > > "The object owns itself and is signalled that its not needed anymore with the > > destroy() call." > > > > but looks like we may not be calling destroy() > > Humm... > It looks leaking after this change. > https://codereview.chromium.org/2043583002/diff/1/third_party/WebKit/Source/w... Thank you. As horo's another comment, this seems to be destructed on WebMessagePortChannelImpl::OnMessagesQueued(). Btw, this class is managed as a unique ptr in blink but as a ref ptr in chromium. This seems to be confusing. It'd be nice if we could simplify such complex memory management. https://codereview.chromium.org/2627123003/diff/120001/content/renderer/share... File content/renderer/shared_worker/websharedworker_proxy.cc (right): https://codereview.chromium.org/2627123003/diff/120001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.cc:46: listener_->connecting(); On 2017/01/13 07:31:34, kinuko wrote: > Why do we need this method separately from workerCreated? Yeah, this is not necessary. Removed. https://codereview.chromium.org/2627123003/diff/120001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.cc:71: delete this; On 2017/01/13 07:31:34, kinuko wrote: > would have been nice if we could remove this... but we're replacing this class > soon, so maybe ok Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2627123003/diff/180001/content/renderer/share... File content/renderer/shared_worker/websharedworker_proxy.h (right): https://codereview.chromium.org/2627123003/diff/180001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.h:37: // This is expected to be called immediately after this proxy is constructed. Can the work for connect be done in the ctor? Then we don't need the calling contract
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you. Updated. https://codereview.chromium.org/2627123003/diff/180001/content/renderer/share... File content/renderer/shared_worker/websharedworker_proxy.h (right): https://codereview.chromium.org/2627123003/diff/180001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.h:37: // This is expected to be called immediately after this proxy is constructed. On 2017/01/18 09:13:30, kinuko wrote: > Can the work for connect be done in the ctor? Then we don't need the calling > contract Done. (Generally I'd prefer only to do initialization in the ctor because it's easier to use it in tests, but the calling contract may be flaky in this case)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This cl lgtm. But as we talked offline, you should write a document about the design goal of the architecture change.
On 2017/01/18 11:53:04, horo wrote: > This cl lgtm. Thank you for reviewing this. > But as we talked offline, you should write a document about the design goal of > the architecture change. Yes, I'll make a grand design and then resume mojofication. This CL is independent of mojofication, so I think it's ok to land this without the design.
lgtm/2
nhiroki@chromium.org changed reviewers: + haraken@chromium.org
+haraken, can you review Source/web/SharedWorkerRepositoryClientImpl.cpp? Thanks.
On 2017/01/18 15:04:14, nhiroki wrote: > +haraken, can you review Source/web/SharedWorkerRepositoryClientImpl.cpp? > Thanks. LGTM
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484784345835160,
"parent_rev": "4d60ed6088425ad6189e7851dd96ea2dd3a4cf89", "commit_rev":
"b28a77ff0b6d09ba31cafb8029eda163accae3c8"}
Message was sent while issue was closed.
Description was changed from
==========
SharedWorker: Simplify connection sequence
This is a cleanup CL and should not change behavior.
Before this CL, shared worker connection sequence consists of two pathes:
1) SharedWorkerRepositoryClientImpl::conenct (in blink)
- SharedWorkerRepository::createSharedWorkerConnector (in chromium)
* sends a worker creation message to the browser
* returns an instance of WebSharedWorkerProxy
2) SharedWorkerRepositoryClientImpl::connect (in blink)
- SharedWorkerConnector::connect (in blink)
- WebSharedWorkerProxy::connect (in chromium)
* sends a connection request to the browser
After this CL, they are merged into one path like this:
1) SharedWorkerRepositoryClientImpl::connect (in blink)
- SharedWorkerRepository::connect (in chromium)
* creates an instance of WebSharedWorkerProxy
- WebSharedWorkerProxy::connect (in chromium)
* sends a worker creation message to the browser
* sends a connection request to the browser
BUG=612308
==========
to
==========
SharedWorker: Simplify connection sequence
This is a cleanup CL and should not change behavior.
Before this CL, shared worker connection sequence consists of two pathes:
1) SharedWorkerRepositoryClientImpl::conenct (in blink)
- SharedWorkerRepository::createSharedWorkerConnector (in chromium)
* sends a worker creation message to the browser
* returns an instance of WebSharedWorkerProxy
2) SharedWorkerRepositoryClientImpl::connect (in blink)
- SharedWorkerConnector::connect (in blink)
- WebSharedWorkerProxy::connect (in chromium)
* sends a connection request to the browser
After this CL, they are merged into one path like this:
1) SharedWorkerRepositoryClientImpl::connect (in blink)
- SharedWorkerRepository::connect (in chromium)
* creates an instance of WebSharedWorkerProxy
- WebSharedWorkerProxy::connect (in chromium)
* sends a worker creation message to the browser
* sends a connection request to the browser
BUG=612308
Review-Url: https://codereview.chromium.org/2627123003
Cr-Commit-Position: refs/heads/master@{#444557}
Committed:
https://chromium.googlesource.com/chromium/src/+/b28a77ff0b6d09ba31cafb8029ed...
==========
Message was sent while issue was closed.
Committed patchset #6 (id:220001) as https://chromium.googlesource.com/chromium/src/+/b28a77ff0b6d09ba31cafb8029ed... |
