[webkit-reviews] review granted: [Bug 20141] Cannot call pointer to function console.log : [Attachment 197513] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Apr 11 11:10:23 PDT 2013


Darin Adler <darin at apple.com> has granted Victor Costan <costan at gmail.com>'s
request for review:
Bug 20141: Cannot call pointer to function console.log
https://bugs.webkit.org/show_bug.cgi?id=20141

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=197513&action=review


I’m going to say review+ but I think we can improve this further.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:191
> -    
> +

It’s strange to do all this whitespace stripping. I’m not sure we normally
encourage that sort of thing.

>> Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:188
>> +	void lenientThisMethod(const WebDOMString& str);
> 
> The parameter name "str" adds no information, so it should be removed. 
[readability/parameter_name] [5]

The style queue complaint is not new to the patch, and not important. The best
way to fix it would be to omit argument names entirely in these automatically
generated declarations. I don’t think we need the names in these files. But
this is specific to the C++ binding. I don’t even know the status of that
binding. Who is using it? Can we remove it? Worth discussing.

> Source/WebCore/page/Console.cpp:272
> +Page* Console::pageFromScriptState(ScriptState* state)

This should be a free function at the top of the file, not a Console member
function.

> Source/WebCore/page/Console.cpp:280
> +    Console* console = window->console();
> +    if (!console)
> +	   return 0;
> +    return console->page();

This seems strange. To get from DOMWindow to page, I would not suggest going
through the console. It will work, but it’s not the right way to do things,
even inside the Console class. The right way is to walk from DOMWindow to Page,
staying with the core classes such as Document, Frame, and Page.

> Source/WebCore/page/Console.h:88
> +    inline static Page* pageFromScriptState(ScriptState*);

I believe the inline here is non-helpful and should be omitted. I suggest
having this be a free function, not a Console member function.


More information about the webkit-reviews mailing list