Implement a JavaScript API for document scanning

Implement a JavaScript API for document scanning

Provide an API for callers to list available document scanners,
and to acquire a single-page scan from one such device.  This
currently takes advantage of the lorgnette scanning API in
ChromeOS but can be extended to use native document acquisition
APIs on other platforms.

BUG=375334
R=asargent@chromium.org, mef@chromium.org, asvitkine@chromium.org

Committed: https://crrev.com/ea73a785e4d97de63c928f0a574d25beb6236aae
Cr-Commit-Position: refs/heads/master@{#302122}

[Alexei Svitkine (slow), 2014-05-20 20:50:41]

https://codereview.chromium.org/286933006/diff/1/extensions/browser/extension...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/286933006/diff/1/extensions/browser/extension...
extensions/browser/extension_function_histogram_value.h:837: DOCUMENT_SCAN_SCAN,
Please also update histograms.xml.

https://codereview.chromium.org/286933006/diff/1/extensions/common/permission...
File extensions/common/permissions/permission_message.h (right):

https://codereview.chromium.org/286933006/diff/1/extensions/common/permission...
extensions/common/permissions/permission_message.h:88: kDocumentScan,
Please also update histograms.xml and add also a comment here about it similar
to the one in extension_function_histogram_value.h.

[Paul Stewart, 2014-05-21 01:09:29]

Thanks for the review!  PTAL.

https://codereview.chromium.org/286933006/diff/1/extensions/browser/extension...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/286933006/diff/1/extensions/browser/extension...
extensions/browser/extension_function_histogram_value.h:837: DOCUMENT_SCAN_SCAN,
On 2014/05/20 20:50:42, Alexei Svitkine wrote:
> Please also update histograms.xml.

Done.

https://codereview.chromium.org/286933006/diff/1/extensions/common/permission...
File extensions/common/permissions/permission_message.h (right):

https://codereview.chromium.org/286933006/diff/1/extensions/common/permission...
extensions/common/permissions/permission_message.h:88: kDocumentScan,
On 2014/05/20 20:50:42, Alexei Svitkine wrote:
> Please also update histograms.xml and add also a comment here about it similar
> to the one in extension_function_histogram_value.h.

Done.  It triggered a pre-submit warnings so please double check me in case I
did something wrong.

[Alexei Svitkine (slow), 2014-05-21 06:30:33]

metrics LGTM, thanks!

[Paul Stewart, 2014-05-21 15:55:19]

Thanks.  I have more work to do, and just noticed that I still have logging at
the WARNING level, which isn't good.  I won't be touching metrics from here on
though.

[asargent_no_longer_on_chrome, 2014-05-21 18:06:24]

Cool API!

Have you done the API proposal process yet?
(http://dev.chromium.org/developers/design-documents/extensions/proposed-chang...)

https://codereview.chromium.org/286933006/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/document_scan.idl (right):

https://codereview.chromium.org/286933006/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/document_scan.idl:11: // The name of an connected
scanner.
nit: "an" -> "a" (I think - if you are a grammar wizard feel free to let me know
I'm wrong along with optional Strunk and White citation :) )

https://codereview.chromium.org/286933006/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/document_scan.idl:26: DOMString? mode;
nit: instead of just using a string, it would be better to define a named enum
for this. E.g.

enum ScanMode { color, gray, lineart };

dictionary ScanOptions {
  ScanMode? mode;

  ...
};


That way you'll get parameter validation at runtime for free from the extension
API plumbing.

https://codereview.chromium.org/286933006/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/document_scan.idl:38: callback ScanCallback = void
(boolean result, ArrayBuffer image_data);
The prevailing style for a long time has been to avoid using explicit boolean
success/failure parameters in callbacks, instead favoring the built in
"chrome.runtime.lastError" which gets auto-populated when your
AsyncApiFunction::Respond override function returns false (and any
developer-readable error message that you put into your error_ member variable
will be available to the JS as chrome.runtime.lastError.message). 

(We've talked about perhaps moving to JS promises in the future for callbacks,
but AFAIK we aren't there yet).

https://codereview.chromium.org/286933006/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/document_scan.idl:46: // Performs a document scan. 
On success, ths image data will be stored in
typo: "ths"

https://codereview.chromium.org/286933006/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/document_scan.idl:48: // sent to the callback.
It seems slightly strange to me to have both a file generated in the downloads
folder and have the data returned back in an array buffer. Perhaps it would make
sense to make this an option either with an explicit parameter to this method,
or (if for instance there was a default that we think most developers would want
most of the time) as a member developers can set in the ScanOptions dictionary. 

Also, when developers choose the "scan to a file" option, it might be cool to be
able to return to them the download id that they could then turn around and call
chrome.downloads.show on
(https://developer.chrome.com/extensions/downloads#method-show). I'm not sure if
calling that method requires the "downloads" permission, but it seems like it
would not necessarily have to. Or we could add an optional flag to this method
to just tell the system that chrome itself should show the download in the
Downloads folder when the scan is done. 

(Apologies if you've already had API design discussions about all this with
other folks from the apps/extensions team, and this is bikeshedding something
there was already a consensus on)

https://codereview.chromium.org/286933006/diff/20001/extensions/common/permis...
File extensions/common/permissions/api_permission.h (right):

https://codereview.chromium.org/286933006/diff/20001/extensions/common/permis...
extensions/common/permissions/api_permission.h:180: kDocumentScan,
nit: I think these are supposed to be sorted (can you also fix up the other ones
at the bottom here too while you're in here?)

[Paul Stewart, 2014-05-22 18:37:50]

Thank you for all your comments Antony!  I believe I've addressed your comments
in a new patchset.  I've also sent a new API request.

https://codereview.chromium.org/286933006/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/document_scan.idl (right):

https://codereview.chromium.org/286933006/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/document_scan.idl:11: // The name of an connected
scanner.
On 2014/05/21 18:06:24, Antony Sargent wrote:
> nit: "an" -> "a" (I think - if you are a grammar wizard feel free to let me
know
> I'm wrong along with optional Strunk and White citation :) )

Done.

https://codereview.chromium.org/286933006/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/document_scan.idl:26: DOMString? mode;
On 2014/05/21 18:06:24, Antony Sargent wrote:
> nit: instead of just using a string, it would be better to define a named enum
> for this. E.g.
> 
> enum ScanMode { color, gray, lineart };
> 
> dictionary ScanOptions {
>   ScanMode? mode;
> 
>   ...
> };
> 
> 
> That way you'll get parameter validation at runtime for free from the
extension
> API plumbing. 

Done.

https://codereview.chromium.org/286933006/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/document_scan.idl:38: callback ScanCallback = void
(boolean result, ArrayBuffer image_data);
On 2014/05/21 18:06:24, Antony Sargent wrote:
> The prevailing style for a long time has been to avoid using explicit boolean
> success/failure parameters in callbacks, instead favoring the built in
> "chrome.runtime.lastError" which gets auto-populated when your
> AsyncApiFunction::Respond override function returns false (and any
> developer-readable error message that you put into your error_ member variable
> will be available to the JS as chrome.runtime.lastError.message). 
> 
> (We've talked about perhaps moving to JS promises in the future for callbacks,
> but AFAIK we aren't there yet). 

Done.

https://codereview.chromium.org/286933006/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/document_scan.idl:46: // Performs a document scan. 
On success, ths image data will be stored in
On 2014/05/21 18:06:24, Antony Sargent wrote:
> typo: "ths"

Done.

https://codereview.chromium.org/286933006/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/document_scan.idl:48: // sent to the callback.
I've been thinking about this myself as well.  I think I've decided
to drop the automatic download-to-file component completely.  If the
app chooses (and has permissions to do so), it can save to disk all
by itself.

https://codereview.chromium.org/286933006/diff/20001/extensions/common/permis...
File extensions/common/permissions/api_permission.h (right):

https://codereview.chromium.org/286933006/diff/20001/extensions/common/permis...
extensions/common/permissions/api_permission.h:180: kDocumentScan,
On 2014/05/21 18:06:24, Antony Sargent wrote:
> nit: I think these are supposed to be sorted (can you also fix up the other
ones
> at the bottom here too while you're in here?)

Done.

[mef, 2014-05-23 18:38:17]

https://codereview.chromium.org/286933006/diff/80001/chrome/common/extensions...
File chrome/common/extensions/api/document_scan.idl (right):

https://codereview.chromium.org/286933006/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/document_scan.idl:40: // is also saved to the
user's download folder.
Is saving to download folder specific to ChromeOS?

[Paul Stewart, 2014-05-23 19:45:36]

https://codereview.chromium.org/286933006/diff/80001/chrome/common/extensions...
File chrome/common/extensions/api/document_scan.idl (right):

https://codereview.chromium.org/286933006/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/document_scan.idl:40: // is also saved to the
user's download folder.
On 2014/05/23 18:38:18, mef wrote:
> Is saving to download folder specific to ChromeOS?

You just found a vestige!  Patch set 6 removes that old verbiage.

[asargent_no_longer_on_chrome, 2014-05-29 17:45:09]

FYI in case you were wondering about the radio silence from me, I'm just waiting
for the results of the API review discussion before giving approval here.

[Paul Stewart, 2014-10-12 14:38:20]

PTAL.  I believe the API proposal has gotten LGTMs.  The code and example
extension has been changed to support the model proposed.  There is a single
TODO to insert API steps between the click of the "Scan" button and the image
data being retrieved from the scanner.  Another API step will be inserted
between retrieving the scan data and passing it to the app.  This will be done
with a follow-on CL.

[mef, 2014-10-13 15:59:54]

TBH I'm not sure why I'm a reviewer on this as I don't know anything about
Chromeos scanning functionality and expected behavior.

Considering that I've made a few comments and also should note that this CL
needs tests.

https://codereview.chromium.org/286933006/diff/230001/chrome/browser/extensio...
File chrome/browser/extensions/api/document_scan/document_scan_api.cc (right):

https://codereview.chromium.org/286933006/diff/230001/chrome/browser/extensio...
chrome/browser/extensions/api/document_scan/document_scan_api.cc:41: #if
defined(OS_CHROMEOS)
Suggest: split into 3 files:

document_scan_interface_chromeos.cc
document_scan_interface_nonchromeos.cc
document_scan_function.cc

https://codereview.chromium.org/286933006/diff/230001/chrome/browser/extensio...
chrome/browser/extensions/api/document_scan/document_scan_api.cc:187: class
DocumentScanInterfaceImpl : public DocumentScanInterface {
If this is not implemented and not intended for implementation outside of
ChromeOS, is it needed at all?

https://codereview.chromium.org/286933006/diff/230001/chrome/browser/extensio...
File chrome/browser/extensions/api/document_scan/document_scan_api.h (right):

https://codereview.chromium.org/286933006/diff/230001/chrome/browser/extensio...
chrome/browser/extensions/api/document_scan/document_scan_api.h:27: class
DocumentScanInterface {
suggest: put this class into document_scan_interface.h

https://codereview.chromium.org/286933006/diff/230001/chrome/browser/extensio...
chrome/browser/extensions/api/document_scan/document_scan_api.h:62: class
DocumentScanScanFunction : public AsyncApiFunction {
suggest: put this class into document_scan_[scan_]function.h

https://codereview.chromium.org/286933006/diff/230001/chrome/browser/extensio...
chrome/browser/extensions/api/document_scan/document_scan_api.h:66:
DocumentScanScanFunction();
Any particular reason to call it 'ScanScan'?

https://codereview.chromium.org/286933006/diff/230001/chrome/chrome_browser_e...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/286933006/diff/230001/chrome/chrome_browser_e...
chrome/chrome_browser_extensions.gypi:209:
'browser/extensions/api/document_scan/document_scan_api.h',
Should this be conditional to chromeos?

https://codereview.chromium.org/286933006/diff/230001/extensions/common/permi...
File extensions/common/permissions/api_permission.h (right):

https://codereview.chromium.org/286933006/diff/230001/extensions/common/permi...
extensions/common/permissions/api_permission.h:81: kDocumentScan,
It appears that this list is NOT in alphabetical order, and new ones must be
added to the end. Could you confirm?

[Paul Stewart, 2014-10-14 19:28:31]

https://codereview.chromium.org/286933006/diff/230001/chrome/browser/extensio...
File chrome/browser/extensions/api/document_scan/document_scan_api.cc (right):

https://codereview.chromium.org/286933006/diff/230001/chrome/browser/extensio...
chrome/browser/extensions/api/document_scan/document_scan_api.cc:41: #if
defined(OS_CHROMEOS)
On 2014/10/13 15:59:54, mef wrote:
> Suggest: split into 3 files:
> 
> document_scan_interface_chromeos.cc
> document_scan_interface_nonchromeos.cc
> document_scan_function.cc
> 

Done, except for consistency sake with the generated references I've retained
the document_scan_api.cc for the function.

https://codereview.chromium.org/286933006/diff/230001/chrome/browser/extensio...
chrome/browser/extensions/api/document_scan/document_scan_api.cc:187: class
DocumentScanInterfaceImpl : public DocumentScanInterface {
On 2014/10/13 15:59:54, mef wrote:
> If this is not implemented and not intended for implementation outside of
> ChromeOS, is it needed at all?

The intent is to make it possible to implement this outside of ChromeOS.
For example, if Google Cloud Devices implements their cloud-based document
scanning using this API (which would be convenient), they could take over
this stub.  The ChromeOS variant may need to merge the GCD results with
the ChromeOS-specific results.  Also, non-ChromeoOS implementations may
choose to expose local OS-specific devices in the future as well.

https://codereview.chromium.org/286933006/diff/230001/chrome/browser/extensio...
File chrome/browser/extensions/api/document_scan/document_scan_api.h (right):

https://codereview.chromium.org/286933006/diff/230001/chrome/browser/extensio...
chrome/browser/extensions/api/document_scan/document_scan_api.h:27: class
DocumentScanInterface {
On 2014/10/13 15:59:54, mef wrote:
> suggest: put this class into document_scan_interface.h

Done.

https://codereview.chromium.org/286933006/diff/230001/chrome/browser/extensio...
chrome/browser/extensions/api/document_scan/document_scan_api.h:62: class
DocumentScanScanFunction : public AsyncApiFunction {
On 2014/10/13 15:59:54, mef wrote:
> suggest: put this class into document_scan_[scan_]function.h

Because of how the API references are generated, I think this really needs to be
document_scan_api.h?

https://codereview.chromium.org/286933006/diff/230001/chrome/browser/extensio...
chrome/browser/extensions/api/document_scan/document_scan_api.h:66:
DocumentScanScanFunction();
On 2014/10/13 15:59:54, mef wrote:
> Any particular reason to call it 'ScanScan'?

It is a little unfortunate but "DocumentScan" refers to the
JavaScript interface, and "Scan" refers to the method.  It would
seem a little rash to call the JavaScript interface "Document".

https://codereview.chromium.org/286933006/diff/230001/chrome/chrome_browser_e...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/286933006/diff/230001/chrome/chrome_browser_e...
chrome/chrome_browser_extensions.gypi:209:
'browser/extensions/api/document_scan/document_scan_api.h',
On 2014/10/13 15:59:54, mef wrote:
> Should this be conditional to chromeos?

I don't think so (I don't expect this interface to be ChromeOS only,
forever, and in either case it would make more sense to gracefully
return that no scanners exist, rather than for an app with this API
permission to fail to find the API at all).  I will, however move
the impl over to a conditional compile when they are split.

https://codereview.chromium.org/286933006/diff/230001/extensions/common/permi...
File extensions/common/permissions/api_permission.h (right):

https://codereview.chromium.org/286933006/diff/230001/extensions/common/permi...
extensions/common/permissions/api_permission.h:81: kDocumentScan,
On 2014/10/13 15:59:54, mef wrote:
> It appears that this list is NOT in alphabetical order, and new ones must be
> added to the end. Could you confirm?

Please see
https://codereview.chromium.org/286933006/diff/20001/extensions/common/permis...
for the specific reference to code review feedback I was given to do this.

[not at google - send to devlin, 2014-10-20 22:36:12]

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

[not at google - send to devlin, 2014-10-20 22:36:13]

API nits (referred to from API proposal).

https://codereview.chromium.org/286933006/diff/210001/chrome/common/extension...
File chrome/common/extensions/api/document_scan.idl (right):

https://codereview.chromium.org/286933006/diff/210001/chrome/common/extension...
chrome/common/extensions/api/document_scan.idl:10: DOMString[]?
accepted_mime_types;
"accepted" is probably unnecessary in this context.

https://codereview.chromium.org/286933006/diff/210001/chrome/common/extension...
chrome/common/extensions/api/document_scan.idl:13: long? scannedImageCount;
Be consistent with underscores vs camel case. We usually use camel case in
extension APIs.

Also: since this API is to do with scanning, the "scan" is somewhat of a
tautology, perhaps "maxImages"?

https://codereview.chromium.org/286933006/diff/210001/chrome/common/extension...
chrome/common/extensions/api/document_scan.idl:20: callback ScanCallback = void
(DOMString[] data_urls, DOMString mime_type);
It's better for backwards compatibility to use a parameters object here, and
just a nicer API IMO. For example, you could have an object ScanResult with
"dataURLs" and "mimeType".

[Paul Stewart, 2014-10-21 02:42:17]

https://codereview.chromium.org/286933006/diff/210001/chrome/common/extension...
File chrome/common/extensions/api/document_scan.idl (right):

https://codereview.chromium.org/286933006/diff/210001/chrome/common/extension...
chrome/common/extensions/api/document_scan.idl:10: DOMString[]?
accepted_mime_types;
On 2014/10/20 22:36:13, kalman wrote:
> "accepted" is probably unnecessary in this context.

Done.

https://codereview.chromium.org/286933006/diff/210001/chrome/common/extension...
chrome/common/extensions/api/document_scan.idl:13: long? scannedImageCount;
On 2014/10/20 22:36:13, kalman wrote:
> Be consistent with underscores vs camel case. We usually use camel case in
> extension APIs.
> 
> Also: since this API is to do with scanning, the "scan" is somewhat of a
> tautology, perhaps "maxImages"?

Done.

https://codereview.chromium.org/286933006/diff/210001/chrome/common/extension...
chrome/common/extensions/api/document_scan.idl:20: callback ScanCallback = void
(DOMString[] data_urls, DOMString mime_type);
On 2014/10/20 22:36:13, kalman wrote:
> It's better for backwards compatibility to use a parameters object here, and
> just a nicer API IMO. For example, you could have an object ScanResult with
> "dataURLs" and "mimeType".

Using "dataUrls" instead of "dataURLs" since the IDL compiler would otherwise
convert to "std::vector<std::string> data_ur_ls"

[not at google - send to devlin, 2014-10-21 17:30:01]

https://codereview.chromium.org/286933006/diff/210001/chrome/common/extension...
File chrome/common/extensions/api/document_scan.idl (right):

https://codereview.chromium.org/286933006/diff/210001/chrome/common/extension...
chrome/common/extensions/api/document_scan.idl:20: callback ScanCallback = void
(DOMString[] data_urls, DOMString mime_type);
On 2014/10/21 02:42:16, Paul Stewart wrote:
> On 2014/10/20 22:36:13, kalman wrote:
> > It's better for backwards compatibility to use a parameters object here, and
> > just a nicer API IMO. For example, you could have an object ScanResult with
> > "dataURLs" and "mimeType".
> 
> Using "dataUrls" instead of "dataURLs" since the IDL compiler would otherwise
> convert to "std::vector<std::string> data_ur_ls"

Ah bummer. We should fix the schema compiler (though not urgently) - the code we
generate internally isn't a reason to chose one API or other. I consciously
chose dataURLs because the web uses dataURL, blobURL for its function names.
It's not that big a deal though, really, I don't feel strongly.

[Paul Stewart, 2014-10-21 18:14:01]

On 2014/10/21 17:30:01, kalman wrote:
>
https://codereview.chromium.org/286933006/diff/210001/chrome/common/extension...
> File chrome/common/extensions/api/document_scan.idl (right):
> 
>
https://codereview.chromium.org/286933006/diff/210001/chrome/common/extension...
> chrome/common/extensions/api/document_scan.idl:20: callback ScanCallback =
void
> (DOMString[] data_urls, DOMString mime_type);
> On 2014/10/21 02:42:16, Paul Stewart wrote:
> > On 2014/10/20 22:36:13, kalman wrote:
> > > It's better for backwards compatibility to use a parameters object here,
and
> > > just a nicer API IMO. For example, you could have an object ScanResult
with
> > > "dataURLs" and "mimeType".
> > 
> > Using "dataUrls" instead of "dataURLs" since the IDL compiler would
otherwise
> > convert to "std::vector<std::string> data_ur_ls"
> 
> Ah bummer. We should fix the schema compiler (though not urgently) - the code
we
> generate internally isn't a reason to chose one API or other. I consciously
> chose dataURLs because the web uses dataURL, blobURL for its function names.
> It's not that big a deal though, really, I don't feel strongly.

It's not completely "internal" since I end up assigning / appending to this data
member from C++ in document_scan_api.cc

[not at google - send to devlin, 2014-10-21 18:19:20]

On 2014/10/21 18:14:01, Paul Stewart wrote:
> On 2014/10/21 17:30:01, kalman wrote:
> >
>
https://codereview.chromium.org/286933006/diff/210001/chrome/common/extension...
> > File chrome/common/extensions/api/document_scan.idl (right):
> > 
> >
>
https://codereview.chromium.org/286933006/diff/210001/chrome/common/extension...
> > chrome/common/extensions/api/document_scan.idl:20: callback ScanCallback =
> void
> > (DOMString[] data_urls, DOMString mime_type);
> > On 2014/10/21 02:42:16, Paul Stewart wrote:
> > > On 2014/10/20 22:36:13, kalman wrote:
> > > > It's better for backwards compatibility to use a parameters object here,
> and
> > > > just a nicer API IMO. For example, you could have an object ScanResult
> with
> > > > "dataURLs" and "mimeType".
> > > 
> > > Using "dataUrls" instead of "dataURLs" since the IDL compiler would
> otherwise
> > > convert to "std::vector<std::string> data_ur_ls"
> > 
> > Ah bummer. We should fix the schema compiler (though not urgently) - the
code
> we
> > generate internally isn't a reason to chose one API or other. I consciously
> > chose dataURLs because the web uses dataURL, blobURL for its function names.
> > It's not that big a deal though, really, I don't feel strongly.
> 
> It's not completely "internal" since I end up assigning / appending to this
data
> member from C++ in document_scan_api.cc

I mean internal in the sense that it's an implementation detail of Chrome, which
extension developers aren't affected by.

[asargent_no_longer_on_chrome, 2014-10-23 22:15:57]

https://codereview.chromium.org/286933006/diff/420001/chrome/browser/extensio...
File chrome/browser/extensions/api/document_scan/document_scan_api.cc (right):

https://codereview.chromium.org/286933006/diff/420001/chrome/browser/extensio...
chrome/browser/extensions/api/document_scan/document_scan_api.cc:95: //
TODO(pstew): Display receied scan in the UI and confirm that this
typo: "receied"

https://codereview.chromium.org/286933006/diff/420001/chrome/common/extension...
File chrome/common/extensions/api/document_scan.idl (right):

https://codereview.chromium.org/286933006/diff/420001/chrome/common/extension...
chrome/common/extensions/api/document_scan.idl:13: long? maxImages;
How are we expecting developers to handle multi-page documents from a scanner?
It seems like having them pass maxImages presumes them having some advance
knowledge of the possible number of pages they might get? 

Is the idea that they will keep calling the scan() function with as high a
maxImages number as they are willing to get pages at once, and then when the
last scan() call returns 0 or something < maxImages, they know that all pages
have been scanned?

https://codereview.chromium.org/286933006/diff/420001/chrome/common/extension...
chrome/common/extensions/api/document_scan.idl:19: DOMString[] dataUrls;
I wonder if it would be better to cache the image data in the browser process
(either in memory or on disk in a temporary file) and just return object urls
instead here. That way we don't have to serialize such big strings over IPC.
Also, there are limitations to how big of a dataUrl chrome will be willing to
handle (sounds like it might be 2MB from this bug:
https://code.google.com/p/chromium/issues/detail?id=44820). 

The trick then becomes how to manage the lifetime of those URLs - the browser
process will want to garbage collect at some point. We do something like this in
the pageCapture API:
https://developer.chrome.com/extensions/pageCapture

The code to implement that lives in: chrome/browser/extensions/api/page_capture/


BTW, if you want to just move forward with this as-is for proof-of-concept or
whatever, and change the API to blob urls later on in follow-up CL's, that would
be fine.

https://codereview.chromium.org/286933006/diff/420001/chrome/common/extension...
chrome/common/extensions/api/document_scan.idl:25: // Callback from the
<code>scan</code> method; on success (result == true)
nit: looks like you removed |result| in favor of just using
chrome.runtime.lastError, so this comment referencing |result| should be updated

[Paul Stewart, 2014-10-23 23:06:58]

https://codereview.chromium.org/286933006/diff/420001/chrome/common/extension...
File chrome/common/extensions/api/document_scan.idl (right):

https://codereview.chromium.org/286933006/diff/420001/chrome/common/extension...
chrome/common/extensions/api/document_scan.idl:13: long? maxImages;
On 2014/10/23 22:15:57, asargent wrote:
> How are we expecting developers to handle multi-page documents from a scanner?
> It seems like having them pass maxImages presumes them having some advance
> knowledge of the possible number of pages they might get? 
> 
> Is the idea that they will keep calling the scan() function with as high a
> maxImages number as they are willing to get pages at once, and then when the
> last scan() call returns 0 or something < maxImages, they know that all pages
> have been scanned?

This is simply a hint to the UI handling the image capture to tell it how many
images the application is expecting.  Think of the example where a bank website
wants you to scan the front and back of your check.  It would expect to get two
images, and wouldn't know what to do with more.  So this hint to the UI would
indicate to the user that they were "all done" after 2 images.

https://codereview.chromium.org/286933006/diff/420001/chrome/common/extension...
chrome/common/extensions/api/document_scan.idl:19: DOMString[] dataUrls;
On 2014/10/23 22:15:57, asargent wrote:
> I wonder if it would be better to cache the image data in the browser process
> (either in memory or on disk in a temporary file) and just return object urls
> instead here. That way we don't have to serialize such big strings over IPC.
> Also, there are limitations to how big of a dataUrl chrome will be willing to
> handle (sounds like it might be 2MB from this bug:
> https://code.google.com/p/chromium/issues/detail?id=44820). 
> 
> The trick then becomes how to manage the lifetime of those URLs - the browser
> process will want to garbage collect at some point. We do something like this
in
> the pageCapture API:
> https://developer.chrome.com/extensions/pageCapture
> 
> The code to implement that lives in:
chrome/browser/extensions/api/page_capture/
> 
> 
> BTW, if you want to just move forward with this as-is for proof-of-concept or
> whatever, and change the API to blob urls later on in follow-up CL's, that
would
> be fine. 

I'd prefer to move forward as-is.  This does work for the current application
and
the API is extensible to use something else if needs be.

https://codereview.chromium.org/286933006/diff/420001/chrome/common/extension...
chrome/common/extensions/api/document_scan.idl:25: // Callback from the
<code>scan</code> method; on success (result == true)
On 2014/10/23 22:15:57, asargent wrote:
> nit: looks like you removed |result| in favor of just using
> chrome.runtime.lastError, so this comment referencing |result| should be
updated

Done.

[Paul Stewart, 2014-10-23 23:07:23]

pstew@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Paul Stewart, 2014-10-23 23:07:55]

Added stevenjb@ to the review due to added mock_lorgnette_manager_client.h

[stevenjb, 2014-10-23 23:09:47]

chromeos/dbus/mock_lorgnette_manager_client.h lgtm

[Paul Stewart, 2014-10-24 04:05:09]

PTAL.  Tests are now in place.

[Paul Stewart, 2014-10-29 00:42:48]

Gentle ping, mostly because I need to rebase quite often to account for other
APIs going in.

[mef, 2014-10-29 19:17:17]

mef@chromium.org changed reviewers:
- mef@chromium.org

[asargent_no_longer_on_chrome, 2014-10-29 23:05:21]

lgtm

https://codereview.chromium.org/286933006/diff/460001/chrome/browser/extensio...
File chrome/browser/extensions/api/document_scan/document_scan_api.cc (right):

https://codereview.chromium.org/286933006/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/document_scan/document_scan_api.cc:14: using
std::vector;
nit: having using statements for std:: members is uncommon in this area of code
; it might be best to omit them

(similar for occurrences elsewhere in CL)

https://codereview.chromium.org/286933006/diff/460001/chrome/common/extension...
File chrome/common/extensions/docs/examples/api/document_scan/background.js
(right):

https://codereview.chromium.org/286933006/diff/460001/chrome/common/extension...
chrome/common/extensions/docs/examples/api/document_scan/background.js:1:
chrome.app.runtime.onLaunched.addListener(function() {
FYI, the presubmit check may require you to have the copyright boilerplate at
the beginning of .js files like this one

[Paul Stewart, 2014-10-30 06:31:51]

Okay.  Done with these comments, and rebased to ToT.

https://codereview.chromium.org/286933006/diff/460001/chrome/browser/extensio...
File chrome/browser/extensions/api/document_scan/document_scan_api.cc (right):

https://codereview.chromium.org/286933006/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/document_scan/document_scan_api.cc:14: using
std::vector;
On 2014/10/29 23:05:21, asargent wrote:
> nit: having using statements for std:: members is uncommon in this area of
code
> ; it might be best to omit them
> 
> (similar for occurrences elsewhere in CL)

Done.

https://codereview.chromium.org/286933006/diff/460001/chrome/common/extension...
File chrome/common/extensions/docs/examples/api/document_scan/background.js
(right):

https://codereview.chromium.org/286933006/diff/460001/chrome/common/extension...
chrome/common/extensions/docs/examples/api/document_scan/background.js:1:
chrome.app.runtime.onLaunched.addListener(function() {
On 2014/10/29 23:05:21, asargent wrote:
> FYI, the presubmit check may require you to have the copyright boilerplate at
> the beginning of .js files like this one

Done.

[Paul Stewart, 2014-10-30 06:32:06]

The CQ bit was checked by pstew@chromium.org

[commit-bot: I haz the power, 2014-10-30 06:32:45]

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

[commit-bot: I haz the power, 2014-10-30 06:36:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-30 06:36:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Paul Stewart, 2014-10-30 06:53:29]

The CQ bit was checked by pstew@chromium.org

[commit-bot: I haz the power, 2014-10-30 06:54:01]

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

[commit-bot: I haz the power, 2014-10-30 07:01:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-30 07:01:52]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Paul Stewart, 2014-10-30 14:42:51]

The CQ bit was checked by pstew@chromium.org

[commit-bot: I haz the power, 2014-10-30 14:44:10]

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

[commit-bot: I haz the power, 2014-10-30 14:51:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-30 14:51:33]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Paul Stewart, 2014-10-30 15:55:29]

The CQ bit was checked by pstew@chromium.org

[commit-bot: I haz the power, 2014-10-30 15:55:49]

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

[commit-bot: I haz the power, 2014-10-30 16:25:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-30 16:25:59]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Paul Stewart, 2014-10-30 16:39:40]

The CQ bit was checked by pstew@chromium.org

[commit-bot: I haz the power, 2014-10-30 16:40:51]

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

[commit-bot: I haz the power, 2014-10-30 17:25:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-30 17:25:20]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Paul Stewart, 2014-10-30 17:33:46]

The CQ bit was checked by pstew@chromium.org

[commit-bot: I haz the power, 2014-10-30 17:34:54]

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

[commit-bot: I haz the power, 2014-10-30 18:55:47]

Committed patchset #24 (id:600001)

[commit-bot: I haz the power, 2014-10-30 18:56:35]

Patchset 24 (id:??) landed as
https://crrev.com/ea73a785e4d97de63c928f0a574d25beb6236aae
Cr-Commit-Position: refs/heads/master@{#302122}

[stevenjb, 2014-11-03 18:16:10]

src/chromeos/ lgtm