Print Preview Print Destination Search Widget

Implements print-destination search widget which replaces the old select drop-down element.

BUG=114206
NOTRY=true
TEST=Use the print preview dialog to switch printers before printing. Also sign-in with a Google Account to gain access to Google Cloud Print Printers.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139820

[Robert Toscano, 2012-05-25 00:50:11]

CL is ready for review. Please take a look.

[dpapad, 2012-05-25 16:18:03]

Will take a closer look at the newly added files later today.

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/cloud_parsers.js (right):

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/cloud_parsers.js:44: * @param
{object} json Object that represents a Google Cloud Print search or
Nit: Should this be !Object?

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/cloud_parsers.js:82: if (typeStr ==
'ANDROID_CHROME_SNAPSHOT' ||
Nit: Maybe define an enum somewhere instead of using string literals.

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/destination.js (right):

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination.js:12: * @param
{print_preview.Destination.Type} type Type of the destination.
Nit: !print_preview.Destination.Type.

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination.js:30: * @type
{print_preview.Destination.Type}
Same here.

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination_store.js:73: * @type
{!print_preview.CloudPrintInterface}
cant use
@type {!Type}
this.blah_ = null;

! means non-nullable, but it is null here.

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination_store.js:92:
this.autoSelectTimeout_ = null;
Isn't this just a timeout id, therefore a number?
https://developer.mozilla.org/en/DOM/window.setTimeout

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination_store.js:262: for (var
dest, i = 0; dest = destinations[i]; i++) {
This can probably be simplified if you use forEach
destinations.forEach(function(dest) {
  // do stuff here.
}, this);

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination_store.js:263: if
(this.insertDestination_(dest)) {
You can merge these 2 nested if statements as follows.

if (insertedDestination = this.insertDestination_(dest) &&
    this.isInAutoSelectMode_ &&
    etc etc....) {
  destinationToAutoSelect = dest;
}

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination_store.js:379: for (var
destInfo, i = 0; destInfo = event.destinationInfos[i]; i++) {
use forEach.

[Robert Toscano, 2012-05-25 19:47:01]

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/cloud_parsers.js (right):

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/cloud_parsers.js:44: * @param
{object} json Object that represents a Google Cloud Print search or
On 2012/05/25 16:18:03, dpapad wrote:
> Nit: Should this be !Object?

Done.

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/cloud_parsers.js:82: if (typeStr ==
'ANDROID_CHROME_SNAPSHOT' ||
On 2012/05/25 16:18:03, dpapad wrote:
> Nit: Maybe define an enum somewhere instead of using string literals.

Done.

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/destination.js (right):

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination.js:12: * @param
{print_preview.Destination.Type} type Type of the destination.
This is a typedef of a string, so I don't think the "!" is necessary.

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination.js:30: * @type
{print_preview.Destination.Type}
See answer above.

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination_store.js:73: * @type
{!print_preview.CloudPrintInterface}
Whoops copy paste error. Done.

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination_store.js:92:
this.autoSelectTimeout_ = null;
I never looked up what exactly was returned by setTimeout. Thanks! Done.

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination_store.js:262: for (var
dest, i = 0; dest = destinations[i]; i++) {
Thanks! Didn't know about this one. Done.

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination_store.js:263: if
(this.insertDestination_(dest)) {
Kausalya recommended not using assignments in "if" conditions since they are
hard to tell whether it was a typo or actually an assignment. What do you think?

http://codereview.chromium.org/10450022/diff/11040/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination_store.js:379: for (var
destInfo, i = 0; destInfo = event.destinationInfos[i]; i++) {
On 2012/05/25 16:18:03, dpapad wrote:
> use forEach.

Done.

[Robert Toscano, 2012-05-26 00:05:23]

Fixed jsdoc as discussed offline.

[dpapad, 2012-05-29 16:41:01]

http://codereview.chromium.org/10450022/diff/14001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

http://codereview.chromium.org/10450022/diff/14001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.js:79: }
else if (this.destination_.type ==
I think that there are helpers now, this.destination_.isLocal, right? Consider
adding helpers for isMobile, isGooglePromoted if they don't already exist.

http://codereview.chromium.org/10450022/diff/14001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/search_box.js (right):

http://codereview.chromium.org/10450022/diff/14001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/search_box.js:18: * @type {Object}
This is probably just a number.

[Robert Toscano, 2012-05-29 18:00:43]

http://codereview.chromium.org/10450022/diff/14001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

http://codereview.chromium.org/10450022/diff/14001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.js:79: }
else if (this.destination_.type ==
I had isLocal from before, but now that I've introduced the "type" field for
destinations, I think it's simpler if we just check that field. I'll leave the
isLocal getter since it was already there.

http://codereview.chromium.org/10450022/diff/14001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/search_box.js (right):

http://codereview.chromium.org/10450022/diff/14001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/search_box.js:18: * @type {Object}
On 2012/05/29 16:41:02, dpapad wrote:
> This is probably just a number.

Done.

[dpapad, 2012-05-29 18:15:08]

LGTM on the JS. You should probably find a reviewer for the CSS code, @estade or
@dbeam.

http://codereview.chromium.org/10450022/diff/14001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

http://codereview.chromium.org/10450022/diff/14001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.js:79: }
else if (this.destination_.type ==
On 2012/05/29 18:00:43, rltoscano wrote:
> I had isLocal from before, but now that I've introduced the "type" field for
> destinations, I think it's simpler if we just check that field. I'll leave the
> isLocal getter since it was already there.

I am fine with that. But I do feel that the "type" is kind of implementation
detail (and therefore should be private) and that it would be a bit cleaner and
less verbose to have 
isLocal, isMobile, isGoolgePromoted etc and use all of them here.
Do you use the type field outside the Destination class anywhere else?

[Robert Toscano, 2012-05-29 19:37:26]

On 2012/05/29 18:15:08, dpapad wrote:
> LGTM on the JS. You should probably find a reviewer for the CSS code, @estade
or
> @dbeam.
> 
>
http://codereview.chromium.org/10450022/diff/14001/chrome/browser/resources/p...
> File chrome/browser/resources/print_preview/search/destination_list_item.js
> (right):
> 
>
http://codereview.chromium.org/10450022/diff/14001/chrome/browser/resources/p...
> chrome/browser/resources/print_preview/search/destination_list_item.js:79: }
> else if (this.destination_.type ==
> On 2012/05/29 18:00:43, rltoscano wrote:
> > I had isLocal from before, but now that I've introduced the "type" field for
> > destinations, I think it's simpler if we just check that field. I'll leave
the
> > isLocal getter since it was already there.
> 
> I am fine with that. But I do feel that the "type" is kind of implementation
> detail (and therefore should be private) and that it would be a bit cleaner
and
> less verbose to have 
> isLocal, isMobile, isGoolgePromoted etc and use all of them here.
> Do you use the type field outside the Destination class anywhere else?

I do use it to check for MOBILE or CLOUD.

[Robert Toscano, 2012-05-29 19:38:45]

Adding dbeam@chromium.org as a reviewer for CSS and style.

Hi Dan,

Demetrios suggested adding you to review my new CSS classes and style. Thanks!

[Dan Beam, 2012-05-29 21:25:50]

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list.css (right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list.css:6:
.destination-list-header {
why not .destination-list header?

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list.css:16:
.destination-list-destination-list-item-container {
is this a copy pasta error?

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list.css:22:
.destination-list-no-destinations-message {
I think just .no-destinations-message should be unique enough, probably don't
need the "destination-list" in the class name as this seems like it'd make

|<-- indent -->|<tag class="destination-list-no-destinations-message">

relatively hard to wrap in 80 chars. If you're really paranoid, you may want to
have a root namespace where this class applies, like I mentioned above, i.e.

  .destination-list .no-destinations-message {}

or

  .destination-list.no-destinations-message {}

if it's the same node.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list.html
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list.html:1: <div
id="destination-list-template" style="display: none;">
s/style="display: none;"/hidden/

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list_item.css
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.css:3: *
found in the LICENSE file.
optional nit: in general I prefer not letting this */ hang over as it saves a
line and a byte

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.css:9:
padding: 3px 2px 3px 18px;
does this work in RTL? seems like you'd wanna do:

  -webkit-padding-end: 2px;
  -webkit-padding-start: 18px;
  padding-bottom: 3px;
  padding-top: 3px;

unless this is not affected by text direction.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.css:20:
margin-right: 8px;
same question about RTL

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.css:21:
opacity: .4;
optional nit: I don't think we have an explicit rule either way, but most of the
code that I've seen in webui has 0.X rather than .X. If print_preview's local
style disagrees, follow it, otherwise you may want to s/.4/0.4/

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_search.css
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_search.css:48:
padding: 12px 44px 12px 12px;
rtl

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_search.css:63:
margin-right: 4px;
rtl

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_search.css:67:
.destination-search-cloudprint-promo-close-button {
really long class

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_search.css:68:
background-image: url('chrome://resources/images/x.png');
nit: btw, you don't need ' inside of url()s, just FYI (again, unless everywhere
else does this)

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_search.html
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_search.html:4: <h1
class="destination-search-title" i18n-content="destinationSearchTitle"></h1>
80 char wrap

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/search_box.html (right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/search_box.html:1: <div
id="search-box" class="search-box">
any reason to have both a class and ID? do you have multiple search-box's on the
page or plan to have more?

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/search_box.html:3: <input
type="text"
seems like this wrapping is taking up too many lines, think the preferred is

  <input type="text" class="search-box-input"
      i18n-values="placeholder:searchBoxPlaceholder"/>

also, you don't really need the / in self-closing tags in HTML (just XHTML), but
that's another religious debate...

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/destination_settings.css
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/destination_settings.css:12:
margin-right: 8px;
same question about RTL

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/destination_settings.css:13:
opacity: .4;
same nit about 0.4

[Robert Toscano, 2012-05-29 22:17:35]

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list.css (right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list.css:6:
.destination-list-header {
On 2012/05/29 21:25:50, Dan Beam wrote:
> why not .destination-list header?

Then I would have to decorate my element like

  <tag class="destination-list header">

but what if in one of the style-sheets I import, there exists a class ".header"
CSS definition? I would unintentionally import those styles into an element
where it shouldn't. So instead, I prefix all classes from the same element. This
is my answer to several of the other related comments. I'll use the label
css-paranoia.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list.css:16:
.destination-list-destination-list-item-container {
On 2012/05/29 21:25:50, Dan Beam wrote:
> is this a copy pasta error?

Nope, but I guess I could rename to .destination-list-item-container?

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list.css:22:
.destination-list-no-destinations-message {
On 2012/05/29 21:25:50, Dan Beam wrote:
> I think just .no-destinations-message should be unique enough, probably don't
> need the "destination-list" in the class name as this seems like it'd make
> 
> |<-- indent -->|<tag class="destination-list-no-destinations-message">
> 
> relatively hard to wrap in 80 chars. If you're really paranoid, you may want
to
> have a root namespace where this class applies, like I mentioned above, i.e.
> 
>   .destination-list .no-destinations-message {}
> 
> or
> 
>   .destination-list.no-destinations-message {}
> 
> if it's the same node.

See css-paranoia

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list.html
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list.html:1: <div
id="destination-list-template" style="display: none;">
On 2012/05/29 21:25:50, Dan Beam wrote:
> s/style="display: none;"/hidden/

These are different. "hidden" will render the element hidden, but it still will
have an effect on the DOM element layout. display=none will be like if the
element wasn't even in the DOM tree.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list_item.css
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.css:3: *
found in the LICENSE file.
On 2012/05/29 21:25:50, Dan Beam wrote:
> optional nit: in general I prefer not letting this */ hang over as it saves a
> line and a byte

Done.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.css:9:
padding: 3px 2px 3px 18px;
On 2012/05/29 21:25:50, Dan Beam wrote:
> does this work in RTL? seems like you'd wanna do:
> 
>   -webkit-padding-end: 2px;
>   -webkit-padding-start: 18px;
>   padding-bottom: 3px;
>   padding-top: 3px;
> 
> unless this is not affected by text direction.

There are a few other things that need to change. Like the icon should be on the
right instead of the left. How far should we support RTL? I can understand
changing text layout direction, but it seems like we would also need to change
icon layout. Should this be done in this CL?

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.css:21:
opacity: .4;
On 2012/05/29 21:25:50, Dan Beam wrote:
> optional nit: I don't think we have an explicit rule either way, but most of
the
> code that I've seen in webui has 0.X rather than .X. If print_preview's local
> style disagrees, follow it, otherwise you may want to s/.4/0.4/

It looks like its not entirely consistent. I've found some .X in
shared/css/tree.css and shared/css/widgets.css. Also if we're concerned about
conserving bytes (as your comment about copyright comment shows) maybe we should
strip the leading 0.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_search.css
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_search.css:67:
.destination-search-cloudprint-promo-close-button {
On 2012/05/29 21:25:50, Dan Beam wrote:
> really long class

See css-paranoia

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_search.css:68:
background-image: url('chrome://resources/images/x.png');
On 2012/05/29 21:25:50, Dan Beam wrote:
> nit: btw, you don't need ' inside of url()s, just FYI (again, unless
everywhere
> else does this)

Done.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_search.html
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_search.html:4: <h1
class="destination-search-title" i18n-content="destinationSearchTitle"></h1>
On 2012/05/29 21:25:50, Dan Beam wrote:
> 80 char wrap

Done.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/search_box.html (right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/search_box.html:1: <div
id="search-box" class="search-box">
On 2012/05/29 21:25:50, Dan Beam wrote:
> any reason to have both a class and ID? do you have multiple search-box's on
the
> page or plan to have more?

The ID is there because I actually reference the element in JS by ID, and the
class is there so that I can style it. I know that I can style based on the ID,
but really the style is based on the class. Right now, this is a unique element,
but I wanted to leave room for it being used in multiple places.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/search_box.html:3: <input
type="text"
On 2012/05/29 21:25:50, Dan Beam wrote:
> seems like this wrapping is taking up too many lines, think the preferred is
> 
>   <input type="text" class="search-box-input"
>       i18n-values="placeholder:searchBoxPlaceholder"/>

Done.

> 
> also, you don't really need the / in self-closing tags in HTML (just XHTML),
but
> that's another religious debate...

Yea I know I don't need "/" for HTML, but if we ever wanted to migrate to XHTML,
it would be one less step. I also would just always end with "/" just so I don't
ever leave it out when I shouldn't.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/destination_settings.css
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/destination_settings.css:13:
opacity: .4;
On 2012/05/29 21:25:50, Dan Beam wrote:
> same nit about 0.4

See related response.

[Dan Beam, 2012-05-29 22:30:17]

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list.css (right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list.css:6:
.destination-list-header {
On 2012/05/29 22:17:36, rltoscano wrote:
> On 2012/05/29 21:25:50, Dan Beam wrote:
> > why not .destination-list header?
> 
> Then I would have to decorate my element like
> 
>   <tag class="destination-list header">
> 
> but what if in one of the style-sheets I import, there exists a class
".header"
> CSS definition? I would unintentionally import those styles into an element
> where it shouldn't. So instead, I prefix all classes from the same element.
This
> is my answer to several of the other related comments. I'll use the label
> css-paranoia.

  .destination-list header

would correspond to:

  <div class="destination-list">
    <header></header><!-- NOTE: header tag, not class="header" -->
  </div>

like what you're already doing.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list.html
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list.html:1: <div
id="destination-list-template" style="display: none;">
On 2012/05/29 22:17:36, rltoscano wrote:
> On 2012/05/29 21:25:50, Dan Beam wrote:
> > s/style="display: none;"/hidden/
> 
> These are different. "hidden" will render the element hidden, but it still
will
> have an effect on the DOM element layout. display=none will be like if the
> element wasn't even in the DOM tree.

[hidden] is just display: none; but from user-agent styles (whereas this code is
more of the "application level").  Just in case it was ambiguous, I meant:

  <div id="destination-list-template" hidden>

It's the same thing, I promise. [hidden] is not visibility: hidden;.

Additionally, I think you meant "render tree" not DOM tree, as you can still use
the DOM/BOM to reference / affect [hidden]/display: none; nodes, it just usually
isn't necessary.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list_item.css
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.css:9:
padding: 3px 2px 3px 18px;
On 2012/05/29 22:17:36, rltoscano wrote:
> On 2012/05/29 21:25:50, Dan Beam wrote:
> > does this work in RTL? seems like you'd wanna do:
> > 
> >   -webkit-padding-end: 2px;
> >   -webkit-padding-start: 18px;
> >   padding-bottom: 3px;
> >   padding-top: 3px;
> > 
> > unless this is not affected by text direction.
> 
> There are a few other things that need to change. Like the icon should be on
the
> right instead of the left. How far should we support RTL? I can understand
> changing text layout direction, but it seems like we would also need to change
> icon layout. Should this be done in this CL?

Supporting RTL means having a reasonable look in right-to-left text directions
(e.g. Hebrew, Arabic). If you make all the padding/margins LTR only it won't
look reasonable / right. Have you tried launching chrome with LANGUAGE=ar or
LANGUAGE=he (if using Linux)?

[Dan Beam, 2012-05-29 23:36:09]

nits

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list_item.css
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.css:21:
opacity: .4;
On 2012/05/29 22:17:36, rltoscano wrote:
> On 2012/05/29 21:25:50, Dan Beam wrote:
> > optional nit: I don't think we have an explicit rule either way, but most of
> the
> > code that I've seen in webui has 0.X rather than .X. If print_preview's
local
> > style disagrees, follow it, otherwise you may want to s/.4/0.4/
> 
> It looks like its not entirely consistent. I've found some .X in
> shared/css/tree.css and shared/css/widgets.css. Also if we're concerned about
> conserving bytes (as your comment about copyright comment shows) maybe we
should
> strip the leading 0.

I'd argue that 90% of these values being [0-9]\.[0-9]+ vs. \.[0-9]+ would be
enough, but like I said - optional.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/component.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/component.js:77: if (!child.isInDocument
&& child.getElement()) {
general optional nit: no curlies in all one line if/else/else if conditionals
and bodies are 1 line (at least in all the webui code I've ever worked in). ex:

  if (this)
    doSomething(this);
  else if (that)
    doSomethingElse(that);
  else
    meInstead();

ignore if this isn't the case in print_preview, but it is in WebUI everywhere
else...

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/data/cloud_capabilities.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/cloud_capabilities.js:193: * @type
{!Object<!CloudCapabilities.Format, string>}
I think you mean

  {!Object.<!CloudCapabilities.Format, string>}

notice the .

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/destination_store.js:184: if
(candidate != null) {
if (candidate) unless a falsey value is useful,
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...,
here and through out.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/destination_store.js:380: var
localDestinations = [];
observation: looks the same as Array#map

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/destination_store.js:396: // TODO
There may be a race condition here. This method is assumed to
// TODO(username) otherwise this will probably never get done.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/data/user_info.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/user_info.js:27: * @type
{cloudprint.CloudPrintInterface}
@type {?cloudprint.CloudPrintInterface}

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/user_info.js:39: };
general comment: every @constructor function declaration has an unnecessary ;
after them (there are a lot).

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/native_layer.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/native_layer.js:695: *     type or {@code
null} if not initially set.
update comment if ? no longer applies

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File
chrome/browser/resources/print_preview/previewarea/margin_control_container.js
(right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/previewarea/margin_control_container.js:34:
* @type {!Object<
!Object.<

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_utils.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_utils.js:199: * @param
{Array.<object>} array Array to check for item.
Array.<Object>, though I don't see anything specific about this function to only
handle array of objects. You could probably just use:

  @param {!Array} array
  @param {*} item

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/search/destination_list.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_list.js:83:
this.noDestinationsMessageEl_ = null;
?HTMLElement

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_list.js:183:
this.noDestinationsMessageEl_ = null;
is there a reason this nulling is required?

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/search/destination_search.js
(right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_search.js:78: * @type
{HTMLElement}
?HTMLElement

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/search/search_box.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/search_box.js:25: * @type
{HTMLInputElement}
?HTMLInputElement

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/search_box.js:50: * @type {number}
@type {!number}
@const

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/settings/destination_settings.js
(right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/settings/destination_settings.js:136: //
Export
probably not necessary...

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/settings/page_settings.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/settings/page_settings.js:35: * @type
{HTMLInputElement}
?HTMLInputElement

[Robert Toscano, 2012-05-30 21:07:58]

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list.css (right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list.css:6:
.destination-list-header {
On 2012/05/29 22:30:18, Dan Beam wrote:
> On 2012/05/29 22:17:36, rltoscano wrote:
> > On 2012/05/29 21:25:50, Dan Beam wrote:
> > > why not .destination-list header?
> > 
> > Then I would have to decorate my element like
> > 
> >   <tag class="destination-list header">
> > 
> > but what if in one of the style-sheets I import, there exists a class
> ".header"
> > CSS definition? I would unintentionally import those styles into an element
> > where it shouldn't. So instead, I prefix all classes from the same element.
> This
> > is my answer to several of the other related comments. I'll use the label
> > css-paranoia.
> 
>   .destination-list header
> 
> would correspond to:
> 
>   <div class="destination-list">
>     <header></header><!-- NOTE: header tag, not class="header" -->
>   </div>
> 
> like what you're already doing.

But wouldn't you have the same problem? You might import a style-sheet which has
a "header" element CSS definition, and your header element would get that
styling unintentionally.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list.html
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list.html:1: <div
id="destination-list-template" style="display: none;">
On 2012/05/29 22:30:18, Dan Beam wrote:
> On 2012/05/29 22:17:36, rltoscano wrote:
> > On 2012/05/29 21:25:50, Dan Beam wrote:
> > > s/style="display: none;"/hidden/
> > 
> > These are different. "hidden" will render the element hidden, but it still
> will
> > have an effect on the DOM element layout. display=none will be like if the
> > element wasn't even in the DOM tree.
> 
> [hidden] is just display: none; but from user-agent styles (whereas this code
is
> more of the "application level").  Just in case it was ambiguous, I meant:
> 
>   <div id="destination-list-template" hidden>
> 
> It's the same thing, I promise. [hidden] is not visibility: hidden;.
> 
> Additionally, I think you meant "render tree" not DOM tree, as you can still
use
> the DOM/BOM to reference / affect [hidden]/display: none; nodes, it just
usually
> isn't necessary.

Sorry, I thought "hidden" = "visibility: hidden". Thanks for the tip! And yea, I
meant "render tree".

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list_item.css
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.css:9:
padding: 3px 2px 3px 18px;
On 2012/05/29 22:30:18, Dan Beam wrote:
> On 2012/05/29 22:17:36, rltoscano wrote:
> > On 2012/05/29 21:25:50, Dan Beam wrote:
> > > does this work in RTL? seems like you'd wanna do:
> > > 
> > >   -webkit-padding-end: 2px;
> > >   -webkit-padding-start: 18px;
> > >   padding-bottom: 3px;
> > >   padding-top: 3px;
> > > 
> > > unless this is not affected by text direction.
> > 
> > There are a few other things that need to change. Like the icon should be on
> the
> > right instead of the left. How far should we support RTL? I can understand
> > changing text layout direction, but it seems like we would also need to
change
> > icon layout. Should this be done in this CL?
> 
> Supporting RTL means having a reasonable look in right-to-left text directions
> (e.g. Hebrew, Arabic). If you make all the padding/margins LTR only it won't
> look reasonable / right. Have you tried launching chrome with LANGUAGE=ar or
> LANGUAGE=he (if using Linux)?

Done.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.css:20:
margin-right: 8px;
On 2012/05/29 21:25:50, Dan Beam wrote:
> same question about RTL

Done.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list_item.css:21:
opacity: .4;
On 2012/05/29 23:36:09, Dan Beam wrote:
> On 2012/05/29 22:17:36, rltoscano wrote:
> > On 2012/05/29 21:25:50, Dan Beam wrote:
> > > optional nit: I don't think we have an explicit rule either way, but most
of
> > the
> > > code that I've seen in webui has 0.X rather than .X. If print_preview's
> local
> > > style disagrees, follow it, otherwise you may want to s/.4/0.4/
> > 
> > It looks like its not entirely consistent. I've found some .X in
> > shared/css/tree.css and shared/css/widgets.css. Also if we're concerned
about
> > conserving bytes (as your comment about copyright comment shows) maybe we
> should
> > strip the leading 0.
> 
> I'd argue that 90% of these values being [0-9]\.[0-9]+ vs. \.[0-9]+ would be
> enough, but like I said - optional.

Done.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_search.css
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_search.css:48:
padding: 12px 44px 12px 12px;
On 2012/05/29 21:25:50, Dan Beam wrote:
> rtl

Done.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_search.css:63:
margin-right: 4px;
On 2012/05/29 21:25:50, Dan Beam wrote:
> rtl

Done.

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/destination_settings.css
(right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/destination_settings.css:12:
margin-right: 8px;
On 2012/05/29 21:25:50, Dan Beam wrote:
> same question about RTL

Done.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/component.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/component.js:77: if (!child.isInDocument
&& child.getElement()) {
I'd rather keep as is since lack of curlies can be error-prone.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/data/cloud_capabilities.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/cloud_capabilities.js:193: * @type
{!Object<!CloudCapabilities.Format, string>}
On 2012/05/29 23:36:10, Dan Beam wrote:
> I think you mean
> 
>   {!Object.<!CloudCapabilities.Format, string>}
> 
> notice the .

Done.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/destination_store.js:184: if
(candidate != null) {
On 2012/05/29 23:36:10, Dan Beam wrote:
> if (candidate) unless a falsey value is useful,
>
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...,
> here and through out.

There are a lot of places in this CL and in other files that would need to be
changed. So in an effort to not grow this CL too large, I'll add this fix with
the renaming CSS classes and extra semi-colons CL.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/destination_store.js:380: var
localDestinations = [];
On 2012/05/29 23:36:10, Dan Beam wrote:
> observation: looks the same as Array#map

Cool thanks!

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/destination_store.js:396: // TODO
There may be a race condition here. This method is assumed to
On 2012/05/29 23:36:10, Dan Beam wrote:
> // TODO(username) otherwise this will probably never get done.

Done.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/data/user_info.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/user_info.js:27: * @type
{cloudprint.CloudPrintInterface}
On 2012/05/29 23:36:10, Dan Beam wrote:
> @type {?cloudprint.CloudPrintInterface}

You only need the ? if the type is a primitive type. E.g. @type {?number} or
@type {?string}, but not @type {?Object} since @type {Object} is equivalent to
@type {?Object}

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/user_info.js:39: };
On 2012/05/29 23:36:10, Dan Beam wrote:
> general comment: every @constructor function declaration has an unnecessary ;
> after them (there are a lot).

I'll remove these in the next CL? (Along with the CSS name refactoring)

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/native_layer.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/native_layer.js:695: *     type or {@code
null} if not initially set.
On 2012/05/29 23:36:10, Dan Beam wrote:
> update comment if ? no longer applies

The reason why I removed the '?' was because it was extraneous, not because it
could no longer be null.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File
chrome/browser/resources/print_preview/previewarea/margin_control_container.js
(right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/previewarea/margin_control_container.js:34:
* @type {!Object<
On 2012/05/29 23:36:10, Dan Beam wrote:
> !Object.<

Done.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_utils.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_utils.js:199: * @param
{Array.<object>} array Array to check for item.
On 2012/05/29 23:36:10, Dan Beam wrote:
> Array.<Object>, though I don't see anything specific about this function to
only
> handle array of objects. You could probably just use:
> 
>   @param {!Array} array
>   @param {*} item

Done.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/search/destination_list.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_list.js:83:
this.noDestinationsMessageEl_ = null;
On 2012/05/29 23:36:10, Dan Beam wrote:
> ?HTMLElement

'?' is extraneous.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_list.js:183:
this.noDestinationsMessageEl_ = null;
On 2012/05/29 23:36:10, Dan Beam wrote:
> is there a reason this nulling is required?

So this object can release its reference to the DOM element and so that the DOM
element can get garbage collected.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/search/destination_search.js
(right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_search.js:78: * @type
{HTMLElement}
On 2012/05/29 23:36:10, Dan Beam wrote:
> ?HTMLElement

The '?' is extraneous.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/search/search_box.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/search_box.js:25: * @type
{HTMLInputElement}
On 2012/05/29 23:36:10, Dan Beam wrote:
> ?HTMLInputElement

The '?' is extraneous.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/search_box.js:50: * @type {number}
On 2012/05/29 23:36:10, Dan Beam wrote:
> @type {!number}
> @const

The "!" is extraneous, but I'll add the const.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/settings/destination_settings.js
(right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/settings/destination_settings.js:136: //
Export
Was done everywhere else in print preview code, so keeping for consistency.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/settings/page_settings.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/settings/page_settings.js:35: * @type
{HTMLInputElement}
On 2012/05/29 23:36:10, Dan Beam wrote:
> ?HTMLInputElement

The '?' is not necessary.

[Dan Beam, 2012-05-31 00:17:58]

lgtm w/nits follow up CLs for other nits

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/destination_list.css (right):

http://codereview.chromium.org/10450022/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/destination_list.css:6:
.destination-list-header {
On 2012/05/30 21:08:00, Robert Toscano wrote:
> On 2012/05/29 22:30:18, Dan Beam wrote:
> > On 2012/05/29 22:17:36, rltoscano wrote:
> > > On 2012/05/29 21:25:50, Dan Beam wrote:
> > > > why not .destination-list header?
> > > 
> > > Then I would have to decorate my element like
> > > 
> > >   <tag class="destination-list header">
> > > 
> > > but what if in one of the style-sheets I import, there exists a class
> > ".header"
> > > CSS definition? I would unintentionally import those styles into an
element
> > > where it shouldn't. So instead, I prefix all classes from the same
element.
> > This
> > > is my answer to several of the other related comments. I'll use the label
> > > css-paranoia.
> > 
> >   .destination-list header
> > 
> > would correspond to:
> > 
> >   <div class="destination-list">
> >     <header></header><!-- NOTE: header tag, not class="header" -->
> >   </div>
> > 
> > like what you're already doing.
> 
> But wouldn't you have the same problem? You might import a style-sheet which
has
> a "header" element CSS definition, and your header element would get that
> styling unintentionally.

This isn't realistic and the answer to fix the other code, like we talked about.
We don't embed third party code into print preview or webui so you can easily
change the "bad" code to suit your needs.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/component.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/component.js:77: if (!child.isInDocument
&& child.getElement()) {
On 2012/05/30 21:08:00, Robert Toscano wrote:
> I'd rather keep as is since lack of curlies can be error-prone.

I agree, just telling you what the majority does.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/destination_store.js:184: if
(candidate != null) {
On 2012/05/30 21:08:00, Robert Toscano wrote:
> On 2012/05/29 23:36:10, Dan Beam wrote:
> > if (candidate) unless a falsey value is useful,
> >
>
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...,
> > here and through out.
> 
> There are a lot of places in this CL and in other files that would need to be
> changed. So in an effort to not grow this CL too large, I'll add this fix with
> the renaming CSS classes and extra semi-colons CL.

sg

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/data/user_info.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/user_info.js:27: * @type
{cloudprint.CloudPrintInterface}
On 2012/05/30 21:08:00, Robert Toscano wrote:
> On 2012/05/29 23:36:10, Dan Beam wrote:
> > @type {?cloudprint.CloudPrintInterface}
> 
> You only need the ? if the type is a primitive type. E.g. @type {?number} or
> @type {?string}, but not @type {?Object} since @type {Object} is equivalent to
> @type {?Object}

Cool, didn't know that. It does annoy me that ! and ? operators' behaviors
differ for built-ins.

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/search/destination_list.js (right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_list.js:183:
this.noDestinationsMessageEl_ = null;
On 2012/05/30 21:08:00, Robert Toscano wrote:
> On 2012/05/29 23:36:10, Dan Beam wrote:
> > is there a reason this nulling is required?
> 
> So this object can release its reference to the DOM element and so that the
DOM
> element can get garbage collected.

If the ref count hits 0 it should do this automagically anyways, you shouldn't
need to do this yourself. The only reason (AFAIK) that you see this done in some
code / libraries is that JScript 5.6-5.8 had bad garbage collectors. If I'm
misunderstanding (like if this object is re-used or something), ignore, just an
observation that we're trying to do pseudo-memory management in a GC'd
language...

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/settings/destination_settings.js
(right):

http://codereview.chromium.org/10450022/diff/2047/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/settings/destination_settings.js:136: //
Export
On 2012/05/30 21:08:00, Robert Toscano wrote:
> Was done everywhere else in print preview code, so keeping for consistency.

ok, cool

http://codereview.chromium.org/10450022/diff/16007/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/search_box.css (right):

http://codereview.chromium.org/10450022/diff/16007/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/search_box.css:15: position:
absolute;
right: 8px; should work

http://codereview.chromium.org/10450022/diff/16007/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/search_box.css:22: right: 8px;
in this case you can simply put `right` in the LTR-based rule,
http://jsfiddle.net/7xNSM/

[Robert Toscano, 2012-05-31 00:59:18]

http://codereview.chromium.org/10450022/diff/16007/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/search/search_box.css (right):

http://codereview.chromium.org/10450022/diff/16007/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/search_box.css:15: position:
absolute;
On 2012/05/31 00:17:58, Dan Beam wrote:
> right: 8px; should work

Done.

http://codereview.chromium.org/10450022/diff/16007/chrome/browser/resources/p...
chrome/browser/resources/print_preview/search/search_box.css:22: right: 8px;
On 2012/05/31 00:17:58, Dan Beam wrote:
> in this case you can simply put `right` in the LTR-based rule,
> http://jsfiddle.net/7xNSM/

Done.

[commit-bot: I haz the power, 2012-05-31 01:08:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rltoscano@google.com/10450022/2056

[commit-bot: I haz the power, 2012-05-31 04:50:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rltoscano@google.com/10450022/2061

[commit-bot: I haz the power, 2012-05-31 05:03:32]

Try job failure for 10450022-2061 (retry) on linux_chromeos for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-05-31 05:40:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rltoscano@google.com/10450022/16089

[commit-bot: I haz the power, 2012-05-31 07:31:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rltoscano@google.com/10450022/6135

[commit-bot: I haz the power, 2012-05-31 07:43:00]

Try job failure for 10450022-6135 (retry) on mac_rel for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-31 18:10:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rltoscano@google.com/10450022/6135

[commit-bot: I haz the power, 2012-05-31 18:14:24]

Change committed as 139820

[Vitaly Pavlenko, 2014-09-18 02:39:47]

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

[Vitaly Pavlenko, 2014-09-18 02:39:48]

https://chromiumcodereview.appspot.com/10450022/diff/6135/chrome/browser/reso...
File chrome/browser/resources/print_preview/data/cloud_parsers.js (right):

https://chromiumcodereview.appspot.com/10450022/diff/6135/chrome/browser/reso...
chrome/browser/resources/print_preview/data/cloud_parsers.js:97: return
print_preview.Destination.Type.GOOGLE_PROMOTED;
There's no such thing like 

print_preview.Destination.Type.GOOGLE_PROMOTED

right now. Is everything with CloudType_ a dead code?