Bug 84220

Created attachment 137644 [details] Patch

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 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?

Created attachment 137705 [details] Patch

Created attachment 137707 [details] Patch

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 on attachment 137707 [details] Patch Marking r- due to the previous comments.

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 on attachment 137705 [details] Patch Attachment 137705 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12423911

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.

Created attachment 137804 [details] Patch

Created attachment 137812 [details] Patch

Added extras translation and more tests.

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

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

Created attachment 138017 [details] Patch

Created attachment 138020 [details] Patch

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 on attachment 138020 [details] Patch Attachment 138020 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12479044

Created attachment 138102 [details] Patch

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

Created attachment 138116 [details] Patch

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

Created attachment 138360 [details] Patch

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

Created attachment 138397 [details] Patch

Comment on attachment 138397 [details] Patch LGTM for WebKit API part and DRT part.

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

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

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

Created attachment 138629 [details] Patch

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

Created attachment 138814 [details] Patch

Created attachment 138817 [details] Patch

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

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

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

Created attachment 138827 [details] Patch

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.

Created attachment 138841 [details] Patch

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

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

Comment on attachment 138841 [details] Patch Clearing flags on attachment: 138841 Committed r115264: <http://trac.webkit.org/changeset/115264>

All reviewed patches have been landed. Closing bug.