Bug 61016

Created attachment 94291 [details] First bit of work This is a first bit of the work - by no means complete. The main challenge is moving static bits of WebCore V8 bindings to per-isolate storage. To this end, I have added a per-isolate embedded data to V8, and this patch utilizes that to cache FuntionTemplates for V8 bindings. Moving remainder of static state over to isolates is in the works. I'd appreciate comments from interested parties.

Comment on attachment 94291 [details] First bit of work View in context: https://bugs.webkit.org/attachment.cgi?id=94291&action=review Loks good overall. Mostly minor style nits - a few more substantive comments. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2165 > + if (!data->rawTemplateMap().contains(&info)) { Use "find" instead of contains to only do the lookup once. (Right now it does contains and get). Sketch of code: result = data->rawTemplateMap().find(&info); if (result != end) return result...; v8::Persistent<v8::FunctionTemplate> rawTemplate = createRawTemplate(); data->rawTemplateMap().add(&info, rawTemplate); return rawTemplate; > Source/WebCore/bindings/v8/V8Binding.cpp:51 > +V8BindingPerIsolateData::V8BindingPerIsolateData(v8::Isolate* isolate) { { on next line (in many places). > Source/WebCore/bindings/v8/V8Binding.cpp:57 > +// static Chromium does these "static" comments but WebKit doesn't. > Source/WebCore/bindings/v8/V8Binding.cpp:60 > + if (embedderData != 0) { Do use {} for single line clauses and avoid comparisons to 0. > Source/WebCore/bindings/v8/V8Binding.cpp:71 > + if (data != 0) { Ditto. > Source/WebCore/bindings/v8/V8Binding.h:59 > + static void dispose(v8::Isolate* isolate); isolate -- param name adds no information so don't include it. > Source/WebCore/bindings/v8/V8Binding.h:67 > + V8BindingPerIsolateData(v8::Isolate* isolate); Ditto. > Source/WebKit/chromium/src/PlatformBridge.cpp:1048 > + return new WorkerMessagingProxy(worker); Is there a different code path for shared workers?

(In reply to comment #2) > (From update of attachment 94291 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94291&action=review > > Loks good overall. > Mostly minor style nits - a few more substantive comments. > > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2165 > > + if (!data->rawTemplateMap().contains(&info)) { > > Use "find" instead of contains to only do the lookup once. (Right now it does contains and get). > > Sketch of code: > > result = data->rawTemplateMap().find(&info); > if (result != end) > return result...; > > v8::Persistent<v8::FunctionTemplate> rawTemplate = createRawTemplate(); > data->rawTemplateMap().add(&info, rawTemplate); > return rawTemplate; > > > Source/WebCore/bindings/v8/V8Binding.cpp:51 > > +V8BindingPerIsolateData::V8BindingPerIsolateData(v8::Isolate* isolate) { > > { on next line (in many places). > > > Source/WebCore/bindings/v8/V8Binding.cpp:57 > > +// static > > Chromium does these "static" comments but WebKit doesn't. > > > Source/WebCore/bindings/v8/V8Binding.cpp:60 > > + if (embedderData != 0) { > > Do use {} for single line clauses and avoid comparisons to 0. > > > Source/WebCore/bindings/v8/V8Binding.cpp:71 > > + if (data != 0) { > > Ditto. > > > Source/WebCore/bindings/v8/V8Binding.h:59 > > + static void dispose(v8::Isolate* isolate); > > isolate -- param name adds no information so don't include it. > > > Source/WebCore/bindings/v8/V8Binding.h:67 > > + V8BindingPerIsolateData(v8::Isolate* isolate); > > Ditto. Thanks for taking a look - fixed all of the above > > > Source/WebKit/chromium/src/PlatformBridge.cpp:1048 > > + return new WorkerMessagingProxy(worker); > > Is there a different code path for shared workers? Yes there is.

Created attachment 95694 [details] This patch makes simple worker actually work in-process in Chrome

(In reply to comment #4) > Created an attachment (id=95694) [details] > This patch makes simple worker actually work in-process in Chrome This is not a landable patch again, but gives a good overview of a minimal set of changes to be done

Comment on attachment 95694 [details] This patch makes simple worker actually work in-process in Chrome View in context: https://bugs.webkit.org/attachment.cgi?id=95694&action=review > Source/WebCore/bindings/v8/V8Binding.cpp:62 > + if (embedderData) { return static_cast<V8BindingPerIsolateData*>(embedderData); } It's possible to avoid this 'if' - by assuming the data is always there. It's possible to initialize it in the ScriptController. The 'if' will consume few clocks every time... Also, we could have V8 expose Isolate::GetCurrentIsolateData() to avoid one extra non-inlinable call. Of course, only if it actually saves part of the expected perf problem :-)

Created attachment 103883 [details] This adds an implementation of in-process dedicated workers to chromium port. The crux of the matter is the reimplementation of WebWorkerClientImpl. WebWorkerClientImpl now implements all three of Worker{Loader,Context,Object}Proxies and delegates the implementation to WebKit's standard WorkerMessagingProxy. For now, we have 3 implementations of workers in chromium WebKit: - In-process dedicated workers (this checkin) - Inter-process shared workers - Inter-process dedicated workers (defunct after this checkin) This patch extracts some new common interfaces (WebCommonWorkerBase and WebCommonWorkerClient) for these three implementations. Removing the remainings of inter-process dedicated workers -related classes is left for a separate patch (it will require coordinated changes on chromuium side). Chromium trybots are happy.

Comment on attachment 103883 [details] This adds an implementation of in-process dedicated workers to chromium port. View in context: https://bugs.webkit.org/attachment.cgi?id=103883&action=review I think we can do better than adding WebCommonFooBase interfaces. Let's try to just make use of WebCommonFoo, or switch to WebFooBase. > Source/WebKit/chromium/public/WebCommonWorkerClient.h:53 > +class WebCommonWorkerClient : public WebCommonWorkerClientBase { WebCommonWorkerClientBase is a funny name. Isn't WebCommonWorkerClient already supposed to be the interface that represents the common functions needed by all types of worker clients? I see this comment above WebCommonWorkerClient: // This interface contains common APIs used by both shared and dedicated // workers. > Source/WebKit/chromium/public/WebCommonWorkerClientBase.h:56 > +} WebKit API headers normally put a comment here to indicate the close of the namespace. You can see this in part 3 of the coding style guide: http://www.webkit.org/coding/coding-style.html > Source/WebKit/chromium/src/WebCommonWorkerBase.h:45 > + virtual WebView* webView() const = 0; we normally drop the "web" prefix on method names that return a WebFoo type. > Source/WebKit/chromium/src/WebCommonWorkerBase.h:47 > + nit: only one new line here please

Comment on attachment 103883 [details] This adds an implementation of in-process dedicated workers to chromium port. View in context: https://bugs.webkit.org/attachment.cgi?id=103883&action=review Re Web*Base classes - as I said, after this patch, we will be in a bit of temporary situation where we will have 2 implementations of dedicated workers, one defunct. I think complete removal of inter-process workers and subsequent clean-up should be a separate patch. Awkward naming reflect that. >> Source/WebKit/chromium/public/WebCommonWorkerClient.h:53 >> +class WebCommonWorkerClient : public WebCommonWorkerClientBase { > > WebCommonWorkerClientBase is a funny name. Isn't WebCommonWorkerClient already > supposed to be the interface that represents the common functions needed by all > types of worker clients? > > I see this comment above WebCommonWorkerClient: > > // This interface contains common APIs used by both shared and dedicated > // workers. WebCommonWorkerClient unfortunately has a bunch of methods that are unnecessary and cumbersome to implement in a new world of in-proc dedicated workers (postMessage* and others that are supposed to work on _main_ thread) - hence the need for a base interface shared between all 3 worker implementation. The idea is that after the dedicated worker implementation cleanup these methods will go away and WebCommonWorkerClientBase will merge back into WebCommonWorkerClient. >> Source/WebKit/chromium/public/WebCommonWorkerClientBase.h:56 >> +} > > WebKit API headers normally put a comment here to indicate the close of the namespace. > > You can see this in part 3 of the coding style guide: http://www.webkit.org/coding/coding-style.html Done. >> Source/WebKit/chromium/src/WebCommonWorkerBase.h:45 >> + virtual WebView* webView() const = 0; > > we normally drop the "web" prefix on method names that return a WebFoo type. Done. >> Source/WebKit/chromium/src/WebCommonWorkerBase.h:47 >> + > > nit: only one new line here please Done.

Created attachment 103954 [details] CR feedback

Comment on attachment 103954 [details] CR feedback View in context: https://bugs.webkit.org/attachment.cgi?id=103954&action=review > Source/WebCore/workers/WorkerMessagingProxy.h:53 > + virtual ~WorkerMessagingProxy(); This shouldn't be exposed because no one should ever call it. This class is self-deleting. (There should probably be a comment before this destructor.) Also it is odd that it is virtual at all as discussed. > Source/WebKit/chromium/ChangeLog:14 > + three implementations. Why are new interfaces needed? Why are we keeping the old interfaces? What does shared workers use? (There is a lot of code that got deleted in WebWorkerClientImpl. Did shared workers avoid using that?) > Source/WebKit/chromium/src/WebSharedWorkerImpl.h:68 > + WebCommonWorkerClientBase* commonClientBase() { return m_client; } What about the other method from WebCommonWorkerBase? > Source/WebKit/chromium/src/WebWorkerBase.h:96 > + virtual WebView* view() const { return m_webView; } The comment about "WebCore::WorkerLoaderProxy methods:" should change to "WebCommonWorkerBase" and the "WebView* view" method should be part of that group. > Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:82 > // return new WorkerMessagingProxy(worker); Should this commented out code be removed? > Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:95 > + : m_proxy(adoptPtr(new WorkerMessagingProxy(worker))) As noted previously, WorkerMessagingProxy is self-deleting so best not to store it in a OwnPtr. Perhaps we should expose a WorkerMessagingProxy::create method which only returns a WorkerMessagingProxy* to emphasize this. > Source/WebKit/chromium/src/WebWorkerClientImpl.h:53 > +using WebCore::SerializedScriptValue; Avoid using in header files. > Source/WebKit/chromium/src/WebWorkerClientImpl.h:-57 > - WebWorkerClientImpl(WebCore::Worker*); Note that Source/WebKit/chromium/src/* try to keep the methods in the file in the same order as the declaration in the class so these rearrangements here should result in the same rearrangements in the .cpp file. > Source/WebKit/chromium/src/WebWorkerClientImpl.h:67 > + virtual ~WebWorkerClientImpl(); Why did this become public and why was it private before? > Source/WebKit/chromium/src/WebWorkerClientImpl.h:114 > + virtual WebCommonWorkerClientBase* commonClientBase() { return this; } WebCommonWorkerBase methods? > Source/WebKit/chromium/src/WebWorkerImpl.cpp:77 > +WebCommonWorkerClientBase* WebWorkerImpl::commonClientBase() Wrong location. > Source/WebKit/chromium/src/WebWorkerImpl.h:69 > + virtual WebCommonWorkerClientBase* commonClientBase(); other methods for WebCommonWorkerBase?

Ok, I get this now. State of the world before this patch: Chromium relies on WebWorkerBase and WebCommonWorkerClient right now to do common operations for Workers across both shared workers and dedicated workers. WebSharedWorker (WSW) and WebWorker derive from WebWorkerBase (WWB). WebSharedWorkerClient (WSWC) and WebWorkerClient derive from WebCommonWorkerClient (WCWC). Oddly, we have "Base" and "Common" both seemingly being used to mean common functionality (maybe one is used for interfaces and the other for implementations....) State of the world with this patch: Chromium will no longer use ipc for dedicated workers, so WebWorker and WebWorkerClient are dead code. Also WWB and WCWC are only used by the ipc plumbing in chromium but no where else and they are essentially no longer base classes but instead part of WSW and WSWC respectively. The introduction of new Base and Common into what will be the new versions of WWB and WCWC is pretty confusing, so instead I suggested renaming the new versions to NewWebWorkerBase and NewWebCommonWorkerClient temporarily while we do the transition of Chromium code to stop using WWB and WCWC. Future patches (my interpretation): 1. Remove usage of WWB in Chromium and switch to either WSW or NewWebWorkerBase. Ditto for WCWC. 2. Change WebKit/chromium code to remove WWB and move the remaining functionality to WSW and typedef NewWWB WWB. Ditto for WCWC. 3. Switch Chromium to use WWB instead of WWB. Ditto for WCWC. 4. Rename NewWWB to WWB. Ditto for WCWC.

Yes this is accurate

Comment on attachment 103954 [details] CR feedback View in context: https://bugs.webkit.org/attachment.cgi?id=103954&action=review >> Source/WebCore/workers/WorkerMessagingProxy.h:53 >> + virtual ~WorkerMessagingProxy(); > > This shouldn't be exposed because no one should ever call it. This class is self-deleting. (There should probably be a comment before this destructor.) > > Also it is odd that it is virtual at all as discussed. Done. >> Source/WebKit/chromium/ChangeLog:14 >> + three implementations. > > Why are new interfaces needed? > > Why are we keeping the old interfaces? > > What does shared workers use? (There is a lot of code that got deleted in WebWorkerClientImpl. Did shared workers avoid using that?) > Why are new interfaces needed? > Why are we keeping the old interfaces? As I noted already, after this patch we will have 3 implementations of workers, one defunct. Doing a full cleanup will require coordinated changes on chromium side, that is why I am doing this in two steps > What does shared workers use? Nomenclature is: WebWorker is dedicated worker, WebSharedWorker is shared worker. These changes only affect dedicated workers. >> Source/WebKit/chromium/src/WebSharedWorkerImpl.h:68 >> + WebCommonWorkerClientBase* commonClientBase() { return m_client; } > > What about the other method from WebCommonWorkerBase? It comes from WebWorkerBase >> Source/WebKit/chromium/src/WebWorkerBase.h:96 >> + virtual WebView* view() const { return m_webView; } > > The comment about "WebCore::WorkerLoaderProxy methods:" should change to "WebCommonWorkerBase" and the "WebView* view" method should be part of that group. Done >> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:82 >> // return new WorkerMessagingProxy(worker); > > Should this commented out code be removed? With pleasure. >> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:95 >> + : m_proxy(adoptPtr(new WorkerMessagingProxy(worker))) > > As noted previously, WorkerMessagingProxy is self-deleting so best not to store it in a OwnPtr. > > Perhaps we should expose a WorkerMessagingProxy::create method which only returns a WorkerMessagingProxy* to emphasize this. Done (removed OwnPtr) >> Source/WebKit/chromium/src/WebWorkerClientImpl.h:53 >> +using WebCore::SerializedScriptValue; > > Avoid using in header files. Done. >> Source/WebKit/chromium/src/WebWorkerClientImpl.h:-57 >> - WebWorkerClientImpl(WebCore::Worker*); > > Note that Source/WebKit/chromium/src/* try to keep the methods in the file in the same order as the declaration in the class so these rearrangements here should result in the same rearrangements in the .cpp file. Done >> Source/WebKit/chromium/src/WebWorkerClientImpl.h:67 >> + virtual ~WebWorkerClientImpl(); > > Why did this become public and why was it private before? Done (change undone). >> Source/WebKit/chromium/src/WebWorkerClientImpl.h:114 > > WebCommonWorkerBase methods? Done.

Created attachment 103993 [details] CR feedback. Addressed the comments and rename confusing *Base and Common*Base stuff into New* classes. Got rid of extra files.

Comment on attachment 103993 [details] CR feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=103993&action=review This looks good to me pending a little bit of clean up and a few comments. > Source/WebKit/chromium/ChangeLog:13 > + This patch extracts some new common interfaces (WebCommonWorkerBase and WebCommonWorkerClient) for these Update names: WebCommonWorkerBase and WebCommonWorkerClient > Source/WebKit/chromium/public/WebCommonWorkerClient.h:47 > +class NewWebCommonWorkerClient { It feels like this class should have a comment just like WebCommonWorkerClient has one. Also all of these methods are simply about calling things on the main webkit thread so I suspect some simplification of the comments below could happen if there were a top level comment. > Source/WebKit/chromium/src/WebSharedWorkerImpl.h:67 > + // WebCommonWorkerBase methods: s/WebCommonWorkerBase/NewWebWorkerBase/ > Source/WebKit/chromium/src/WebWorkerBase.h:61 > +class NewWebWorkerBase : public WebCore::WorkerLoaderProxy { Comment for this class (like there is one for WWB below). > Source/WebKit/chromium/src/WebWorkerClientImpl.h:41 > + It doesn't seem like a blank line is needed here. > Source/WebKit/chromium/src/WebWorkerImpl.h:68 > + // WebCommonWorkerBase methods: WebCommonWorkerBase update name.

Comment on attachment 103993 [details] CR feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=103993&action=review >> Source/WebKit/chromium/src/WebSharedWorkerImpl.h:67 >> + // WebCommonWorkerBase methods: > > s/WebCommonWorkerBase/NewWebWorkerBase/ Done. >> Source/WebKit/chromium/src/WebWorkerBase.h:61 >> +class NewWebWorkerBase : public WebCore::WorkerLoaderProxy { > > Comment for this class (like there is one for WWB below). Done. >> Source/WebKit/chromium/src/WebWorkerClientImpl.h:41 >> + > > It doesn't seem like a blank line is needed here. The blank line divides WebCore and WebKit headers. Most of WebKit follows this convention. >> Source/WebKit/chromium/src/WebWorkerImpl.h:68 >> + // WebCommonWorkerBase methods: > > WebCommonWorkerBase update name. Done.

Created attachment 104084 [details] CR feedback (changes in comments)

Comment on attachment 104084 [details] CR feedback (changes in comments) View in context: https://bugs.webkit.org/attachment.cgi?id=104084&action=review LGTM. > Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:56 > +class WebCommonWorkerClientBase; Rename issue.

Created attachment 104089 [details] Rename issue fixed (thanks David!)

Comment on attachment 104089 [details] Rename issue fixed (thanks David!) LGTM!

Comment on attachment 104089 [details] Rename issue fixed (thanks David!) We'll deal with any other feedback about the Chromium WebKit public api in follow up patches.

Comment on attachment 104089 [details] Rename issue fixed (thanks David!) View in context: https://bugs.webkit.org/attachment.cgi?id=104089&action=review > Source/WebKit/chromium/public/WebCommonWorkerClient.h:47 > +class NewWebCommonWorkerClient { Interfaces are supposed to be named with a Web prefix. However, I understand that this is just a temporary thing. > Source/WebKit/chromium/public/WebCommonWorkerClient.h:58 > + nit: only one new line here > Source/WebKit/chromium/src/WebWorkerBase.h:60 > + nit: only one new line here > Source/WebKit/chromium/src/WebWorkerBase.h:70 > + nit: only one new line here

Landed in http://trac.webkit.org/changeset/93330. Closing bug.