Bug 84220 - Implement object-literal constructor for Intent object
Summary: Implement object-literal constructor for Intent object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Greg Billock
URL:
Keywords:
Depends on:
Blocks: 75123
  Show dependency treegraph
 
Reported: 2012-04-17 17:56 PDT by Greg Billock
Modified: 2012-04-25 17:28 PDT (History)
10 users (show)

See Also:


Attachments
Patch (17.08 KB, patch)
2012-04-17 17:59 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (20.18 KB, patch)
2012-04-18 09:16 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (22.89 KB, patch)
2012-04-18 09:33 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (24.24 KB, patch)
2012-04-18 16:58 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (26.06 KB, patch)
2012-04-18 18:20 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (26.31 KB, patch)
2012-04-19 18:05 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (26.83 KB, patch)
2012-04-19 18:12 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (27.04 KB, patch)
2012-04-20 09:04 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (27.19 KB, patch)
2012-04-20 10:37 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (27.50 KB, patch)
2012-04-23 09:14 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (27.55 KB, patch)
2012-04-23 12:17 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (33.16 KB, patch)
2012-04-24 13:27 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (33.50 KB, patch)
2012-04-25 08:24 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (33.52 KB, patch)
2012-04-25 08:28 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (33.61 KB, patch)
2012-04-25 09:56 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (33.91 KB, patch)
2012-04-25 10:43 PDT, Greg Billock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Billock 2012-04-17 17:56:12 PDT
Implement object-literal constructor for Intent object
Comment 1 Greg Billock 2012-04-17 17:59:43 PDT
Created attachment 137644 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-17 18:02:41 PDT
Attachment 137644 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Kentaro Hara 2012-04-17 18:16:25 PDT
Comment on attachment 137644 [details]
Patch

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

Before looking into more details, maybe we want to confirm that the code works as expected (i.e. want test cases).

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]

Please add test cases in fast/events/constructors/. (Please look at other tests in that directory.)

> Source/WebCore/ChangeLog:17
> +        * Modules/intents/DeliveredIntent.h: Copied from Source/WebCore/Modules/intents/Intent.h.
> +        (WebCore):
> +        (DeliveredIntentClient):
> +        (WebCore::DeliveredIntentClient::~DeliveredIntentClient):
> +        (WebCore::DeliveredIntentClient::postResult):
> +        (WebCore::DeliveredIntentClient::postFailure):
> +        (DeliveredIntent):
> +        (WebCore::DeliveredIntent::~DeliveredIntent):

This ChangeLog seems wrong.

> Source/WebCore/bindings/v8/Dictionary.cpp:422
> +    v8::Local<v8::Value> v8Value;
> +    if (!getKey(key, v8Value))
> +        return false;
> +
> +    if (v8Value->IsObject())
> +        value = Dictionary(v8Value);

I am not sure if this implementation is correct. Could you add test cases as I mentioned above?
Comment 4 Greg Billock 2012-04-18 09:16:11 PDT
Created attachment 137705 [details]
Patch
Comment 5 Greg Billock 2012-04-18 09:33:38 PDT
Created attachment 137707 [details]
Patch
Comment 6 Kentaro Hara 2012-04-18 09:34:47 PDT
Comment on attachment 137705 [details]
Patch

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

We need more comprehensive test cases (before discussing details). Making the test cases work correctly would be the fastest way to make the patch correct, rather than discussing the maybe-wrong code.

- You need to add fast/events/constructors/web-intents-constructor.html. The test should cover almost all possible constructor patterns. Please look at fast/events/constructors/message-event-constructor.html. Also please add test cases for each exception that the constructor can throw (I know that some exceptions are difficult to test. Then you can skip them).

- web-intents-obj-constructor.html should cover more use cases of WebKitIntent.

> Source/WebCore/ChangeLog:9
> +        Implement object-literal constructor for Intent object. This will
> +        hopefully be temporary, as we'll convert to just using this constructor,
> +        which can then use codegen.
> +        Added layout tests for object constructor:
> +        webintents/web-intent-obj-constructor.html
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=84220

Please follow the ChangeLog convention:

1. Title
2. Bug link
3. Reviewed by
4. Description
5. Tests

> LayoutTests/webintents/web-intents-api-expected.txt:3
> +PASS new WebKitIntent('a') threw exception Error: SYNTAX_ERR: DOM Exception 12.

This would be wrong. The previous test result seems correct.

> LayoutTests/webintents/web-intents-api.html:-11
> -      shouldThrow("new WebKitIntent('a')", "'TypeError: Not enough arguments'");

Ditto.

> LayoutTests/webintents/web-intents-obj-constructor.html:17
> +<input type="button" id="button" value="Start Web Intent" onmouseup="buttonClicked()">

We do not need to click a button to run this test.
Comment 7 Kentaro Hara 2012-04-18 09:35:29 PDT
Comment on attachment 137707 [details]
Patch

Marking r- due to the previous comments.
Comment 8 WebKit Review Bot 2012-04-18 09:36:42 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 9 WebKit Review Bot 2012-04-18 10:58:32 PDT
Comment on attachment 137705 [details]
Patch

Attachment 137705 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12423911
Comment 10 Greg Billock 2012-04-18 16:54:57 PDT
Comment on attachment 137705 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        https://bugs.webkit.org/show_bug.cgi?id=84220
> 
> Please follow the ChangeLog convention:
> 
> 1. Title
> 2. Bug link
> 3. Reviewed by
> 4. Description
> 5. Tests

Done.

>> LayoutTests/webintents/web-intents-api-expected.txt:3
>> +PASS new WebKitIntent('a') threw exception Error: SYNTAX_ERR: DOM Exception 12.
> 
> This would be wrong. The previous test result seems correct.

The new constructor is single-arg (the object-literal). 'a' isn't a dictionary, so it errors out. I'm using EXCEPTION_BLOCK on this, which perhaps ought to throw a type mismatch error? Anyway, this is just stock behavior I believe.

> LayoutTests/webintents/web-intents-obj-constructor.html:1
> +<html>

I put the constructor here rather than with the other constructors since there's already the right exclusions in platforms for LayoutTests/webintents. The other constructor form is tested in web-intents-api.html

>> LayoutTests/webintents/web-intents-obj-constructor.html:17
>> +<input type="button" id="button" value="Start Web Intent" onmouseup="buttonClicked()">
> 
> We do not need to click a button to run this test.

A user gesture is required for webkitStartActivity.
Comment 11 Greg Billock 2012-04-18 16:58:26 PDT
Created attachment 137804 [details]
Patch
Comment 12 Greg Billock 2012-04-18 18:20:12 PDT
Created attachment 137812 [details]
Patch
Comment 13 Greg Billock 2012-04-18 18:21:11 PDT
Added extras translation and more tests.
Comment 14 WebKit Review Bot 2012-04-18 19:16:38 PDT
Comment on attachment 137812 [details]
Patch

Attachment 137812 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12441074
Comment 15 Kentaro Hara 2012-04-19 16:02:05 PDT
Comment on attachment 137812 [details]
Patch

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

Please add more test cases. As commented below, we need to confirm the behavior when we pass various parameters to the Intent constructor. (Please look at existing tests in fast/events/constructors/.)

> Source/WebCore/Modules/intents/Intent.cpp:67
> +    String action;
> +    bool actionExists = parameters.getWithUndefinedOrNullCheck("action", action);
> +    String type;
> +    bool typeExists = parameters.getWithUndefinedOrNullCheck("type", type);
> +    String service;
> +    parameters.get("service", service);

Is this behavior speced? (i.e. what should happen when we pass null or undefined?)
 
Anyway please add test cases for all possible parameters.

> Source/WebCore/Modules/intents/Intent.cpp:71
> +    if (!actionExists || action.isEmpty()) {
>          ec = SYNTAX_ERR;
>          return 0;

Ditto.

> Source/WebCore/Modules/intents/Intent.cpp:75
> +    if (!typeExists || type.isEmpty()) {
>          ec = SYNTAX_ERR;
>          return 0;

Ditto.

> Source/WebCore/Modules/intents/Intent.idl:31
> +      Constructor(in Dictionary parameters),

Remove this. This IDL attribute makes no sense under [CustomConstructor].

> Source/WebCore/bindings/v8/Dictionary.cpp:101
> -    if (!options->Has(v8Key))
> +    if (!options->Has(v8Key->ToString()))

What is the rational for this change?

> Source/WebCore/bindings/v8/Dictionary.cpp:437
> +      if (key.IsEmpty() || !key->Length() || !wrapper->Has(key))

Can't this be just 'if (!wrapper->Has(key))'?

> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:86
> +    if (ec)
> +        return throwError(ec);

Nit: Move these two lines to just below 'RefPtr<Intent> impl = ...'.
Comment 16 Greg Billock 2012-04-19 18:05:29 PDT
Created attachment 138017 [details]
Patch
Comment 17 Greg Billock 2012-04-19 18:12:19 PDT
Created attachment 138020 [details]
Patch
Comment 18 Greg Billock 2012-04-19 18:13:26 PDT
Comment on attachment 137812 [details]
Patch

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

>> Source/WebCore/Modules/intents/Intent.cpp:67
>> +    parameters.get("service", service);
> 
> Is this behavior speced? (i.e. what should happen when we pass null or undefined?)
> 
> Anyway please add test cases for all possible parameters.

The only missing one now in the test is "transfer". I've got that working now. It's fairly nasty, though, since by the time I get the data in the Dictionary, there's no straightforward way to run the v8-side code to extract transferables. I will likely punt for now and only support MessagePortArray.

For "service", if the field is present at all, we interpret the intent as explicit, so this is a bug (I need checks to verify that.) BTW, is there any way to do better signaling of the error? Can I pass back an error string to help users out? DOM Error 12 is pretty unpromising as an error message. :-(

>> Source/WebCore/Modules/intents/Intent.cpp:71
>>          return 0;
> 
> Ditto.

Yes, this is speced -- at present, you can't legally invoke an intent without action/type.

>> Source/WebCore/Modules/intents/Intent.idl:31
>> +      Constructor(in Dictionary parameters),
> 
> Remove this. This IDL attribute makes no sense under [CustomConstructor].

Should I get rid of the other one, too?

>> Source/WebCore/bindings/v8/Dictionary.cpp:101
>> +    if (!options->Has(v8Key->ToString()))
> 
> What is the rational for this change?

Oh, drat. I think I changed this in error while editing this file.

>> Source/WebCore/bindings/v8/Dictionary.cpp:437
>> +      if (key.IsEmpty() || !key->Length() || !wrapper->Has(key))
> 
> Can't this be just 'if (!wrapper->Has(key))'?

Done.

>> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:86
>> +        return throwError(ec);
> 
> Nit: Move these two lines to just below 'RefPtr<Intent> impl = ...'.

Done.
Comment 19 WebKit Review Bot 2012-04-19 18:43:06 PDT
Comment on attachment 138020 [details]
Patch

Attachment 138020 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12479044
Comment 20 Greg Billock 2012-04-20 09:04:01 PDT
Created attachment 138102 [details]
Patch
Comment 21 WebKit Review Bot 2012-04-20 09:36:27 PDT
Comment on attachment 138102 [details]
Patch

Attachment 138102 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12476198
Comment 22 Greg Billock 2012-04-20 10:37:02 PDT
Created attachment 138116 [details]
Patch
Comment 23 Kent Tamura 2012-04-20 18:42:14 PDT
Comment on attachment 138116 [details]
Patch

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

> Source/WebCore/Modules/intents/Intent.cpp:98
> +    if (dataExists) {
> +      bool didThrow = false;
> +      serializedData = dataValue.serialize(scriptState, &ports, 0, didThrow);
> +      if (didThrow) {
> +          ec = DATA_CLONE_ERR;
> +          return 0;
> +      }
> +    }

Wrong indentation

> Source/WebCore/bindings/v8/Dictionary.cpp:445
> +    for (uint32_t i = 0; i < properties->Length(); ++i) {
> +      v8::Local<v8::String> key = properties->Get(i)->ToString();
> +      if (!wrapper->Has(key))
> +          continue;
> +
> +      v8::Local<v8::Value> value = wrapper->Get(key);
> +      String stringKey = v8ValueToWebCoreString(key);
> +      String stringValue = v8ValueToWebCoreString(value);
> +      if (!stringKey.isEmpty())
> +          hashMap.set(stringKey, stringValue);
> +    }

Wrong indentation.

> Source/WebKit/chromium/public/WebIntent.h:68
> +    WEBKIT_EXPORT WebString extrasValue(const WebString&) const;

We should explain what is the argument and what's returned if the argument is not good.
Comment 24 Greg Billock 2012-04-23 08:56:42 PDT
Comment on attachment 138116 [details]
Patch

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

>> Source/WebCore/Modules/intents/Intent.cpp:98
>> +    }
> 
> Wrong indentation

Done.

>> Source/WebCore/bindings/v8/Dictionary.cpp:445
>> +    }
> 
> Wrong indentation.

Done.

>> Source/WebKit/chromium/public/WebIntent.h:68
>> +    WEBKIT_EXPORT WebString extrasValue(const WebString&) const;
> 
> We should explain what is the argument and what's returned if the argument is not good.

Added comments. The intended usage is to call extrasNames() and then iterate through the names getting the values. (There's no "WebHashMap" or I'd have used that.)
Comment 25 Greg Billock 2012-04-23 09:14:22 PDT
Created attachment 138360 [details]
Patch
Comment 26 Kentaro Hara 2012-04-23 10:51:54 PDT
Comment on attachment 138360 [details]
Patch

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

First round of comments.

> Source/WebCore/Modules/intents/Intent.cpp:63
> +    bool actionExists = parameters.getWithUndefinedOrNullCheck("action", action);

Why do you use getWithUndefinedOrNullCheck() for action and type, but use get() for service? Is it speced?

> Source/WebCore/Modules/intents/Intent.cpp:68
> +    if (parameters.get("service", service)
> +        && (service.isNull() || service.isEmpty())) {

This condition ('service.isNull() || service.isEmpty()') sounds strange. Is it speced? We need to take case how Empty, Null and Undefined should be treated. Also please add test cases for Empty, Null and Undefined.

Nit: No line break please.

> Source/WebCore/Modules/intents/Intent.cpp:73
> +    if (!actionExists || action.isEmpty()) {

Nit: actionExists would not be necessary. 'if (!paramters.get("action", action) || action.isEmpty)'.

> Source/WebCore/Modules/intents/Intent.cpp:77
> +    if (!typeExists || type.isEmpty()) {

Ditto.

> Source/WebCore/Modules/intents/Intent.cpp:85
> +    if (portsExists)

Ditto.

> Source/WebCore/Modules/intents/Intent.cpp:91
> +    if (dataExists) {

Ditto.

> Source/WebCore/Modules/intents/Intent.cpp:95
> +            ec = DATA_CLONE_ERR;

Please add a test case for this exception.

> Source/WebCore/Modules/intents/Intent.cpp:103
> +    if (extrasExists)

Ditto. extrasExists would not be necessary.

> Source/WebCore/Modules/intents/Intent.h:72
> +

Nit: No line break please.

> Source/WebCore/Modules/intents/Intent.h:75
> +

Ditto.
Comment 27 Greg Billock 2012-04-23 12:05:34 PDT
Comment on attachment 138360 [details]
Patch

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

>> Source/WebCore/Modules/intents/Intent.cpp:63
>> +    bool actionExists = parameters.getWithUndefinedOrNullCheck("action", action);
> 
> Why do you use getWithUndefinedOrNullCheck() for action and type, but use get() for service? Is it speced?

It's speced that action/type must be present. (Service is optional.)

>> Source/WebCore/Modules/intents/Intent.cpp:68
>> +        && (service.isNull() || service.isEmpty())) {
> 
> This condition ('service.isNull() || service.isEmpty()') sounds strange. Is it speced? We need to take case how Empty, Null and Undefined should be treated. Also please add test cases for Empty, Null and Undefined.
> 
> Nit: No line break please.

If service is present, it's speced to be a URL, so a null or empty value is a caller error. Is there a way to be more explicit with the message?

>> Source/WebCore/Modules/intents/Intent.cpp:73
>> +    if (!actionExists || action.isEmpty()) {
> 
> Nit: actionExists would not be necessary. 'if (!paramters.get("action", action) || action.isEmpty)'.

Done. Much better.

>> Source/WebCore/Modules/intents/Intent.cpp:77
>> +    if (!typeExists || type.isEmpty()) {
> 
> Ditto.

Done.

>> Source/WebCore/Modules/intents/Intent.cpp:85
>> +    if (portsExists)
> 
> Ditto.

Done.

>> Source/WebCore/Modules/intents/Intent.cpp:91
>> +    if (dataExists) {
> 
> Ditto.

Done.

>> Source/WebCore/Modules/intents/Intent.cpp:95
>> +            ec = DATA_CLONE_ERR;
> 
> Please add a test case for this exception.

Done.

>> Source/WebCore/Modules/intents/Intent.cpp:103
>> +    if (extrasExists)
> 
> Ditto. extrasExists would not be necessary.

Done.

>> Source/WebCore/Modules/intents/Intent.h:72
>> +
> 
> Nit: No line break please.

Done.

>> Source/WebCore/Modules/intents/Intent.h:75
>> +
> 
> Ditto.

Done.
Comment 28 Greg Billock 2012-04-23 12:17:23 PDT
Created attachment 138397 [details]
Patch
Comment 29 Kent Tamura 2012-04-23 17:51:02 PDT
Comment on attachment 138397 [details]
Patch

LGTM for WebKit API part and DRT part.
Comment 30 WebKit Review Bot 2012-04-24 01:56:15 PDT
Comment on attachment 138397 [details]
Patch

Attachment 138397 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12528183
Comment 31 Kentaro Hara 2012-04-24 09:25:06 PDT
Comment on attachment 138397 [details]
Patch

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

> Source/WebCore/Modules/intents/Intent.cpp:63
> +    if (!parameters.getWithUndefinedOrNullCheck("action", action) || action.isEmpty()) {

> > Why do you use getWithUndefinedOrNullCheck() for action and type, but use get() for service? Is it speced?
> 
> It's speced that action/type must be present. (Service is optional.)

- 'must be present' and 'undefined or null' are different things. Please refer to http://www.w3.org/TR/WebIDL/#es-DOMString

{type: undefined} => present. type is treated as a string "undefined".
{type: null} => present. type is treated as a string "null".
{type: ""} => present. type is treated as "".
{} => not present. type is treated as a null string.

Unless the spec explicitly says that undefined or null should be treated as a null string, we should not use parameters.getUndefinedOrNullCheck().

- I think you do not need to check action.isEmpty() here. Isn't it the work of Intent::create()?

- As I commented repeatedly, please add test cases for these. Missing type, type = undefined, type = null, type = "". Same for action, service, extras etc. Please look at other constructor test cases in fast/events/constructors/*. As we have discussed, we often mis-implement the behavior for these corner cases.

> Source/WebCore/Modules/intents/Intent.cpp:69
> +    if (!parameters.getWithUndefinedOrNullCheck("type", type) || type.isEmpty()) {

Ditto.

> Source/WebCore/Modules/intents/Intent.cpp:77
> +    if (parameters.get("service", service) && (service.isNull() || service.isEmpty())) {
> +        ec = SYNTAX_ERR;
> +        return 0;

> If service is present, it's speced to be a URL, so a null or empty value is a caller error. Is there a way to be more explicit with the message?

The check should be service.isEmpty(). String::isNull() returns true if the string is null (i.e. not initialized). String::isEmpty() returns true if the string is null or an empty string. Thus if isEmpty() is true, then isNull() is true.

But I think that the check should be done in Intent::create().

> Source/WebCore/Modules/intents/Intent.cpp:91
> +            ec = DATA_CLONE_ERR;

Please add a test case for this.

> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:66
> +        return toV8(impl.release(), args.Holder());

I would guess that V8Proxy::toV8() no longer exists (since I removed it:-) Please use setJSWrapperForDOMObject(), just like you are doing below.

> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:81
> +        return v8::Undefined();

Shouldn't we return some exception? (I am not sure though.)
Comment 32 Greg Billock 2012-04-24 11:11:41 PDT
(In reply to comment #31)
> (From update of attachment 138397 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138397&action=review
> 
> > Source/WebCore/Modules/intents/Intent.cpp:63
> > +    if (!parameters.getWithUndefinedOrNullCheck("action", action) || action.isEmpty()) {
> 
> > > Why do you use getWithUndefinedOrNullCheck() for action and type, but use get() for service? Is it speced?
> > 
> > It's speced that action/type must be present. (Service is optional.)
> 
> - 'must be present' and 'undefined or null' are different things. Please refer to http://www.w3.org/TR/WebIDL/#es-DOMString
> 
> {type: undefined} => present. type is treated as a string "undefined".
> {type: null} => present. type is treated as a string "null".
> {type: ""} => present. type is treated as "".
> {} => not present. type is treated as a null string.
> 
> Unless the spec explicitly says that undefined or null should be treated as a null string, we should not use parameters.getUndefinedOrNullCheck().

OK, fixed.
 
> - I think you do not need to check action.isEmpty() here. Isn't it the work of Intent::create()?

Done.

> - As I commented repeatedly, please add test cases for these. Missing type, type = undefined, type = null, type = "". Same for action, service, extras etc. Please look at other constructor test cases in fast/events/constructors/*. As we have discussed, we often mis-implement the behavior for these corner cases.

Will do. My intention was to have some of those covered by getUndefinedOrNullCheck, but I'll add more.
 
> > Source/WebCore/Modules/intents/Intent.cpp:69
> > +    if (!parameters.getWithUndefinedOrNullCheck("type", type) || type.isEmpty()) {
> 
> Ditto.

Done.
 
> > Source/WebCore/Modules/intents/Intent.cpp:77
> > +    if (parameters.get("service", service) && (service.isNull() || service.isEmpty())) {
> > +        ec = SYNTAX_ERR;
> > +        return 0;
> 
> > If service is present, it's speced to be a URL, so a null or empty value is a caller error. Is there a way to be more explicit with the message?
> 
> The check should be service.isEmpty(). String::isNull() returns true if the string is null (i.e. not initialized). String::isEmpty() returns true if the string is null or an empty string. Thus if isEmpty() is true, then isNull() is true.
> 
> But I think that the check should be done in Intent::create().

Done.

> > Source/WebCore/Modules/intents/Intent.cpp:91
> > +            ec = DATA_CLONE_ERR;
> 
> Please add a test case for this.

This is present.
 
> > Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:66
> > +        return toV8(impl.release(), args.Holder());
> 
> I would guess that V8Proxy::toV8() no longer exists (since I removed it:-) Please use setJSWrapperForDOMObject(), just like you are doing below.

Done.

> > Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:81
> > +        return v8::Undefined();
> 
> Shouldn't we return some exception? (I am not sure though.)

Yes. Done.
Comment 33 Greg Billock 2012-04-24 13:27:35 PDT
Created attachment 138629 [details]
Patch
Comment 34 Greg Billock 2012-04-24 13:32:10 PDT
I added tons more constructor edge case tests here. I'm a bit unhappy about them WRT to the treatment of 'extras'. Right now the semantics are "stringify all own properties into a map". Would it be nicer if it would detect, say, string or number or array types, and then throw TYPE_ERR? The Dictionary constructor takes arbitrary v8::Value's, so EXCEPTION_BLOCK basically always works. I think that's acceptable, but it'd be less surprising to TYPE_ERR on those cases. Is there a good detection routine for such cases?
Comment 35 Kentaro Hara 2012-04-24 23:21:35 PDT
Comment on attachment 138629 [details]
Patch

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

Almost looks OK except for test cases.

> Source/WebCore/Modules/intents/Intent.cpp:60
> +PassRefPtr<Intent> Intent::create(ScriptState* scriptState, const Dictionary& parameters, ExceptionCode& ec)

Nit: Shall we rename 'parameters' to 'options', since you are using 'options' in the caller side?

> Source/WebCore/Modules/intents/Intent.cpp:137
> +const KURL& Intent::service() const
> +{
> +    return m_service;
> +}

Nit: you can put this code in the header.

> Source/WebCore/Modules/intents/Intent.cpp:152
> +const WTF::HashMap<String, String>& Intent::extras() const
> +{
> +    return m_extras;
> +}

Nit: you can put this code in the header.

> Source/WebCore/bindings/v8/Dictionary.cpp:432
> +    if (isUndefinedOrNull())
> +        return false;
> +    if (!m_options->IsObject())

You can write:

    if (!isObject())
        return false;

> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:67
> +        V8DOMWrapper::setDOMWrapper(args.Holder(), &info, impl.get());

Nit: args.Holder() => wrapper.

> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:69
> +        return args.Holder();

Nit: args.Holder() => wrapper.

> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:93
> +    return args.Holder();

Nit: args.Holder() => wrapper.

> LayoutTests/webintents/web-intents-obj-constructor.html:2
> +  <head>

Nit: 4 space indent please. Or no indent is OK. WebKit does not use 2 space indent.

> LayoutTests/webintents/web-intents-obj-constructor.html:32
> +        shouldNotThrow("new WebKitIntent({'action':'a','type':'b'})");

Instead, we might write the test like this:

shouldBeEqualToString('(new WebKitIntent({"action":"a","type":"b"})).action', 'a');
shouldBeEqualToString('(new WebKitIntent({"action":"a","type":"b"})).type', 'b');

The same comment for the following tests. (Though I know the number of tests increases.)
Comment 36 Greg Billock 2012-04-25 08:03:27 PDT
Comment on attachment 138629 [details]
Patch

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

>> Source/WebCore/Modules/intents/Intent.cpp:60
>> +PassRefPtr<Intent> Intent::create(ScriptState* scriptState, const Dictionary& parameters, ExceptionCode& ec)
> 
> Nit: Shall we rename 'parameters' to 'options', since you are using 'options' in the caller side?

Done.

>> Source/WebCore/Modules/intents/Intent.cpp:137
>> +}
> 
> Nit: you can put this code in the header.

Done.

>> Source/WebCore/Modules/intents/Intent.cpp:152
>> +}
> 
> Nit: you can put this code in the header.

Done.

>> Source/WebCore/bindings/v8/Dictionary.cpp:432
>> +    if (!m_options->IsObject())
> 
> You can write:
> 
>     if (!isObject())
>         return false;

Done.

>> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:67
>> +        V8DOMWrapper::setDOMWrapper(args.Holder(), &info, impl.get());
> 
> Nit: args.Holder() => wrapper.

Done.

>> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:69
>> +        return args.Holder();
> 
> Nit: args.Holder() => wrapper.

Done.

>> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:93
>> +    return args.Holder();
> 
> Nit: args.Holder() => wrapper.

Done.

>> LayoutTests/webintents/web-intents-obj-constructor.html:2
>> +  <head>
> 
> Nit: 4 space indent please. Or no indent is OK. WebKit does not use 2 space indent.

Done.

>> LayoutTests/webintents/web-intents-obj-constructor.html:32
>> +        shouldNotThrow("new WebKitIntent({'action':'a','type':'b'})");
> 
> Instead, we might write the test like this:
> 
> shouldBeEqualToString('(new WebKitIntent({"action":"a","type":"b"})).action', 'a');
> shouldBeEqualToString('(new WebKitIntent({"action":"a","type":"b"})).type', 'b');
> 
> The same comment for the following tests. (Though I know the number of tests increases.)

Done. I like it. Currently the spec doesn't have accessors for |service| and |extras|, so I've left them as is.
Comment 37 Greg Billock 2012-04-25 08:24:33 PDT
Created attachment 138814 [details]
Patch
Comment 38 Greg Billock 2012-04-25 08:28:12 PDT
Created attachment 138817 [details]
Patch
Comment 39 Kentaro Hara 2012-04-25 08:52:49 PDT
Comment on attachment 138817 [details]
Patch

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

Sorry for the iterative comments. Almost r+.

> Source/WebCore/ChangeLog:9
> +        This will hopefully be temporary, as we'll convert to just using this
> +        constructor, which can then use codegen.

Please update the description. It is a good idea to add the link to the spec that supports your change.
http://dvcs.w3.org/hg/web-intents/raw-file/tip/spec/Overview.html ?

> Source/WebCore/ChangeLog:11
> +        Added layout tests for object constructor:
> +        webintents/web-intent-obj-constructor.html

Our convention is to write

        Test: webintents/web-intent-obj-constructor.html

Please look at other ChangeLogs.

> Source/WebCore/Modules/intents/Intent.cpp:60
> +PassRefPtr<Intent> Intent::create(ScriptState* scriptState, const Dictionary& options, ExceptionCode& ec)

How about removing scriptState from the method arguments, and call ScriptState::current() just before dataValue.serialize()?

> Source/WebCore/Modules/intents/Intent.cpp:77
> +    options.get("service", service);
> +    if (!service.isEmpty()) {

if (options.get("service", service) && !service.isEmpty())

> Source/WebKit/chromium/src/WebIntent.cpp:139
> +    for (int i = 0; keyIter != m_private->extras().end().keys(); ++keyIter, ++i)

Nit: int => size_t?

> LayoutTests/webintents/web-intents-obj-constructor.html:43
> +        shouldNotThrow("new WebKitIntent({'action':'a','type':'b','service':''})");

>> The same comment for the following tests. (Though I know the number of tests increases.)
>>
> Currently the spec doesn't have accessors for |service| and |extras|, so I've left them as is.

It seems the spec has service and extras as a readonly attribute: http://dvcs.w3.org/hg/web-intents/raw-file/tip/spec/Overview.html

Maybe you need to add service and extras to Intent.idl?

At least, given that you implemented accessors for service and extras in this patch, we should test them. Otherwise, we might want to remove accessors for service and extras from this patch for now.
Comment 40 Greg Billock 2012-04-25 09:42:17 PDT
Comment on attachment 138817 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        constructor, which can then use codegen.
> 
> Please update the description. It is a good idea to add the link to the spec that supports your change.
> http://dvcs.w3.org/hg/web-intents/raw-file/tip/spec/Overview.html ?

Done.

>> Source/WebCore/ChangeLog:11
>> +        webintents/web-intent-obj-constructor.html
> 
> Our convention is to write
> 
>         Test: webintents/web-intent-obj-constructor.html
> 
> Please look at other ChangeLogs.

Done.

>> Source/WebCore/Modules/intents/Intent.cpp:60
>> +PassRefPtr<Intent> Intent::create(ScriptState* scriptState, const Dictionary& options, ExceptionCode& ec)
> 
> How about removing scriptState from the method arguments, and call ScriptState::current() just before dataValue.serialize()?

I put it like this to be more codegen-alike. It looks like ScriptState::current is a v8-ism.

>> Source/WebCore/Modules/intents/Intent.cpp:77
>> +    if (!service.isEmpty()) {
> 
> if (options.get("service", service) && !service.isEmpty())

Done.

>> Source/WebKit/chromium/src/WebIntent.cpp:139
>> +    for (int i = 0; keyIter != m_private->extras().end().keys(); ++keyIter, ++i)
> 
> Nit: int => size_t?

Done.

> LayoutTests/webintents/web-intents-obj-constructor-expected.txt:3
> +Explicit intent service: http://explicit.com/

Here's where service and extras get tested (although not from JS).
Comment 41 Greg Billock 2012-04-25 09:45:11 PDT
(In reply to comment #39)
> (From update of attachment 138817 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138817&action=review
> 
> Sorry for the iterative comments. Almost r+.

yay! :-) no worries.

> >> The same comment for the following tests. (Though I know the number of tests increases.)
> >>
> > Currently the spec doesn't have accessors for |service| and |extras|, so I've left them as is.
> 
> It seems the spec has service and extras as a readonly attribute: http://dvcs.w3.org/hg/web-intents/raw-file/tip/spec/Overview.html
> 
> Maybe you need to add service and extras to Intent.idl?
> 
> At least, given that you implemented accessors for service and extras in this patch, we should test them. Otherwise, we might want to remove accessors for service and extras from this patch for now.

The attributes are on the object literal, though, not on the intent object. These tests do test those accessors (from the DRT code), but not from JS.
Comment 42 Kentaro Hara 2012-04-25 09:48:59 PDT
(In reply to comment #40)
> >> Source/WebCore/Modules/intents/Intent.cpp:60
> >> +PassRefPtr<Intent> Intent::create(ScriptState* scriptState, const Dictionary& options, ExceptionCode& ec)
> > 
> > How about removing scriptState from the method arguments, and call ScriptState::current() just before dataValue.serialize()?
> 
> I put it like this to be more codegen-alike. It looks like ScriptState::current is a v8-ism.

Makes sense.

> > At least, given that you implemented accessors for service and extras in this patch, we should test them. Otherwise, we might want to remove accessors for service and extras from this patch for now.
>>
> The attributes are on the object literal, though, not on the intent object. These tests do test those accessors (from the DRT code), but not from JS.

Makes sense.
Comment 43 Greg Billock 2012-04-25 09:56:49 PDT
Created attachment 138827 [details]
Patch
Comment 44 Kentaro Hara 2012-04-25 10:06:35 PDT
Comment on attachment 138827 [details]
Patch

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

Looks OK. Thank you very much for the long discussion:)

> Source/WebCore/ChangeLog:8
> +        This will hopefully be temporary, as we'll convert to just using this
> +        constructor, which can then use codegen.

Please explain why this patch is temporary. Recently there's been webkit-dev@ discussion on making ChangeLogs descriptive.
Comment 45 Greg Billock 2012-04-25 10:43:59 PDT
Created attachment 138841 [details]
Patch
Comment 46 Greg Billock 2012-04-25 10:45:17 PDT
(In reply to comment #44)
> (From update of attachment 138827 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138827&action=review
> 
> Looks OK. Thank you very much for the long discussion:)

Thanks for the review!

> 
> > Source/WebCore/ChangeLog:8
> > +        This will hopefully be temporary, as we'll convert to just using this
> > +        constructor, which can then use codegen.
> 
> Please explain why this patch is temporary. Recently there's been webkit-dev@ discussion on making ChangeLogs descriptive.

Updated with an improved changelog description.
Comment 47 Kentaro Hara 2012-04-25 14:38:56 PDT
BTW, if you want to commit it, please change "cq" to "cq?". Then I can change it to "cq+".
Comment 48 WebKit Review Bot 2012-04-25 17:28:05 PDT
Comment on attachment 138841 [details]
Patch

Clearing flags on attachment: 138841

Committed r115264: <http://trac.webkit.org/changeset/115264>
Comment 49 WebKit Review Bot 2012-04-25 17:28:36 PDT
All reviewed patches have been landed.  Closing bug.