Bug 56898 - [chromium] WebBindings{.h,.cpp}: should use consistent argument naming
Summary: [chromium] WebBindings{.h,.cpp}: should use consistent argument naming
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: noel gordon
URL:
Keywords:
Depends on:
Blocks: 56996
  Show dependency treegraph
 
Reported: 2011-03-23 00:03 PDT by noel gordon
Modified: 2011-03-23 23:48 PDT (History)
5 users (show)

See Also:


Attachments
Patch (13.38 KB, patch)
2011-03-23 00:22 PDT, noel gordon
no flags Details | Formatted Diff | Diff
Patch - remove linux warning (13.38 KB, patch)
2011-03-23 00:32 PDT, noel gordon
ojan: review-
ojan: commit-queue-
Details | Formatted Diff | Diff
Patch (part 1) style change (13.05 KB, patch)
2011-03-23 22:27 PDT, noel gordon
ojan: review+
ojan: commit-queue-
Details | Formatted Diff | Diff
Patch (mod 2) style change (13.05 KB, patch)
2011-03-23 22:45 PDT, noel gordon
no flags Details | Formatted Diff | Diff
Patch (mod 2) style change changelog (13.03 KB, patch)
2011-03-23 22:48 PDT, noel gordon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description noel gordon 2011-03-23 00:03:47 PDT
Resolves a FIXME I added here on r81676.
Comment 1 noel gordon 2011-03-23 00:22:55 PDT
Created attachment 86578 [details]
Patch
Comment 2 WebKit Review Bot 2011-03-23 00:27:46 PDT
Attachment 86578 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8221791
Comment 3 noel gordon 2011-03-23 00:32:39 PDT
Created attachment 86579 [details]
Patch - remove linux warning
Comment 4 Ojan Vafai 2011-03-23 00:41:38 PDT
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.
Comment 5 noel gordon 2011-03-23 22:26:11 PDT
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.
Comment 6 noel gordon 2011-03-23 22:27:05 PDT
Created attachment 86741 [details]
Patch (part 1) style change
Comment 7 Ojan Vafai 2011-03-23 22:34:29 PDT
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.
Comment 8 noel gordon 2011-03-23 22:42:27 PDT
Ok changing the bug title.
Comment 9 noel gordon 2011-03-23 22:45:07 PDT
Created attachment 86742 [details]
Patch (mod 2) style change
Comment 10 noel gordon 2011-03-23 22:48:35 PDT
Created attachment 86743 [details]
Patch (mod 2) style change changelog
Comment 11 noel gordon 2011-03-23 22:55:05 PDT
Opened bug 56996 re the NULL check.
Comment 12 WebKit Commit Bot 2011-03-23 23:48:16 PDT
Comment on attachment 86743 [details]
Patch (mod 2) style change changelog 

Clearing flags on attachment: 86743

Committed r81854: <http://trac.webkit.org/changeset/81854>
Comment 13 WebKit Commit Bot 2011-03-23 23:48:24 PDT
All reviewed patches have been landed.  Closing bug.