[Webkit-unassigned] [Bug 20141] Cannot call pointer to function console.log

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20141


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #197513|review?                     |review+
               Flag|                            |




--- Comment #13 from Darin Adler <darin at apple.com>  2013-04-11 11:08:40 PST ---
(From update of attachment 197513)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.