Bug 86791 – Add suggestions field to web intents API.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Greg Billock]

Add suggestions field to web intents API.

[Greg Billock]

Created attachment 142593 [details]
Patch

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

[Adam Barth]

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 ?

[Adam Barth]

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

[Greg Billock]

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.

[Greg Billock]

Created attachment 142766 [details]
Patch

[Adam Barth]

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

[Greg Billock]

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.

[Greg Billock]

Created attachment 142796 [details]
Patch

[Greg Billock]

(In reply to comment #9)
> Created an attachment (id=142796) [details]
> Patch

This adds the test as well.

[Adam Barth]

Thanks!

[WebKit Review Bot]

Comment on attachment 142796 [details]
Patch

Clearing flags on attachment: 142796

Committed r117793: <http://trac.webkit.org/changeset/117793>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.