Summary: | Add suggestions field to web intents API. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Greg Billock <gbillock> | ||||||||
Component: | New Bugs | Assignee: | Greg Billock <gbillock> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, jamesr, tkent, tkent+wkapi, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 75123 | ||||||||||
Attachments: |
|
Description
Greg Billock
2012-05-17 17:36:32 PDT
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. |