Fix most of the downloads api in incognito profiles

Fix most of the downloads api in incognito: search(), pause(), resume(), cancel(), getFileIcon().

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

[benjhayden, 2012-05-02 16:00:14]

PTAL

[Randy Smith (Not in Mondays), 2012-05-02 18:57:01]

A couple of high level comments:
* It'd be good to have an issue that spells out in detail the problem you're
trying to solve; it's not completely clear from context, at least at the CL
description level.

* When referring to another CL, it's nice to the reviewers to spell it out in
URL form; it just makes it simpler to get to.

http://codereview.chromium.org/9425014/diff/19008/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/9425014/diff/19008/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:469: // understand OTR, so
this change will depend on DownloadManager::DownloadUrl.
I'm confused; render_process_host_id and render_view_host_routing_id don't
understand OTR, or DownloadRequestHandle doesn't understand OTR?

Also: This comments shouldn't be committed as shown; it should either have a
TODO (and probably no code) or not be in at all.  My guess is that you're aiming
to commit DownloadUrl first, and then this--the ideal way to deal with that is
to work in a git branch that's branched off of DownloadUrl, and thus arrange for
this CL to show the diffs from that CL, and thus no-one but you has to worry
about commit order.  Let me know if you have any interest in pointers on setting
that up :-}.

http://codereview.chromium.org/9425014/diff/19008/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:687: otr_profile =
profile()->GetOffTheRecordProfile();
I don't think it's cool to allow access to OTR downloads from an extension
running in a regular window.  That seems like it punches a hole in the privacy
protection.  Am I confused about what you're doing here?

http://codereview.chromium.org/9425014/diff/19008/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.downloads.json (right):

http://codereview.chromium.org/9425014/diff/19008/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.downloads.json:147: "description":
"True if this download is off-the-record, false or undefined otherwise.",
Why the option (false versus undefined)?

[Randy Smith (Not in Mondays), 2012-05-02 18:57:23]

Whoops, one other high level comment: I think you'll want someone other than me
to review the extension specific parts of this .

[benjhayden, 2012-05-03 13:54:39]

PTAL

http://codereview.chromium.org/9425014/diff/19008/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/9425014/diff/19008/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:469: // understand OTR, so
this change will depend on DownloadManager::DownloadUrl.
On 2012/05/02 18:57:01, rdsmith wrote:
> I'm confused; render_process_host_id and render_view_host_routing_id don't
> understand OTR, or DownloadRequestHandle doesn't understand OTR?
> 
> Also: This comments shouldn't be committed as shown; it should either have a
> TODO (and probably no code) or not be in at all.  My guess is that you're
aiming
> to commit DownloadUrl first, and then this--the ideal way to deal with that is
> to work in a git branch that's branched off of DownloadUrl, and thus arrange
for
> this CL to show the diffs from that CL, and thus no-one but you has to worry
> about commit order.  Let me know if you have any interest in pointers on
setting
> that up :-}.

Done.

http://codereview.chromium.org/9425014/diff/19008/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:687: otr_profile =
profile()->GetOffTheRecordProfile();
On 2012/05/02 18:57:01, rdsmith wrote:
> I don't think it's cool to allow access to OTR downloads from an extension
> running in a regular window.  That seems like it punches a hole in the privacy
> protection.  Am I confused about what you're doing here?

Done.

http://codereview.chromium.org/9425014/diff/19008/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.downloads.json (right):

http://codereview.chromium.org/9425014/diff/19008/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.downloads.json:147: "description":
"True if this download is off-the-record, false or undefined otherwise.",
On 2012/05/02 18:57:01, rdsmith wrote:
> Why the option (false versus undefined)?

Done.

[Randy Smith (Not in Mondays), 2012-05-03 19:07:21]

I'd also like to review the new CL description before stamping.

http://codereview.chromium.org/9425014/diff/31007/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/9425014/diff/31007/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:317: profile =
browser->profile();
This seems like a weird bit of functionality at the interface--we override
profile if we can, but accept what the consumer passed in if we can't override
it.  What are the requirements driving that interface?

http://codereview.chromium.org/9425014/diff/31007/chrome/browser/download/dow...
File chrome/browser/download/download_extension_test.cc (right):

http://codereview.chromium.org/9425014/diff/31007/chrome/browser/download/dow...
chrome/browser/download/download_extension_test.cc:263:
CHECK(downloads_directory_.CreateUniqueTempDir());
So I don't like doing CHECKs in tests (comment applies throughout file) as if
the CHECK fails, the test core-dumps, which I believe is considered a more
serious failure than just failing (requires being disabled, or some such--it's a
bigger deal).  Is there a way to keep the functionality of just failing the test
if the things targeted by CHECKs fail?

[benjhayden, 2012-05-03 21:08:12]

PTAL

http://codereview.chromium.org/9425014/diff/31007/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/9425014/diff/31007/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:317: profile =
browser->profile();
On 2012/05/03 19:07:21, rdsmith wrote:
> This seems like a weird bit of functionality at the interface--we override
> profile if we can, but accept what the consumer passed in if we can't override
> it.  What are the requirements driving that interface?

GetCurrentBrowser() may sometimes return NULL if there is no current browser.
If there is a current browser and it's incognito, then
GetCurrentBrowser()->profile() will be incognito, whereas
ExtensionFunction::profile() will not be incognito.
So we need to check for GetCurrentBrowser() && GetCurrentBrowser()->profile()
first, then fall back to profile(). All callers of GetItem() need to do this, so
it made sense to roll the functionality into GetItem().
I'll add convenience methods to {As,S}yncDownloadFunction to wrap this GetItem()
so that callers don't need to even look at GetCurrentBrowser() or profile().

http://codereview.chromium.org/9425014/diff/31007/chrome/browser/download/dow...
File chrome/browser/download/download_extension_test.cc (right):

http://codereview.chromium.org/9425014/diff/31007/chrome/browser/download/dow...
chrome/browser/download/download_extension_test.cc:263:
CHECK(downloads_directory_.CreateUniqueTempDir());
On 2012/05/03 19:07:21, rdsmith wrote:
> So I don't like doing CHECKs in tests (comment applies throughout file) as if
> the CHECK fails, the test core-dumps, which I believe is considered a more
> serious failure than just failing (requires being disabled, or some such--it's
a
> bigger deal).  Is there a way to keep the functionality of just failing the
test
> if the things targeted by CHECKs fail?

Done.

[benjhayden, 2012-05-04 13:53:50]

Mihai, would you mind reviewing my understanding of profile() vs.
GetCurrentBrowser()->profile()?

[Randy Smith (Not in Mondays), 2012-05-04 15:25:46]

LGTM, but if Mihai's willing, I'd like him to give the whole thing a scan for
compatibility with extension api programming patterns.

[benjhayden, 2012-05-14 17:17:08]

merge

[benjhayden, 2012-05-14 20:54:54]

"

[Mihai Parparita -not on Chrome, 2012-05-14 21:05:48]

I'm not actually the best person for this review. I believe Matt knows the most
about extension APIs and incognito (split vs. spanning, etc.).

[Matt Perry, 2012-05-14 22:57:11]

http://codereview.chromium.org/9425014/diff/50009/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/9425014/diff/50009/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:315: if ((browser != NULL) &&
nit: prefer "browser" to "browser != NULL"

http://codereview.chromium.org/9425014/diff/50009/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:682: Profile* original_profile
= NULL;
you can just do profile()->GetOriginalProfile() here. It will do the right thing
in both cases.

http://codereview.chromium.org/9425014/diff/50009/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:695: ->GetDownloadManager();
wrap with the operator at the end of the preceding line:
  foo->
      bar()

http://codereview.chromium.org/9425014/diff/50009/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.h (right):

http://codereview.chromium.org/9425014/diff/50009/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.h:181: bool include_incognito_;
how is this different from include_incognito_ defined in ExtensionFunction?

http://codereview.chromium.org/9425014/diff/50009/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_downloads.json (right):

http://codereview.chromium.org/9425014/diff/50009/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_downloads.json:147: "description":
"True if this download is off-the-record, false otherwise.",
off-the-record is a term internal to chrome. Use "incognito"

http://codereview.chromium.org/9425014/diff/50009/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_downloads.json:277: "incognito": {
I don't think we should expose this to the API. It should be more transparent.
This is how I think it should work:
- If the extension is not enabled in incognito, it can't see incognito
downloads.
- If it is enabled, and uses "spanning" incognito mode, it can see all
downloads.
- If it is enabled, and uses "split" incognito mode, then there will be 2
extension processes:
--- the regular one can only see regular downloads.
--- the incognito one can only see incognito downloads.

[Matt Perry, 2012-05-14 22:58:55]

http://codereview.chromium.org/9425014/diff/50009/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_downloads.json (right):

http://codereview.chromium.org/9425014/diff/50009/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_downloads.json:277: "incognito": {
On 2012/05/14 22:57:11, Matt Perry wrote:
> I don't think we should expose this to the API. It should be more transparent.
> This is how I think it should work:
> - If the extension is not enabled in incognito, it can't see incognito
> downloads.
> - If it is enabled, and uses "spanning" incognito mode, it can see all
> downloads.
> - If it is enabled, and uses "split" incognito mode, then there will be 2
> extension processes:
> --- the regular one can only see regular downloads.
> --- the incognito one can only see incognito downloads.

BTW, ExtensionFunction::include_incognito_ is intended to facilitate this. If
the value is true, then you should include info from both profiles. If the value
is false, then you should only include info from ExtensionFunction::profile_
(which will be either regular or incognito, depending on which process is
calling the function).

[benjhayden, 2012-05-15 17:16:25]

Thanks, Matt!
PTAL

http://codereview.chromium.org/9425014/diff/50009/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/9425014/diff/50009/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:315: if ((browser != NULL) &&
On 2012/05/14 22:57:11, Matt Perry wrote:
> nit: prefer "browser" to "browser != NULL"

Done.

http://codereview.chromium.org/9425014/diff/50009/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:682: Profile* original_profile
= NULL;
On 2012/05/14 22:57:11, Matt Perry wrote:
> you can just do profile()->GetOriginalProfile() here. It will do the right
thing
> in both cases.

Done.

http://codereview.chromium.org/9425014/diff/50009/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:695: ->GetDownloadManager();
On 2012/05/14 22:57:11, Matt Perry wrote:
> wrap with the operator at the end of the preceding line:
>   foo->
>       bar()

Done.

http://codereview.chromium.org/9425014/diff/50009/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.h (right):

http://codereview.chromium.org/9425014/diff/50009/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.h:181: bool include_incognito_;
On 2012/05/14 22:57:11, Matt Perry wrote:
> how is this different from include_incognito_ defined in ExtensionFunction?

It was caller-configurable instead of automatic based on user preferences and
usage. An extension could dynamically specify either incognito:none, or
incognito:only, or let it default to incognito:both.

I changed it to be automatic as you suggested.

http://codereview.chromium.org/9425014/diff/50009/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_downloads.json (right):

http://codereview.chromium.org/9425014/diff/50009/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_downloads.json:147: "description":
"True if this download is off-the-record, false otherwise.",
On 2012/05/14 22:57:11, Matt Perry wrote:
> off-the-record is a term internal to chrome. Use "incognito"

Done.

http://codereview.chromium.org/9425014/diff/50009/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_downloads.json:277: "incognito": {
On 2012/05/14 22:57:11, Matt Perry wrote:
> I don't think we should expose this to the API. It should be more transparent.
> This is how I think it should work:
> - If the extension is not enabled in incognito, it can't see incognito
> downloads.
> - If it is enabled, and uses "spanning" incognito mode, it can see all
> downloads.
> - If it is enabled, and uses "split" incognito mode, then there will be 2
> extension processes:
> --- the regular one can only see regular downloads.
> --- the incognito one can only see incognito downloads.

Done.

http://codereview.chromium.org/9425014/diff/50009/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_downloads.json:277: "incognito": {
On 2012/05/14 22:58:55, Matt Perry wrote:
> On 2012/05/14 22:57:11, Matt Perry wrote:
> > I don't think we should expose this to the API. It should be more
transparent.
> > This is how I think it should work:
> > - If the extension is not enabled in incognito, it can't see incognito
> > downloads.
> > - If it is enabled, and uses "spanning" incognito mode, it can see all
> > downloads.
> > - If it is enabled, and uses "split" incognito mode, then there will be 2
> > extension processes:
> > --- the regular one can only see regular downloads.
> > --- the incognito one can only see incognito downloads.
> 
> BTW, ExtensionFunction::include_incognito_ is intended to facilitate this. If
> the value is true, then you should include info from both profiles. If the
value
> is false, then you should only include info from ExtensionFunction::profile_
> (which will be either regular or incognito, depending on which process is
> calling the function).

Done.

[Matt Perry, 2012-05-15 19:02:22]

http://codereview.chromium.org/9425014/diff/53009/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/9425014/diff/53009/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:98: const char
kIncognitoOnly[] = "only";
This can go away.

http://codereview.chromium.org/9425014/diff/53009/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:320: // this does not appear
to be the case for extension_function_test_utils, so
I'd rather fix extension_function_test_utils if this is broken.

http://codereview.chromium.org/9425014/diff/53009/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:608: } else if
(query_json_field.key() == kIncognito) {
This is obsolete.

[benjhayden, 2012-05-15 21:05:05]

PTAL

http://codereview.chromium.org/9425014/diff/53009/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/9425014/diff/53009/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:98: const char
kIncognitoOnly[] = "only";
On 2012/05/15 19:02:23, Matt Perry wrote:
> This can go away.

Done.

http://codereview.chromium.org/9425014/diff/53009/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:320: // this does not appear
to be the case for extension_function_test_utils, so
On 2012/05/15 19:02:23, Matt Perry wrote:
> I'd rather fix extension_function_test_utils if this is broken.

Found another way around; see download_extension_test.cc.

http://codereview.chromium.org/9425014/diff/53009/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:608: } else if
(query_json_field.key() == kIncognito) {
On 2012/05/15 19:02:23, Matt Perry wrote:
> This is obsolete.

Done.

[Matt Perry, 2012-05-15 21:46:58]

cool, LGTM.

http://codereview.chromium.org/9425014/diff/41012/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/9425014/diff/41012/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:317: GetDownloadManager();
indent += 2

http://codereview.chromium.org/9425014/diff/41012/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:366: return
GetActiveItemInternal(
This can probably fit on 1 line now.

[benjhayden, 2012-05-16 15:01:26]

Thanks!

Committing this afternoon if there are no objections.

http://codereview.chromium.org/9425014/diff/41012/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/9425014/diff/41012/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:317: GetDownloadManager();
On 2012/05/15 21:46:58, Matt Perry wrote:
> indent += 2

Done.

http://codereview.chromium.org/9425014/diff/41012/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:366: return
GetActiveItemInternal(
On 2012/05/15 21:46:58, Matt Perry wrote:
> This can probably fit on 1 line now.

Done.

[commit-bot: I haz the power, 2012-05-16 17:30:05]

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

[commit-bot: I haz the power, 2012-05-16 19:18:25]

Change committed as 137478