[webkit-reviews] review denied: [Bug 86791] Add suggestions field to web intents API. : [Attachment 142593] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Greg Billock <gbillock at google.com>'s
request for review:
Bug 86791: Add suggestions field to web intents API.
https://bugs.webkit.org/show_bug.cgi?id=86791

Attachment 142593: Patch
https://bugs.webkit.org/attachment.cgi?id=142593&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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 ?