Bug 73039 – Implement registration of WebIntents

• Component: New Bugs
• Status: RESOLVED FIXED

[Greg Billock]

[Web Intents] Flagged-off implementation of an intent tag for registration.

[Greg Billock]

Created attachment 116390 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 116390 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116390&action=review

> Source/WebCore/html/HTMLIntentElement.cpp:51
> +    document()->frame()->loader()->client()->registerIntentService(

given a Document, the frame() method can return null.  however, i'm not sure if it can really be null in this case.  i see other insertedIntoDocument() implementations checking frame() though.

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1630
> +    WebCore::KURL serviceURL(baseURL, href); // need TextEncoding?

I think you should be using {Web}Document::completeURL() for this instead.

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1634
> +        m_webFrame->client()->registerIntentService(webFrame(), service);

nit: use m_webFrame instead of webFrame()

> Source/WebKit/chromium/src/FrameLoaderClientImpl.h:211
> +    virtual void registerIntentService(

when you are inside the implementation of WebKit, you don't need the WTF:: prefix on WTF types.

> Source/WebKit/chromium/src/WebIntentServiceInfo.cpp:36
> +WebIntentServiceInfo::WebIntentServiceInfo(const WebString& action,

This class is also defined in the patch from https://bugs.webkit.org/show_bug.cgi?id=73036.

[Alexey Proskuryakov]

Is it intentional that you are adding new files under LGPL?

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
> Is it intentional that you are adding new files under LGPL?

No, that's wrong.  Thanks for catching that Alexey!

[Greg Billock]

(In reply to comment #4)
> Is it intentional that you are adding new files under LGPL?

I picked an adjacent file with a Google copyright notice on it in WebCore/html. I now see there are at least two copyright notices. If I got the wrong one, which one should I use?

(I'm not sure which one I used. FileInputType maybe?)

[Greg Billock]

Comment on attachment 116390 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116390&action=review

>> Source/WebCore/html/HTMLIntentElement.cpp:51
>> +    document()->frame()->loader()->client()->registerIntentService(
> 
> given a Document, the frame() method can return null.  however, i'm not sure if it can really be null in this case.  i see other insertedIntoDocument() implementations checking frame() though.

Added checks here. It looks like Frame always has a FrameLoader. I'm not sure about the others, though...

>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1630
>> +    WebCore::KURL serviceURL(baseURL, href); // need TextEncoding?
> 
> I think you should be using {Web}Document::completeURL() for this instead.

Done. Thanks.

>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1634
>> +        m_webFrame->client()->registerIntentService(webFrame(), service);
> 
> nit: use m_webFrame instead of webFrame()

Done.

>> Source/WebKit/chromium/src/FrameLoaderClientImpl.h:211
>> +    virtual void registerIntentService(
> 
> when you are inside the implementation of WebKit, you don't need the WTF:: prefix on WTF types.

Done. (Usage in this file predominates to WTF::String though.)

>> Source/WebKit/chromium/src/WebIntentServiceInfo.cpp:36
>> +WebIntentServiceInfo::WebIntentServiceInfo(const WebString& action,
> 
> This class is also defined in the patch from https://bugs.webkit.org/show_bug.cgi?id=73036.

Yes. I plan on checking in that one first and merging here. I included it here for completeness (so the patch builds).

[Greg Billock]

Created attachment 116460 [details]
Patch

[Greg Billock]

Created attachment 116784 [details]
Patch

[Greg Billock]

Licenses fixed and patch applied. In a related change (https://bugs.webkit.org/show_bug.cgi?id=73051) I'm making a "Modules/intents" directory. Should tag files go in there as well? Or do those need to stay in WebCore/html?

[Adam Barth]

> Should tag files go in there as well? Or do those need to stay in WebCore/html?

The "Modules" concept is fairly new, so we're learning together.  IMHO, HTML tags should go in WebCore/html.

[Adam Barth]

Is the intents tag part of the HTML spec?

[Greg Billock]

[tag files in /html] sounds good.

[spec] The intent tag is proposed here: http://www.chromium.org/developers/design-documents/webintentsapi. James Hawkins (jhawkins) is working on getting that into a W3C standards draft format. Our Task Force is up and running ([e-mail]) but there's no consensus as yet on whether the intent tag will make it through the standardization process. So all this work is flagged off, as it is still experimental.

Speaking of which, I think I have flagged off the intent tag correctly. Is there a similar incantation for the 'disposition' attribute name I added? Or is that not needed, since this file is basically just an input to the codegen constants, and if it isn't in any IDL, it won't parse anyway?

[Greg Billock]

See also http://webintents.org/faq.html#tag for discussion of this tag in the proposal. We've gotten a lot of good feedback about using a specific HTML tag as a discovery/registration marker.

[Adam Barth]

It's not clear to me whether creating a new HTML tag is the right thing to do.  Generally, that sort of thing is pretty expensive.  Is there an archive of a mailing list discussion (e.g., in the W3C or the WhatWG) I can read to get a sense of the pros and cons?

[Greg Billock]

(In reply to comment #15)
> It's not clear to me whether creating a new HTML tag is the right thing to do.  Generally, that sort of thing is pretty expensive.  Is there an archive of a mailing list discussion (e.g., in the W3C or the WhatWG) I can read to get a sense of the pros and cons?

I hear you. While we like the accessibility and discoverability of the tag, it isn't clear to everyone it is worth it. A bigger concern than the impact on the HTML spec, so far, has been over what happens if the tag contents change, or even worse, if it disappears. We've had discussions about this reported on https://groups.google.com/group/web-intents.

The W3C task force group (public-web-intents; http://lists.w3.org/Archives/Public/public-web-intents/) just started up, but we will be discussing these issues in more depth there. Our prototype code also has ways to register intents in the manifest of a web app. So this tag isn't the only discovery/registration mechanism proposed.

Since this is a prototype implementation, all the code for this tag is flagged off. People creating custom browser builds or enabling the tag with browser flags is the most we except to do until these issues get resolved. If we decide to not pursue the tag, all this tag code will be backed out again. The only place this isn't the case, as I've noted, is with the "disposition" attribute. I could add a way for conditional emission of attributes. I don't think there is one now the way there is with tag names. Do you think that is desirable? Or should I change it to x-webkit-intent?

[Adam Barth]

Another concern is how can an SVG document use intents?  If intents were just a DOM API, then an SVG document could just call the API.  Because you've coupled intents to HTML, the SVG document needs to go through some gymnastics to cook up an HTML element.  (The proper forum for this discussion is a standards body, not a bug thread.)

I'm not super excited about this patch, but I'm not going to stand in the way if another reviewer thinks we should accept it.

[Greg Billock]

(In reply to comment #17)
> Another concern is how can an SVG document use intents?  If intents were just a DOM API, then an SVG document could just call the API.  Because you've coupled intents to HTML, the SVG document needs to go through some gymnastics to cook up an HTML element.  (The proper forum for this discussion is a standards body, not a bug thread.)

Do you mean invoking an intent? If so, see this patch: https://bugs.webkit.org/show_bug.cgi?id=73051

This bug is about the intent tag, which in the proposal is purely for registration and discovery, not for invocation or delivery. (Those are handled by a DOM API call on navigator for invocation, and by a 'window.intent' object placed on DOMWindow for delivery.)

I'm not sure how an SVG document would call a DOM API. Can SVG scripts get access to the navigator object? I'm not familiar with all the constraints there. Certainly an HTML wrapper with inline SVG could use the intent tag as well as navigator.startActivity, but I don't know how HTML scripts interact with inline SVG scripts.


And definitely join the discussion on public-web-intents! You're right; that's where final decisions will get made on the feature.

[Adam Barth]

> Do you mean invoking an intent?

No, I mean register an intent.

> I'm not sure how an SVG document would call a DOM API. Can SVG scripts get access to the navigator object?

Yes.

> I'm not familiar with all the constraints there. Certainly an HTML wrapper with inline SVG could use the intent tag as well as navigator.startActivity, but I don't know how HTML scripts interact with inline SVG scripts.

An SVG document is a full-fledged part of the web platform.  It can use all the same DOM APIs an HTML document can use.

> And definitely join the discussion on public-web-intents! You're right; that's where final decisions will get made on the feature.

Unfortunately, I don't have time to join ever standards mailing list that interests me.  I'm happy to abide by the decisions of the working group, but it doesn't seem like the working group has made a decision in this regard.  If that's an incorrect understanding, please point me toward the record of such a decision.

It concerns me that the folks designing this API don't understand the relationship between HTML and SVG documents.  That makes me worry that they'll make suboptimal design decisions because they don't understand how the web platform works.

As I wrote above, I'm not super excited about this patch, but I'm not going to stand in the way if another reviewer thinks we should accept it.

[Greg Billock]

(In reply to comment #19)
> > Do you mean invoking an intent?
> 
> No, I mean register an intent.
> 
> > I'm not sure how an SVG document would call a DOM API. Can SVG scripts get access to the navigator object?
> 
> Yes.
> 
> > I'm not familiar with all the constraints there. Certainly an HTML wrapper with inline SVG could use the intent tag as well as navigator.startActivity, but I don't know how HTML scripts interact with inline SVG scripts.
> 
> An SVG document is a full-fledged part of the web platform.  It can use all the same DOM APIs an HTML document can use.
> 
> > And definitely join the discussion on public-web-intents! You're right; that's where final decisions will get made on the feature.
> 
> Unfortunately, I don't have time to join ever standards mailing list that interests me.  I'm happy to abide by the decisions of the working group, but it doesn't seem like the working group has made a decision in this regard.  If that's an incorrect understanding, please point me toward the record of such a decision.

You're right, we're still too early in the process to have produced decisions. :-) This patch is more experimental in nature, and is intended to give developers the ability to enable the functionality with build or command line flags.
 
> It concerns me that the folks designing this API don't understand the relationship between HTML and SVG documents.  That makes me worry that they'll make suboptimal design decisions because they don't understand how the web platform works.
> 
> As I wrote above, I'm not super excited about this patch, but I'm not going to stand in the way if another reviewer thinks we should accept it.

I'm not sure I understand the objection yet. I mean, surely just because a browser can display an image/jpeg or a text/plain file doesn't mean special consideration has to be given to how such files interact with various web platform features, right? Or is there a role for SVG files that I don't understand?

Put more formally, the functionality in this patch (the <intent> tag), is a proposal for how HTML-based resources can annotate themselves for discovery by the browser (or other third-party mechanisms) as offering services accessible through the Web Intents DOM API. As I mentioned, it is not even the only such mechanism in the overall proposal. I'm happy to admit that accounting for discoverability of SVG files as services accessible through the Web Intents DOM API is a non-goal of this functionality. As such, this patch proposes a change to HTMLTagNames.in, not svgnames.in. (That is, it appears to me that the WebKit implementation doesn't treat these as part of the same namespace.) Is there something I'm missing here that needs fixing to be semantically correct? (It's a very real possibility! :-))

[Adam Barth]

Comment on attachment 116784 [details]
Patch

I'm going to mark this patch as R- because I don't think we should be adding HTML tags (even as experiments) without at least consulting with the HTML working group.  Other reviewers should feel free to override this decision if they think otherwise.

To answer your question more specifically, SVG has a very different relationship with the web platform than PNG.  Specifically, SVG participates in the DOM whereas PNG does not.

[Darin Fisher (:fishd, Google)]

I've been pretty supportive of having a declarative way of registering intents.  It seems good for enabling search engines to find apps that handle various intents!

However, I can imagine it making sense to also have an imperative method.  I haven't considered SVG documents wanting to register an intents handler before.  Hmm...

[Adam Barth]

> I've been pretty supportive of having a declarative way of registering intents.  It seems good for enabling search engines to find apps that handle various intents!

Have you considered using a meta or a rel extension point for HTML?  Those are much lighter weight than a new HTML element.

In a similar vein, http://webintents.org/ makes it look like you mean for <intent> to be a self-closing tag, which has implications for the HTML parsing algorithm and for interoperability with user agents that don't implement intents.

In any case, if we want to go the HTML element route, we should communicate with the HTML standards community.  I don't think it's even possible to define HTML elements outside of the HTML spec.  By contrast, defining a meta or a rel extension doesn't require interacting with the HTML working group.

[Greg Billock]

(In reply to comment #23)
> > I've been pretty supportive of having a declarative way of registering intents.  It seems good for enabling search engines to find apps that handle various intents!
> 
> Have you considered using a meta or a rel extension point for HTML?  Those are much lighter weight than a new HTML element.

Yes. See for example http://lists.w3.org/Archives/Public/public-web-intents/2011Nov/0017.html and the surrounding thread context.

 
> In a similar vein, http://webintents.org/ makes it look like you mean for <intent> to be a self-closing tag, which has implications for the HTML parsing algorithm and for interoperability with user agents that don't implement intents.

Is there a way for the IDL to require or forbid self-closing? I may not be using all the right options in this patch. 
 
> In any case, if we want to go the HTML element route, we should communicate with the HTML standards community.  I don't think it's even possible to define HTML elements outside of the HTML spec.  By contrast, defining a meta or a rel extension doesn't require interacting with the HTML working group.

Absolutely. We know this needs to be done, and James Hawkins is planning on initiating that discussion. (We think it is worth talking to them even if we end up using <link rel> and <meta>, as well, although it'd definitely be a different kind of discussion.)

[back on embedded svg] Yes, svg can be embedded in html, but that case should be handled fine by this patch as is -- the browser's resource is then an HTML document, which can use the intent tag as it exists in the patch:

<html><head><intent ...></intent></head><body><svg ...> ...

Whether <svg><script> would then work is something we haven't tried to support at this point, but this patch isn't intended to address that question at all. Is there something about embedded svg tags in html that needs to be fixed in this patch for the intent tag to work correctly as above?

[Adam Barth]

> Is there a way for the IDL to require or forbid self-closing? I may not be using all the right options in this patch. 

Nope.  You need to modify the parsing algorithm.  The parsing algorithm is defined in the HTML specification.  Generally, we don't accept patches that cause us to diverge from the specification.  Instead, we prefer for the spec to change first and then we implement the specced algorithm.

> Yes, svg can be embedded in html,

I'm talking about SVG documents, not SVG-in-HTML.  SVG documents are first class citizens in the web platform.

[Greg Billock]

(In reply to comment #25)
> > Is there a way for the IDL to require or forbid self-closing? I may not be using all the right options in this patch. 
> 
> Nope.  You need to modify the parsing algorithm.  The parsing algorithm is defined in the HTML specification.  Generally, we don't accept patches that cause us to diverge from the specification.  Instead, we prefer for the spec to change first and then we implement the specced algorithm.

Makes sense. Just to be clear, given that there's no change to the parser here, the tag isn't self-closing? Or will the parser still admit a self-closing tag? I looked at the HTMLParser a bit to try to get some insight. It looks to me like it just annotates self-closing tags, but I didn't see a place where our parser is restrictive on which tags are allowed to self-close. Is this a characteristic of other parsers, then, that we need to be careful for in the spec?
 
> > Yes, svg can be embedded in html,
> 
> I'm talking about SVG documents, not SVG-in-HTML.  SVG documents are first class citizens in the web platform.

I think I see what you mean finally. :-) I was worried I'd inadvertently impacted SVG syntax since it can be embedded. As Darin said, we haven't considered the use case for SVG resources to register intents directly. My instinct would be to do that in a different patch, though, which would introduce a corresponding change to svgnames. Should I instead include it here? (I'm not suggesting aborting the discussion with the HTML WG, just asking about mechanics, although having a "complete" patch to show the WG might be an aid to understanding. :-))

[Adam Barth]

> Makes sense. Just to be clear, given that there's no change to the parser here, the tag isn't self-closing?

Correct.

> Or will the parser still admit a self-closing tag?

Nope.  The /> syntax is for XML, not HTML.

> Is this a characteristic of other parsers, then, that we need to be careful for in the spec?

I'm not sure what question you're asking.  To change the parsing behavior, you need to change the HTML specification.

HTML does not have distributed extensibility for tags.  The extensibility is centralized in the HTML working group.

> I think I see what you mean finally. :-) I was worried I'd inadvertently impacted SVG syntax since it can be embedded. As Darin said, we haven't considered the use case for SVG resources to register intents directly. My instinct would be to do that in a different patch, though, which would introduce a corresponding change to svgnames. Should I instead include it here?

Are you suggesting adding an SVG element for intents also?  That would be something you should discuss with the SVG working group.  This approach doesn't seem very scalable.  For example, what about MathML?

> (I'm not suggesting aborting the discussion with the HTML WG, just asking about mechanics, although having a "complete" patch to show the WG might be an aid to understanding. :-))

I'm suspect the HTML working group is capable of understanding the pros and cons of adding a new element without a complete patch.

[Adam Barth]

At a higher level, it's not possible to add a new HTML element in a satellite specification.  Doing so requires changing the HTML specification, which means you need to engage with the community around that specification.  You can take your pick of the WhatWG or the W3C HTML working group.  Both are actively engaged in charting the future course of HTML.

More specifically:

1) Adding a new HTML element is not a slam dunk.  Everyone thinks their feature ought to have a new element.  Sometimes that's a good idea and sometimes it's not.

2) The correct forum for coming to consensus on that topic is the HTML working group, not a bug thread.  Before landing code in WebKit trunk that adds a new HTML element (even behind an ifdef), you should discuss the topic with the HTML working group.  You don't necessarily need to convince the working group out of the gate, but you should at least engage with them.

3) Before modifying the HTML parser, we require the corresponding change to already have been made in the HTML specification.  This has been the standard operating procedure for that code since we re-wrote it to match the spec.  Yes, this can be annoying, but it's important to ensure we don't diverge from the spec.

[Adam Barth]

Comment on attachment 116784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116784&action=review

> Source/WebCore/html/HTMLAttributeNames.in:95
> +disposition

This is fine to have unconditionally.  It's only visible internally to WebCore, not to the platform.

> Source/WebCore/html/HTMLIntentElement.cpp:43
> +inline HTMLIntentElement::HTMLIntentElement(const QualifiedName& tagName, Document* document)

Why "inline"?  It's not declared inline in the header.

> Source/WebCore/html/HTMLIntentElement.cpp:54
> +void HTMLIntentElement::insertedIntoDocument()

Do we need a notification when this element is removed from the document?  I guess there's no way to unregister an intent once you've registered it?  Do we need some kind of max-age on intents to avoid broken intent links?

> Source/WebCore/html/HTMLIntentElement.cpp:59
> +    if (!document()->frame() || !document()->frame()->loader()->client())
> +        return;

There's no need to null check the loader()->client().  It's never null.

> Source/WebCore/html/HTMLIntentElement.cpp:62
> +    document()->frame()->loader()->client()->registerIntentService(
> +        getAttribute(actionAttr), getAttribute(typeAttr), getAttribute(hrefAttr), getAttribute(titleAttr), getAttribute(dispositionAttr));

I think we can use fastGetAttribute for these.

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1622
> +        const WTF::String& action,

Should we add a "using WTF::String" to this file?

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1627
> +    WebURL url = m_webFrame->document().completeURL(href);

It seems like WebCore should call completeURL and "href" should be a KURL.

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1628
> +    if (m_webFrame->client()) {

Prefer early return.

> Source/WebKit/chromium/src/WebIntentServiceInfo.cpp:47
> +      m_disposition(disposition) { }

{ } should be on separate lines.  Also, the "," should be aligned below the ":".

[Greg Billock]

Comment on attachment 116784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116784&action=review

>> Source/WebCore/html/HTMLAttributeNames.in:95
>> +disposition
> 
> This is fine to have unconditionally.  It's only visible internally to WebCore, not to the platform.

OK, sounds good.

>> Source/WebCore/html/HTMLIntentElement.cpp:43
>> +inline HTMLIntentElement::HTMLIntentElement(const QualifiedName& tagName, Document* document)
> 
> Why "inline"?  It's not declared inline in the header.

Got rid of this.

>> Source/WebCore/html/HTMLIntentElement.cpp:54
>> +void HTMLIntentElement::insertedIntoDocument()
> 
> Do we need a notification when this element is removed from the document?  I guess there's no way to unregister an intent once you've registered it?  Do we need some kind of max-age on intents to avoid broken intent links?

I don't think so. This is part of the larger problem around the fact that it's up to the UA to detect when a page used to have an intent tag and no longer does. A page can stop handling intents at any time, but the UA may not become aware of it until it loads the page to respond to an intent invocation.

>> Source/WebCore/html/HTMLIntentElement.cpp:59
>> +        return;
> 
> There's no need to null check the loader()->client().  It's never null.

OK, thanks. Done.

>> Source/WebCore/html/HTMLIntentElement.cpp:62
>> +        getAttribute(actionAttr), getAttribute(typeAttr), getAttribute(hrefAttr), getAttribute(titleAttr), getAttribute(dispositionAttr));
> 
> I think we can use fastGetAttribute for these.

Done.

>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1622
>> +        const WTF::String& action,
> 
> Should we add a "using WTF::String" to this file?

There's an implicit using (other code is just using "String", so I changed this to do likewise.

>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1627
>> +    WebURL url = m_webFrame->document().completeURL(href);
> 
> It seems like WebCore should call completeURL and "href" should be a KURL.

Done.

>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1628
>> +    if (m_webFrame->client()) {
> 
> Prefer early return.

Done.

>> Source/WebKit/chromium/src/WebIntentServiceInfo.cpp:47
>> +      m_disposition(disposition) { }
> 
> { } should be on separate lines.  Also, the "," should be aligned below the ":".

Done.

[Greg Billock]

Created attachment 118098 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 118098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118098&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This will cause the commit-queue to fail.  All patches require testing if possible.  You can replace this with an explanation of why testing is impractical/impossible for this change.

[Greg Billock]

Created attachment 123854 [details]
Patch

[Adam Barth]

Your ChangeLog diff looks corrupted.  It's causing trouble for the code review tool...

[Greg Billock]

Created attachment 123859 [details]
Patch

[Greg Billock]

(In reply to comment #34)
> Your ChangeLog diff looks corrupted.  It's causing trouble for the code review tool...

Fixed. Sorry about that, just trying to do a quick merge to head to merge in the other changes I've made.

[Adam Barth]

Comment on attachment 123859 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123859&action=review

So, the main question here is whether we should have an <intent> tag or whether intents should us the <meta> tag.  It's probably a good idea to add a discussion of that issue in the ChangeLog, including links to the spec and the relevant discussion on standards mailing lists.

I'm sorry to mark your patch r- for a ChangeLog issue, but in this case I think it's important to explain the "why" behind the patch in the ChangeLog.  (Aside from the <intent> issue, the rest of this patch looks great.)

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1640
> +void FrameLoaderClientImpl::registerIntentService(
> +        const String& action,
> +        const String& type,
> +        const KURL& href,
> +        const String& title,
> +        const String& disposition) {

This should all be on one line.

[Greg Billock]

Yes. I didn't really mean to re-open the review, as the discussion about method is still ongoing, just doing some housekeeping since this patch needed a merge to head given other check-ins.

(In reply to comment #37)
> (From update of attachment 123859 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123859&action=review
> 
> So, the main question here is whether we should have an <intent> tag or whether intents should us the <meta> tag.  It's probably a good idea to add a discussion of that issue in the ChangeLog, including links to the spec and the relevant discussion on standards mailing lists.
> 
> I'm sorry to mark your patch r- for a ChangeLog issue, but in this case I think it's important to explain the "why" behind the patch in the ChangeLog.  (Aside from the <intent> issue, the rest of this patch looks great.)
> 
> > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1640
> > +void FrameLoaderClientImpl::registerIntentService(
> > +        const String& action,
> > +        const String& type,
> > +        const KURL& href,
> > +        const String& title,
> > +        const String& disposition) {
> 
> This should all be on one line.

[Greg Billock]

Created attachment 140065 [details]
Patch

[Adam Barth]

Comment on attachment 140065 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140065&action=review

Thanks for taking the time to talk this over with the standards community.  I also appreciate your using a separate ENABLE so that folks can enable both halves separately.

> Source/WebCore/ChangeLog:7
> +

Please add some more information to the ChangeLog.  For example, in email you told me about the state of standardization for <intent>.  That's good information to include in the ChangeLog so folks following along at home can know that we've done our due diligence.

> Source/WebCore/page/DOMWindow.idl:491
> +#if defined(ENABLE_WEB_INTENTS_TAG) && ENABLE_WEB_INTENTS_TAG
> +        attribute HTMLIntentElementConstructor HTMLIntentElement;
> +#endif

You can just use the [Conditional] attribute.  See two lines above how that's done for HTMLSourceElement.

[Greg Billock]

Created attachment 140267 [details]
Patch

[Greg Billock]

Comment on attachment 140065 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140065&action=review

>> Source/WebCore/ChangeLog:7
>> +
> 
> Please add some more information to the ChangeLog.  For example, in email you told me about the state of standardization for <intent>.  That's good information to include in the ChangeLog so folks following along at home can know that we've done our due diligence.

:-) I was just adding this to that most recent patch with the test. Should be in the most recent patch.

>> Source/WebCore/page/DOMWindow.idl:491
>> +#endif
> 
> You can just use the [Conditional] attribute.  See two lines above how that's done for HTMLSourceElement.

Done.

[Greg Billock]

Created attachment 140285 [details]
Patch

[Adam Barth]

Comment on attachment 140285 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140285&action=review

> LayoutTests/webintents/intent-tag.html:6
> +    <intent action="action" type="type" title="Title" href="http://example.com/service" disposition="inline"></intent>

In a follow up patch, consider testing that you can register an intent using DOM APIs (e.g., document.createElement("intent"); document.body.appendChild(...) )

[WebKit Review Bot]

Comment on attachment 140285 [details]
Patch

Attachment 140285 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12628270

New failing tests:
webintents/intent-tag.html

[WebKit Review Bot]

Created attachment 140302 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 140285 [details]
Patch

Attachment 140285 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12631265

New failing tests:
webintents/intent-tag.html

[WebKit Review Bot]

Created attachment 140316 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Greg Billock]

Created attachment 140551 [details]
Patch

[WebKit Review Bot]

Comment on attachment 140551 [details]
Patch

Clearing flags on attachment: 140551

Committed r116384: <http://trac.webkit.org/changeset/116384>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.