Bug 76014

Summary: Web Intents chromium API modifications to track IntentRequest invocation method
Product: WebKit Reporter: Greg Billock <gbillock>
Component: New BugsAssignee: Greg Billock <gbillock>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, fishd, 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
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Greg Billock 2012-01-10 17:43:12 PST
Web Intents chromium API modifications to track IntentRequest invocation method
Comment 1 Greg Billock 2012-01-10 17:43:39 PST
Created attachment 121951 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-10 17:46:37 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Adam Barth 2012-01-10 17:54:37 PST
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?
Comment 4 Greg Billock 2012-01-11 11:26:11 PST
Created attachment 122056 [details]
Patch
Comment 5 Greg Billock 2012-01-11 11:50:43 PST
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.
Comment 6 Adam Barth 2012-01-11 13:26:59 PST
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.
Comment 7 Greg Billock 2012-01-11 13:37:19 PST
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.
Comment 8 Darin Fisher (:fishd, Google) 2012-01-11 13:42:59 PST
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.
Comment 9 Greg Billock 2012-01-11 14:49:30 PST
Created attachment 122099 [details]
Patch
Comment 10 Greg Billock 2012-01-11 14:51:04 PST
(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?
Comment 11 Adam Barth 2012-01-11 14:58:01 PST
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.
Comment 12 Greg Billock 2012-01-11 15:37:12 PST
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.
Comment 13 Darin Fisher (:fishd, Google) 2012-01-12 12:37:04 PST
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.
Comment 14 Greg Billock 2012-01-12 14:19:58 PST
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.
Comment 15 Greg Billock 2012-01-12 14:22:03 PST
Created attachment 122309 [details]
Patch
Comment 16 Adam Barth 2012-01-12 14:35:02 PST
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.
Comment 17 Greg Billock 2012-01-12 14:56:23 PST
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.
Comment 18 Greg Billock 2012-01-13 10:01:03 PST
Created attachment 122445 [details]
Patch
Comment 19 Darin Fisher (:fishd, Google) 2012-01-13 10:14:57 PST
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
Comment 20 Greg Billock 2012-01-13 11:32:14 PST
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.
Comment 21 Greg Billock 2012-01-13 11:33:06 PST
Created attachment 122474 [details]
Patch
Comment 22 Greg Billock 2012-01-13 18:52:41 PST
Created attachment 122527 [details]
Patch
Comment 23 Greg Billock 2012-01-17 12:17:07 PST
Created attachment 122797 [details]
Patch
Comment 24 Greg Billock 2012-01-17 15:40:44 PST
Created attachment 122827 [details]
Patch
Comment 25 Darin Fisher (:fishd, Google) 2012-01-18 15:02:04 PST
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());
Comment 26 Greg Billock 2012-01-18 16:49:28 PST
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.
Comment 27 Greg Billock 2012-01-18 16:50:13 PST
Created attachment 123038 [details]
Patch
Comment 28 Darin Fisher (:fishd, Google) 2012-01-18 21:33:03 PST
(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!
Comment 29 Greg Billock 2012-01-19 07:51:02 PST
(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.
Comment 30 Greg Billock 2012-01-19 07:54:02 PST
Created attachment 123127 [details]
Patch
Comment 31 Darin Fisher (:fishd, Google) 2012-01-19 14:02:39 PST
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
Comment 32 Greg Billock 2012-01-19 14:07:22 PST
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
Comment 33 Greg Billock 2012-01-19 14:20:31 PST
Created attachment 123196 [details]
Patch
Comment 34 WebKit Review Bot 2012-01-19 18:04:02 PST
Comment on attachment 123196 [details]
Patch

Clearing flags on attachment: 123196

Committed r105469: <http://trac.webkit.org/changeset/105469>
Comment 35 WebKit Review Bot 2012-01-19 18:04:08 PST
All reviewed patches have been landed.  Closing bug.