Bug 84220 – Implement object-literal constructor for Intent object

• Component: New Bugs
• Status: RESOLVED FIXED

[Greg Billock]

Implement object-literal constructor for Intent object

[Greg Billock]

Created attachment 137644 [details]
Patch

[WebKit Review Bot]

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.

[Kentaro Hara]

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?

[Greg Billock]

Created attachment 137705 [details]
Patch

[Greg Billock]

Created attachment 137707 [details]
Patch

[Kentaro Hara]

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.

[Kentaro Hara]

Comment on attachment 137707 [details]
Patch

Marking r- due to the previous comments.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 137705 [details]
Patch

Attachment 137705 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12423911

[Greg Billock]

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.

[Greg Billock]

Created attachment 137804 [details]
Patch

[Greg Billock]

Created attachment 137812 [details]
Patch

[Greg Billock]

Added extras translation and more tests.

[WebKit Review Bot]

Comment on attachment 137812 [details]
Patch

Attachment 137812 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12441074

[Kentaro Hara]

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 = ...'.

[Greg Billock]

Created attachment 138017 [details]
Patch

[Greg Billock]

Created attachment 138020 [details]
Patch

[Greg Billock]

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.

[WebKit Review Bot]

Comment on attachment 138020 [details]
Patch

Attachment 138020 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12479044

[Greg Billock]

Created attachment 138102 [details]
Patch

[WebKit Review Bot]

Comment on attachment 138102 [details]
Patch

Attachment 138102 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12476198

[Greg Billock]

Created attachment 138116 [details]
Patch

[Kent Tamura]

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.

[Greg Billock]

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.)

[Greg Billock]

Created attachment 138360 [details]
Patch

[Kentaro Hara]

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.

[Greg Billock]

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.

[Greg Billock]

Created attachment 138397 [details]
Patch

[Kent Tamura]

Comment on attachment 138397 [details]
Patch

LGTM for WebKit API part and DRT part.

[WebKit Review Bot]

Comment on attachment 138397 [details]
Patch

Attachment 138397 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12528183

[Kentaro Hara]

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.)

[Greg Billock]

(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.

[Greg Billock]

Created attachment 138629 [details]
Patch

[Greg Billock]

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?

[Kentaro Hara]

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.)

[Greg Billock]

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.

[Greg Billock]

Created attachment 138814 [details]
Patch

[Greg Billock]

Created attachment 138817 [details]
Patch

[Kentaro Hara]

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.

[Greg Billock]

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).

[Greg Billock]

(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.

[Kentaro Hara]

(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.

[Greg Billock]

Created attachment 138827 [details]
Patch

[Kentaro Hara]

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.

[Greg Billock]

Created attachment 138841 [details]
Patch

[Greg Billock]

(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.

[Kentaro Hara]

BTW, if you want to commit it, please change "cq" to "cq?". Then I can change it to "cq+".

[WebKit Review Bot]

Comment on attachment 138841 [details]
Patch

Clearing flags on attachment: 138841

Committed r115264: <http://trac.webkit.org/changeset/115264>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.