experimental.downloads extension api docs

experimental.downloads extension api docs
staged: http://goto.google.com/jojso
BUG=12133
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126688

[cbentzel, 2012-02-23 20:10:04]

do you need someone from extensions team to review this?

[benjhayden, 2012-02-23 20:19:01]

On 2012/02/23 20:10:04, cbentzel wrote:
> do you need someone from extensions team to review this?

That would probably be a good idea. :-)

Mihai?

[cbentzel, 2012-02-24 01:44:04]

Did you just remove/add the nodoc's and use the automated doc generation for the
rest?

This seems fine, but I'm not really a good authority on it.

http://codereview.chromium.org/9452019/diff/1/chrome/common/extensions/docs/t...
File chrome/common/extensions/docs/themes.html (right):

http://codereview.chromium.org/9452019/diff/1/chrome/common/extensions/docs/t...
chrome/common/extensions/docs/themes.html:99: <div class="g-unit g-first"
id="gc-toc" style="">
Do you know why the style attribute got added here and elsewhere?

[Mihai Parparita -not on Chrome, 2012-02-24 01:47:14]

Meggin is a better reviewer for extension docs changes.

[benjhayden, 2012-02-24 14:29:12]

http://codereview.chromium.org/9452019/diff/1/chrome/common/extensions/docs/t...
File chrome/common/extensions/docs/themes.html (right):

http://codereview.chromium.org/9452019/diff/1/chrome/common/extensions/docs/t...
chrome/common/extensions/docs/themes.html:99: <div class="g-unit g-first"
id="gc-toc" style="">
On 2012/02/24 01:44:05, cbentzel wrote:
> Do you know why the style attribute got added here and elsewhere?

No idea. I tried to revert them. I'll dig into it a little.

[mkearney, 2012-02-24 16:01:24]

hey guys, just to give you heads up - i'm in noogler training this week, so
it's likely that i won't get to this review until early next week.

On Thu, Feb 23, 2012 at 5:47 PM, <mihaip@chromium.org> wrote:

> Meggin is a better reviewer for extension docs changes.
>
>
http://codereview.chromium.**org/9452019/<http://codereview.chromium.org/9452...
>

[benjhayden, 2012-02-24 16:21:55]

No worries. Thanks for the heads up!

On Fri, Feb 24, 2012 at 11:01 AM, Meggin Kearney <mkearney@google.com>wrote:

> hey guys, just to give you heads up - i'm in noogler training this week,
> so it's likely that i won't get to this review until early next week.
>
>
> On Thu, Feb 23, 2012 at 5:47 PM, <mihaip@chromium.org> wrote:
>
>> Meggin is a better reviewer for extension docs changes.
>>
>>
http://codereview.chromium.**org/9452019/<http://codereview.chromium.org/9452...
>>
>
>

[Randy Smith (Not in Mondays), 2012-02-24 18:48:03]

First of all, a caveat that I'm really not the right person to review the guts
of how the docs are created; I'm leaving that to others and just reviewing the
output HTML.

Which brings me to a aigh level question: When I do that, I see docs on a *lot*
besides downloads.download (pause/resume/search/onChanged/onCreated/onErased). 
If all we're "releasing" at the moment is downloads.download(), do we really
want all that?

[Randy Smith (Not in Mondays), 2012-02-24 18:50:08]

I also note that we don't have an overview section, but if you click other
experimental sections (specifically
chrome/common/extensions/docs/experimental.devtools.audits.html ) you do get
such a section at the beginning before you dive into the state/type docs.  I
think this would be a really good idea to have for downloads too.

[benjhayden, 2012-02-28 15:50:49]

On 2012/02/24 18:50:08, rdsmith wrote:
> I also note that we don't have an overview section, but if you click other
> experimental sections (specifically
> chrome/common/extensions/docs/experimental.devtools.audits.html ) you do get
> such a section at the beginning before you dive into the state/type docs.  I
> think this would be a really good idea to have for downloads too.

Done.

[benjhayden, 2012-02-28 15:55:10]

On 2012/02/24 18:48:03, rdsmith wrote:
> First of all, a caveat that I'm really not the right person to review the guts
> of how the docs are created; I'm leaving that to others and just reviewing the
> output HTML.
> 
> Which brings me to a aigh level question: When I do that, I see docs on a
*lot*
> besides downloads.download (pause/resume/search/onChanged/onCreated/onErased).

> If all we're "releasing" at the moment is downloads.download(), do we really
> want all that?

It's implemented and is working correctly enough as far as I know, so we want
feedback on it, just not as urgently as we want feedback on download(), right?

[cbentzel, 2012-02-28 20:18:44]

On 2012/02/28 15:55:10, benjhayden_chromium wrote:
> On 2012/02/24 18:48:03, rdsmith wrote:
> > First of all, a caveat that I'm really not the right person to review the
guts
> > of how the docs are created; I'm leaving that to others and just reviewing
the
> > output HTML.
> > 
> > Which brings me to a aigh level question: When I do that, I see docs on a
> *lot*
> > besides downloads.download
(pause/resume/search/onChanged/onCreated/onErased).
> 
> > If all we're "releasing" at the moment is downloads.download(), do we really
> > want all that?
> 
> It's implemented and is working correctly enough as far as I know, so we want
> feedback on it, just not as urgently as we want feedback on download(), right?

I talked to Randy about this. I think we should just restrict to
downloads.download for now. 

Two reasons.

  - It sends a consistent message about what we want feedback on.

  - It lets us know which functionality to release next, because
download.downloads() isn't sufficient for a particular use case.

[benjhayden, 2012-02-28 21:33:29]

PTAL

[asanka, 2012-02-28 21:56:48]

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

http://codereview.chromium.org/9452019/diff/16001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.downloads.json:430: "id":
"DownloadFileIconOptions",
This isn't used anymore.

[benjhayden, 2012-02-28 22:25:39]

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

http://codereview.chromium.org/9452019/diff/16001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.downloads.json:430: "id":
"DownloadFileIconOptions",
On 2012/02/28 21:56:48, asanka wrote:
> This isn't used anymore.

Done.

[Randy Smith (Not in Mondays), 2012-02-29 20:51:54]

Reminder: My review is based on the generated experimental.downloads.html; I'm
leaving the details of transformation to others.  Chris, I'd be interested in
your feedback on my points; several of these touch on whether we're releasing at
the right granularity. 

A couple of points:

* If we aren't advertising any events as part of this API, shouldn't we
eliminate that section? 

* The meaning of "id" for the callback function isn't specified.  

* Related to this, if we specified it, that would be odd since there's nothing
for the user to do with it without search.  In the context of this release,
downloads.download might be best considered a "fire and forget" routine. 

* I don't see anywhere where the errors we defined could be returned; do they
belong in this document?  Maybe another argument to the callback function?  I'm
a little reluctant to do this, because that will mean that we expose some errors
on downloading, but not others (anything that happens after the download is
started would require OnChanged() to be seen).  

* I don't believe that the |args| convention applies to extension API
description; at least, I didn't see it anywhere else I quickly looked for
extension documentation.  Mihai?

* We refer to downloadId in the downloads.download doc, and id in the callback
doc; this should be regularized.

[benjhayden, 2012-03-01 19:21:04]

Doc staged to
http://www/~benjhayden/no_crawl/downloads_docs/experimental.downloads.html


On 2012/02/29 20:51:54, rdsmith wrote:
> Reminder: My review is based on the generated experimental.downloads.html; I'm
> leaving the details of transformation to others.  Chris, I'd be interested in
> your feedback on my points; several of these touch on whether we're releasing
at
> the right granularity. 
> 
> A couple of points:
> 
> * If we aren't advertising any events as part of this API, shouldn't we
> eliminate that section? 

I don't see any way to do that.

> 
> * The meaning of "id" for the callback function isn't specified.  

Done.

> 
> * Related to this, if we specified it, that would be odd since there's nothing
> for the user to do with it without search.  In the context of this release,
> downloads.download might be best considered a "fire and forget" routine. 

There are many odd things. It ain't broke, so I don't intend to fix it.

> 
> * I don't see anywhere where the errors we defined could be returned; do they
> belong in this document?  Maybe another argument to the callback function? 
I'm
> a little reluctant to do this, because that will mean that we expose some
errors
> on downloading, but not others (anything that happens after the download is
> started would require OnChanged() to be seen). 

Clarified that downloads.ERROR_* may be returned via
chrome.extensions.lastError.

> 
> * I don't believe that the |args| convention applies to extension API
> description; at least, I didn't see it anywhere else I quickly looked for
> extension documentation.  Mihai?

I don't see |args| anywhere in experimental.downloads.json. Can you be more
specific?

> 
> * We refer to downloadId in the downloads.download doc, and id in the callback
> doc; this should be regularized.

Standardized on downloadId.

[cbentzel, 2012-03-01 20:13:59]

On Thu, Mar 1, 2012 at 2:21 PM, <benjhayden@chromium.org> wrote:

> Doc staged to
> http://www/~benjhayden/no_**crawl/downloads_docs/**
>
experimental.downloads.html<http://www/~benjhayden/no_crawl/downloads_docs/experimental.downloads.html>
>

Thanks for staging, very helpful. A few issues:

  - Other documentation don't use |variable_name|, there's markup which
seems to make the text fixed-width on green on other pages.
  - The ERROR_GENERIC is likely different than we want to ship with.
  - It doesn't see a properties section on other APIs. Is this because they
don't have any properties as part of the API, or because they aren't
documented?
  - Can you remove the Events section?
  - Do we support the binaryValue option of the headers object?
  - Text I've seen for other callbacks is "The callback *parameter* should
specify a function that looks like this:", perhaps this should match? You
do explicitly specify that the callback is optional.
  - Probably some other consistency issues but I'm too lazy to look right
now.


>

> On 2012/02/29 20:51:54, rdsmith wrote:
>
>> Reminder: My review is based on the generated
>> experimental.downloads.html; I'm
>> leaving the details of transformation to others.  Chris, I'd be
>> interested in
>> your feedback on my points; several of these touch on whether we're
>> releasing
>>
> at
>
>> the right granularity.
>>
>
>  A couple of points:
>>
>
>  * If we aren't advertising any events as part of this API, shouldn't we
>> eliminate that section?
>>
>
> I don't see any way to do that.
>
>
>
>  * The meaning of "id" for the callback function isn't specified.
>>
>
> Done.
>
>
>
>  * Related to this, if we specified it, that would be odd since there's
>> nothing
>> for the user to do with it without search.  In the context of this
>> release,
>> downloads.download might be best considered a "fire and forget" routine.
>>
>
> There are many odd things. It ain't broke, so I don't intend to fix it.
>
>
>
>  * I don't see anywhere where the errors we defined could be returned; do
>> they
>> belong in this document?  Maybe another argument to the callback function?
>>
> I'm
>
>> a little reluctant to do this, because that will mean that we expose some
>>
> errors
>
>> on downloading, but not others (anything that happens after the download
>> is
>> started would require OnChanged() to be seen).
>>
>
> Clarified that downloads.ERROR_* may be returned via
> chrome.extensions.lastError.
>
>
>
>  * I don't believe that the |args| convention applies to extension API
>> description; at least, I didn't see it anywhere else I quickly looked for
>> extension documentation.  Mihai?
>>
>
> I don't see |args| anywhere in experimental.downloads.json. Can you be more
> specific?
>
>
>
>  * We refer to downloadId in the downloads.download doc, and id in the
>> callback
>> doc; this should be regularized.
>>
>
> Standardized on downloadId.
>
>
http://codereview.chromium.**org/9452019/<http://codereview.chromium.org/9452...
>

[benjhayden, 2012-03-01 21:17:03]

On 2012/03/01 20:13:59, cbentzel wrote:
> On Thu, Mar 1, 2012 at 2:21 PM, <mailto:benjhayden@chromium.org> wrote:
> 
> > Doc staged to
> > http://www/%7Ebenjhayden/no_**crawl/downloads_docs/**
> >
>
experimental.downloads.html<http://www/~benjhayden/no_crawl/downloads_docs/experimental.downloads.html>
> >
> 
> Thanks for staging, very helpful. A few issues:
> 
>   - Other documentation don't use |variable_name|, there's markup which
> seems to make the text fixed-width on green on other pages.

Done.

>   - The ERROR_GENERIC is likely different than we want to ship with.

It's still in experimental, so we can still change it. We can revisit how to
spell it when we move it out of experimental. That's part of the reason that
it's a property in the first place: so that extensions can refer to it without
caring about its value.

>   - It doesn't see a properties section on other APIs. Is this because they
> don't have any properties as part of the API, or because they aren't
> documented?

Because they don't have a properties section in their json. If there's a
properties section but all of the properties are nodoc, then I suspect that
there'd be an empty properties section like there's an empty Events section
here.

>   - Can you remove the Events section?

No. See my reply to Randy's comment to the same effect.
The "events" section in the json is an array of objects (all of which have
nodoc) so I can't set nodoc on the events section itself. build.py creates the
Events section automatically. I could try to make build.py omit the events
section if all events have nodoc, but it seems like a job for another CL.

>   - Do we support the binaryValue option of the headers object?

Not yet. I'll add that to my auth basic CL.

>   - Text I've seen for other callbacks is "The callback *parameter* should
> specify a function that looks like this:", perhaps this should match? You
> do explicitly specify that the callback is optional.

Are you referring to this text?
"If you specify the callback parameter, it should specify a function that looks
like this:"
It's automatically generated.

>   - Probably some other consistency issues but I'm too lazy to look right
> now.
> 
> 
> >
> 
> > On 2012/02/29 20:51:54, rdsmith wrote:
> >
> >> Reminder: My review is based on the generated
> >> experimental.downloads.html; I'm
> >> leaving the details of transformation to others.  Chris, I'd be
> >> interested in
> >> your feedback on my points; several of these touch on whether we're
> >> releasing
> >>
> > at
> >
> >> the right granularity.
> >>
> >
> >  A couple of points:
> >>
> >
> >  * If we aren't advertising any events as part of this API, shouldn't we
> >> eliminate that section?
> >>
> >
> > I don't see any way to do that.
> >
> >
> >
> >  * The meaning of "id" for the callback function isn't specified.
> >>
> >
> > Done.
> >
> >
> >
> >  * Related to this, if we specified it, that would be odd since there's
> >> nothing
> >> for the user to do with it without search.  In the context of this
> >> release,
> >> downloads.download might be best considered a "fire and forget" routine.
> >>
> >
> > There are many odd things. It ain't broke, so I don't intend to fix it.
> >
> >
> >
> >  * I don't see anywhere where the errors we defined could be returned; do
> >> they
> >> belong in this document?  Maybe another argument to the callback function?
> >>
> > I'm
> >
> >> a little reluctant to do this, because that will mean that we expose some
> >>
> > errors
> >
> >> on downloading, but not others (anything that happens after the download
> >> is
> >> started would require OnChanged() to be seen).
> >>
> >
> > Clarified that downloads.ERROR_* may be returned via
> > chrome.extensions.lastError.
> >
> >
> >
> >  * I don't believe that the |args| convention applies to extension API
> >> description; at least, I didn't see it anywhere else I quickly looked for
> >> extension documentation.  Mihai?
> >>
> >
> > I don't see |args| anywhere in experimental.downloads.json. Can you be more
> > specific?
> >
> >
> >
> >  * We refer to downloadId in the downloads.download doc, and id in the
> >> callback
> >> doc; this should be regularized.
> >>
> >
> > Standardized on downloadId.
> >
> >
>
http://codereview.chromium.**org/9452019/%3Chttp://codereview.chromium.org/94...>
> >

[benjhayden, 2012-03-01 21:18:28]

Shortified the stage link:
http://goto.google.com/jojso

[mkearney, 2012-03-02 01:56:06]

Do you want me to review the samples too or just the docs?

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

http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.downloads.json:434: "description":
"Download a URL. If the URL uses the HTTP[S] protocol, then the request will
include all cookies currently set for its hostname. If the download started
successfully, <code>callback</code> will be called with the new DownloadItem’s
<code>id</code>. If there was an error starting the download, then
<code>callback</code> will be called with <code>downloadId=undefined</code> and
chrome.extension.lastError will be set. If the URL’s hostname is not specified
in the <code>permissions</code> section of the extension’s manifest, then the
<code>chrome.extensions.lastError</code> object will indicate that the extension
does not have permission to access that hostname. <code>downloads.ERROR_*</code>
are some of the errors that may be returned via
<code>chrome.extensions.lastError</code>.",
I'd like to see this sentence move up to the top of the API doc (maybe in its
own section called something like 'Specify URLs in Permissions'.

http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.downloads.json:468: "description":
"post body",
This is missing a full-stop. Also, out of curiosity, what sort of stuff might go
in the post body string?

http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/do...
File chrome/common/extensions/docs/static/experimental.downloads.html (right):

http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/do...
chrome/common/extensions/docs/static/experimental.downloads.html:1: The
downloads API allows you to programmatically initiate downloads, examine and
monitor properties of downloads, and manipulate downloads.
This should be wrapped in <p> tag.

http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/do...
chrome/common/extensions/docs/static/experimental.downloads.html:1: The
downloads API allows you to programmatically initiate downloads, examine and
monitor properties of downloads, and manipulate downloads.
Is it too early to include an example of a use case here?

'Use this API to download URLs ... for example ...'

It might be a little early still.

http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/do...
chrome/common/extensions/docs/static/experimental.downloads.html:1: The
downloads API allows you to programmatically initiate downloads, examine and
monitor properties of downloads, and manipulate downloads.
Can we move the information about the need to include any URL being downloaded
in the extension's manifest in the API static doc rather than in the method
description?

http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/do...
chrome/common/extensions/docs/static/experimental.downloads.html:1: The
downloads API allows you to programmatically initiate downloads, examine and
monitor properties of downloads, and manipulate downloads.
Do you want me to review the samples, or is it too early? If the samples are
going out at the same time, then what about including a example section with a
link to those samples?

[benjhayden, 2012-03-02 16:27:06]

Thanks!
PTAL

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

http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.downloads.json:434: "description":
"Download a URL. If the URL uses the HTTP[S] protocol, then the request will
include all cookies currently set for its hostname. If the download started
successfully, <code>callback</code> will be called with the new DownloadItem’s
<code>id</code>. If there was an error starting the download, then
<code>callback</code> will be called with <code>downloadId=undefined</code> and
chrome.extension.lastError will be set. If the URL’s hostname is not specified
in the <code>permissions</code> section of the extension’s manifest, then the
<code>chrome.extensions.lastError</code> object will indicate that the extension
does not have permission to access that hostname. <code>downloads.ERROR_*</code>
are some of the errors that may be returned via
<code>chrome.extensions.lastError</code>.",
On 2012/03/02 01:56:07, mkearney wrote:
> I'd like to see this sentence move up to the top of the API doc (maybe in its
> own section called something like 'Specify URLs in Permissions'.

Which sentence? All of the sentences are on the same line. :-)

http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.downloads.json:468: "description":
"post body",
On 2012/03/02 01:56:07, mkearney wrote:
> This is missing a full-stop. Also, out of curiosity, what sort of stuff might
go
> in the post body string?

Usually URL-encoded form data, but it could be anything.
http://www.jmarshall.com/easy/http/#postmethod
http://tools.ietf.org/html/rfc2616#section-9.5

http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/do...
File chrome/common/extensions/docs/static/experimental.downloads.html (right):

http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/do...
chrome/common/extensions/docs/static/experimental.downloads.html:1: The
downloads API allows you to programmatically initiate downloads, examine and
monitor properties of downloads, and manipulate downloads.
On 2012/03/02 01:56:07, mkearney wrote:
> This should be wrapped in <p> tag.

Done.

http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/do...
chrome/common/extensions/docs/static/experimental.downloads.html:1: The
downloads API allows you to programmatically initiate downloads, examine and
monitor properties of downloads, and manipulate downloads.
On 2012/03/02 01:56:07, mkearney wrote:
> Can we move the information about the need to include any URL being downloaded
> in the extension's manifest in the API static doc rather than in the method
> description? 

Can I ask why? There are going to be several other methods, and the url
permission restriction will only apply to download(). I thought that this static
file should apply to the API as a whole.

http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/do...
chrome/common/extensions/docs/static/experimental.downloads.html:1: The
downloads API allows you to programmatically initiate downloads, examine and
monitor properties of downloads, and manipulate downloads.
On 2012/03/02 01:56:07, mkearney wrote:
> Do you want me to review the samples, or is it too early? If the samples are
> going out at the same time, then what about including a example section with a
> link to those samples?

Done.
Mihai reviewed download_links on its way in, but feel free to review it if you
like.

http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/do...
chrome/common/extensions/docs/static/experimental.downloads.html:1: The
downloads API allows you to programmatically initiate downloads, examine and
monitor properties of downloads, and manipulate downloads.
On 2012/03/02 01:56:07, mkearney wrote:
> Is it too early to include an example of a use case here?
> 
> 'Use this API to download URLs ... for example ...'
> 
> It might be a little early still.

There's a sample extension.

[benjhayden, 2012-03-06 19:14:56]

Ping?

[mkearney, 2012-03-06 19:27:06]

Hey, Ben

I'm in meetings this morning, so won't be able to go through review
comments until after lunch.

Talk to you soon.

Meggin

On Tue, Mar 6, 2012 at 11:14 AM, <benjhayden@chromium.org> wrote:

> Ping?
>
>
http://codereview.chromium.**org/9452019/<http://codereview.chromium.org/9452...
>

[Randy Smith (Not in Mondays), 2012-03-07 00:00:29]

Next round:

* mkearney@: Both cbentzel@ & I have expressed discomfort about shipping with an
empty "Events" section, which is apparently hard to fix given the current
structure (we have events, they're just marked "nodoc").  Do you think it's a
problem to have an extension API shipping with a null events section, and if so,
any idea how we should fix it?

* The overview section refers to functionality we aren't exporting in this
release ("examine and monitor properties of downloads, and manipulate
downloads.")

* I'd like to see a bit more detail on inter-argument restrictions;
specifically, can filename and saveAs both be specified, and what happens if
they are?  Same for a "GET" method and a post body?  (If it's just "request
body" we should say that rather than "post body".).  Same for value and
binaryValue in the http header list.

[benjhayden, 2012-03-07 16:07:57]

On 2012/03/07 00:00:29, rdsmith wrote:
> Next round:
> 
> * mkearney@: Both cbentzel@ & I have expressed discomfort about shipping with
an
> empty "Events" section, which is apparently hard to fix given the current
> structure (we have events, they're just marked "nodoc").  Do you think it's a
> problem to have an extension API shipping with a null events section, and if
so,
> any idea how we should fix it?

I figured out how to fix it, but I'll need to find the right reviewer, and I'm
not sure if this is the right CL for it. PTAL.

> 
> * The overview section refers to functionality we aren't exporting in this
> release ("examine and monitor properties of downloads, and manipulate
> downloads.")

Changed tense.

> 
> * I'd like to see a bit more detail on inter-argument restrictions;
> specifically, can filename and saveAs both be specified, and what happens if

Done.

> they are?  Same for a "GET" method and a post body?  (If it's just "request
> body" we should say that rather than "post body".).  Same for value and

My sense of how people expect HTTP to work, both with respect to this issue
specifically and in general, is basically this:
http://stackoverflow.com/questions/978061/http-get-with-request-body
So, if a developer expects to be able to send a body with a GET request, then
most servers, proxies, and other clients will disappoint and surprise them, not
just download().
Is there a specific reason to document the expected behavior for this edge case?

> binaryValue in the http header list.

The HttpHeader section says "either value or binaryValue." Which details in
particular are lacking?

[benjhayden, 2012-03-07 16:22:38]

kalman: you were the last person to touch api_template.html. Are you the right
person to review this? I changed 3 lines of javascript to hide sections (Events,
Methods, Types) if all their members are nodoc. I can split it out into a
separate CL if you want, though I'm not sure why a docs CL would be reverted.
Please also let me know if there are any tests that I should run besides
build.py.
Thanks!

[Randy Smith (Not in Mondays), 2012-03-08 18:32:58]

LGTM (though some response to comments below; no actions requested).

On 2012/03/07 16:07:57, benjhayden_chromium wrote:
> On 2012/03/07 00:00:29, rdsmith wrote:
> > Next round:
> > 
> > * mkearney@: Both cbentzel@ & I have expressed discomfort about shipping
with
> an
> > empty "Events" section, which is apparently hard to fix given the current
> > structure (we have events, they're just marked "nodoc").  Do you think it's
a
> > problem to have an extension API shipping with a null events section, and if
> so,
> > any idea how we should fix it?
> 
> I figured out how to fix it, but I'll need to find the right reviewer, and I'm
> not sure if this is the right CL for it. PTAL.
> 
> > 
> > * The overview section refers to functionality we aren't exporting in this
> > release ("examine and monitor properties of downloads, and manipulate
> > downloads.")
> 
> Changed tense.
> 
> > 
> > * I'd like to see a bit more detail on inter-argument restrictions;
> > specifically, can filename and saveAs both be specified, and what happens if
> 
> Done.
> 
> > they are?  Same for a "GET" method and a post body?  (If it's just "request
> > body" we should say that rather than "post body".).  Same for value and
> 
> My sense of how people expect HTTP to work, both with respect to this issue
> specifically and in general, is basically this:
> http://stackoverflow.com/questions/978061/http-get-with-request-body
> So, if a developer expects to be able to send a body with a GET request, then
> most servers, proxies, and other clients will disappoint and surprise them,
not
> just download().

Sure, completely agreed.  I just dislike underspecified argument documentation;
something like "Will be ignored if the method is not 'POST' would fix my
concern.  But it's not a big deal--I agree it's pretty obvious what is meant
from context.

> Is there a specific reason to document the expected behavior for this edge
case?
> 
> > binaryValue in the http header list.
> 
> The HttpHeader section says "either value or binaryValue." Which details in
> particular are lacking?

My ability to read text closely?  :-}  It's fine as it is.

[benjhayden, 2012-03-09 19:04:14]

kalman, mkearney: PTAL

[mkearney, 2012-03-09 21:15:05]

Hi, Ben

I did a second review from the latest source, so I'm just going to put the
review comments in this email, rather than adding them to the files
themselves.

Here's review comments for http://codereview.chromium.**
org/9452019/diff/25006/chrome/**common/extensions/docs/static/**
experimental.downloads.html<http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/docs/static/experimental.downloads.html#newcode1>

Suggested rewrite for second paragraph in the introduction:

The downloads API allows you to programmatically initiate downloads. In the
future, you will also be able to monitor and manipulate downloads.

I would suggest adding a manifest section which is similar to the
'infobars' experimental API. The section would look something like this:

Manifest

The downloads API is currently experimental, so you must declare the
"experimental" permission to use it. Also, you must specify the hostname of
any URLs to be downloaded. For example:

{
  "name": "Download Selected Links",
  "description": "Select links on a page and download them.",
  "version": "0.1",
  "permissions": [
    "experimental", "http://*/*", "https://*/*"
  ],

}


If the URL’s hostname is not specified in the permissions, then the
chrome.extensions.lastError object will indicate that the extension does
not have permission to access that hostname. downloads.ERROR_* are some of
the errors that may be returned.

In the above text, you could also put in a link to the errors for
'downloads.ERROR_*.

Here's review comments for the json:

http://codereview.chromium.**org/9452019/diff/25006/chrome/**
common/extensions/api/**experimental.downloads.json<http://codereview.chromium.org/9452019/diff/25006/chrome/common/extensions/api/experimental.downloads.json#newcode468>

A suggested rewrite for download method (assuming some text moves into the
manifest):

download

Download a URL. If the URL uses the HTTP[S] protocol, then the request will
include all cookies currently set for its hostname. If the download started
successfully, callback will be called with the new DownloadItem’s id. If
there was an error starting the download, then callback will be called with
downloadId=undefined and chrome.extension.lastError will be set. If both
filename and saveAs are specified, then the Save As dialog will be
displayed, pre-populated with the specified filename.

(I got rid of 'deduplicated' too in method above.)

Hopefully this is clearer.

Meggin

On Fri, Mar 9, 2012 at 11:04 AM, <benjhayden@chromium.org> wrote:

> kalman, mkearney: PTAL
>
>
http://codereview.chromium.**org/9452019/<http://codereview.chromium.org/9452...
>

[not at google - send to devlin, 2012-03-09 23:07:09]

LGTM, thanks.

[benjhayden, 2012-03-13 14:49:34]

mkearney PTAL.
(docs rebuilt and restaged.)

[mkearney, 2012-03-13 17:51:51]

one last very small comment...

http://codereview.chromium.org/9452019/diff/65015/chrome/common/extensions/do...
File chrome/common/extensions/docs/static/experimental.downloads.html (right):

http://codereview.chromium.org/9452019/diff/65015/chrome/common/extensions/do...
chrome/common/extensions/docs/static/experimental.downloads.html:16: ]}</pre>
Very small comment to move } down a line. It looks a little better and makes it
a little easier for developers to copy/paste.

[benjhayden, 2012-03-13 18:16:13]

PTAL
docs rebuilt and restaged

http://codereview.chromium.org/9452019/diff/65015/chrome/common/extensions/do...
File chrome/common/extensions/docs/static/experimental.downloads.html (right):

http://codereview.chromium.org/9452019/diff/65015/chrome/common/extensions/do...
chrome/common/extensions/docs/static/experimental.downloads.html:16: ]}</pre>
On 2012/03/13 17:51:51, mkearney wrote:
> Very small comment to move } down a line. It looks a little better and makes
it
> a little easier for developers to copy/paste.

Done.

[mkearney, 2012-03-13 18:20:56]

lgtm

[commit-bot: I haz the power, 2012-03-13 18:24:52]

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

[commit-bot: I haz the power, 2012-03-13 18:24:59]

Presubmit check for 9452019-69005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

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

Presubmit checks took 1.3s to calculate.

[Mihai Parparita -not on Chrome, 2012-03-13 18:31:10]

OWNERS rubberstamp LGTM

[commit-bot: I haz the power, 2012-03-13 18:43:55]

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

[commit-bot: I haz the power, 2012-03-13 19:13:38]

Try job failure for 9452019-69005 (retry) on linux_rel for steps
"crypto_unittests, cacheinvalidation_unittests, printing_unittests,
jingle_unittests, ui_tests, remoting_unittests, browser_tests, net_unittests,
unit_tests, gpu_unittests, base_unittests, media_unittests, gfx_unittests,
ipc_tests, sql_unittests, safe_browsing_tests, content_unittests,
sync_unit_tests".
It's a second try, previously, steps "crypto_unittests,
cacheinvalidation_unittests, printing_unittests, jingle_unittests, ui_tests,
remoting_unittests, browser_tests, net_unittests, unit_tests, gpu_unittests,
base_unittests, media_unittests, gfx_unittests, ipc_tests, sql_unittests,
safe_browsing_tests, content_unittests, sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-03-14 15:13:25]

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

[commit-bot: I haz the power, 2012-03-14 16:03:42]

Try job failure for 9452019-76001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-03-14 17:10:04]

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

[commit-bot: I haz the power, 2012-03-14 18:32:08]

Change committed as 126688