Added chrome.printerProvider API definitions.

Added chrome.printerProvider API definitions.

BUG=408772
Committed: https://crrev.com/28538857f02e3c96b90cabdc19ed6e4f0c39c0f6
Cr-Commit-Position: refs/heads/master@{#302326}

[Vitaly Buka (NO REVIEWS), 2014-10-29 08:19:22]

vitalybuka@chromium.org changed reviewers:
+ kalman@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-10-29 08:25:00]

Please review API.

[not at google - send to devlin, 2014-10-29 17:13:56]

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/printer_provider.idl (right):

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:6: // which can be used from
browser print preview.
A better comment would be the one that you have in the API proposal.

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:9: // success, <code>OK</code>
should be used.
Consider adding comments to each enum value, and not including this last
sentence.

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:25: // Human readable
description of printers.
printers -> printer.

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:26: DOMString?
printerDescription;
The "printer" in these names looks redundant given the type is already named
PrinterInfo. Could be just id, name, description.

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:34: // Event fired when print
preview requests printer l
"l"?

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:46: 
extra blank line

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:57: ArrayBuffer document,
Rather than long argument lists, use an object.

(Consider using objects for *every* parameter really, better for backwards
compatibility)

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/permissions/chrome_api_permissions.cc (right):

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/permissions/chrome_api_permissions.cc:370: "proxy",
I presume... all of this reformatting was clang-format? Isn't it supposed to
only format lines that you changed? Not a huge deal, the reformat is an
improvement IMO.

[Vitaly Buka (NO REVIEWS), 2014-10-30 00:43:43]

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/printer_provider.idl (right):

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:6: // which can be used from
browser print preview.
On 2014/10/29 17:13:56, kalman wrote:
> A better comment would be the one that you have in the API proposal.

Changed, but I am not sure which commend you are referring to.

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:9: // success, <code>OK</code>
should be used.
On 2014/10/29 17:13:56, kalman wrote:
> Consider adding comments to each enum value, and not including this last
> sentence.

Done.

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:25: // Human readable
description of printers.
On 2014/10/29 17:13:56, kalman wrote:
> printers -> printer.

Done.

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:26: DOMString?
printerDescription;
On 2014/10/29 17:13:56, kalman wrote:
> The "printer" in these names looks redundant given the type is already named
> PrinterInfo. Could be just id, name, description.

Done.

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:34: // Event fired when print
preview requests printer l
On 2014/10/29 17:13:56, kalman wrote:
> "l"?

Done.

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:46: 
On 2014/10/29 17:13:56, kalman wrote:
> extra blank line

Done.

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:57: ArrayBuffer document,
On 2014/10/29 17:13:56, kalman wrote:
> Rather than long argument lists, use an object.
> 
> (Consider using objects for *every* parameter really, better for backwards
> compatibility)

Done for this event.
No sure if about onGetCapabilityRequested.

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/permissions/chrome_api_permissions.cc (right):

https://codereview.chromium.org/530593002/diff/40001/chrome/common/extensions...
chrome/common/extensions/permissions/chrome_api_permissions.cc:370: "proxy",
Have not notices this uploading the last CL.
I'd rather split this into a separate CL.

On 2014/10/29 17:13:56, kalman wrote:
> I presume... all of this reformatting was clang-format? Isn't it supposed to
> only format lines that you changed? Not a huge deal, the reformat is an
> improvement IMO.

[not at google - send to devlin, 2014-10-30 14:51:29]

lgtm, with some more comment-comments.

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/api/printer_provider.idl (right):

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:6: // printers, get printer
capabilities and to submit print jobs.
Yes, thanks, this is a much better description (I didn't actually look at your
API proposal description - but since I understood it, but not this comment, I
figured there was something better in there somewhere).

Grammar nits:
- "This API exposes" or just "Exposes". Either one, but not "API provides".
- "Extensions" not "extension".

And ideally you'd imply more strongly that it's not the *Extension* that is
discovering printers (and so forth), but the *print manager* queries this API to
discover printers (and so forth).

All concisely :) note that this description is what appears in the table at the
top of the API reference pages. E.g. see
https://developer.chrome.com/extensions/storage - which I think is too brief an
explanation - but something too long would look odd as well. 

Also note that paragraph breaks in this comment will be respected in that
description.

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:17: // settings from print
ticket. It's recomeneded to ignore them and don't
recommended

and perhaps "ignore this failure and gracefully fail" rather than "ignore them
and don't fail if possible"?

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:21: // Document is invalid. If
data is corrupted or format is incompartible with
"If data is corrupted..." -> "For example, data may be corrupted or the format
incompatible with the Extension".

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:26: // Printer description for
<code>onGetPrintersRequested()</code> event.
Rather than <code>onGetPrintersRequested</code> use
$(ref:onGetPrintersRequested).

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:28: // Unique id of printer.
"ID" everywhere, not "id".

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:38: // Parameters of
|onPrintRequested|.
Likewise.

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:60: // Event fired when print
preview requests printers provided by extension.
You mention "preview" in each of these event descriptions, but I thought it was
actually just general printing, not limited to print preview? Could you phrase
each of these like "Event fired when the print manager requests the printers
provided by the Extension", and so on?

[Vitaly Buka (NO REVIEWS), 2014-10-30 17:57:54]

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/api/printer_provider.idl (right):

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:6: // printers, get printer
capabilities and to submit print jobs.
Done. 

On 2014/10/30 14:51:28, kalman wrote:
> Yes, thanks, this is a much better description (I didn't actually look at your
> API proposal description - but since I understood it, but not this comment, I
> figured there was something better in there somewhere).
> 
> Grammar nits:
> - "This API exposes" or just "Exposes". Either one, but not "API provides".
> - "Extensions" not "extension".
> 
> And ideally you'd imply more strongly that it's not the *Extension* that is
> discovering printers (and so forth), but the *print manager* queries this API
to
> discover printers (and so forth).
> 
> All concisely :) note that this description is what appears in the table at
the
> top of the API reference pages. E.g. see
> https://developer.chrome.com/extensions/storage - which I think is too brief
an
> explanation - but something too long would look odd as well. 
> 
> Also note that paragraph breaks in this comment will be respected in that
> description.

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:17: // settings from print
ticket. It's recomeneded to ignore them and don't
On 2014/10/30 14:51:28, kalman wrote:
> recommended
> 
> and perhaps "ignore this failure and gracefully fail" rather than "ignore them
> and don't fail if possible"?

Done.

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:21: // Document is invalid. If
data is corrupted or format is incompartible with
On 2014/10/30 14:51:28, kalman wrote:
> "If data is corrupted..." -> "For example, data may be corrupted or the format
> incompatible with the Extension".

Done.

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:26: // Printer description for
<code>onGetPrintersRequested()</code> event.
On 2014/10/30 14:51:28, kalman wrote:
> Rather than <code>onGetPrintersRequested</code> use
> $(ref:onGetPrintersRequested).

Done.

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:28: // Unique id of printer.
On 2014/10/30 14:51:28, kalman wrote:
> "ID" everywhere, not "id".

Done.

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:38: // Parameters of
|onPrintRequested|.
On 2014/10/30 14:51:28, kalman wrote:
> Likewise.

Done.

https://codereview.chromium.org/530593002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/printer_provider.idl:60: // Event fired when print
preview requests printers provided by extension.
On Chrome OS print preview is going to be the only entry point for printing.
Still I am OK with abstract "print manager".

On 2014/10/30 14:51:28, kalman wrote:
> You mention "preview" in each of these event descriptions, but I thought it
was
> actually just general printing, not limited to print preview? Could you phrase
> each of these like "Event fired when the print manager requests the printers
> provided by the Extension", and so on?

[Vitaly Buka (NO REVIEWS), 2014-10-31 19:30:50]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-10-31 19:31:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/530593002/100001

[commit-bot: I haz the power, 2014-10-31 19:31:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/530593002/100001

[commit-bot: I haz the power, 2014-10-31 21:08:17]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-10-31 21:09:36]

Patchset 6 (id:??) landed as
https://crrev.com/28538857f02e3c96b90cabdc19ed6e4f0c39c0f6
Cr-Commit-Position: refs/heads/master@{#302326}