Update UI for extension destinations in print preview destination list

Update UI for extension destinations in print preview destination list

Adds the extension name and icon to destination list item for extension
destinations. The icon is clickable and links to
chrome://extension?id=<extension-id>.

For selected extension destination use the extension name as hint (instead of
description)

Also, bring back comment for
PrinterProviderAPI::DispatchGetPrintersRequested (which got deleted by mistake)

BUG=454596
Committed: https://crrev.com/599ffa1d8087230b83f5b097cba7a06fc6963f77
Cr-Commit-Position: refs/heads/master@{#319422}

[tbarzic, 2015-03-05 23:50:58]

tbarzic@chromium.org changed reviewers:
+ alekseys@chromium.org, vitalybuka@chromium.org

[tbarzic, 2015-03-05 23:50:59]

screenshot:
https://drive.google.com/a/google.com/file/d/0B4oTsS_XgGwFaVJqX3FWekFHeDQ/vie...

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/data/local_parsers.js (left):

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/data/local_parsers.js:85: {description:
destinationInfo.description || '',
with extension name being displayed as hint for printer, the only thing
description is useful is to be matched against during destination search. Let me
know if you think that would be worth keeping it around (otherwise, I'll remove
it from the printerProvider API)

[Aleksey Shlyapnikov, 2015-03-06 00:27:39]

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/data/destination.js (right):

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/data/destination.js:478: },
If it's not part of the generic iconUrl logic, can we move this knowledge to
presentation layer?

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/data/local_parsers.js (left):

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/data/local_parsers.js:85: {description:
destinationInfo.description || '',
On 2015/03/05 23:50:59, tbarzic wrote:
> with extension name being displayed as hint for printer, the only thing
> description is useful is to be matched against during destination search. Let
me
> know if you think that would be worth keeping it around (otherwise, I'll
remove
> it from the printerProvider API)

Would it occur to anyone to search for the printer by the extension name? Don't
we expect the name of the printer announced by the extension already be somehow
related to the extension? I'm not opposed to passing and exposing the extension
name, especially that it's already implemented.

One question though, the new field, why not pass extension name in the
description here? Just a thought, maybe we should let extension to specify the
printer description and default it to the extension name?

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/search/destination_list_item.html
(right):

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/search/destination_list_item.html:14:
<span class="extension-icon" role="button" tabindex="0"></span>
Why don't we want to use extension icon as destination-list-item-icon, as it
used to be? If it's a button, you'll need alt text for accessibility,

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/search/destination_list_item.js:152: var
extensionId = this.destination_.extensionId;
What do you need this var for?

[tbarzic, 2015-03-06 03:16:12]

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/data/destination.js (right):

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/data/destination.js:478: },
On 2015/03/06 00:27:39, Aleksey Shlyapnikov wrote:
> If it's not part of the generic iconUrl logic, can we move this knowledge to
> presentation layer?

Done.

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/data/local_parsers.js (left):

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/data/local_parsers.js:85: {description:
destinationInfo.description || '',
On 2015/03/06 00:27:39, Aleksey Shlyapnikov wrote:
> On 2015/03/05 23:50:59, tbarzic wrote:
> > with extension name being displayed as hint for printer, the only thing
> > description is useful is to be matched against during destination search.
Let
> me
> > know if you think that would be worth keeping it around (otherwise, I'll
> remove
> > it from the printerProvider API)
> 
> Would it occur to anyone to search for the printer by the extension name?
Don't
> we expect the name of the printer announced by the extension already be
somehow
> related to the extension? I'm not opposed to passing and exposing the
extension
> name, especially that it's already implemented.
> 
> One question though, the new field, why not pass extension name in the
> description here? Just a thought, maybe we should let extension to specify the
> printer description and default it to the extension name?

The point of showing extension name is not as much to enable user to search
destinations by extension name as it is to let the user know which extension
supplied the extension (and make it easier for the user to disable unwanted
extensions).

My first idea was to pass the extension name as part of the description, but
that would produce a weird result during the destination search if the extension
name is a hit (basically, we'd show the extension name twice: the extension name
is shown by default in the UI, and we also add description string that matches
search query).

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/search/destination_list_item.html
(right):

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/search/destination_list_item.html:14:
<span class="extension-icon" role="button" tabindex="0"></span>
On 2015/03/06 00:27:39, Aleksey Shlyapnikov wrote:
> Why don't we want to use extension icon as destination-list-item-icon, as it
> used to be? If it's a button, you'll need alt text for accessibility,

We want the icon to link to the extension webui with the extension highlighted,
so it would be strange if some destination-list-item-icons were actionable and
some are not.

The title set for the icon in .js should take care of accessibility for the
icon, right?

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

https://codereview.chromium.org/979303002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/search/destination_list_item.js:152: var
extensionId = this.destination_.extensionId;
On 2015/03/06 00:27:39, Aleksey Shlyapnikov wrote:
> What do you need this var for?

removed

[Aleksey Shlyapnikov, 2015-03-06 03:54:30]

lgtm

[Vitaly Buka (NO REVIEWS), 2015-03-06 07:16:25]

The CQ bit was checked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2015-03-06 07:16:25]

lgtm

[commit-bot: I haz the power, 2015-03-06 07:17:37]

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

[commit-bot: I haz the power, 2015-03-06 07:59:53]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-06 08:00:26]

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