Bug 41426 - Add WKArrayRef API to WebKit2
Summary: Add WKArrayRef API to WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-30 13:24 PDT by Sam Weinig
Modified: 2010-06-30 15:59 PDT (History)
1 user (show)

See Also:


Attachments
Patch (19.50 KB, patch)
2010-06-30 13:25 PDT, Sam Weinig
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2010-06-30 13:24:27 PDT
Add WKArrayRef API to WebKit2.
Comment 1 Sam Weinig 2010-06-30 13:25:15 PDT
Created attachment 60144 [details]
Patch
Comment 2 WebKit Review Bot 2010-06-30 13:30:43 PDT
Attachment 60144 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit2/Shared/WebArray.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/Shared/WebArray.h:59:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/UIProcess/API/C/WKArray.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/API/C/WKAPICast.h:60:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/UIProcess/API/C/WKAPICast.h:71:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2010-06-30 14:11:29 PDT
Comment on attachment 60144 [details]
Patch

> +        * Shared/WebArray.cpp: Added.
> +        (WebKit::WebArray::WebArray):
> +        (WebKit::WebArray::~WebArray):
> +        * Shared/WebArray.h: Added.
> +        (WebKit::WebArray::create):
> +        (WebKit::WebArray::adopt):
> +        (WebKit::WebArray::at):
> +        (WebKit::WebArray::size):
> +        (WebKit::WebArray::):
> +        * UIProcess/API/C/WKAPICast.h:
> +        (WebKit::):
> +        * UIProcess/API/C/WKArray.cpp: Added.
> +        (WKArrayGetItemAtIndex):
> +        (WKArrayGetSize):
> +        (WKArrayRetain):
> +        (WKArrayRelease):

I don’t see any reason to leave in these lists of added functions. I normally just delete them for added files.

Also, that broken "(WebKit::)" definitely should not be left in.

> +WebArray::WebArray(const void** entries, size_t size, const WebArrayCallbacks* callbacks)
> +    : m_entries((void**)fastMalloc(size * sizeof(void*)))

How about using "new" instead of fastMalloc and avoiding the type casts? How about using static_cast if you need a typecast?

> +    enum AdoptTag { Adopt };
> +    WebArray(AdoptTag, void** entries, size_t size, const WebArrayCallbacks* callbacks);

Maybe the AdoptTag should be the last argument. Might generate better code even.

I suggest leaving out the argument name "callbacks".

> +WK_EXPORT const void* WKArrayGetItemAtIndex(WKArrayRef array, size_t index);

What is behavior when you use a bad index? Guarantee of returning 0? Undefined?

Seems OK, r=me
Comment 4 Sam Weinig 2010-06-30 14:21:05 PDT
(In reply to comment #3)
> (From update of attachment 60144 [details])
> > +        * Shared/WebArray.cpp: Added.
> > +        (WebKit::WebArray::WebArray):
> > +        (WebKit::WebArray::~WebArray):
> > +        * Shared/WebArray.h: Added.
> > +        (WebKit::WebArray::create):
> > +        (WebKit::WebArray::adopt):
> > +        (WebKit::WebArray::at):
> > +        (WebKit::WebArray::size):
> > +        (WebKit::WebArray::):
> > +        * UIProcess/API/C/WKAPICast.h:
> > +        (WebKit::):
> > +        * UIProcess/API/C/WKArray.cpp: Added.
> > +        (WKArrayGetItemAtIndex):
> > +        (WKArrayGetSize):
> > +        (WKArrayRetain):
> > +        (WKArrayRelease):
> 
> I don’t see any reason to leave in these lists of added functions. I normally just delete them for added files.

Ok.

> 
> Also, that broken "(WebKit::)" definitely should not be left in.
> 
> > +WebArray::WebArray(const void** entries, size_t size, const WebArrayCallbacks* callbacks)
> > +    : m_entries((void**)fastMalloc(size * sizeof(void*)))
> 
> How about using "new" instead of fastMalloc and avoiding the type casts? How about using static_cast if you need a typecast?

Will change.

> > +    enum AdoptTag { Adopt };
> > +    WebArray(AdoptTag, void** entries, size_t size, const WebArrayCallbacks* callbacks);
> 
> Maybe the AdoptTag should be the last argument. Might generate better code even.
> 
> I suggest leaving out the argument name "callbacks".
> 

Will fix both.

> > +WK_EXPORT const void* WKArrayGetItemAtIndex(WKArrayRef array, size_t index);
> 
> What is behavior when you use a bad index? Guarantee of returning 0? Undefined?
> 

The behavior is the undefined.  RIght now we read off the end of the buffer and ASSERT in debug builds.

> Seems OK, r=me

Anders wants me to remove the 'Web' prefix from this class and instead we decided to call it ImmutableArray.  Does that sound ok?
Comment 5 Sam Weinig 2010-06-30 15:59:25 PDT
Landed with changes in r62212.