Add CupsFilter extraction from resolved ppds for printing.

Add CupsFilter extraction from resolved ppds for printing.

This is a step towards component downloads of filters, but the
extracted filters are currently ignored.

BUG=710282
Review-Url: https://codereview.chromium.org/2814633002
Cr-Commit-Position: refs/heads/master@{#464102}
Committed: https://chromium.googlesource.com/chromium/src/+/96bf4bd8c5c82dcedec738e785d5a5d8fc37acab

[Carlson, 2017-04-11 00:43:56]

justincarlson@chromium.org changed reviewers:
+ skau@chromium.org

[Carlson, 2017-04-11 00:43:57]

Sean, can you take an initial look at this?

[Carlson, 2017-04-11 16:14:29]

The CQ bit was checked by justincarlson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 16:15:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 16:56:53]

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

[commit-bot: I haz the power, 2017-04-11 16:56:54]

Dry run: This issue passed the CQ dry run.

[skau, 2017-04-11 16:59:37]

Generally looks good.  Just curious if we can make the parsing code slightly
easier to read.  I spent a few minutes squinting to figure out what it was
doing.

https://codereview.chromium.org/2814633002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/printer_configurer.cc (right):

https://codereview.chromium.org/2814633002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/printer_configurer.cc:124: const
std::vector<std::string>& ppd_filters) {
Can you comment this as unused for now?

https://codereview.chromium.org/2814633002/diff/20001/chromeos/printing/ppd_p...
File chromeos/printing/ppd_provider.cc (right):

https://codereview.chromium.org/2814633002/diff/20001/chromeos/printing/ppd_p...
chromeos/printing/ppd_provider.cc:87: for (int i = 0; i < num_mime_types + 2;
++i) {
Can you make this slightly less opaque?  Shouldn't it always be the last token
on the line?

https://codereview.chromium.org/2814633002/diff/20001/chromeos/printing/ppd_p...
File chromeos/printing/ppd_provider.h (right):

https://codereview.chromium.org/2814633002/diff/20001/chromeos/printing/ppd_p...
chromeos/printing/ppd_provider.h:64: // Otherwise, theses fields will be empty.
theses

[Carlson, 2017-04-11 19:06:49]

Let me know which option you'd prefer for filter extraction.

https://codereview.chromium.org/2814633002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/printer_configurer.cc (right):

https://codereview.chromium.org/2814633002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/printer_configurer.cc:124: const
std::vector<std::string>& ppd_filters) {
On 2017/04/11 16:59:37, skau wrote:
> Can you comment this as unused for now?

Done.

https://codereview.chromium.org/2814633002/diff/20001/chromeos/printing/ppd_p...
File chromeos/printing/ppd_provider.cc (right):

https://codereview.chromium.org/2814633002/diff/20001/chromeos/printing/ppd_p...
chromeos/printing/ppd_provider.cc:87: for (int i = 0; i < num_mime_types + 2;
++i) {
On 2017/04/11 16:59:37, skau wrote:
> Can you make this slightly less opaque?  Shouldn't it always be the last token
> on the line?

Could do, but the tradeoffs are:

* We accept more malformed lines. (We already accept some here)

* We lose some bounding on how much input we process.  If someone for some
reason gives us a PPD consisting of

*cupsFilter2: a a a a a a (x whatever limit we have on filesize) we could spend
potentially a nontrivial amount of time parsing tokens pointlessly.  

Not sure which tradeoff is better here.  For now I just changed num_mime_types
to num_value_tokens, which gets rid of the +2 magic and maybe is a little more
straightforward.  If you want to go straight-up for "grab the last token" I'll
do that instead.

https://codereview.chromium.org/2814633002/diff/20001/chromeos/printing/ppd_p...
File chromeos/printing/ppd_provider.h (right):

https://codereview.chromium.org/2814633002/diff/20001/chromeos/printing/ppd_p...
chromeos/printing/ppd_provider.h:64: // Otherwise, theses fields will be empty.
On 2017/04/11 16:59:37, skau wrote:
> theses

Done.

[skau, 2017-04-11 22:14:57]

lgtm

https://codereview.chromium.org/2814633002/diff/20001/chromeos/printing/ppd_p...
File chromeos/printing/ppd_provider.cc (right):

https://codereview.chromium.org/2814633002/diff/20001/chromeos/printing/ppd_p...
chromeos/printing/ppd_provider.cc:87: for (int i = 0; i < num_mime_types + 2;
++i) {
On 2017/04/11 19:06:49, Carlson wrote:
> On 2017/04/11 16:59:37, skau wrote:
> > Can you make this slightly less opaque?  Shouldn't it always be the last
token
> > on the line?
> 
> Could do, but the tradeoffs are:
> 
> * We accept more malformed lines. (We already accept some here)
> 
> * We lose some bounding on how much input we process.  If someone for some
> reason gives us a PPD consisting of
> 
> *cupsFilter2: a a a a a a (x whatever limit we have on filesize) we could
spend
> potentially a nontrivial amount of time parsing tokens pointlessly.  
> 
> Not sure which tradeoff is better here.  For now I just changed num_mime_types
> to num_value_tokens, which gets rid of the +2 magic and maybe is a little more
> straightforward.  If you want to go straight-up for "grab the last token" I'll
> do that instead.

Thanks for the explanation.  I wasn't thinking of the pathological example. 
Keeping it bounded should be the right choice.  Would we want to run the file
through cupstestppd before trying to read it to make sure it isn't pathological?
 TODO would be fine.

I just realized that StringTokenizer skips consecutive delimiters. Is that
behavior well known (it may just be me)?  If not, leave a comment.

[Carlson, 2017-04-11 22:25:54]

The CQ bit was checked by justincarlson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 22:26:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Carlson, 2017-04-11 23:20:43]

justincarlson@chromium.org changed reviewers:
+ dpapad@chromium.org

[Carlson, 2017-04-11 23:20:44]

dpapad@ can you sign off on the trivial change to cups_printers_handler?  skau@
has owners everywhere else.

[commit-bot: I haz the power, 2017-04-11 23:23:40]

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

[commit-bot: I haz the power, 2017-04-11 23:23:42]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-04-12 18:58:57]

On 2017/04/11 at 23:20:44, justincarlson wrote:
> dpapad@ can you sign off on the trivial change to cups_printers_handler? 
skau@ has owners everywhere else.

LGTM chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h

[Carlson, 2017-04-12 19:00:16]

The CQ bit was checked by justincarlson@chromium.org

[commit-bot: I haz the power, 2017-04-12 19:01:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 19:14:50]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492023616148600,
"parent_rev": "4aac67d6f941be4e56812b0f5c2b219f13641708", "commit_rev":
"96bf4bd8c5c82dcedec738e785d5a5d8fc37acab"}

[commit-bot: I haz the power, 2017-04-12 19:15:44]

Description was changed from

==========
Add CupsFilter extraction from resolved ppds for printing.

This is a step towards component downloads of filters, but the
extracted filters are currently ignored.

BUG=710282
==========

to

==========
Add CupsFilter extraction from resolved ppds for printing.

This is a step towards component downloads of filters, but the
extracted filters are currently ignored.

BUG=710282

Review-Url: https://codereview.chromium.org/2814633002
Cr-Commit-Position: refs/heads/master@{#464102}
Committed:
https://chromium.googlesource.com/chromium/src/+/96bf4bd8c5c82dcedec738e785d5...
==========

[commit-bot: I haz the power, 2017-04-12 19:15:48]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/96bf4bd8c5c82dcedec738e785d5...