Misc. cleanup

chrome/browser/resources/bookmark_manager/css/bmm.css

Index: chrome/browser/resources/bookmark_manager/css/bmm.css
diff --git a/chrome/browser/resources/bookmark_manager/css/bmm.css b/chrome/browser/resources/bookmark_manager/css/bmm.css
index 1d039178d0437b63bd843a33079f9844d9bcfdd4..a161392748ed0ce65493e312056f110cd369e658 100644
--- a/chrome/browser/resources/bookmark_manager/css/bmm.css
+++ b/chrome/browser/resources/bookmark_manager/css/bmm.css
@@ -44,17 +44,52 @@ list > * > * {
   box-sizing: border-box;
   overflow: hidden;
   text-overflow: ellipsis;
-  white-space: pre; /* Don't collapse whitespace */
+  white-space: pre;
 }
 
-list > * > .label {
-  -webkit-transition: all 150ms;
+list .label {

[Dan Beam, 2016/09/01 06:11:04]

so are you absolutely sure that this is functionally the same selector?  as in:
the only .label elements are 2 levels below <list>?

[Peter Kasting, 2016/09/01 07:13:31]

A codesearch for "className = 'label" shows that only BookmarkListItem in
bookmark_list.js applies here.  AFAIK the bookmark manager's right pane only
ever shows a list of BookmarkListItem, which would mean yes, these two selectors
are the same.

If, hypothetically, there was another place that .label elements could appear,
then I think the old code would have been wrong to not do this for those as
well.  And the selectors below that assume .labels inside .folders only appear
as children of lists are wrong too -- they should be "list .folder > .label"
instead, right?

   color: black;
-  display: inline-block; /* We need to use inline-block here due to RTL. */
+  display: inline-block; /* Makes the image start-align in RTL. */
+}
+
+html[dir=rtl] list .label {
+  background-position: 100% 50%;
+}
+
+<if expr="is_macosx">
+list > .folder > .label,
+.tree-label,
+.tree-row[may-have-children] > .tree-label,
+.tree-item[expanded] > .tree-row > .tree-label {
+  background-image: -webkit-image-set(
+      url(../../../../app/theme/default_100_percent/mac/bookmark_bar_folder.png) 1x,
+      url(../../../../app/theme/default_200_percent/mac/bookmark_bar_folder.png) 2x);
 }
+</if>
 
-list > * > .url {
-  direction: ltr; /* URLs always read LTR */
+<if expr="not is_macosx">
+list > .folder > .label {
+  background-image: -webkit-image-set(
+      url(../../../../../ui/resources/default_100_percent/common/folder_closed.png) 1x,
+      url(../../../../../ui/resources/default_200_percent/common/folder_closed.png) 2x);
+}
+
+html[dir=rtl] list > .folder > .label {
+  background-image: -webkit-image-set(
+      url(../../../../../ui/resources/default_100_percent/common/folder_closed_rtl.png) 1x,
+      url(../../../../../ui/resources/default_200_percent/common/folder_closed_rtl.png) 2x);
+}
+</if>
+
+/* We need to ensure that even empty labels take up space, or the row could be
+ * the wrong height. */
+list .label:empty::after {
+  content: ' ';
+  white-space: pre;
+}
+
+list .url {
+  direction: ltr;
   display: none;
   flex: 1;
 }
@@ -68,31 +103,20 @@ list > [selected] > .url {
  * |div.url||div.label| - so we need padding at the right of URL, not at the
  * left. And since url is always LTR, that is padding at the end, not the start.
  */
-html[dir=rtl] .url {
+html[dir=rtl] list .url {
   -webkit-padding-end: 20px;
   -webkit-padding-start: 0;
 }
 
-html[dir=rtl] list .label {
-  background-position: 100% 50%;
-}
-
-list > .folder > .label {
-  background-image: -webkit-image-set(
-      url(../../../../../ui/resources/default_100_percent/common/folder_closed.png) 1x,
-      url(../../../../../ui/resources/default_200_percent/common/folder_closed.png) 2x);
-}
-
-/* We need to ensure that even empty labels take up space */

[Dan Beam, 2016/09/01 06:11:04]

^ is this comment incorrect?

[Peter Kasting, 2016/09/01 07:13:31]

You'll see that when I moved the .label portion of the block up, I expanded on
the comment to say why we need to ensure this.

For the .url part, I don't understand why this is needed, hence the "why did we
have these?" question.  From my initial email on this CR: "I can't figure out
why the URL field needs to always be nonempty, unless maybe the URL font could
potentially be taller than the font used for the bookmark name, so without this,
hovering a line could enlarge it?  Can you figure out why we had these?  I'd
like to delete them but don't want to break anything."

[Dan Beam, 2016/09/02 23:17:23]

Acknowledged.

-list > * > .label:empty::after,
-list > * > .url:empty::after {
+/* Why did we have these?
+list .url:empty::after {

[Dan Beam, 2016/09/02 23:17:23]

delete this instead of commenting it out

[Peter Kasting, 2016/09/02 23:22:22]

Yup

   content: ' ';
   white-space: pre;
 }
-
 list > .folder > .url:empty::after {
   content: '';
 }
+*/
 
 list > * > button {
   -webkit-transition: opacity 150ms;
@@ -183,23 +207,6 @@ list [editing] input {
 
 /* end editing */
 
-html[dir=rtl] list > .folder > .label {
-  background-image: -webkit-image-set(
-      url(../../../../../ui/resources/default_100_percent/common/folder_closed_rtl.png) 1x,
-      url(../../../../../ui/resources/default_200_percent/common/folder_closed_rtl.png) 2x);
-}
-
-<if expr="is_macosx">
-list > .folder > .label,
-.tree-label,
-.tree-row[may-have-children] > .tree-label,
-.tree-item[expanded] > .tree-row > .tree-label {
-  background-image: -webkit-image-set(
-      url(../../../../app/theme/default_100_percent/mac/bookmark_bar_folder.png) 1x,
-      url(../../../../app/theme/default_200_percent/mac/bookmark_bar_folder.png) 2x);
-}
-</if>
-
 .main {
   border-top: 1px solid rgb(156, 194, 239);
   display: flex;