Update chromium code to use WebIntentRequest.

Update chromium code to use WebIntentRequest.

R=darin@chromium.org,jhawkins@chromium.org
BUG=None
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=118577

[Greg Billock, 2012-01-11 22:58:38]

Note that this depends on https://bugs.webkit.org/show_bug.cgi?id=76014

After both of these CLs land, I'll make another Chromium webkit API change to
remove WebIntent.

[James Hawkins, 2012-01-12 00:11:58]

http://codereview.chromium.org/9186021/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/9186021/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.h:531: WebKit::WebIntentRequest* intent);
Just curious, why does this need to come through as a pointer instead of
const-ref?

http://codereview.chromium.org/9186021/diff/1/content/renderer/web_intents_ho...
File content/renderer/web_intents_host.h (right):

http://codereview.chromium.org/9186021/diff/1/content/renderer/web_intents_ho...
content/renderer/web_intents_host.h:48: int id_counter_;
Document these vars.

[Greg Billock, 2012-01-12 18:54:45]

http://codereview.chromium.org/9186021/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/9186021/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.h:531: WebKit::WebIntentRequest* intent);
On 2012/01/12 00:11:58, James Hawkins wrote:
> Just curious, why does this need to come through as a pointer instead of
> const-ref?

Yeah, we're talking about that on the webkit CR. Basically we need the
refcounted WebCore::IntentRequest this thing holds. So I could use a copy
constructor with a const ref, or pass it like this. This way looks more explicit
to me as an ownership transfer, so I wrote it that way, but a copy constructor
would work equally well, and cause some niceness in default empty
implementations.

[James Hawkins, 2012-01-12 19:07:36]

http://codereview.chromium.org/9186021/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/9186021/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.h:531: WebKit::WebIntentRequest* intent);
On 2012/01/12 18:54:45, Greg Billock wrote:
> On 2012/01/12 00:11:58, James Hawkins wrote:
> > Just curious, why does this need to come through as a pointer instead of
> > const-ref?
> 
> Yeah, we're talking about that on the webkit CR. Basically we need the
> refcounted WebCore::IntentRequest this thing holds. So I could use a copy
> constructor with a const ref, or pass it like this. This way looks more
explicit
> to me as an ownership transfer, so I wrote it that way, but a copy constructor
> would work equally well, and cause some niceness in default empty
> implementations.

Up to you, was just curious.

[Greg Billock, 2012-01-13 21:27:44]

On 2012/01/12 19:07:36, James Hawkins wrote:
>
http://codereview.chromium.org/9186021/diff/1/content/renderer/render_view_im...
> File content/renderer/render_view_impl.h (right):
> 
>
http://codereview.chromium.org/9186021/diff/1/content/renderer/render_view_im...
> content/renderer/render_view_impl.h:531: WebKit::WebIntentRequest* intent);
> On 2012/01/12 18:54:45, Greg Billock wrote:
> > On 2012/01/12 00:11:58, James Hawkins wrote:
> > > Just curious, why does this need to come through as a pointer instead of
> > > const-ref?
> > 
> > Yeah, we're talking about that on the webkit CR. Basically we need the
> > refcounted WebCore::IntentRequest this thing holds. So I could use a copy
> > constructor with a const ref, or pass it like this. This way looks more
> explicit
> > to me as an ownership transfer, so I wrote it that way, but a copy
constructor
> > would work equally well, and cause some niceness in default empty
> > implementations.
> 
> Up to you, was just curious.

OK, I think this is in final form now. The adjustment is that I talked to Darin
and we're going to pass these by value as smart pointers, so I've updated the
usage to reflect that. Should be ready to go, assuming the webkit Chromium patch
lands much as it is now. This needs to go in after that roll.

[James Hawkins, 2012-01-13 21:30:03]

LGTM with nits.

http://codereview.chromium.org/9186021/diff/6001/content/renderer/web_intents...
File content/renderer/web_intents_host.h (right):

http://codereview.chromium.org/9186021/diff/6001/content/renderer/web_intents...
content/renderer/web_intents_host.h:36: // Called by the RenderView to register
a new web intents invocation.
nit: s/web intents/Web Intent/

(no s).

http://codereview.chromium.org/9186021/diff/6001/content/renderer/web_intents...
content/renderer/web_intents_host.h:47: int id_counter_;
Document vars.

[Greg Billock, 2012-01-17 23:52:01]

http://codereview.chromium.org/9186021/diff/6001/content/renderer/web_intents...
File content/renderer/web_intents_host.h (right):

http://codereview.chromium.org/9186021/diff/6001/content/renderer/web_intents...
content/renderer/web_intents_host.h:36: // Called by the RenderView to register
a new web intents invocation.
On 2012/01/13 21:30:03, James Hawkins wrote:
> nit: s/web intents/Web Intent/
> 
> (no s).

Done.

http://codereview.chromium.org/9186021/diff/6001/content/renderer/web_intents...
content/renderer/web_intents_host.h:47: int id_counter_;
On 2012/01/13 21:30:03, James Hawkins wrote:
> Document vars.

Done.

[darin (slow to review), 2012-01-19 07:23:57]

http://codereview.chromium.org/9186021/diff/13004/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9186021/diff/13004/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3165: WebKit::WebFrame* frame, const
WebKit::WebIntentRequest& intentRequest) {
nit: no need for WebKit:: prefix in front of WebFrame.  please add a using
directive for WebIntentRequest at the top of this file for consistency and drop
the WebKit:: prefix.

http://codereview.chromium.org/9186021/diff/13004/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3166: WebKit::WebIntentRequest
request(intentRequest);
this temporary variable should be unnecessary.

http://codereview.chromium.org/9186021/diff/13004/content/renderer/web_intent...
File content/renderer/web_intents_host.cc (right):

http://codereview.chromium.org/9186021/diff/13004/content/renderer/web_intent...
content/renderer/web_intents_host.cc:22: using WebKit::WebString;
add "using WebKit::WebIntentRequest" here too?  and one for
WebSerializedScriptValue?

http://codereview.chromium.org/9186021/diff/13004/content/renderer/web_intent...
File content/renderer/web_intents_host.h (right):

http://codereview.chromium.org/9186021/diff/13004/content/renderer/web_intent...
content/renderer/web_intents_host.h:9: #include <map>
nit: add a new line after this include

http://codereview.chromium.org/9186021/diff/13004/content/renderer/web_intent...
content/renderer/web_intents_host.h:49: int id_counter_;
perhaps you should be using IDMap<T> here?  see base/id_map.h

[Greg Billock, 2012-01-19 18:51:59]

http://codereview.chromium.org/9186021/diff/13004/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9186021/diff/13004/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3165: WebKit::WebFrame* frame, const
WebKit::WebIntentRequest& intentRequest) {
On 2012/01/19 07:23:57, darin wrote:
> nit: no need for WebKit:: prefix in front of WebFrame.  please add a using
> directive for WebIntentRequest at the top of this file for consistency and
drop
> the WebKit:: prefix.

Done.

http://codereview.chromium.org/9186021/diff/13004/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3166: WebKit::WebIntentRequest
request(intentRequest);
On 2012/01/19 07:23:57, darin wrote:
> this temporary variable should be unnecessary.

Done.

http://codereview.chromium.org/9186021/diff/13004/content/renderer/web_intent...
File content/renderer/web_intents_host.cc (right):

http://codereview.chromium.org/9186021/diff/13004/content/renderer/web_intent...
content/renderer/web_intents_host.cc:22: using WebKit::WebString;
On 2012/01/19 07:23:57, darin wrote:
> add "using WebKit::WebIntentRequest" here too?  and one for
> WebSerializedScriptValue?

Done.

http://codereview.chromium.org/9186021/diff/13004/content/renderer/web_intent...
File content/renderer/web_intents_host.h (right):

http://codereview.chromium.org/9186021/diff/13004/content/renderer/web_intent...
content/renderer/web_intents_host.h:9: #include <map>
On 2012/01/19 07:23:57, darin wrote:
> nit: add a new line after this include

Done.

http://codereview.chromium.org/9186021/diff/13004/content/renderer/web_intent...
content/renderer/web_intents_host.h:49: int id_counter_;
On 2012/01/19 07:23:57, darin wrote:
> perhaps you should be using IDMap<T> here?  see base/id_map.h

It looks like that class really wants to store pointers. I could probably swivel
to that, but I think it muddies the usage of the smart-pointer-like objects.

[darin (slow to review), 2012-01-19 22:12:20]

LGTM

Yeah, good point about IDMap.

IDMap has one other very nice feature.  It provides mutation-safe iteration. 
You don't currently iterate over all elements of the intent_requests_ map, but
if you did, then you might need to worry about mutations to the map occurring
while iterating over it.  That's very tricky to get right.

[Greg Billock, 2012-01-19 22:35:25]

On 2012/01/19 22:12:20, darin wrote:
> LGTM
> 
> Yeah, good point about IDMap.
> 
> IDMap has one other very nice feature.  It provides mutation-safe iteration. 
> You don't currently iterate over all elements of the intent_requests_ map, but
> if you did, then you might need to worry about mutations to the map occurring
> while iterating over it.  That's very tricky to get right.

Agreed. That is nice. At this point I don't foresee needing to enumerate
in-flight intents.

[commit-bot: I haz the power, 2012-01-20 19:25:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/9186021/28001

[commit-bot: I haz the power, 2012-01-20 21:48:21]

Try job failure for 9186021-28001 (retry) on win_rel for step "base_unittests".
It's a second try, previously, step "base_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-01-20 22:07:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/9186021/28001

[commit-bot: I haz the power, 2012-01-21 01:23:17]

Change committed as 118577