[webkit-reviews] review denied: [Bug 26146] Change to use ThreadableLoader to load the worker script in order to check URL origin for redirection. : [Attachment 31122] Proposed Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 26146: Change to use ThreadableLoader to load the worker script in order to
check URL origin for redirection.
https://bugs.webkit.org/show_bug.cgi?id=26146

Attachment 31122: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=31122&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Tthis is looking pretty good.  As far the code itself, it all looks good as far
as the algorithm. I have one concern about the script encoding below when
loading the worker script itself.

Several style/comment/etc. comments below.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2009-06-09  Jian Li	<jianli at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Bug 26146: Change to use ThreadableLoader to load the worker script
> +	   in order to check URL origin for redirection.
> +	   https://bugs.webkit.org/show_bug.cgi?id=26146
> +
> +	   Test: http/tests/workers/worker-redirect.html
> +
> +	   * WebCore.xcodeproj/project.pbxproj:
> +	   * workers/Worker.cpp:
> +	   (WebCore::Worker::Worker):
> +	   (WebCore::Worker::notifyFinished):
> +	   * workers/Worker.h:
> +	   * workers/WorkerContext.cpp:
> +	   (WebCore::WorkerContext::importScripts):
> +	   * workers/WorkerImportScriptsClient.cpp: Removed.
> +	   * workers/WorkerImportScriptsClient.h: Removed.
> +	   * workers/WorkerScriptLoader.cpp: Renamed from
workers/WorkerImportScriptsClient.cpp.
> +	   (WebCore::WorkerScriptLoader::loadSynchronously):
> +	   (WebCore::WorkerScriptLoader::loadAsynchronously):
> +	   (WebCore::WorkerScriptLoader::didFinishLoading):
> +	   (WebCore::WorkerScriptLoader::didFail):
> +	   (WebCore::WorkerScriptLoader::didFailRedirectCheck):
> +	   (WebCore::WorkerScriptLoader::didReceiveAuthenticationCancellation):

> +	   (WebCore::WorkerScriptLoader::notifyFinished):
> +	   * workers/WorkerScriptLoader.h: Renamed from
workers/WorkerImportScriptsClient.h.
> +

It would be nice to have some more function level of file level comments.  For
example, why was 
WorkerScriptLoader.cpp renamed from workers/WorkerImportScriptsClient.cpp could
be one comment.

It would have been nice to separate the two changes as I see them:
1. Making WorkerImportScriptsClient more generic so other could use it.
2. Making WorkerContext use it for loading scripts.



> diff --git a/WebCore/WebCore.xcodeproj/project.pbxproj
b/WebCore/WebCore.xcodeproj/project.pbxproj

There are several other makefiles that need to be fixed up.



> diff --git a/WebCore/workers/Worker.h b/WebCore/workers/Worker.h
> +	   OwnPtr<WorkerScriptLoader> m_workerScriptLoader;

What about m_scriptLoader as the variable name?


> diff --git a/WebCore/workers/WorkerContext.cpp
b/WebCore/workers/WorkerContext.cpp

> +	   WorkerScriptLoader workerScriptLoader;
Consider using scriptLoader instead of orkerScriptLoader (just because shorter
variable names are easier to skim and not get confused).



> diff --git a/WebCore/workers/WorkerScriptLoader.cpp
b/WebCore/workers/WorkerScriptLoader.cpp
> new file mode 100644
> index 0000000..a0f3c48
> --- /dev/null
> +++ b/WebCore/workers/WorkerScriptLoader.cpp


> +	// We only support synchronous script loading for importScripts in
WorkerContext.
I wonder if this comment is necessary.	Especially because it could easily get
out of date and I'm not sure that it adds value.

How about just removing it?  The assert and the code right below it makes it
clear that the sync load path is only for use by workers.


> +void WorkerScriptLoader::didReceiveResponse(const ResourceResponse&
response)
> +{
> +    if (response.httpStatusCode() / 100 != 2 && response.httpStatusCode() !=
0) {

Avoid comparisons to zero.

s/response.httpStatusCode() != 0/response.httpStatusCode()/


+   m_responseEncoding = response.textEncodingName();
About this for the script for the Worker itself. dimich did a lot of
investigation about the encodings for worker scripts, so
we should get a comment from him if this is the right thing or not.


> +
> +}
Add the namespace comment.


> diff --git a/WebCore/workers/WorkerScriptLoader.h
b/WebCore/workers/WorkerScriptLoader.h


> +    class WorkerScriptLoaderClient {
> +    public:
> +	   virtual void notifyFinished() { }
> +	   
> +    protected:
> +	   virtual ~WorkerScriptLoaderClient() { }
> +    };
> +
It would be good to have WorkerScriptLoaderClient in a separate header file.

Why does the destructor need to be defined (as virtual)?



> +    class WorkerScriptLoader : public ThreadableLoaderClient {

There are a lot of parameter names that don't add any information.  Please
remove them.

First example "ScriptExecutionContext* scriptExecutionContext"


>
> +}
Add namespace comment.

> +#endif // ENABLE(WORKERS)
> +#endif
Add comment about what this ends.