New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 734450 link

Starred by 1 user
Status: Fixed
Owner:
joelhockey@chromium.org
Closed: Oct 2017
Cc:
█ sashab@chromium.org
haraken@chromium.org
dcheng@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature

Blocked on:
issue 729500
Blocking:
issue 712963



Sign in to add a comment

Design & Implement a strategy for providing modules objects to core.

Project Member Reported by slangley@chromium.org, Jun 19 2017 
As part of the project to merge web/ into core/ & modules/ we've been using callbacks that are created in ModulesInitializer to register callbacks in core to create various modules classes at runtime.

See https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/ModulesInitializer.cpp?rcl=49bbf6942103c54451fa4f27030ad4c0aa0a151c&l=107 and code below.

While this loose coupling is good, it can also be confusing to debug and follow how the code works.

Lets see if we can come up with a centalized way to register and invoke these callbacks.

Comment 1 by slangley@chromium.org, Jun 19 2017

Blocking: 712963

Comment 3 by sashab@chromium.org, Jun 19 2017

Blockedon: 729500
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 21 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8add4064dbf436338874768379180925805b4d15

commit 8add4064dbf436338874768379180925805b4d15
Author: Joel Hockey <joelhockey@chromium.org>
Date: Fri Jul 21 03:22:51 2017

Put all ModulesInitializer callbacks in CoreInitializer

Move all callbacks where core/ is accessing code from modules/
into singleton instance of CoreInitializer.  Classes that need to
execute these callbacks can access them via CoreInitializer.

Bug:  734450 
Change-Id: I16c1372a36f7fcce663da620e9b7edeb1c820557
Reviewed-on: https://chromium-review.googlesource.com/577491
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488567}
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/CoreInitializer.cpp
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/CoreInitializer.h
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/exported/WebDevToolsAgentImpl.cpp
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/exported/WebSharedWorkerImpl.cpp
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/exported/WebSharedWorkerImpl.h
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/frame/LocalFrame.cpp
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/frame/LocalFrame.h
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/html/HTMLMediaElement.h
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/html/media/MediaControls.h
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/inspector/BUILD.gn
[delete] https://crrev.com/9e24929e2983a0bd95da806f307cfdffc5367688/third_party/WebKit/Source/core/inspector/InspectorBaseAgent.cpp
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/inspector/InspectorBaseAgent.h
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/page/ChromeClient.cpp
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/page/ChromeClient.h
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/workers/Worker.cpp
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/workers/Worker.h
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/core/workers/WorkerClients.h
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/modules/ModulesInitializer.cpp
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/modules/exported/WebEmbeddedWorkerImpl.cpp
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/modules/exported/WebEmbeddedWorkerImpl.h
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
[modify] https://crrev.com/8add4064dbf436338874768379180925805b4d15/third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.h

Comment 6 by joelhockey@chromium.org, Jul 21 2017

Status: Fixed (was: Assigned)

Comment 7 by slangley@chromium.org, Jul 21 2017 

I don't think this is fixes as we still need a factory of some sort or cases where objects are just being created from core. (i.e. in LocalFrameClientImpl)

Comment 8 by joelhockey@chromium.org, Jul 21 2017

Status: Assigned (was: Fixed)

Comment 9 by joelhockey@chromium.org, Jul 21 2017 

I don't see how LocalFrameClientImpl::CreateServiceWorkerLinkResource is different to any of the others in [Core|Modules]Initializer.

This CL shows how I think we can handle it.
https://chromium-review.googlesource.com/c/580119/

I'm not against having another class in Core called something like ModulesFactory, but if we are still going to use ModulesInitializer to set the callback into that other class, then I think we might as well store the factory callback on CoreInitializer.
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 24 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ae916e0e564184f87d58dabab8e25c34671d62a6

commit ae916e0e564184f87d58dabab8e25c34671d62a6
Author: Joel Hockey <joelhockey@chromium.org>
Date: Mon Jul 24 01:03:21 2017

Add CoreInitializer::GetInstance, change CallModules.. to virtual.

Change callbacks to be virtual definitions on CoreInitializer
and implemented in ModulesInitialier.

Bug:  734450 
Change-Id: I10654439166ad945ed3ed2ce0e85a255db90c41a
Reviewed-on: https://chromium-review.googlesource.com/579894
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Stuart Langley <slangley@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488900}
[modify] https://crrev.com/ae916e0e564184f87d58dabab8e25c34671d62a6/third_party/WebKit/Source/core/CoreInitializer.h
[modify] https://crrev.com/ae916e0e564184f87d58dabab8e25c34671d62a6/third_party/WebKit/Source/core/exported/WebDevToolsAgentImpl.cpp
[modify] https://crrev.com/ae916e0e564184f87d58dabab8e25c34671d62a6/third_party/WebKit/Source/core/exported/WebSharedWorkerImpl.cpp
[modify] https://crrev.com/ae916e0e564184f87d58dabab8e25c34671d62a6/third_party/WebKit/Source/core/frame/LocalFrame.cpp
[modify] https://crrev.com/ae916e0e564184f87d58dabab8e25c34671d62a6/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
[modify] https://crrev.com/ae916e0e564184f87d58dabab8e25c34671d62a6/third_party/WebKit/Source/core/page/ChromeClient.cpp
[modify] https://crrev.com/ae916e0e564184f87d58dabab8e25c34671d62a6/third_party/WebKit/Source/core/workers/Worker.cpp
[modify] https://crrev.com/ae916e0e564184f87d58dabab8e25c34671d62a6/third_party/WebKit/Source/modules/ModulesInitializer.cpp
[modify] https://crrev.com/ae916e0e564184f87d58dabab8e25c34671d62a6/third_party/WebKit/Source/modules/ModulesInitializer.h
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 24 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/16d92118b7a8bf2b8907258273d8e81c04fbec17

commit 16d92118b7a8bf2b8907258273d8e81c04fbec17
Author: Joel Hockey <joelhockey@chromium.org>
Date: Mon Jul 24 01:49:23 2017

Move CreateServiceWorkerLinkResource from LocalFrameClientImpl to [Core|Modules]Initializer

Remove one of the modules/ dependencies from LocalFrameClientImpl
so it can move into core/.

Bug:  734450 
Change-Id: Ia7a7233c74c3c3c3db911d4f3e88866770c2bb75
Reviewed-on: https://chromium-review.googlesource.com/580119
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488904}
[modify] https://crrev.com/16d92118b7a8bf2b8907258273d8e81c04fbec17/third_party/WebKit/Source/core/CoreInitializer.h
[modify] https://crrev.com/16d92118b7a8bf2b8907258273d8e81c04fbec17/third_party/WebKit/Source/core/frame/LocalFrameClient.h
[modify] https://crrev.com/16d92118b7a8bf2b8907258273d8e81c04fbec17/third_party/WebKit/Source/core/html/HTMLLinkElement.cpp
[modify] https://crrev.com/16d92118b7a8bf2b8907258273d8e81c04fbec17/third_party/WebKit/Source/modules/ModulesInitializer.cpp
[modify] https://crrev.com/16d92118b7a8bf2b8907258273d8e81c04fbec17/third_party/WebKit/Source/modules/ModulesInitializer.h
[modify] https://crrev.com/16d92118b7a8bf2b8907258273d8e81c04fbec17/third_party/WebKit/Source/web/LocalFrameClientImpl.cpp
[modify] https://crrev.com/16d92118b7a8bf2b8907258273d8e81c04fbec17/third_party/WebKit/Source/web/LocalFrameClientImpl.h

Comment 12 by joelhockey@chromium.org, Oct 30 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment