After dedicated workers move in-process, they will no longer be using WebWorkerBase as a base implementation class.
Created attachment 103504 [details] Implementation This patch moves implementation of allowFileSystem, openFileSystem and allowDatabase from WebWorkerBase to respectively LocalFileSystemChromium and DatabaseObserver, parametrizing them with relevant data from WebWorker.
(chromium trybots are happy)
Comment on attachment 103504 [details] Implementation View in context: https://bugs.webkit.org/attachment.cgi?id=103504&action=review > Source/WebKit/chromium/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=65997. Please add an empty line after the link. > Source/WebKit/chromium/ChangeLog:9 > + * src/DatabaseObserver.cpp: Please add more detail regarding this particular file here. For example, you can add: Move allowDatabase from WebWorkerBase and update the caller. > Source/WebKit/chromium/src/DatabaseObserver.cpp:90 > + createCallbackTask(&didComplete, WebCore::AllowCrossThreadAccess(this), result), m_mode); You can join this line and the line above it. > Source/WebKit/chromium/src/DatabaseObserver.cpp:107 > + if (!commonClient) You can flip the if condition and swap the two actions. > Source/WebKit/chromium/src/DatabaseObserver.cpp:124 > +bool allowDatabaseForWorker(WebCommonWorkerClient* commonClient, WebFrame* frame, const WebString& name, const WebString& displayName, unsigned long estimatedSize) I think allowDatabaseOnWorker sounds better. > Source/WebKit/chromium/src/DatabaseObserver.cpp:130 > + WebCore::WorkerLoaderProxy* workerLoaderProxy = &workerContext->thread()->workerLoaderProxy(); You can replace "workerContext->thread()" with workerThread. > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:145 > + WebCore::WorkerLoaderProxy* workerLoaderProxy = &workerContext->thread()->workerLoaderProxy(); ditto. Also, can you use a helper function to get workerThread? > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:168 > + WebCore::WorkerLoaderProxy* workerLoaderProxy = &workerContext->thread()->workerLoaderProxy(); ditto. > Source/WebKit/chromium/src/WebWorkerBase.cpp:53 > +#include "WorkerLoaderProxy.h" It seems that you can also remove the including of WorkerFileSystemCallbacksBridge.h, WorkerScriptController.h and others. > Source/WebKit/chromium/src/WebWorkerBase.h:95 > + WebView* webView() { return m_webView; } Please add const modifier. > Source/WebKit/chromium/src/WebWorkerBase.h:97 > + virtual WebCommonWorkerClient* commonClient() = 0; Please add empty line after this. > Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:42 > +#include "WorkerLoaderProxy.h" You can use forward declaration for WorkerLoaderProxy, instead of including it. Do not forget to remove the forward declaration for WebWorkerBase.
Comment on attachment 103504 [details] Implementation View in context: https://bugs.webkit.org/attachment.cgi?id=103504&action=review Thanks for review! >> Source/WebKit/chromium/ChangeLog:4 >> + https://bugs.webkit.org/show_bug.cgi?id=65997. > > Please add an empty line after the link. Done. >> Source/WebKit/chromium/ChangeLog:9 >> + * src/DatabaseObserver.cpp: > > Please add more detail regarding this particular file here. For example, you can add: > Move allowDatabase from WebWorkerBase and update the caller. Done >> Source/WebKit/chromium/src/DatabaseObserver.cpp:90 >> + createCallbackTask(&didComplete, WebCore::AllowCrossThreadAccess(this), result), m_mode); > > You can join this line and the line above it. Changed formatting to make code clearer. >> Source/WebKit/chromium/src/DatabaseObserver.cpp:107 >> + if (!commonClient) > > You can flip the if condition and swap the two actions. Done although this was the orinal code. >> Source/WebKit/chromium/src/DatabaseObserver.cpp:124 >> +bool allowDatabaseForWorker(WebCommonWorkerClient* commonClient, WebFrame* frame, const WebString& name, const WebString& displayName, unsigned long estimatedSize) > > I think allowDatabaseOnWorker sounds better. Why for? We are allowing database _for_ worker. Also consistent with other naming (openFileSystemForWorker) >> Source/WebKit/chromium/src/DatabaseObserver.cpp:130 >> + WebCore::WorkerLoaderProxy* workerLoaderProxy = &workerContext->thread()->workerLoaderProxy(); > > You can replace "workerContext->thread()" with workerThread. Done. >> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:145 >> + WebCore::WorkerLoaderProxy* workerLoaderProxy = &workerContext->thread()->workerLoaderProxy(); > > ditto. > > Also, can you use a helper function to get workerThread? I need both workerThread and workerContext out of this 3 lines of code, so I do not feel separate function is very useful. >> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:168 >> + WebCore::WorkerLoaderProxy* workerLoaderProxy = &workerContext->thread()->workerLoaderProxy(); > > ditto. Done. >> Source/WebKit/chromium/src/WebWorkerBase.cpp:53 >> +#include "WorkerLoaderProxy.h" > > It seems that you can also remove the including of WorkerFileSystemCallbacksBridge.h, WorkerScriptController.h and others. Done >> Source/WebKit/chromium/src/WebWorkerBase.h:95 >> + WebView* webView() { return m_webView; } > > Please add const modifier. Done. >> Source/WebKit/chromium/src/WebWorkerBase.h:97 >> + virtual WebCommonWorkerClient* commonClient() = 0; > > Please add empty line after this. Done. >> Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:42 >> +#include "WorkerLoaderProxy.h" > > You can use forward declaration for WorkerLoaderProxy, instead of including it. > > Do not forget to remove the forward declaration for WebWorkerBase. Done.
Created attachment 103523 [details] CR feedback addressed
Comment on attachment 103523 [details] CR feedback addressed Almost there. Just some more minor comments. View in context: https://bugs.webkit.org/attachment.cgi?id=103523&action=review > Source/WebKit/chromium/ChangeLog:8 > + parametrizing them with relevant data from WebWorker. typo: parameterizing > Source/WebKit/chromium/ChangeLog:10 > + * src/DatabaseObserver.cpp:Move allowDatabase from WebWorkerBase and update the caller. Please add a whitespace after ':'. > Source/WebKit/chromium/ChangeLog:20 > + * src/LocalFileSystemChromium.cpp:Move allowFileSystem and openFileSystem from WebWorkerBase and update the caller. ditto. > Source/WebKit/chromium/ChangeLog:30 > + * src/WorkerFileSystemCallbacksBridge.h: Please also add more comment here, like: Change WorkerFileSystemCallbacksBridge constructor to take WorkerLoaderProxy argument. > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:122 > + if (!commonClient) Since you're already in this code, why not just making it better? > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:184 > + Please remove an extra blank line.
(In reply to comment #6) > (From update of attachment 103523 [details]) > Almost there. Just some more minor comments. > > View in context: https://bugs.webkit.org/attachment.cgi?id=103523&action=review > > > Source/WebKit/chromium/ChangeLog:8 > > + parametrizing them with relevant data from WebWorker. > > typo: parameterizing Done. > > > Source/WebKit/chromium/ChangeLog:10 > > + * src/DatabaseObserver.cpp:Move allowDatabase from WebWorkerBase and update the caller. > > Please add a whitespace after ':'. Done. > > > Source/WebKit/chromium/ChangeLog:20 > > + * src/LocalFileSystemChromium.cpp:Move allowFileSystem and openFileSystem from WebWorkerBase and update the caller. > > ditto. Done. > > > Source/WebKit/chromium/ChangeLog:30 > > + * src/WorkerFileSystemCallbacksBridge.h: > > Please also add more comment here, like: > Change WorkerFileSystemCallbacksBridge constructor to take WorkerLoaderProxy argument. Done. > > > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:122 > > + if (!commonClient) > > Since you're already in this code, why not just making it better? Done. > > > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:184 > > + > > Please remove an extra blank line. Done.
Created attachment 103528 [details] Extra feedback
Comment on attachment 103528 [details] Extra feedback View in context: https://bugs.webkit.org/attachment.cgi?id=103528&action=review > Source/WebKit/chromium/src/DatabaseObserver.cpp:147 > + Extra empty line. > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:75 > + Extra empty line.
Created attachment 103530 [details] Empty lines
Comment on attachment 103530 [details] Empty lines Rejecting attachment 103530 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 1 Last 500 characters of output: 1031bf49c725076cb9a13180c3c1151164e7e6d7 r92799 = 42efb1ad980c022006972b156e9dcde70e43ac9e Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Updating chromium port dependencies using gclient... ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/9336822
ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
You need to leave the "Reviewed by NOBODY (OOPS!)" line int he ChangeLog so that the bot knows where to insert the reviewer information.
Created attachment 103547 [details] Fixed changelog
Comment on attachment 103547 [details] Fixed changelog Clearing flags on attachment: 103547 Committed r92802: <http://trac.webkit.org/changeset/92802>
All reviewed patches have been landed. Closing bug.