[Web Intents] WebIntentsPickerController will now fetch icon for suggested extensions.

[Web Intents] WebIntentsPickerController will now fetch icon for suggested extensions.

BUG=113476
TEST=WebIntentPickerControllerBrowserTest.FetchExtensionIcon

TBR=pkasting@chromium.org
TBR=estade@chromium.org
TBR=sail@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=124006

[Greg Billock, 2012-02-23 18:38:33]

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:342: void
WebIntentPickerController::OnExtensionIconURLFetchComplete(
This'll get called by the fetcher thread, right? Is it safe to use that thread
to do the icon decoding, or is that a no-no?

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:345: return;
Need to AsyncOperationFinished here?

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:347:
scoped_ptr<std::string> response(new std::string());
Can kill inner "()"

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:352:
content::BrowserThread::PostBlockingPoolTask(
These cascades are why the frontends ended up using state-machine-driven
callback systems. After a while it gets hard to track the closures and error
states. :-)

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:379: SkBitmap
resized_icon = skia::ImageOperations::Resize(
Is this doing an extra image copy?

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:388:
base::Bind(callback, icon_image));
So gfx::Image is pointer-ish and cool to pass like this?

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:410: if
(--pending_async_count_ == 0) {
So we don't need a mutex here because Decode() is posting the callbacks back to
the browser thread? Would it just be easier to have them call directly? There's
very little contention risk... Or would the model freak out to be called
multi-threaded?

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc
(right):

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:87: class
DummyURLFetcherFactory : public content::URLFetcherFactory {
Would this factory be worth putting in the TestURLFetcher class? Seems like a
useful machine.

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:186: //
The FakeURLFetcherFactory will return a NULL URLFetcher if a request is
I don't understand this. Aren't the only urls you fetch going to be
pre-populated?

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_model.h (right):

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_model.h:117: const gfx::Image&
image);
indentation

[binji, 2012-02-23 19:06:13]

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:342: void
WebIntentPickerController::OnExtensionIconURLFetchComplete(
On 2012/02/23 18:38:34, Greg Billock wrote:
> This'll get called by the fetcher thread, right? Is it safe to use that thread
> to do the icon decoding, or is that a no-no?

AFAIK this is called back on the UI thread, so it seemed like we shouldn't do
much work on it (even if decoding and resizing a PNG is lightweight).

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:345: return;
On 2012/02/23 18:38:34, Greg Billock wrote:
> Need to AsyncOperationFinished here?

Done.

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:347:
scoped_ptr<std::string> response(new std::string());
On 2012/02/23 18:38:34, Greg Billock wrote:
> Can kill inner "()"

Done.

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:352:
content::BrowserThread::PostBlockingPoolTask(
On 2012/02/23 18:38:34, Greg Billock wrote:
> These cascades are why the frontends ended up using state-machine-driven
> callback systems. After a while it gets hard to track the closures and error
> states. :-)

Agreed. Though I think it's not too hard to follow here...

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:379: SkBitmap
resized_icon = skia::ImageOperations::Resize(
On 2012/02/23 18:38:34, Greg Billock wrote:
> Is this doing an extra image copy?

Yes, it doesn't resize in-place.

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:388:
base::Bind(callback, icon_image));
On 2012/02/23 18:38:34, Greg Billock wrote:
> So gfx::Image is pointer-ish and cool to pass like this?

Yes, gfx::Image has internal refcounted storage.

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:410: if
(--pending_async_count_ == 0) {
On 2012/02/23 18:38:34, Greg Billock wrote:
> So we don't need a mutex here because Decode() is posting the callbacks back
to
> the browser thread? Would it just be easier to have them call directly?
There's
> very little contention risk... Or would the model freak out to be called
> multi-threaded?

This callback is only used to notify a test that all operations are finished.
Having it call multi-threaded makes those tests more complicated, I think.

I'll add a thread check for this function to make it a little clearer.

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc
(right):

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:87: class
DummyURLFetcherFactory : public content::URLFetcherFactory {
On 2012/02/23 18:38:34, Greg Billock wrote:
> Would this factory be worth putting in the TestURLFetcher class? Seems like a
> useful machine.

Maybe. The only reason I need it is because I use FakeURLFetcherFactory in a
browser_test, which no-one else does really. More often you see
TestURLFetcherFactory, which  works without it. It's trickier for me to use that
here because it requires a lot more modification to the picker controller and
CWS intents registry to use.

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:186: //
The FakeURLFetcherFactory will return a NULL URLFetcher if a request is
On 2012/02/23 18:38:34, Greg Billock wrote:
> I don't understand this. Aren't the only urls you fetch going to be
> pre-populated?

I thought so too, but it turns out some other systems fire off URL fetches in
browser process tests.

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_model.h (right):

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_model.h:117: const gfx::Image&
image);
On 2012/02/23 18:38:34, Greg Billock wrote:
> indentation

Done.

[Greg Billock, 2012-02-23 22:15:49]

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc
(right):

http://codereview.chromium.org/9430025/diff/1025/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:186: //
The FakeURLFetcherFactory will return a NULL URLFetcher if a request is
On 2012/02/23 19:06:13, binji wrote:
> On 2012/02/23 18:38:34, Greg Billock wrote:
> > I don't understand this. Aren't the only urls you fetch going to be
> > pre-populated?
> 
> I thought so too, but it turns out some other systems fire off URL fetches in
> browser process tests.

So where are the requests getting delayed? Does the FakeURLFetcher just eat
requests?

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:313:
net::URLRequestContextGetter* context =
It looks like this can just move down to
SetRequestContext(wrapper_->profile()->GetReqCtx())?

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:336:
icon_url_fetcher->Start();
So the URLFetcher always delegates the callback to the UI thread? That seems
like something it'd let the caller manage, but OK.

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:351: if
(source->GetResponseAsString(response.get())) {
How about if (!...) { AsyncOpFinished; return; }

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:352:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
Can take this out (you have it above now).

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:359: base::Bind(
I think it'd read easier to extract these two callbacks as named variables.

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:391:
base::Bind(callback, icon_image));
It might be easier to get a WebIntentPickerController ref and just call these
known methods on it. If this isn't an instance method, you could just not make
it static and stick it local to this .cc file and not publish it at all. That
might be the best given the thread situation.

[binji, 2012-02-23 22:33:41]

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:313:
net::URLRequestContextGetter* context =
On 2012/02/23 22:15:50, Greg Billock wrote:
> It looks like this can just move down to
> SetRequestContext(wrapper_->profile()->GetReqCtx())?

Done.

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:336:
icon_url_fetcher->Start();
On 2012/02/23 22:15:50, Greg Billock wrote:
> So the URLFetcher always delegates the callback to the UI thread? That seems
> like something it'd let the caller manage, but OK.

I wasn't 100% on this, so I took a look. It looks like the delegate is always
called on the same thread as Create is called -- probably just for convenience.

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:351: if
(source->GetResponseAsString(response.get())) {
On 2012/02/23 22:15:50, Greg Billock wrote:
> How about if (!...) { AsyncOpFinished; return; }

Done.

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:352:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2012/02/23 22:15:50, Greg Billock wrote:
> Can take this out (you have it above now).

Oh, right. :-)

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:359: base::Bind(
On 2012/02/23 22:15:50, Greg Billock wrote:
> I think it'd read easier to extract these two callbacks as named variables.

Done.

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:391:
base::Bind(callback, icon_image));
On 2012/02/23 22:15:50, Greg Billock wrote:
> It might be easier to get a WebIntentPickerController ref and just call these
> known methods on it. If this isn't an instance method, you could just not make
> it static and stick it local to this .cc file and not publish it at all. That
> might be the best given the thread situation.

This was the original way I wrote it, but it didn't seem possible. I need to get
a weak pointer to the WebIntentPickerController to ensure that the callback
doesn't happen after it is destroyed, but that is not allowed on a different
thread than created the WeakPtrFactory.

In fact, I was getting an assert just dereferencing the weak "this" from a
different thread, which is why DecodeExtensionIconAndResize is static.

I suppose it could be file local, but then it should be in an anonymous
namespace at the top of the file, which I think makes it a little harder to
follow. What do you think?

[Greg Billock, 2012-02-23 22:51:30]

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:391:
base::Bind(callback, icon_image));
You want to put some of these threading constraints in a comment? There's quite
a bit of hidden information about what's possible that'd be good to write down.

I think I was trying to ask about a couple different options in this comment,
but it came out kind of garbled. What I meant was, if it isn't possible to have
this be an instance method (because of the weak ptr), then how about just making
it file-local? (Up by the trampoline)

On 2012/02/23 22:33:41, binji wrote:
> On 2012/02/23 22:15:50, Greg Billock wrote:
> > It might be easier to get a WebIntentPickerController ref and just call
these
> > known methods on it. If this isn't an instance method, you could just not
make
> > it static and stick it local to this .cc file and not publish it at all.
That
> > might be the best given the thread situation.
> 
> This was the original way I wrote it, but it didn't seem possible. I need to
get
> a weak pointer to the WebIntentPickerController to ensure that the callback
> doesn't happen after it is destroyed, but that is not allowed on a different
> thread than created the WeakPtrFactory.
> 
> In fact, I was getting an assert just dereferencing the weak "this" from a
> different thread, which is why DecodeExtensionIconAndResize is static.
> 
> I suppose it could be file local, but then it should be in an anonymous
> namespace at the top of the file, which I think makes it a little harder to
> follow. What do you think?

[binji, 2012-02-23 23:34:48]

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:391:
base::Bind(callback, icon_image));
On 2012/02/23 22:51:30, Greg Billock wrote:
> You want to put some of these threading constraints in a comment? There's
quite
> a bit of hidden information about what's possible that'd be good to write
down.
> 
> I think I was trying to ask about a couple different options in this comment,
> but it came out kind of garbled. What I meant was, if it isn't possible to
have
> this be an instance method (because of the weak ptr), then how about just
making
> it file-local? (Up by the trampoline)
> 
> On 2012/02/23 22:33:41, binji wrote:
> > On 2012/02/23 22:15:50, Greg Billock wrote:
> > > It might be easier to get a WebIntentPickerController ref and just call
> these
> > > known methods on it. If this isn't an instance method, you could just not
> make
> > > it static and stick it local to this .cc file and not publish it at all.
> That
> > > might be the best given the thread situation.
> > 
> > This was the original way I wrote it, but it didn't seem possible. I need to
> get
> > a weak pointer to the WebIntentPickerController to ensure that the callback
> > doesn't happen after it is destroyed, but that is not allowed on a different
> > thread than created the WeakPtrFactory.
> > 
> > In fact, I was getting an assert just dereferencing the weak "this" from a
> > different thread, which is why DecodeExtensionIconAndResize is static.
> > 
> > I suppose it could be file local, but then it should be in an anonymous
> > namespace at the top of the file, which I think makes it a little harder to
> > follow. What do you think?
> 

Done.

[Greg Billock, 2012-02-24 00:08:31]

On 2012/02/23 23:34:48, binji wrote:
>
http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
> File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):
> 
>
http://codereview.chromium.org/9430025/diff/7002/chrome/browser/ui/intents/we...
> chrome/browser/ui/intents/web_intent_picker_controller.cc:391:
> base::Bind(callback, icon_image));
> On 2012/02/23 22:51:30, Greg Billock wrote:
> > You want to put some of these threading constraints in a comment? There's
> quite
> > a bit of hidden information about what's possible that'd be good to write
> down.
> > 
> > I think I was trying to ask about a couple different options in this
comment,
> > but it came out kind of garbled. What I meant was, if it isn't possible to
> have
> > this be an instance method (because of the weak ptr), then how about just
> making
> > it file-local? (Up by the trampoline)
> > 
> > On 2012/02/23 22:33:41, binji wrote:
> > > On 2012/02/23 22:15:50, Greg Billock wrote:
> > > > It might be easier to get a WebIntentPickerController ref and just call
> > these
> > > > known methods on it. If this isn't an instance method, you could just
not
> > make
> > > > it static and stick it local to this .cc file and not publish it at all.
> > That
> > > > might be the best given the thread situation.
> > > 
> > > This was the original way I wrote it, but it didn't seem possible. I need
to
> > get
> > > a weak pointer to the WebIntentPickerController to ensure that the
callback
> > > doesn't happen after it is destroyed, but that is not allowed on a
different
> > > thread than created the WeakPtrFactory.
> > > 
> > > In fact, I was getting an assert just dereferencing the weak "this" from a
> > > different thread, which is why DecodeExtensionIconAndResize is static.
> > > 
> > > I suppose it could be file local, but then it should be in an anonymous
> > > namespace at the top of the file, which I think makes it a little harder
to
> > > follow. What do you think?
> > 
> 
> Done.

lgtm

[commit-bot: I haz the power, 2012-02-24 00:31:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/binji@chromium.org/9430025/12002

[commit-bot: I haz the power, 2012-02-24 03:50:08]

Try job failure for 9430025-12002 (retry) on win_rel for steps "browser_tests,
ui_tests".
It's a second try, previously, steps "browser_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[binji, 2012-03-05 18:20:36]

pkasting: views OWNER
estade: gtk OWNER
sail: cocoa OWNER