Bug 76014 – Web Intents chromium API modifications to track IntentRequest invocation method

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

[Greg Billock]

Web Intents chromium API modifications to track IntentRequest invocation method

[Greg Billock]

Created attachment 121951 [details]
Patch

[WebKit Review Bot]

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

[Adam Barth]

Comment on attachment 121951 [details]
Patch

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

> Source/WebKit/chromium/public/WebIntent.h:66
> +        void postResult(const WebString&);
> +        void postFailure(const WebString&);

Virtual?

[Greg Billock]

Created attachment 122056 [details]
Patch

[Greg Billock]

Comment on attachment 121951 [details]
Patch

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

>> Source/WebKit/chromium/public/WebIntent.h:66
>> +        void postFailure(const WebString&);
> 
> Virtual?

Yes. Botched this just before leaving last night.

[Adam Barth]

Comment on attachment 122056 [details]
Patch

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

I wonder if it would be better to use two separate objects in the interface, a WebIntent object and a WebIntentRequest object.  Each of them could wrap a RefPtr to the WebCore object, like we do for WebCore::Node.  That seems less error prone w.r.t. memory management.

(I'm also happy to defer to fishd on these matters.)

> Source/WebKit/chromium/public/WebIntent.h:65
> +        ~WebIntentReplyBroker() { }

Should the destructor be virtual too?

> Source/WebKit/chromium/public/WebIntent.h:76
> +    explicit WebIntent(const WTF::PassRefPtr<WebCore::IntentRequest>&);

Should we rename this interface to WebIntentRequest to match the WebCore object?  Maybe we should have a WebIntentRequest that has a WebIntent as a member to mirror the structure of the WebCore classes.

> Source/WebKit/chromium/src/WebIntent.cpp:88
> +    delete m_broker;

Do we not have an OwnPtr variant for the API?

> Source/WebKit/chromium/src/WebIntent.cpp:133
> -    m_identifier = identifier;
> +    WebIntentReplyBroker* broker = m_broker;
> +    m_broker = 0;
> +    return broker;

I see.  This transfers ownership to the embedder.  I don't remember how we typically manage memory in the API.  I'll let fishd comment on that aspect.  From this design, it seems clear that WebIntentReplyBroker needs a virtual destructor.

[Greg Billock]

Comment on attachment 122056 [details]
Patch

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

>> Source/WebKit/chromium/public/WebIntent.h:65
>> +        ~WebIntentReplyBroker() { }
> 
> Should the destructor be virtual too?

yes

>> Source/WebKit/chromium/public/WebIntent.h:76
>> +    explicit WebIntent(const WTF::PassRefPtr<WebCore::IntentRequest>&);
> 
> Should we rename this interface to WebIntentRequest to match the WebCore object?  Maybe we should have a WebIntentRequest that has a WebIntent as a member to mirror the structure of the WebCore classes.

Yeah, I re-used the WebIntent object because I'd already created it while writing the chromium-side code. I agree it isn't the best name. Shall I make a new one now, or put it off to later?

>> Source/WebKit/chromium/src/WebIntent.cpp:88
>> +    delete m_broker;
> 
> Do we not have an OwnPtr variant for the API?

I looked at WebPrivatePtr and WebPrivateOwnPtr before doing this manually, but those are not really appropriate. WebPrivateOwnPtr is close, but it has to be manually cleared before exit anyway, and the semantics aren't right. "WebOwnPtr" is probably what we need. Is it worth it to make such a thing? I looked around, but didn't find a pattern elsewhere where we transfer object ownership up like this. There's a lot of classes, though. Is there one I should copy?

>> Source/WebKit/chromium/src/WebIntent.cpp:133
>> +    return broker;
> 
> I see.  This transfers ownership to the embedder.  I don't remember how we typically manage memory in the API.  I'll let fishd comment on that aspect.  From this design, it seems clear that WebIntentReplyBroker needs a virtual destructor.

Yes.

[Darin Fisher (:fishd, Google)]

Comment on attachment 122056 [details]
Patch

As discussed over IM, please just use WebPrivatePtr to wrap WebCore::IntentRequest in a simple fashion.  See WebNode for example.

[Greg Billock]

Created attachment 122099 [details]
Patch

[Greg Billock]

(In reply to comment #8)
> (From update of attachment 122056 [details])
> As discussed over IM, please just use WebPrivatePtr to wrap WebCore::IntentRequest in a simple fashion.  See WebNode for example.

Yeah, this comes out nicely. I'm just transferring ownership of the WebIntentRequest directly, not using any pass-through object wrapper as before. Does that look OK? Should I add code to delete this param in the empty impl of WebFrameClient?

[Adam Barth]

Comment on attachment 122099 [details]
Patch

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

> Source/WebKit/chromium/public/WebIntentRequest.h:57
> +    WEBKIT_EXPORT WebString action() const;
> +
> +    WEBKIT_EXPORT WebString type() const;
> +
> +    WEBKIT_EXPORT WebString data() const;

Do you want to expose these accessors on WebIntentRequest, or do you just want to expose an access for the WebIntent itself?

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1636
> +    m_webFrame->client()->dispatchIntent(webFrame(), new WebIntentRequest(intentRequest));

Do you want to use "new" here, or have it stack allocated?

> Source/WebKit/chromium/src/WebIntentRequest.cpp:59
> +    if (m_intentRequest.get() && m_intentRequest->intent())

is m_intentRequest->intent() ever null?  It seems meaningless to have an intent request without an intent.

[Greg Billock]

Comment on attachment 122099 [details]
Patch

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

>> Source/WebKit/chromium/public/WebIntentRequest.h:57
>> +    WEBKIT_EXPORT WebString data() const;
> 
> Do you want to expose these accessors on WebIntentRequest, or do you just want to expose an access for the WebIntent itself?

I think it is easier to just use these. See http://codereview.chromium.org/9186021/ for the chromium-side code that uses this. This way I can remove the WebIntent object in a follow-on.

>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1636
>> +    m_webFrame->client()->dispatchIntent(webFrame(), new WebIntentRequest(intentRequest));
> 
> Do you want to use "new" here, or have it stack allocated?

I'm passing ownership to the client. I think if I stack-allocate it I'd have to make a copy in the client anyway. Is that preferable? It makes the stub WebFrameClient default code simpler, but to me it obscures that the callee is expected to take ownership of the object. I see WebNode ownership transfer happening in, i.e. WebDOMEvent, but it is using a mechanism similar to the one I had before, where there's an intermediary object you grab the reference from (but that's just because of the larger interface there, it seems).

>> Source/WebKit/chromium/src/WebIntentRequest.cpp:59
>> +    if (m_intentRequest.get() && m_intentRequest->intent())
> 
> is m_intentRequest->intent() ever null?  It seems meaningless to have an intent request without an intent.

We discussed before that we may want to clear it out, since it might be heavy. That kind of makes this API state-dependent, which is awkward, but it may be preferable to carrying a large Intent object payload for a long time.

[Darin Fisher (:fishd, Google)]

Comment on attachment 122099 [details]
Patch

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

> Source/WebKit/chromium/public/WebIntentRequest.h:51
> +    ~WebIntentRequest();

remember, if you do not implement a method inline or behind a WEBKIT_IMPLEMENTATION guard, then it needs to be decorated with WEBKIT_EXPORT.  else the DLL build will fail to link.

>>> Source/WebKit/chromium/src/WebIntentRequest.cpp:59
>>> +    if (m_intentRequest.get() && m_intentRequest->intent())
>> 
>> is m_intentRequest->intent() ever null?  It seems meaningless to have an intent request without an intent.
> 
> We discussed before that we may want to clear it out, since it might be heavy. That kind of makes this API state-dependent, which is awkward, but it may be preferable to carrying a large Intent object payload for a long time.

Sorry to create more work for you, but we really try to make the WebKit API just be a thin wrapper for WebCore.  You might as well expose WebIntent since it can easily wrap WebCore::Intent as that is also ref-counted.  This way as these objects grow, you don't have to have the WebKit API concepts differ from the WebCore concepts.  Past experience suggests that keeping these aligned helps.

> Source/WebKit/chromium/src/WebIntentRequest.cpp:86
> +void WebIntentRequest::postResult(const WebKit::WebString& data)

nit: postResult and postFailure should take WebSerializedScriptValue.  this will help when you want to post more than just a string, which i'm sure we will want to do.  it also will reduce the code for the WebKit layer, which is always a good thing.

> Source/WebKit/chromium/src/WebIntentRequest.cpp:99
> +    if (m_intentRequest.get())

it does not look possible to create a WebIntentRequest that has a null m_intentRequest.  why are you null checking?  just ASSERT or let it crash.

[Greg Billock]

Comment on attachment 122099 [details]
Patch

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

>> Source/WebKit/chromium/public/WebIntentRequest.h:51
>> +    ~WebIntentRequest();
> 
> remember, if you do not implement a method inline or behind a WEBKIT_IMPLEMENTATION guard, then it needs to be decorated with WEBKIT_EXPORT.  else the DLL build will fail to link.

Moved the empty impl here inline

>>> Source/WebKit/chromium/public/WebIntentRequest.h:57
>>> +    WEBKIT_EXPORT WebString data() const;
>> 
>> Do you want to expose these accessors on WebIntentRequest, or do you just want to expose an access for the WebIntent itself?
> 
> I think it is easier to just use these. See http://codereview.chromium.org/9186021/ for the chromium-side code that uses this. This way I can remove the WebIntent object in a follow-on.

Followed Darin's advice and just put in the WebIntent object.

>>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1636
>>> +    m_webFrame->client()->dispatchIntent(webFrame(), new WebIntentRequest(intentRequest));
>> 
>> Do you want to use "new" here, or have it stack allocated?
> 
> I'm passing ownership to the client. I think if I stack-allocate it I'd have to make a copy in the client anyway. Is that preferable? It makes the stub WebFrameClient default code simpler, but to me it obscures that the callee is expected to take ownership of the object. I see WebNode ownership transfer happening in, i.e. WebDOMEvent, but it is using a mechanism similar to the one I had before, where there's an intermediary object you grab the reference from (but that's just because of the larger interface there, it seems).

Any view on this, Darin? Pass pointer or use a copy constructor?

>>>> Source/WebKit/chromium/src/WebIntentRequest.cpp:59
>>>> +    if (m_intentRequest.get() && m_intentRequest->intent())
>>> 
>>> is m_intentRequest->intent() ever null?  It seems meaningless to have an intent request without an intent.
>> 
>> We discussed before that we may want to clear it out, since it might be heavy. That kind of makes this API state-dependent, which is awkward, but it may be preferable to carrying a large Intent object payload for a long time.
> 
> Sorry to create more work for you, but we really try to make the WebKit API just be a thin wrapper for WebCore.  You might as well expose WebIntent since it can easily wrap WebCore::Intent as that is also ref-counted.  This way as these objects grow, you don't have to have the WebKit API concepts differ from the WebCore concepts.  Past experience suggests that keeping these aligned helps.

OK, switched to that.

>> Source/WebKit/chromium/src/WebIntentRequest.cpp:86
>> +void WebIntentRequest::postResult(const WebKit::WebString& data)
> 
> nit: postResult and postFailure should take WebSerializedScriptValue.  this will help when you want to post more than just a string, which i'm sure we will want to do.  it also will reduce the code for the WebKit layer, which is always a good thing.

Done.

>> Source/WebKit/chromium/src/WebIntentRequest.cpp:99
>> +    if (m_intentRequest.get())
> 
> it does not look possible to create a WebIntentRequest that has a null m_intentRequest.  why are you null checking?  just ASSERT or let it crash.

Done.

[Greg Billock]

Created attachment 122309 [details]
Patch

[Adam Barth]

Comment on attachment 122309 [details]
Patch

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

> Source/WebKit/chromium/public/WebIntent.h:70
> +#if WEBKIT_IMPLEMENTATION

Presumably we'll need this storage in this class even when WEBKIT_IMPLEMENTATION isn't defined.

[Greg Billock]

Comment on attachment 122309 [details]
Patch

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

>> Source/WebKit/chromium/public/WebIntent.h:70
>> +#if WEBKIT_IMPLEMENTATION
> 
> Presumably we'll need this storage in this class even when WEBKIT_IMPLEMENTATION isn't defined.

I've changed the impl to just directly use the fields in the wrapped type. Is this going to be impossible? That is, I can't use ENABLE(WEB_INTENTS) outside the WEBKIT_IMPLEMENTATION ifdef, since Chromium code doesn't know about it. But does that mean the linker is confused about what the methods will be doing internally? My mental image is that the webkit compile will create the object file, and the defines are to not let client code know about these implementation details.

[Greg Billock]

Created attachment 122445 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 122445 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrameClient.h:388
> +    virtual void dispatchIntent(WebFrame*, WebIntentRequest*) { }

WebIntentRequest should be passed using |const Foo&|.  the chromium code should
retain a WebIntentRequest by using the copy-constructor.  the only oddity about
this is that it strips the const, but that's the pattern we've been following.
treat it like a RefPtr.

> Source/WebKit/chromium/public/WebIntent.h:57
> +    // FIXME(gbillock): delete this.

nit: WebKit style does not put names on FIXME comments like this.

> Source/WebKit/chromium/public/WebIntent.h:62
> +    WebIntent(const WebIntent&);

why don't you want to allow the embedder to copy a WebIntent?  i think
you will have trouble using WebIntentRequest::intent() if you don't
make the copy-constructor available.

There's a pattern for writing classes like this.  For an example that
is simpler than WebNode, see WebUserMediaRequest.  You should have
the following methods generally speaking on classes that just wrap
a ref-counted WebCore type:

  default constructor
  copy constructor
  destructor
  operator=
  reset
  isNull
  assign

you can optionally add equals and operator== if needed.

treat this as boilerplate.  maybe we should have macros to make this
easier to replicate, but we've so far favored the readability of not
using macros.

same deal for WebIntentRequest.h

[Greg Billock]

Comment on attachment 122445 [details]
Patch

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

>> Source/WebKit/chromium/public/WebFrameClient.h:388
>> +    virtual void dispatchIntent(WebFrame*, WebIntentRequest*) { }
> 
> WebIntentRequest should be passed using |const Foo&|.  the chromium code should
> retain a WebIntentRequest by using the copy-constructor.  the only oddity about
> this is that it strips the const, but that's the pattern we've been following.
> treat it like a RefPtr.

Done.

>> Source/WebKit/chromium/public/WebIntent.h:57
>> +    // FIXME(gbillock): delete this.
> 
> nit: WebKit style does not put names on FIXME comments like this.

Done

>> Source/WebKit/chromium/public/WebIntent.h:62
>> +    WebIntent(const WebIntent&);
> 
> why don't you want to allow the embedder to copy a WebIntent?  i think
> you will have trouble using WebIntentRequest::intent() if you don't
> make the copy-constructor available.
> 
> There's a pattern for writing classes like this.  For an example that
> is simpler than WebNode, see WebUserMediaRequest.  You should have
> the following methods generally speaking on classes that just wrap
> a ref-counted WebCore type:
> 
>   default constructor
>   copy constructor
>   destructor
>   operator=
>   reset
>   isNull
>   assign
> 
> you can optionally add equals and operator== if needed.
> 
> treat this as boilerplate.  maybe we should have macros to make this
> easier to replicate, but we've so far favored the readability of not
> using macros.
> 
> same deal for WebIntentRequest.h

OK. I was trying to only implement the ones I needed, but if there's a macro-like consistency I'll do that.

[Greg Billock]

Created attachment 122474 [details]
Patch

[Greg Billock]

Created attachment 122527 [details]
Patch

[Greg Billock]

Created attachment 122797 [details]
Patch

[Greg Billock]

Created attachment 122827 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 122827 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrameClient.h:385
> +    // the WebFrame starting the activity. The callee uses this object to

This comment is incorrect now, right?  "Replies to this request should be sent to __the request object__."

> Source/WebKit/chromium/public/WebFrameClient.h:387
> +    // soon as possible when it is no longer needed.)

This parenthetical comment is perhaps a bit confusing since there is no way
to release a WebIntentRequest.

> Source/WebKit/chromium/public/WebIntent.h:47
> +    WebIntent();

constructors should be implemented inline.

> Source/WebKit/chromium/public/WebIntent.h:48
> +    WebIntent(const WebIntent&);

this should just be an inline call to assign.

> Source/WebKit/chromium/public/WebIntentRequest.h:50
> +    WebIntentRequest();

implement constructors inline

> Source/WebKit/chromium/public/WebIntentRequest.h:70
> +#if ENABLE(WEB_INTENTS)

since you've unconditionally declared WebCore::IntentRequest, it seems like this
function does not need to be protected by ENABLE(WEB_INTENTS).  in generaly, it'd
be nice to avoid using ENABLE macros in public header files, even though this one
is protected by WEBKIT_IMPLEMENTATION.

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1637
> +    WebIntentRequest request(intentRequest);

nit: you should be able to delete this line and just pass intentRequest directly to dispatchIntent.  it should be implicitly converted to WebIntentRequest.

> Source/WebKit/chromium/src/WebIntent.cpp:66
> +#endif

nit: use #else here and elsewhere instead of relying on first return statement wins.

> Source/WebKit/chromium/src/WebIntentRequest.cpp:109
> +    RefPtr<WebCore::SerializedScriptValue> serializedValue = 

nit: we'd normally write this like so:

  m_private->postFailure(PassRefPtr<WebCore::SerializedScriptValue>(data).get());

[Greg Billock]

Comment on attachment 122827 [details]
Patch

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

>> Source/WebKit/chromium/public/WebFrameClient.h:385
>> +    // the WebFrame starting the activity. The callee uses this object to
> 
> This comment is incorrect now, right?  "Replies to this request should be sent to __the request object__."

Fixed up now. Took out the bad information and condensed.

>> Source/WebKit/chromium/public/WebFrameClient.h:387
>> +    // soon as possible when it is no longer needed.)
> 
> This parenthetical comment is perhaps a bit confusing since there is no way
> to release a WebIntentRequest.

Removed.

>> Source/WebKit/chromium/public/WebIntent.h:47
>> +    WebIntent();
> 
> constructors should be implemented inline.

Done. I moved these to the .cc file while I had the ifdefs around the m_private member to make sure they got created in the webkit code. That whole thing was a bad idea, though, now fixed up.

>> Source/WebKit/chromium/public/WebIntent.h:48
>> +    WebIntent(const WebIntent&);
> 
> this should just be an inline call to assign.

Done.

>> Source/WebKit/chromium/public/WebIntentRequest.h:50
>> +    WebIntentRequest();
> 
> implement constructors inline

Done (ditto).

>> Source/WebKit/chromium/public/WebIntentRequest.h:70
>> +#if ENABLE(WEB_INTENTS)
> 
> since you've unconditionally declared WebCore::IntentRequest, it seems like this
> function does not need to be protected by ENABLE(WEB_INTENTS).  in generaly, it'd
> be nice to avoid using ENABLE macros in public header files, even though this one
> is protected by WEBKIT_IMPLEMENTATION.

Agreed. Removed this.

>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1637
>> +    WebIntentRequest request(intentRequest);
> 
> nit: you should be able to delete this line and just pass intentRequest directly to dispatchIntent.  it should be implicitly converted to WebIntentRequest.

That constructor is marked explicit, so I need to make the temporary. It looks to me like those constructors are typically not marked explicit in the chromium API. Shall I change them?

>> Source/WebKit/chromium/src/WebIntent.cpp:66
>> +#endif
> 
> nit: use #else here and elsewhere instead of relying on first return statement wins.

Done.

>> Source/WebKit/chromium/src/WebIntentRequest.cpp:109
>> +    RefPtr<WebCore::SerializedScriptValue> serializedValue = 
> 
> nit: we'd normally write this like so:
> 
>   m_private->postFailure(PassRefPtr<WebCore::SerializedScriptValue>(data).get());

Done here and above.

[Greg Billock]

Created attachment 123038 [details]
Patch

[Darin Fisher (:fishd, Google)]

(In reply to comment #26)
> >> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1637
> >> +    WebIntentRequest request(intentRequest);
> > 
> > nit: you should be able to delete this line and just pass intentRequest directly to dispatchIntent.  it should be implicitly converted to WebIntentRequest.
> 
> That constructor is marked explicit, so I need to make the temporary. It looks to me like those constructors are typically not marked explicit in the chromium API. Shall I change them?

Ah, you should not mark that WebIntentRequest constructor explicit.  The point is to benefit from implicit conversion where you are just converting between like types.  A WebIntentRequest is really just a RefPtr<IntentRequest>, and implicit conversion between those should not be something a developer really needs to think about.  We've preferred less verbosity when crossing the API boundary in other words!

[Greg Billock]

(In reply to comment #28)
> (In reply to comment #26)
> > >> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1637
> > >> +    WebIntentRequest request(intentRequest);
> > > 
> > > nit: you should be able to delete this line and just pass intentRequest directly to dispatchIntent.  it should be implicitly converted to WebIntentRequest.
> > 
> > That constructor is marked explicit, so I need to make the temporary. It looks to me like those constructors are typically not marked explicit in the chromium API. Shall I change them?
> 
> Ah, you should not mark that WebIntentRequest constructor explicit.  The point is to benefit from implicit conversion where you are just converting between like types.  A WebIntentRequest is really just a RefPtr<IntentRequest>, and implicit conversion between those should not be something a developer really needs to think about.  We've preferred less verbosity when crossing the API boundary in other words!

Sounds good. Done.

[Greg Billock]

Created attachment 123127 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 123127 [details]
Patch

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

> Source/WebKit/chromium/public/WebIntent.h:58
> +    WEBKIT_EXPORT bool equals(const WebIntent&);

nit: I think equals should be a const method.

> Source/WebKit/chromium/public/WebIntentRequest.h:61
> +    WEBKIT_EXPORT bool equals(const WebIntentRequest&);

nit: equals should be const

> Source/WebKit/chromium/src/WebIntentRequest.cpp:93
> +//    RefPtr<WebCore::SerializedScriptValue> serializedValue = 

nit: delete commented out code

> Source/WebKit/chromium/src/WebIntentRequest.cpp:104
> +//        (PassRefPtr<WebCore::SerializedScriptValue>)data;

nit: delete commented out code

[Greg Billock]

Comment on attachment 123127 [details]
Patch

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

>> Source/WebKit/chromium/public/WebIntent.h:58
>> +    WEBKIT_EXPORT bool equals(const WebIntent&);
> 
> nit: I think equals should be a const method.

Done

>> Source/WebKit/chromium/public/WebIntentRequest.h:61
>> +    WEBKIT_EXPORT bool equals(const WebIntentRequest&);
> 
> nit: equals should be const

Done

>> Source/WebKit/chromium/src/WebIntentRequest.cpp:93
>> +//    RefPtr<WebCore::SerializedScriptValue> serializedValue = 
> 
> nit: delete commented out code

Done

>> Source/WebKit/chromium/src/WebIntentRequest.cpp:104
>> +//        (PassRefPtr<WebCore::SerializedScriptValue>)data;
> 
> nit: delete commented out code

Done

[Greg Billock]

Created attachment 123196 [details]
Patch

[WebKit Review Bot]

Comment on attachment 123196 [details]
Patch

Clearing flags on attachment: 123196

Committed r105469: <http://trac.webkit.org/changeset/105469>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.