Bug 86791

Created attachment 142593 [details] Patch

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 142593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142593&action=review Tests? > Source/WebCore/Modules/intents/DeliveredIntent.cpp:52 > + : Intent(action, type, data, PassOwnPtr<MessagePortChannelArray>(), extras, KURL(), WTF::Vector<KURL>()) WTF::Vector -> Vector Vector.h has a "using" declaration in it. > Source/WebCore/Modules/intents/Intent.cpp:56 > WTF::HashMap<String, String> extras; > KURL serviceUrl; > + WTF::Vector<KURL> suggestions; These WTF prefixes aren't needed. > Source/WebCore/Modules/intents/Intent.cpp:110 > + for (HashSet<AtomicString>::iterator iter = suggestionsStrings.begin(); > + iter != suggestionsStrings.end(); ++iter) { You can put this on one line. > Source/WebCore/Modules/intents/Intent.cpp:111 > + KURL suggestionURL = KURL(KURL(), *iter); This is unlikely to be correct. Do you mean to resolve these URLs relative to some base URL? One approach is to add a [CallWith=ScriptExectionContext] to the IDL and to use that script execution to resolve the URL. > Source/WebCore/Modules/intents/Intent.cpp:114 > + ec = SYNTAX_ERR; > + return 0; Four-space indent. > Source/WebCore/Modules/intents/Intent.cpp:116 > + suggestions.append(suggestionURL); suggestionURL -> suggestedURL ?

Comment on attachment 142593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142593&action=review > Source/WebCore/Modules/intents/Intent.cpp:55 > KURL serviceUrl; Random style nit: serviceUrl should be serviceURL

Comment on attachment 142593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142593&action=review >> Source/WebCore/Modules/intents/DeliveredIntent.cpp:52 >> + : Intent(action, type, data, PassOwnPtr<MessagePortChannelArray>(), extras, KURL(), WTF::Vector<KURL>()) > > WTF::Vector -> Vector > > Vector.h has a "using" declaration in it. Done. >> Source/WebCore/Modules/intents/Intent.cpp:55 >> KURL serviceUrl; > > Random style nit: serviceUrl should be serviceURL Done. >> Source/WebCore/Modules/intents/Intent.cpp:56 >> + WTF::Vector<KURL> suggestions; > > These WTF prefixes aren't needed. Done. >> Source/WebCore/Modules/intents/Intent.cpp:110 >> + iter != suggestionsStrings.end(); ++iter) { > > You can put this on one line. Done. >> Source/WebCore/Modules/intents/Intent.cpp:111 >> + KURL suggestionURL = KURL(KURL(), *iter); > > This is unlikely to be correct. Do you mean to resolve these URLs relative to some base URL? One approach is to add a [CallWith=ScriptExectionContext] to the IDL and to use that script execution to resolve the URL. No, they're supposed to be absolute urls. (That's why I check validity after this.) >> Source/WebCore/Modules/intents/Intent.cpp:114 >> + return 0; > > Four-space indent. Oops! Fixed. >> Source/WebCore/Modules/intents/Intent.cpp:116 >> + suggestions.append(suggestionURL); > > suggestionURL -> suggestedURL ? Done.

Created attachment 142766 [details] Patch

Comment on attachment 142766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142766&action=review > Source/WebCore/Modules/intents/Intent.cpp:107 > + WTF::HashSet<AtomicString> suggestionsStrings; > + WTF::Vector<KURL> suggestions; I meant that you can drop the WTF prefixes everywhere. :)

Comment on attachment 142766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142766&action=review >> Source/WebCore/Modules/intents/Intent.cpp:107 >> + WTF::Vector<KURL> suggestions; > > I meant that you can drop the WTF prefixes everywhere. :) Gah. I got most of them, but not these. Removing.

Created attachment 142796 [details] Patch

(In reply to comment #9) > Created an attachment (id=142796) [details] > Patch This adds the test as well.

Thanks!

Comment on attachment 142796 [details] Patch Clearing flags on attachment: 142796 Committed r117793: <http://trac.webkit.org/changeset/117793>

All reviewed patches have been landed. Closing bug.