Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2600113003:
(SUSPENDED) SharedWorker: Mojofy Renderer(Document)->Browser communication for SharedWorker
DescriptionSharedWorker: Mojofy Renderer(Document)->Browser communication for SharedWorker
This CL mojofies following Renderer(Document)->Browser communication:
* SharedWorkerRepository --(CreateWorker)--> SharedWorkerMessageFilter
* SharedWorkerRepository --(DocumentDetached)--> SharedWorkerMessageFilter
* WebSharedWorkerProxy --(ConnectToWorker)--> SharedWorkerMessageFilter
This just replaces legacy IPC mechanism with mojo and should not change
behavior.
BUG=612308
Patch Set 1 #
Total comments: 27
Patch Set 2 : wip #Patch Set 3 : wip #Patch Set 4 : fix unit tests #Patch Set 5 : rebase #Patch Set 6 : add TODO comment and minor cleanup #
Total comments: 7
Patch Set 7 : address review comments #
Total comments: 6
Messages
Total messages: 68 (51 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...)
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_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: Mojofy renderer(Document)->browser communication for SharedWorker BUG=612308 ========== to ========== SharedWorker: Mojofy Renderer(Document)->Browser communication for SharedWorker BUG=612308 ==========
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: Mojofy Renderer(Document)->Browser communication for SharedWorker BUG=612308 ========== to ========== SharedWorker: Mojofy Renderer(Document)->Browser communication for SharedWorker This CL mojofies all Renderer(Document)->Browser communication for SharedWorker as follows: * SharedWorkerRepository --(CreateWorker)--> SharedWorkerMessageFilter * SharedWorkerRepository --(DocumentDetached)--> SharedWorkerMessageFilter * WebSharedWorkerProxy --(ConnectToWorker)--> SharedWorkerMessageFilter This just replaces legacy IPC mechanism with mojo and should not change behavior. BUG=612308 ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== SharedWorker: Mojofy Renderer(Document)->Browser communication for SharedWorker This CL mojofies all Renderer(Document)->Browser communication for SharedWorker as follows: * SharedWorkerRepository --(CreateWorker)--> SharedWorkerMessageFilter * SharedWorkerRepository --(DocumentDetached)--> SharedWorkerMessageFilter * WebSharedWorkerProxy --(ConnectToWorker)--> SharedWorkerMessageFilter This just replaces legacy IPC mechanism with mojo and should not change behavior. BUG=612308 ========== to ========== SharedWorker: Mojofy Renderer(Document)->Browser communication for SharedWorker This CL mojofies following Renderer(Document)->Browser communication: * SharedWorkerRepository --(CreateWorker)--> SharedWorkerMessageFilter * SharedWorkerRepository --(DocumentDetached)--> SharedWorkerMessageFilter * WebSharedWorkerProxy --(ConnectToWorker)--> SharedWorkerMessageFilter This just replaces legacy IPC mechanism with mojo and should not change behavior. BUG=612308 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:10006) has been deleted
Patchset #1 (id:50001) has been deleted
Patchset #1 (id:70001) has been deleted
Patchset #1 (id:90001) has been deleted
Patchset #1 (id:110001) has been deleted
Patchset #1 (id:130001) has been deleted
nhiroki@chromium.org changed reviewers: + horo@chromium.org, shimazu@chromium.org
PTAL, thanks!
https://codereview.chromium.org/2600113003/diff/150001/content/renderer/share... File content/renderer/shared_worker_repository.cc (right): https://codereview.chromium.org/2600113003/diff/150001/content/renderer/share... content/renderer/shared_worker_repository.cc:41: static_cast<mojom::WebContentSecurityPolicyType>(security_policy_type); I plan to add enum traits or replace blink::Enum with mojom::Enum for removing these static_cast<>s in a subsequent CL.
https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... File content/renderer/websharedworker_proxy.cc (right): https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... content/renderer/websharedworker_proxy.cc:41: bool WebSharedWorkerProxy::Send(int message_port_id) { TODO(nhiroki): Make this void.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... File content/common/shared_worker.mojom (right): https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... content/common/shared_worker.mojom:11: enum WebContentSecurityPolicyType; You should also create a typemap file to use [Native]. ScrollbarButtonsPlacement which is mapped to WebScrollbarButtonsPlacement can be an example of the [Native] enum: https://cs.chromium.org/chromium/src/content/common/native_types_mac.typemap?... https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... File content/renderer/websharedworker_proxy.cc (right): https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... content/renderer/websharedworker_proxy.cc:59: for (size_t i = 0; i < queued_messages_.size(); ++i) How about range-based for? https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... File content/renderer/websharedworker_proxy.h (right): https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... content/renderer/websharedworker_proxy.h:49: bool Send(int message_port_id); How about renaming to TryToSendConnectToWorker to clarify which IPC this method sends? https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... content/renderer/websharedworker_proxy.h:67: std::vector<int> queued_messages_; I prefer |queued_connect_request_| or something to make what this vector contains clearer.
(some lightweight dbc) https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... File content/common/shared_worker.mojom (right): https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... content/common/shared_worker.mojom:63: // MSG_ROUTING_NONE and an error type. This comment looks stale https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... content/common/shared_worker.mojom:63: // MSG_ROUTING_NONE and an error type. Are we eventually deprecating route_id from the worker code? If so could we comment about that? Having notion of 'route_id' in mojom file feels a bit weird. https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... content/common/shared_worker.mojom:66: => (SharedWorker_CreateWorker_Reply reply); We could probably just return route_id and error without introducing a struct? https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... content/common/shared_worker.mojom:70: // processes (SharedWorkers are shut down when their last associated document 'worker processes' -> 'the renderer process where the associated worker runs' ? https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... content/common/shared_worker.mojom:75: // worker. a worker -> the worker that is specified by route_id ? https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... File content/renderer/websharedworker_proxy.cc (right): https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... content/renderer/websharedworker_proxy.cc:41: bool WebSharedWorkerProxy::Send(int message_port_id) { Doesn't 'Send' sound too generic? https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... File content/renderer/websharedworker_proxy.h (right): https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... content/renderer/websharedworker_proxy.h:32: // If the worker not loaded yet, route_id == MSG_ROUTING_NONE (Would be nice to rebase)
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/2600113003/diff/150001/content/common/shared_... File content/common/shared_worker.mojom (right): https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... content/common/shared_worker.mojom:64: [Sync] We don't need to throw a DOM exception for URLMismatch now. https://codereview.chromium.org/2604733002/diff/80001/third_party/WebKit/Sour... So I think we can change the CreateWorker IPC to asynchronous.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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: This issue passed the CQ dry run.
On 2017/01/06 04:18:13, horo wrote: > https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... > File content/common/shared_worker.mojom (right): > > https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... > content/common/shared_worker.mojom:64: [Sync] > We don't need to throw a DOM exception for URLMismatch now. > https://codereview.chromium.org/2604733002/diff/80001/third_party/WebKit/Sour... > So I think we can change the CreateWorker IPC to asynchronous. That's cooool!
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...
nhiroki@chromium.org changed reviewers: + kinuko@chromium.org
Thank you for your comments! Updated. https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... File content/common/shared_worker.mojom (right): https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... content/common/shared_worker.mojom:11: enum WebContentSecurityPolicyType; On 2017/01/05 02:24:03, shimazu wrote: > You should also create a typemap file to use [Native]. > > ScrollbarButtonsPlacement which is mapped to WebScrollbarButtonsPlacement can be > an example of the [Native] enum: > > https://cs.chromium.org/chromium/src/content/common/native_types_mac.typemap?... Thank you for the pointer. Done. https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... content/common/shared_worker.mojom:63: // MSG_ROUTING_NONE and an error type. On 2017/01/05 08:13:13, kinuko wrote: > Are we eventually deprecating route_id from the worker code? If so could we > comment about that? Having notion of 'route_id' in mojom file feels a bit > weird. You're right. Added comments. https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... content/common/shared_worker.mojom:63: // MSG_ROUTING_NONE and an error type. On 2017/01/05 08:13:13, kinuko wrote: > This comment looks stale Done. https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... content/common/shared_worker.mojom:64: [Sync] On 2017/01/06 04:18:13, horo wrote: > We don't need to throw a DOM exception for URLMismatch now. > https://codereview.chromium.org/2604733002/diff/80001/third_party/WebKit/Sour... > So I think we can change the CreateWorker IPC to asynchronous. Good point. I'd like to work on it in a separate CL to minimize diffs. I filed an issue and left a TODO comment here. https://bugs.chromium.org/p/chromium/issues/detail?id=679654 https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... content/common/shared_worker.mojom:66: => (SharedWorker_CreateWorker_Reply reply); On 2017/01/05 08:13:14, kinuko wrote: > We could probably just return route_id and error without introducing a struct? Done. https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... content/common/shared_worker.mojom:70: // processes (SharedWorkers are shut down when their last associated document On 2017/01/05 08:13:14, kinuko wrote: > 'worker processes' -> 'the renderer process where the associated worker runs' ? Done. https://codereview.chromium.org/2600113003/diff/150001/content/common/shared_... content/common/shared_worker.mojom:75: // worker. On 2017/01/05 08:13:13, kinuko wrote: > a worker -> the worker that is specified by route_id ? Done. https://codereview.chromium.org/2600113003/diff/150001/content/renderer/share... File content/renderer/shared_worker_repository.cc (right): https://codereview.chromium.org/2600113003/diff/150001/content/renderer/share... content/renderer/shared_worker_repository.cc:41: static_cast<mojom::WebContentSecurityPolicyType>(security_policy_type); On 2016/12/28 09:24:30, nhiroki wrote: > I plan to add enum traits or replace blink::Enum with mojom::Enum for removing > these static_cast<>s in a subsequent CL. Added typemap in this CL. https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... File content/renderer/websharedworker_proxy.cc (right): https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... content/renderer/websharedworker_proxy.cc:41: bool WebSharedWorkerProxy::Send(int message_port_id) { On 2017/01/05 08:13:14, kinuko wrote: > Doesn't 'Send' sound too generic? This function was removed by other cleanup CLs. https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... content/renderer/websharedworker_proxy.cc:59: for (size_t i = 0; i < queued_messages_.size(); ++i) On 2017/01/05 02:24:03, shimazu wrote: > How about range-based for? This part was removed by other cleanup CLs. https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... File content/renderer/websharedworker_proxy.h (right): https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... content/renderer/websharedworker_proxy.h:32: // If the worker not loaded yet, route_id == MSG_ROUTING_NONE On 2017/01/05 08:13:14, kinuko wrote: > (Would be nice to rebase) Done. https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... content/renderer/websharedworker_proxy.h:49: bool Send(int message_port_id); On 2017/01/05 02:24:03, shimazu wrote: > How about renaming to TryToSendConnectToWorker to clarify which IPC this method > sends? This function was removed by other cleanup CLs. https://codereview.chromium.org/2600113003/diff/150001/content/renderer/websh... content/renderer/websharedworker_proxy.h:67: std::vector<int> queued_messages_; On 2017/01/05 02:24:03, shimazu wrote: > I prefer |queued_connect_request_| or something to make what this vector > contains clearer. Queuing mechanism was removed by other cleanup CLs.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2600113003/diff/250001/content/common/shared_... File content/common/shared_worker.mojom (right): https://codereview.chromium.org/2600113003/diff/250001/content/common/shared_... content/common/shared_worker.mojom:22: struct SharedWorker_CreateWorker_Params { I think it's okay to obey the usual naming convention like mojom::SharedWorkerCreateParams as long as you won't use [Native] mapped to ViewHostMsg_CreateWorker_Params. https://codereview.chromium.org/2600113003/diff/250001/content/common/shared_... content/common/shared_worker.mojom:51: interface SharedWorkerMessageFilter { How about mojofying SharedWorkerService instead of SharedWorkerMessageFilter? (I believe it's a subtle change). https://codereview.chromium.org/2600113003/diff/250001/content/common/shared_... content/common/shared_worker.mojom:55: // TODO(nhiroki): This route id will be removed when browser->renderer(worker) The route_id can be removed after mojofying WebSharedWorkerConnector (browser->renderer(document's worker objects)) like the following example. OnCreateWorker can be async after the change. Could you update the comment? == // The implementation will be blink::WebSharedWorkerConnector interface SharedWorkerConnector { OnCreated(); OnConnected(); OnScriptLoadFailed(); } interface SharedWorkerMessageFilter { // SharedWorkerConnector could be a member of SharedWorkerCreateParams. OnCreateWorker(SharedWorkerCreateParams params, SharedWorkerConnector connector); }
lgtm. But please get lgtm from shimazhi@ who knows more about mojo :)
Thank you. Updated. https://codereview.chromium.org/2600113003/diff/250001/content/common/shared_... File content/common/shared_worker.mojom (right): https://codereview.chromium.org/2600113003/diff/250001/content/common/shared_... content/common/shared_worker.mojom:22: struct SharedWorker_CreateWorker_Params { On 2017/01/11 03:40:17, shimazu wrote: > I think it's okay to obey the usual naming convention like > mojom::SharedWorkerCreateParams as long as you won't use [Native] mapped to > ViewHostMsg_CreateWorker_Params. Done. https://codereview.chromium.org/2600113003/diff/250001/content/common/shared_... content/common/shared_worker.mojom:51: interface SharedWorkerMessageFilter { On 2017/01/11 03:40:17, shimazu wrote: > How about mojofying SharedWorkerService instead of SharedWorkerMessageFilter? > (I believe it's a subtle change). SharedWorkerServiceImpl is a singleton object and needs to have SharedWorkerMessageFilter per request (see SharedWorkerServiceImpl::CreateWorker for example). If SharedWorkerServiceImpl directly receives RPC calls, how does it take the filter? (I'm wondering if this becomes easier after browser->renderer(document) is mojofied as your other comment) https://codereview.chromium.org/2600113003/diff/250001/content/common/shared_... content/common/shared_worker.mojom:55: // TODO(nhiroki): This route id will be removed when browser->renderer(worker) On 2017/01/11 03:40:17, shimazu wrote: > The route_id can be removed after mojofying WebSharedWorkerConnector > (browser->renderer(document's worker objects)) like the following example. > OnCreateWorker can be async after the change. > Could you update the comment? > > == > > // The implementation will be blink::WebSharedWorkerConnector > interface SharedWorkerConnector { > OnCreated(); > OnConnected(); > OnScriptLoadFailed(); > } > > interface SharedWorkerMessageFilter { > // SharedWorkerConnector could be a member of SharedWorkerCreateParams. > OnCreateWorker(SharedWorkerCreateParams params, > SharedWorkerConnector connector); > } I see. I wrongly assumed this |route_id| represents a shared worker, but actually it represents this document. Revised the comment.
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...
code changes l-g-t-m (deferring detailed review to others), some nit/meta comments https://codereview.chromium.org/2600113003/diff/270001/content/common/shared_... File content/common/shared_worker.mojom (right): https://codereview.chromium.org/2600113003/diff/270001/content/common/shared_... content/common/shared_worker.mojom:49: // Each InterfacePtrs on the same render process will be bound to the same nit: Each InterfacePtrs -> Each InterfacePtr or All InterfacePtrs https://codereview.chromium.org/2600113003/diff/270001/content/common/shared_... content/common/shared_worker.mojom:58: // TODO(nhiroki): Make this asynchronous (https://crbug.com/679654). I feel we could do the necessary refactoring to make it work asyncly first (without changing IPC change) and then introduce mojo, then we don't need to introduce controversial sync IPC changes in new mojo code. Up to you but worth considering. https://codereview.chromium.org/2600113003/diff/270001/content/renderer/share... File content/renderer/shared_worker/websharedworker_proxy.cc (right): https://codereview.chromium.org/2600113003/diff/270001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.cc:63: // The worker is created - nowconnect to the worker. nowconnect -> now connect
lgtm https://codereview.chromium.org/2600113003/diff/250001/content/common/shared_... File content/common/shared_worker.mojom (right): https://codereview.chromium.org/2600113003/diff/250001/content/common/shared_... content/common/shared_worker.mojom:51: interface SharedWorkerMessageFilter { On 2017/01/11 05:16:58, nhiroki (OOO until Jan 9) wrote: > On 2017/01/11 03:40:17, shimazu wrote: > > How about mojofying SharedWorkerService instead of SharedWorkerMessageFilter? > > (I believe it's a subtle change). > > SharedWorkerServiceImpl is a singleton object and needs to have > SharedWorkerMessageFilter per request (see SharedWorkerServiceImpl::CreateWorker > for example). If SharedWorkerServiceImpl directly receives RPC calls, how does > it take the filter? > > (I'm wondering if this becomes easier after browser->renderer(document) is > mojofied as your other comment) Oops, sorry, I got it. I missed that currently route_id is unique among each process. It seems okay as is.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you :) Comment reply only. https://codereview.chromium.org/2600113003/diff/270001/content/common/shared_... File content/common/shared_worker.mojom (right): https://codereview.chromium.org/2600113003/diff/270001/content/common/shared_... content/common/shared_worker.mojom:58: // TODO(nhiroki): Make this asynchronous (https://crbug.com/679654). On 2017/01/11 06:06:43, kinuko wrote: > I feel we could do the necessary refactoring to make it work asyncly first > (without changing IPC change) and then introduce mojo, then we don't need to > introduce controversial sync IPC changes in new mojo code. Up to you but worth > considering. Hmmm... I tried this and was stuck. How do we notify WebSharedWorkerProxy of route_id in response to a worker creation message? WebSharedWorkerProxy needs route_id to receive a message, but route_id is notified via a message. I came up with using a temporary route_id generated in a renderer-side, but found this confuses MessageRouter. Do you have any ideas on this? This could be easier resolved after mojofication. WebSharedWorkerProxy can send a mojofied object like shimazu's comment to SharedWorkerMessageFilter, and the filter can reply to the proxy over the mojofied object.
https://codereview.chromium.org/2600113003/diff/270001/content/common/shared_... File content/common/shared_worker.mojom (right): https://codereview.chromium.org/2600113003/diff/270001/content/common/shared_... content/common/shared_worker.mojom:58: // TODO(nhiroki): Make this asynchronous (https://crbug.com/679654). On 2017/01/11 07:48:56, nhiroki (OOO until Jan 9) wrote: > On 2017/01/11 06:06:43, kinuko wrote: > > I feel we could do the necessary refactoring to make it work asyncly first > > (without changing IPC change) and then introduce mojo, then we don't need to > > introduce controversial sync IPC changes in new mojo code. Up to you but > worth > > considering. > > Hmmm... I tried this and was stuck. How do we notify WebSharedWorkerProxy of > route_id in response to a worker creation message? WebSharedWorkerProxy needs > route_id to receive a message, but route_id is notified via a message. I came up > with using a temporary route_id generated in a renderer-side, but found this > confuses MessageRouter. Do you have any ideas on this? I have imagined that we do necessary refactoring for async/mojo-fying first just with the given route_id, e.g. createSharedWorkerConnector directly takes ConnectListener and we can immediately do connect() operation without returning WebSharedWorkerConnector to blink code etc. > This could be easier resolved after mojofication. WebSharedWorkerProxy can send > a mojofied object like shimazu's comment to SharedWorkerMessageFilter, and the > filter can reply to the proxy over the mojofied object. I agree with it. But then it feels it's more straightforward to mojofy OnCreateWorker with the rest of mojofication work? (We'll need to change OnCreateWorker in the next patch too anyways)
Thank you for the reply. https://codereview.chromium.org/2600113003/diff/270001/content/common/shared_... File content/common/shared_worker.mojom (right): https://codereview.chromium.org/2600113003/diff/270001/content/common/shared_... content/common/shared_worker.mojom:58: // TODO(nhiroki): Make this asynchronous (https://crbug.com/679654). On 2017/01/11 11:51:46, kinuko wrote: > On 2017/01/11 07:48:56, nhiroki (OOO until Jan 9) wrote: > > On 2017/01/11 06:06:43, kinuko wrote: > > > I feel we could do the necessary refactoring to make it work asyncly first > > > (without changing IPC change) and then introduce mojo, then we don't need to > > > introduce controversial sync IPC changes in new mojo code. Up to you but > > worth > > > considering. > > > > Hmmm... I tried this and was stuck. How do we notify WebSharedWorkerProxy of > > route_id in response to a worker creation message? WebSharedWorkerProxy needs > > route_id to receive a message, but route_id is notified via a message. I came > up > > with using a temporary route_id generated in a renderer-side, but found this > > confuses MessageRouter. Do you have any ideas on this? > > I have imagined that we do necessary refactoring for async/mojo-fying first just > with the given route_id, e.g. createSharedWorkerConnector directly takes > ConnectListener and we can immediately do connect() operation without returning > WebSharedWorkerConnector to blink code etc. Ah, I understand what you mean. You mean the following 2-steps, right? (1) Refactoring SharedWorkerConnector/WebSharedWorkerConnector (2) Making it async with introducing mojo I misread your comment as following steps :p (1) Refactoring them and making it async (2) Introducing mojo Your idea sounds reasonable. I'll make a separate refactoring CL. > > This could be easier resolved after mojofication. WebSharedWorkerProxy can > send > > a mojofied object like shimazu's comment to SharedWorkerMessageFilter, and the > > filter can reply to the proxy over the mojofied object. > > I agree with it. But then it feels it's more straightforward to mojofy > OnCreateWorker with the rest of mojofication work? (We'll need to change > OnCreateWorker in the next patch too anyways)
Sorry for my delayed status update. I'm now slowly illustrating the overview of SharedWorker mojofication. I'll resume this after the plan becomes more concrete. |
