Re-enable Cloud printers for ChromeOS kiosks.

Re-enable Cloud printers for ChromeOS kiosks.

In a previous CL (https://codereview.chromium.org/2760753002) we made an
assumption that only local printers would appear in the local printer
list.  This turns out to be incorrect.  The actual condition is that only
cloud printers appear in the cloud list and sometimes appear in the local
list (when they're device printers).  This CL accounts for this behavior
and adds a test so we don't break it in the future.

BUG=713831
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2838603004
Cr-Commit-Position: refs/heads/master@{#466895}
Committed: https://chromium.googlesource.com/chromium/src/+/e02af5b94d73c62ad0708593d4514aa15be4ee6d

[skau, 2017-04-25 00:27:40]

Description was changed from

==========
Re-enable Cloud printers for ChromeOS kiosks.

In a previous CL (https://codereview.chromium.org/2760753002) we made an
assumption that only local printers would appear in the local printer
list.  This turns out to be incorrect.  The actual condition is that only
cloud printers appear in the cloud list and sometimes appear in the local
list (when they're device printers).  This CL accounts for this behavior
and adds a test so we don't break it in the future.

BUG=713831
==========

to

==========
Re-enable Cloud printers for ChromeOS kiosks.

In a previous CL (https://codereview.chromium.org/2760753002) we made an
assumption that only local printers would appear in the local printer
list.  This turns out to be incorrect.  The actual condition is that only
cloud printers appear in the cloud list and sometimes appear in the local
list (when they're device printers).  This CL accounts for this behavior
and adds a test so we don't break it in the future.

BUG=713831
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[skau, 2017-04-25 00:29:36]

skau@chromium.org changed reviewers:
+ thestig@chromium.org, xdai@chromium.org

[skau, 2017-04-25 00:29:36]

Please take a look.  I've included a verbose description since I found the
behavior to be surprising.

[skau, 2017-04-25 00:29:42]

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

[commit-bot: I haz the power, 2017-04-25 00:30:00]

Dry run: CQ is trying da patch.

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

[xdai1, 2017-04-25 00:33:30]

On 2017/04/25 00:29:36, skau wrote:
> Please take a look.  I've included a verbose description since I found the
> behavior to be surprising.

lgtm though the actual situation seems wired. Thanks for the fix.

[xdai1, 2017-04-25 00:33:56]

On 2017/04/25 00:33:30, xdai1 wrote:
> On 2017/04/25 00:29:36, skau wrote:
> > Please take a look.  I've included a verbose description since I found the
> > behavior to be surprising.
> 
> lgtm though the actual situation seems wired. Thanks for the fix.

weird*

[Lei Zhang, 2017-04-25 00:42:36]

https://codereview.chromium.org/2838603004/diff/1/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/search/destination_search.js
(right):

https://codereview.chromium.org/2838603004/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_search.js:588: var
destinationItem;
Looking at renderDestinations_() above, should this block instead be the
following?

var destinationItem =
    destination.isLocal ||
    destination.origin == print_preview.Destination.Origin.DEVICE ?
        this.localList_.getDestinationItem(destination.id) :
        this.cloudList_.getDestinationItem(destination.id);

See:
https://crrev.com/296501
https://crrev.com/301675
https://crbug.com/416701 <-- Why is this assigned to me?!

[skau, 2017-04-25 00:57:42]

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

[skau, 2017-04-25 00:58:05]

I've narrowed the criteria.  PTAL.

https://codereview.chromium.org/2838603004/diff/1/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/search/destination_search.js
(right):

https://codereview.chromium.org/2838603004/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_search.js:588: var
destinationItem;
On 2017/04/25 00:42:36, Lei Zhang wrote:
> Looking at renderDestinations_() above, should this block instead be the
> following?
> 
> var destinationItem =
>     destination.isLocal ||
>     destination.origin == print_preview.Destination.Origin.DEVICE ?
>         this.localList_.getDestinationItem(destination.id) :
>         this.cloudList_.getDestinationItem(destination.id);
> 
> See:
> https://crrev.com/296501
> https://crrev.com/301675
> https://crbug.com/416701 <-- Why is this assigned to me?!

Hmm.  Looks like somebody needs to think about how printer types should be
handled in the UI...

But in the meantime, I've narrowed the condition to just Origin.DEVICE printers.

[commit-bot: I haz the power, 2017-04-25 00:58:10]

Dry run: CQ is trying da patch.

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

[Lei Zhang, 2017-04-25 01:20:46]

lgtm

https://codereview.chromium.org/2838603004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/search/destination_search.js
(right):

https://codereview.chromium.org/2838603004/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_search.js:592:
destination.origin == print_preview.Destination.Origin.DEVICE) ?
Indentation looks wonky. Should be 4 from var, and next line is 4 more from
where "destination" starts.

[skau, 2017-04-25 01:45:47]

Thanks for the review.

https://codereview.chromium.org/2838603004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/search/destination_search.js
(right):

https://codereview.chromium.org/2838603004/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_search.js:592:
destination.origin == print_preview.Destination.Origin.DEVICE) ?
On 2017/04/25 01:20:45, Lei Zhang wrote:
> Indentation looks wonky. Should be 4 from var, and next line is 4 more from
> where "destination" starts.

I ran it through the C++ formatter and positioned the ternary operator in the
appropriate place.

[skau, 2017-04-25 01:45:50]

The CQ bit was checked by skau@chromium.org

[skau, 2017-04-25 01:45:51]

The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2838603004/#ps40001
(title: "fix indentation")

[commit-bot: I haz the power, 2017-04-25 01:46:12]

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-25 05:34:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493084750898960,
"parent_rev": "854591636767ea4f9d8076fe0be440cf3a7c0643", "commit_rev":
"e02af5b94d73c62ad0708593d4514aa15be4ee6d"}

[commit-bot: I haz the power, 2017-04-25 05:34:45]

Description was changed from

==========
Re-enable Cloud printers for ChromeOS kiosks.

In a previous CL (https://codereview.chromium.org/2760753002) we made an
assumption that only local printers would appear in the local printer
list.  This turns out to be incorrect.  The actual condition is that only
cloud printers appear in the cloud list and sometimes appear in the local
list (when they're device printers).  This CL accounts for this behavior
and adds a test so we don't break it in the future.

BUG=713831
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Re-enable Cloud printers for ChromeOS kiosks.

In a previous CL (https://codereview.chromium.org/2760753002) we made an
assumption that only local printers would appear in the local printer
list.  This turns out to be incorrect.  The actual condition is that only
cloud printers appear in the cloud list and sometimes appear in the local
list (when they're device printers).  This CL accounts for this behavior
and adds a test so we don't break it in the future.

BUG=713831
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2838603004
Cr-Commit-Position: refs/heads/master@{#466895}
Committed:
https://chromium.googlesource.com/chromium/src/+/e02af5b94d73c62ad0708593d451...
==========

[commit-bot: I haz the power, 2017-04-25 05:34:45]

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

[pkalinnikov, 2017-04-25 14:23:44]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2840593003/ by pkalinnikov@chromium.org.

The reason for reverting is: Seems causing the "Linux MSan Tests" bot fail on
PrintPreviewDestinationSearchTest.Select test..