[webkit-reviews] review denied: [Bug 22720] Make XMLHttpRequest work in Workers : [Attachment 26838] Address console messages and resourceRetrievedByXMLHttpRequest in XMLHttpRequest.cpp

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied David Levin
<levin at chromium.org>'s request for review:
Bug 22720: Make XMLHttpRequest work in Workers
https://bugs.webkit.org/show_bug.cgi?id=22720

Attachment 26838: Address console messages and
resourceRetrievedByXMLHttpRequest in XMLHttpRequest.cpp
https://bugs.webkit.org/attachment.cgi?id=26838&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> +	   Provde a generic class to handle the pattern of task callbacks
across worker/parent threads.  It

Typo: Provde.

> -#include "PlatformString.h"

I still don't see why it's ok to remove this include.

> +void Document::addMessageToConsole(MessageSource source, MessageLevel level,
const String& message, unsigned lineNumber, const String& sourceURL, bool
inspectorOnly)

We're becoming more and more passionate about preferring named enums to boolean
constants.

> +#include "wtf/PassRefPtr.h"

This should be a "<>" include.

> +	   bool isCallbackAllowed()

There is no "callback" in this context, so the name is not ideal. Maybe
canPerformTask()?

> +    PassRefPtr<GenericWorkerTask6<P1, MP1, P2, MP2, P3, MP3, P4, MP4, P5,
MP5, P6, MP6> > createCallbackTask(
> +	   WorkerMessagingProxy* messagingProxy,
> +	   void (*method)(ScriptExecutionContext*, MP1, MP2, MP3, MP4, MP5,
MP6),

I think there is a typedef for this type (Method) that you could use here. Same
for return type.

In general, this class is almost a generic wrapper for method invocation,
having it all just for workers may be sub-optimal. I'm not requesting that you
do anything about this now.

> +static void AddMessageToConsoleCallback(ScriptExecutionContext* context,
MessageSource source, MessageLevel level, const String& message, unsigned
lineNumber, const String& sourceURL, bool inspectorOnly)

Please fix this to use a lower case "a" in "add".

> +void WorkerContext::addMessageToConsole(MessageSource source, MessageLevel
level, const String& message, unsigned lineNumber, const String& sourceURL,
bool inspectorOnly)
> +{
> +    PassRefPtr<Task> task;

This should be RefPtr. PassRefPtr is just for passing function arguments and
return values (and something that we will replace with C++0x move semantics one
day in far, far future).

> +    // TODO(levin): The implementation is pending the fixes in
https://bugs.webkit.org/show_bug.cgi?id=23175
> +    ASSERT(0);

We use FIXME, not TODO, and don't add commenter's name (svn blame to the
rescue). You could also use notImplemented() macro, or ASSERT_NOT_REACHED().

> +	   void postTaskToParentContext(PassRefPtr<Task> task); // Executes the
task on the parent's context's thread asynchronously.

I think that you may have too many "'s" here.

> +    context->addMessageToConsole(JSMessageSource, ErrorMessageLevel,
message, 1, String(), false);

Here is the reason why we prefer named enumerations - it's not obvious at all
what "false" means here.

This is almost ready to land, but there are more minor nitpicks than it is
practical to correct when landing, so I'll ask for one more quick round.