chrome.downloads.open()

Implement chrome.downloads.open()
BUG=12133
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171886

[benjhayden, 2012-12-04 20:50:51]

PTAL

[Randy Smith (Not in Mondays), 2012-12-05 14:17:42]

LGTM.

https://codereview.chromium.org/10836003/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/10836003/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/api/downloads/downloads_api.cc:350: DownloadItem*
GetActiveItem(Profile* profile, bool include_incognito, int id) {
Maybe regularize the names of these two functions?  (E.g. GetActiveDownload()
instead of GetDownload())?

https://codereview.chromium.org/10836003/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/api/downloads/downloads_api.cc:812: if (!download_item
|| !download_item->IsComplete()) {
I think I agree with the behavioral choice to not have "open before complete"
set auto-open, but be aware that since this isn't the way the UI behaves, people
might be surprised by it, which means you should make sure that it's mentioned
in the docs (I haven't checked the docs).

[benjhayden, 2012-12-05 18:41:02]

https://codereview.chromium.org/10836003/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/10836003/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/api/downloads/downloads_api.cc:350: DownloadItem*
GetActiveItem(Profile* profile, bool include_incognito, int id) {
On 2012/12/05 14:17:42, rdsmith wrote:
> Maybe regularize the names of these two functions?  (E.g. GetActiveDownload()
> instead of GetDownload())?
> 

Is GetDownloadIfInProgress ok?

https://codereview.chromium.org/10836003/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/api/downloads/downloads_api.cc:812: if (!download_item
|| !download_item->IsComplete()) {
On 2012/12/05 14:17:42, rdsmith wrote:
> I think I agree with the behavioral choice to not have "open before complete"
> set auto-open, but be aware that since this isn't the way the UI behaves,
people
> might be surprised by it, which means you should make sure that it's mentioned
> in the docs (I haven't checked the docs).

Decided to use SetOpenWhenComplete. PTAL.

[Randy Smith (Not in Mondays), 2012-12-05 19:12:05]

LGTM still stands despite my whining below--I'll accept whatever judgement you
make on these issues.  Alternatively, you're welcome to toss it back for another
review.

https://codereview.chromium.org/10836003/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/10836003/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/api/downloads/downloads_api.cc:350: DownloadItem*
GetActiveItem(Profile* profile, bool include_incognito, int id) {
On 2012/12/05 18:41:02, benjhayden_chromium wrote:
> On 2012/12/05 14:17:42, rdsmith wrote:
> > Maybe regularize the names of these two functions?  (E.g.
GetActiveDownload()
> > instead of GetDownload())?
> > 
> 
> Is GetDownloadIfInProgress ok?

Sure.

https://codereview.chromium.org/10836003/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/api/downloads/downloads_api.cc:812: if (!download_item
|| !download_item->IsComplete()) {
On 2012/12/05 18:41:02, benjhayden_chromium wrote:
> On 2012/12/05 14:17:42, rdsmith wrote:
> > I think I agree with the behavioral choice to not have "open before
complete"
> > set auto-open, but be aware that since this isn't the way the UI behaves,
> people
> > might be surprised by it, which means you should make sure that it's
mentioned
> > in the docs (I haven't checked the docs).
> 
> Decided to use SetOpenWhenComplete. PTAL.

*wince*  I guess that's fine.  I'm uncomfortable with it because I think
long-term we want to remove the auto-open behavior from DownloadItem completely,
and make it the responsibility of the callers (e.g. they should observe the item
and call Open when it completes.)  But if auto-opening is the behavior you want
at the extension level, that objection certainly shouldn't stop you
(functionality should drive mechanism, not the other way around :-}), and that
is the way to accomplish what you want in the current scheme.  We'll just need
to port it if/when we move auto-open out of content.

Separate from the above, could you say a little bit about why you want to
present the auto-open interface to extension authors rather than making them
wait for completion?  Because it's one less event they have to handle, which'll
result in simpler code?

https://codereview.chromium.org/10836003/diff/23004/chrome/browser/extensions...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/10836003/diff/23004/chrome/browser/extensions...
chrome/browser/extensions/api/downloads/downloads_api.cc:820:
download_item->OpenDownload();
You know that download_item->OpenDownload() currently implements this
functionality, right?  For minimality of interface, I'm tempted to suggest you
just call OpenDownload(), but see caveats below.

Caveat: Temporary downloads block this behavior, which is, IMO, a good thing.

Caveat2: OpenDownload() is a toggle, which you may not want.  (And is a broken
interface, but never mind :-}).

[benjhayden, 2012-12-05 19:29:47]

https://codereview.chromium.org/10836003/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/10836003/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/api/downloads/downloads_api.cc:812: if (!download_item
|| !download_item->IsComplete()) {
On 2012/12/05 19:12:05, rdsmith wrote:
> On 2012/12/05 18:41:02, benjhayden_chromium wrote:
> > On 2012/12/05 14:17:42, rdsmith wrote:
> > > I think I agree with the behavioral choice to not have "open before
> complete"
> > > set auto-open, but be aware that since this isn't the way the UI behaves,
> > people
> > > might be surprised by it, which means you should make sure that it's
> mentioned
> > > in the docs (I haven't checked the docs).
> > 
> > Decided to use SetOpenWhenComplete. PTAL.
> 
> *wince*  I guess that's fine.  I'm uncomfortable with it because I think
> long-term we want to remove the auto-open behavior from DownloadItem
completely,
> and make it the responsibility of the callers (e.g. they should observe the
item
> and call Open when it completes.)  But if auto-opening is the behavior you
want
> at the extension level, that objection certainly shouldn't stop you
> (functionality should drive mechanism, not the other way around :-}), and that
> is the way to accomplish what you want in the current scheme.  We'll just need
> to port it if/when we move auto-open out of content.
> 
> Separate from the above, could you say a little bit about why you want to
> present the auto-open interface to extension authors rather than making them
> wait for completion?  Because it's one less event they have to handle,
which'll
> result in simpler code?

Actually, the primary reason that I'd like for open() to set open_when_complete
is what you said originally: it's the least surprising because it's in keeping
with the current UI. I don't think it'll be a terrible burden to implement it as
an observer if/when SetOpenWhenComplete is removed.
Are there plans to remove the open_when_complete feature from the UI at some
point?

[Randy Smith (Not in Mondays), 2012-12-05 19:35:12]

On 2012/12/05 19:29:47, benjhayden_chromium wrote:
>
https://codereview.chromium.org/10836003/diff/12001/chrome/browser/extensions...
> File chrome/browser/extensions/api/downloads/downloads_api.cc (right):
> 
>
https://codereview.chromium.org/10836003/diff/12001/chrome/browser/extensions...
> chrome/browser/extensions/api/downloads/downloads_api.cc:812: if
(!download_item
> || !download_item->IsComplete()) {
> On 2012/12/05 19:12:05, rdsmith wrote:
> > On 2012/12/05 18:41:02, benjhayden_chromium wrote:
> > > On 2012/12/05 14:17:42, rdsmith wrote:
> > > > I think I agree with the behavioral choice to not have "open before
> > complete"
> > > > set auto-open, but be aware that since this isn't the way the UI
behaves,
> > > people
> > > > might be surprised by it, which means you should make sure that it's
> > mentioned
> > > > in the docs (I haven't checked the docs).
> > > 
> > > Decided to use SetOpenWhenComplete. PTAL.
> > 
> > *wince*  I guess that's fine.  I'm uncomfortable with it because I think
> > long-term we want to remove the auto-open behavior from DownloadItem
> completely,
> > and make it the responsibility of the callers (e.g. they should observe the
> item
> > and call Open when it completes.)  But if auto-opening is the behavior you
> want
> > at the extension level, that objection certainly shouldn't stop you
> > (functionality should drive mechanism, not the other way around :-}), and
that
> > is the way to accomplish what you want in the current scheme.  We'll just
need
> > to port it if/when we move auto-open out of content.
> > 
> > Separate from the above, could you say a little bit about why you want to
> > present the auto-open interface to extension authors rather than making them
> > wait for completion?  Because it's one less event they have to handle,
> which'll
> > result in simpler code?
> 
> Actually, the primary reason that I'd like for open() to set
open_when_complete
> is what you said originally: it's the least surprising because it's in keeping
> with the current UI. I don't think it'll be a terrible burden to implement it
as
> an observer if/when SetOpenWhenComplete is removed.
> Are there plans to remove the open_when_complete feature from the UI at some
> point?

I don't think so (up to Asanka), but there are plans for it to be implemented
inside the UI rather than in DownloadItem.

The caveats I put on my other comment are relevant here: What you currently have
is close to the UI behavior but doesn't match it, and matching it would produce
a somewhat messy interface (open if complete, but toggle auto open if not
complete but only if !temporary).  

I'm good with what you decide, just putting all the arguments out there.

[benjhayden, 2012-12-06 21:45:14]

I think it matches the UI now.

We're planning on moving everything to do with Open up into the UI, right? In
that case, this function will need to call into that code anyway, so that code
might as well allow this function access to whatever form of open_when_complete
we have. I don't think that this function creates an unsolvable problem, just
something that we'll need to be aware of when we move Open up into the UI, and I
think we'll be able to handle it.

[Randy Smith (Not in Mondays), 2012-12-07 17:24:31]

On 2012/12/06 21:45:14, benjhayden_chromium wrote:
> I think it matches the UI now.

Still LGTM :-}.

> We're planning on moving everything to do with Open up into the UI, right? 

Nope.  Current plan is to keep the basics of open in content (current thought
is: invalid before COMPLETE, no auto-open support functionality, embedder hook
to hijack) and make other functionality be supported by the consumers.  If those
consumers need special UI support, they'll need to talk with the UI.  

> In
> that case, this function will need to call into that code anyway, 

That strikes me as ugly architecture, and I'd like to avoid it--we should find a
split of code so that there's rarely a need for other subsystems to call into
the UI, and they only call in in a generic fashion.  If possible :-}.

> so that code
> might as well allow this function access to whatever form of
open_when_complete
> we have. I don't think that this function creates an unsolvable problem, just
> something that we'll need to be aware of when we move Open up into the UI, and
I
> think we'll be able to handle it.

Asanka, do you have an opinion on this?  I'm good with it, but I think it's
going to be more complex when we get to the later refactoring stage than Ben
thinks, so I want to pull you in early.

[commit-bot: I haz the power, 2012-12-07 17:45:07]

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

[commit-bot: I haz the power, 2012-12-07 17:45:14]

Presubmit check for 10836003-46002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/common/extensions/api

Presubmit checks took 4.8s to calculate.

[benjhayden, 2012-12-07 17:47:02]

Mkearney, would you mind reviewing chrome/common/extensions/api/downloads.idl
for ownership?

[mkearney1, 2012-12-07 18:01:42]

On 2012/12/07 17:47:02, benjhayden_chromium wrote:
> Mkearney, would you mind reviewing chrome/common/extensions/api/downloads.idl
> for ownership?

Hey, Ben

Is there an associated bug? No worries if there isn't-- just looking for
context.

Meggin

[benjhayden, 2012-12-07 18:07:01]

On 2012/12/07 18:01:42, mkearney1 wrote:
> On 2012/12/07 17:47:02, benjhayden_chromium wrote:
> > Mkearney, would you mind reviewing
chrome/common/extensions/api/downloads.idl
> > for ownership?
> 
> Hey, Ben
> 
> Is there an associated bug? No worries if there isn't-- just looking for
> context.
> 
> Meggin

Added BUG=12133. The context is that I'm implementing the rest of the API.

[mkearney1, 2012-12-07 20:10:25]

https://codereview.chromium.org/10836003/diff/46002/chrome/common/extensions/...
File chrome/common/extensions/api/downloads.idl (right):

https://codereview.chromium.org/10836003/diff/46002/chrome/common/extensions/...
chrome/common/extensions/api/downloads.idl:364: // |downloadId|: The identifier
for the download.
What about "The identifier for the downloaded file." -- ?

[benjhayden, 2012-12-07 20:17:55]

https://codereview.chromium.org/10836003/diff/46002/chrome/common/extensions/...
File chrome/common/extensions/api/downloads.idl (right):

https://codereview.chromium.org/10836003/diff/46002/chrome/common/extensions/...
chrome/common/extensions/api/downloads.idl:364: // |downloadId|: The identifier
for the download.
On 2012/12/07 20:10:25, mkearney1 wrote:
> What about "The identifier for the downloaded file." -- ?

Done.

[mkearney1, 2012-12-07 20:39:39]

lgtm

[commit-bot: I haz the power, 2012-12-07 20:41:12]

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

[commit-bot: I haz the power, 2012-12-07 20:41:19]

Presubmit check for 10836003-45005 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
File chrome/common/extensions/api/downloads.idl may have an old-style <a> link
to an API page. Please run docs/server2/link_converter.py to convert the
link[s], or convert them manually.

Suggested changes are: 

Presubmit checks took 4.8s to calculate.

[commit-bot: I haz the power, 2012-12-07 20:59:27]

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

[commit-bot: I haz the power, 2012-12-07 20:59:37]

Presubmit check for 10836003-45005 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
File chrome/common/extensions/api/downloads.idl may have an old-style <a> link
to an API page. Please run docs/server2/link_converter.py to convert the
link[s], or convert them manually.

Suggested changes are: 

Presubmit checks took 6.8s to calculate.

[commit-bot: I haz the power, 2012-12-07 21:24:56]

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

[commit-bot: I haz the power, 2012-12-07 23:56:06]

Change committed as 171886