Bug 56898 – [chromium] WebBindings{.h,.cpp}: should use consistent argument naming

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[noel gordon]

Resolves a FIXME I added here on r81676.

[noel gordon]

Created attachment 86578 [details]
Patch

[WebKit Review Bot]

Attachment 86578 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8221791

[noel gordon]

Created attachment 86579 [details]
Patch - remove linux warning

[Ojan Vafai]

Comment on attachment 86579 [details]
Patch - remove linux warning

View in context: https://bugs.webkit.org/attachment.cgi?id=86579&action=review

omg, the style in this file is so inconsistent.

> Source/WebKit/chromium/public/WebBindings.h:52
> +    WEBKIT_API static bool construct(NPP, NPObject*, const NPVariant* args, uint32_t count, NPVariant* result);

Why change this? argCount seems more clear to me.

> Source/WebKit/chromium/public/WebBindings.h:76
> +    WEBKIT_API static void getStringIdentifiers(const NPUTF8** names, int32_t count, NPIdentifier*);

ditto

> Source/WebKit/chromium/src/WebBindings.cpp:207
> +    if (!object || (object->_class != npScriptObjectClass))
> +        return false;

Since this is a code change could we do the style change first and then a code change?

> Source/WebKit/chromium/src/WebBindings.cpp:227
> +    V8NPObject* v8npobject = reinterpret_cast<V8NPObject*>(object);

To match the class name, this maybe should be v8NPObject.

> Source/WebKit/chromium/src/WebBindings.cpp:228
> +    v8::Handle<v8::Object> v8object(v8npobject->v8Object);

Ditto. This should probably be v8Object.

[noel gordon]

Comment on attachment 86579 [details]
Patch - remove linux warning

View in context: https://bugs.webkit.org/attachment.cgi?id=86579&action=review

>> Source/WebKit/chromium/public/WebBindings.h:52
>> +    WEBKIT_API static bool construct(NPP, NPObject*, const NPVariant* args, uint32_t count, NPVariant* result);
> 
> Why change this? argCount seems more clear to me.

Done.

>> Source/WebKit/chromium/public/WebBindings.h:76
>> +    WEBKIT_API static void getStringIdentifiers(const NPUTF8** names, int32_t count, NPIdentifier*);
> 
> ditto

Done. And argCount in the remaining places below.

>> Source/WebKit/chromium/src/WebBindings.cpp:207
>> +        return false;
> 
> Since this is a code change could we do the style change first and then a code change?

Understood.  Removed the code change.

>> Source/WebKit/chromium/src/WebBindings.cpp:227
>> +    V8NPObject* v8npobject = reinterpret_cast<V8NPObject*>(object);
> 
> To match the class name, this maybe should be v8NPObject.

Done.

>> Source/WebKit/chromium/src/WebBindings.cpp:228
>> +    v8::Handle<v8::Object> v8object(v8npobject->v8Object);
> 
> Ditto. This should probably be v8Object.

Done.

[noel gordon]

Created attachment 86741 [details]
Patch (part 1) style change

[Ojan Vafai]

Comment on attachment 86741 [details]
Patch (part 1) style change

View in context: https://bugs.webkit.org/attachment.cgi?id=86741&action=review

> Source/WebKit/chromium/ChangeLog:10
> +        [chromium] WebBindings::getRangeImpl() should NULL check its NPObject argument
> +        https://bugs.webkit.org/show_bug.cgi?id=56898
> +
> +        Address FIXME I noted/added in r81676.  Wanted to just copy/paste the checking
> +        code from elsewhere in the file, but inconsistent argument naming made it more
> +        difficult than it need be so, to clean up the commons, add consistent naming.

This description no longer matches the patch. It's a bit lame, but since you'll be doing the NULL check in a separate patch, this cleanup should be done on a different bug.

[noel gordon]

Ok changing the bug title.

[noel gordon]

Created attachment 86742 [details]
Patch (mod 2) style change

[noel gordon]

Created attachment 86743 [details]
Patch (mod 2) style change changelog

[noel gordon]

Opened bug 56996 re the NULL check.

[WebKit Commit Bot]

Comment on attachment 86743 [details]
Patch (mod 2) style change changelog 

Clearing flags on attachment: 86743

Committed r81854: <http://trac.webkit.org/changeset/81854>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.