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 tree / graph
 
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
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

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.