Implement chrome.downloads.drag()

Implement chrome.downloads.drag()
PS1@r172879
BUG=12133
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175043

[Randy Smith (Not in Mondays), 2012-12-13 16:52:21]

I think I'm the wrong person for this review--I really don't have a good sense
of how the gfx interface works, nor of whether it's ok to do a nested message
loop in this case (my instinct is "nested message loop *bad*", but I do know
there are exceptions).   And worse, I'm not sure who the right person would be. 
Asanka?  Jian Li?

[benjhayden, 2012-12-13 17:08:59]

On 2012/12/13 16:52:21, rdsmith wrote:
> I think I'm the wrong person for this review--I really don't have a good sense
> of how the gfx interface works, nor of whether it's ok to do a nested message
> loop in this case (my instinct is "nested message loop *bad*", but I do know
> there are exceptions).   And worse, I'm not sure who the right person would
be. 
> Asanka?  Jian Li?

I just copied the implementation from downloads_dom_handler.cc.

[Randy Smith (Not in Mondays), 2012-12-13 18:26:19]

[+sky@]

Yeah, that doesn't reassure me.  It makes it likely the gfx stuff is right, but
I could easily imagine that the best practices around nested message loops have
changed since the DDH implementation (which was a while ago).

Scott: Are you a reasonable person to review this?   If not, can you suggest
someone who is?  My major concern is around the nested message loop, a minor one
around the use of gfx:: (with which I'm not familiar).  As Ben says, the code is
duplicated from DownloadsDOMHandler, so if there's a problem, that problem
probably exists there as well.

[sky, 2012-12-13 20:23:22]

You should rightly be worried about nested message loops. The biggest pain is
all objects on the stack have to be deleted while the nested message loop is
running. Or rather when the message loop completes and the stack unravels
objects may be deleted. This requires fragile code in places. Presumably
DownloadsDragFunction::RunImpl() is a callback, so the stack should be small.

I encourage you to write a test for coverage.

LGTM

[benjhayden, 2012-12-14 15:36:17]

On 2012/12/13 20:23:22, sky wrote:
> You should rightly be worried about nested message loops. The biggest pain is
> all objects on the stack have to be deleted while the nested message loop is
> running. Or rather when the message loop completes and the stack unravels
> objects may be deleted. This requires fragile code in places. Presumably
> DownloadsDragFunction::RunImpl() is a callback, so the stack should be small.
> 
> I encourage you to write a test for coverage.
> 
> LGTM

I'm not sure if RunImpl is close enough to a callback.

RunImpl is called by ExtensionFunction:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
which is called by ExtensionFunctionDispatcher::Dispatch:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
which is called by TabHelper::OnRequest, ExtensionHost::OnRequest,
ShellWindow::OnRequest:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/extensio...
which handle the IPC from the renderer:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/extensions...

Do I need to bounce Drag off the UI thread, or is there a preferred alternative
to nested message loops?

[sky, 2012-12-14 17:46:21]

On Fri, Dec 14, 2012 at 7:36 AM,  <benjhayden@chromium.org> wrote:
> On 2012/12/13 20:23:22, sky wrote:
>>
>> You should rightly be worried about nested message loops. The biggest pain
>> is
>> all objects on the stack have to be deleted while the nested message loop
>> is
>> running. Or rather when the message loop completes and the stack unravels
>> objects may be deleted. This requires fragile code in places. Presumably
>> DownloadsDragFunction::RunImpl() is a callback, so the stack should be
>> small.
>
>
>> I encourage you to write a test for coverage.
>
>
>> LGTM
>
>
> I'm not sure if RunImpl is close enough to a callback.
>
> RunImpl is called by ExtensionFunction:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
> which is called by ExtensionFunctionDispatcher::Dispatch:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
> which is called by TabHelper::OnRequest, ExtensionHost::OnRequest,
> ShellWindow::OnRequest:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/extensio...
> which handle the IPC from the renderer:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/extensions...
>
> Do I need to bounce Drag off the UI thread, or is there a preferred
> alternative
> to nested message loops?

We run bookmark drag and drop on the UI thread too. I don't know
enough about that call stack to say how well it can deal with being
deleted while the nested message loop is running.

  -Scott

[benjhayden, 2012-12-14 18:35:34]

On 2012/12/14 17:46:21, sky wrote:
> On Fri, Dec 14, 2012 at 7:36 AM,  <mailto:benjhayden@chromium.org> wrote:
> > On 2012/12/13 20:23:22, sky wrote:
> >>
> >> You should rightly be worried about nested message loops. The biggest pain
> >> is
> >> all objects on the stack have to be deleted while the nested message loop
> >> is
> >> running. Or rather when the message loop completes and the stack unravels
> >> objects may be deleted. This requires fragile code in places. Presumably
> >> DownloadsDragFunction::RunImpl() is a callback, so the stack should be
> >> small.
> >
> >
> >> I encourage you to write a test for coverage.
> >
> >
> >> LGTM
> >
> >
> > I'm not sure if RunImpl is close enough to a callback.
> >
> > RunImpl is called by ExtensionFunction:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
> > which is called by ExtensionFunctionDispatcher::Dispatch:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
> > which is called by TabHelper::OnRequest, ExtensionHost::OnRequest,
> > ShellWindow::OnRequest:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/extensio...
> > which handle the IPC from the renderer:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/extensions...
> >
> > Do I need to bounce Drag off the UI thread, or is there a preferred
> > alternative
> > to nested message loops?
> 
> We run bookmark drag and drop on the UI thread too. I don't know
> enough about that call stack to say how well it can deal with being
> deleted while the nested message loop is running.
> 
>   -Scott

I just manually tested drag().
Here's an unpacked extension if anybody would care to patch this CL in and give
it a spin:
~benjhayden/www/downloads

[mkearney1, 2013-01-03 19:45:07]

lgtm

[commit-bot: I haz the power, 2013-01-03 20:21:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/11566012/38001

[commit-bot: I haz the power, 2013-01-03 23:17:40]

Change committed as 175043