Bug 42613 – WebScriptObject Should Allow Safely Checking For Key Existence

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Joseph Pecoraro]

This would be equivalent to JavaScript's `in` syntax.

For instance, the following cannot be done easily with WebScriptObject:

    var obj = {};
    var exists = ("key" in obj);

Methods include try/catch blocks with valueForKey, or overriding
undefinedKeySupport to return a value (not quite perfect):

    @implementation WebScriptObject(Info8_pCMUndefinedKeySupport)
    - (id)valueForUndefinedKey:(NSString *)key
    {
        NSLog(@"called valueForUndefinedKey: %@", key);
        return nil;
    }
    @end

Neither are ideal.

[Joseph Pecoraro]

Created attachment 62031 [details]
[PATCH] Adds `hasWebScriptKey`

Wise words of Geoff Garen:

> I agree that throwing an exception from valueForUndefinedKey: is
> fragile. It's unfortunate that NSObject chose an exception as the
> default implementation. Perhaps WebScriptObject should have overridden
> this behavior from the beginning. On the other hand, maybe overriding
> the default would have been confusing to authors used to the default.
> [...]
> 
> I know that Safari, at least, depends on the exception throwing
> behavior to detect properties that are not present. We could always
> change Safari, but I'm worried that other clients may have the same
> dependency. So I don't think we should change valueForUndefinedKey:.
> 
> Joe: The hasKey: addition sounds fine, with one modification:
> WebScriptObject is trying to conform to the key-value coding protocol,
> and it takes care to call out its own additions to the protocol
> using a special prefix. So, to match "removeWebScriptKey:", which
> is an addition to key-value coding, I would suggest "hasWebScriptKey:".

That is the approach I took here.

[Joseph Pecoraro]

<rdar://problem/8037314>

[Mark Rowe (bdash)]

This new method should not be added to the public header until it has been through API review.  Until that time it should be added in the private header in a category on the class named to indicate that it’s pending API review.

[Mark Rowe (bdash)]

Comment on attachment 62031 [details]
[PATCH] Adds `hasWebScriptKey`

I’m also not keen on the location you chose for the test case.  The test has nothing to do with plugins as far as I can tell.

[Joseph Pecoraro]

Created attachment 62090 [details]
[PATCH] Addressed Comments

(In reply to comment #3)
> This new method should not be added to the public header until it has been through API review.
> Until that time it should be added in the private header in a category on the class named to
> indicate that it’s pending API review.

Done. I put a "forward declaring @interface" in DRT, should I export the Private header instead?


(In reply to comment #4)
> (From update of attachment 62031 [details])
> I’m also not keen on the location you chose for the test case.  The test has nothing to do
> with plugins as far as I can tell.

Ahh, you're correct. I moved it to "LayoutTests/platform/mac/fast/objc/".

[WebKit Review Bot]

Attachment 62090 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/bindings/objc/WebScriptObjectPrivate.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 62090 [details]
[PATCH] Addressed Comments

r=me

[Joseph Pecoraro]

Thanks Geoff!

Committed r63763
	M	WebCore/ChangeLog
	M	WebCore/bindings/objc/WebScriptObject.mm
	M	WebCore/bindings/objc/WebScriptObjectPrivate.h
	A	LayoutTests/platform/mac/fast/objc/webScriptObject-hasWebScriptKey.html
	A	LayoutTests/platform/mac/fast/objc/script-tests/webScriptObject-hasWebScriptKey.js
	A	LayoutTests/platform/mac/fast/objc/script-tests/TEMPLATE.html
	A	LayoutTests/platform/mac/fast/objc/webScriptObject-hasWebScriptKey-expected.txt
	M	LayoutTests/ChangeLog
	M	WebKitTools/DumpRenderTree/mac/ObjCController.m
	M	WebKitTools/ChangeLog
r63763 = 7d5f4421d7f37dd7503c6bbe688a3a3b81e09e62 (refs/remotes/trunk)
http://trac.webkit.org/changeset/63763