Bug 86791

Summary: Add suggestions field to web intents API.
Product: WebKit Reporter: Greg Billock <gbillock>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
none
Patch none

Description Greg Billock 2012-05-17 17:36:32 PDT
Add suggestions field to web intents API.
Comment 1 Greg Billock 2012-05-17 17:38:28 PDT
Created attachment 142593 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-17 17:42:44 PDT
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 3 Adam Barth 2012-05-17 18:08:54 PDT
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 4 Adam Barth 2012-05-17 18:09:44 PDT
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 5 Greg Billock 2012-05-18 12:49:01 PDT
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.
Comment 6 Greg Billock 2012-05-18 12:51:09 PDT
Created attachment 142766 [details]
Patch
Comment 7 Adam Barth 2012-05-18 12:54:05 PDT
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 8 Greg Billock 2012-05-18 13:24:24 PDT
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.
Comment 9 Greg Billock 2012-05-18 15:30:42 PDT
Created attachment 142796 [details]
Patch
Comment 10 Greg Billock 2012-05-18 15:44:20 PDT
(In reply to comment #9)
> Created an attachment (id=142796) [details]
> Patch

This adds the test as well.
Comment 11 Adam Barth 2012-05-20 15:50:29 PDT
Thanks!
Comment 12 WebKit Review Bot 2012-05-21 09:23:20 PDT
Comment on attachment 142796 [details]
Patch

Clearing flags on attachment: 142796

Committed r117793: <http://trac.webkit.org/changeset/117793>
Comment 13 WebKit Review Bot 2012-05-21 09:23:25 PDT
All reviewed patches have been landed.  Closing bug.