[Password manager] Human readable origins for Android credentials on chrome://settings/passwords

chrome/browser/resources/options/password_manager_list.js

Index: chrome/browser/resources/options/password_manager_list.js
diff --git a/chrome/browser/resources/options/password_manager_list.js b/chrome/browser/resources/options/password_manager_list.js
index 58bd5efce0d831ca12c006fbb9a0b77277e8f14e..fa28dcba80fd4e410ec0dbed65dd02d054297789 100644
--- a/chrome/browser/resources/options/password_manager_list.js
+++ b/chrome/browser/resources/options/password_manager_list.js
@@ -10,9 +10,10 @@ cr.define('options.passwordManager', function() {
 
   // The following constants should be synchronized with the constants in
   // chrome/browser/ui/webui/options/password_manager_handler.cc.
-  /** @const */ var ORIGIN_FIELD = 'origin';
-  /** @const */ var SHOWN_URL_FIELD = 'shownUrl';
+  /** @const */ var URL_FIELD = 'url';
+  /** @const */ var SHOWN_ORIGIN_FIELD = 'shownOrigin';
   /** @const */ var IS_ANDROID_URI_FIELD = 'isAndroidUri';
+  /** @const */ var IS_CLICKABLE_FIELD = 'isClickable';
   /** @const */ var IS_SECURE_FIELD = 'isSecure';
   /** @const */ var USERNAME_FIELD = 'username';
   /** @const */ var PASSWORD_FIELD = 'password';
@@ -42,15 +43,15 @@ cr.define('options.passwordManager', function() {
   }
 
   /**
-   * Returns title for password's origin. If the origin is  Android URI, returns
-   * the origin as it is. Removes the scheme if the url is insecure and removes
-   * trailing punctuation symbols.
+   * Returns title for password's origin. If the origin is not clickable,
+   * returns the origin as it is. For clickable origins, removes the scheme if
+   * the url is insecure and removes trailing punctuation symbols.
    * @param {Object} item A dictionary of data on the list item.
    * @return {string} The title for password's origin.
    */
   function getTitleForPasswordOrigin(item) {
     var title = item.url;
-    if (item.isAndroidUri)
+    if (!item.isClickable)
       return title;
     if (!item.isSecure) {
       var ind = title.indexOf('://');
@@ -74,16 +75,20 @@ cr.define('options.passwordManager', function() {
    */
   function createUrlLink(item, urlDiv) {
     var urlLink;
-    if (!item.isAndroidUri) {
+    if (item.isClickable) {
       urlLink = item.ownerDocument.createElement('a');
       urlLink.href = item.url;
       urlLink.setAttribute('target', '_blank');
-      urlLink.textContent = item.shownUrl.split('').reverse().join('');
-
+      var linkText = item.shownOrigin;
+      // Since the string will be reversed for RTL direction, the string starts
+      // with closed bracket and ends with open bracket.
+      if (item.isAndroidUri)
+        linkText += ' )Android(';

[Evan Stade, 2016/03/15 18:48:58]

how do you know this is RTL? And wouldn't proper bidi handling mean that the
parens take on the directionality of the closest strongly directional character
('A' or 'd')

[kolos1, 2016/03/15 19:31:34]

This is always RTL, because of CSS class left_elided_url
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...)

Sorry, I don't understand the rest of comment. What do you mean? 

[kolos1, 2016/03/15 19:36:16]

I think I understand. The parens should inherit direction from the closest
character, right? I didn't see anything like that. 

[Evan Stade, 2016/03/15 20:05:26]

OK, so, you're reversing the string in JS then overriding the bidi handling. Now
you have to use the wrong semantic brackets. I don't like this solution. How
does it affect screen readers? Maybe dbeam has some ideas.

[kolos1, 2016/03/15 21:00:41]

Yes, the solution is a bit confusing, because we had to override bidi for
hostnames elided from the left. 

For screen readers, it will look like "accounts.nytimes.com (Android)". As I
understand, it will be a temporary solution since the password page will be
converted into material design.  

[Dan Beam, 2016/03/17 22:24:08]

Ultimately, I think anything involving text concatenation should be left up to
translators and this should use loadTimeData.getStringF().  As in: the "
(Android)" should be in a translation, like:

  <message ...>
    <ph name="URL">$1<ex>android.com</ex></ph> (Android)
  </message>

P.S. I have written a JS-based ellider and it does kinda suck.
https://github.com/danbeam/ellipsis (this could be updated to do left-side
elliding, but meh)

+      urlLink.textContent = linkText.split('').reverse().join('');
       urlDiv.classList.add('left-elided-url');
     } else {
       urlLink = item.ownerDocument.createElement('span');
-      urlLink.textContent = item.shownUrl;
+      urlLink.textContent = item.shownOrigin;
     }
     urlLink.addEventListener('focus', function() {
       item.handleFocus();
@@ -266,21 +271,21 @@ cr.define('options.passwordManager', function() {
      * @type {string}
      */
     get url() {
-      return this.dataItem[ORIGIN_FIELD];
+      return this.dataItem[URL_FIELD];
     },
     set url(url) {
-      this.dataItem[ORIGIN_FIELD] = url;
+      this.dataItem[URL_FIELD] = url;
     },
 
     /**
-     * Get and set the shown url for the entry.
+     * Get and set the shown origin for the entry.
      * @type {string}
      */
-    get shownUrl() {
-      return this.dataItem[SHOWN_URL_FIELD];
+    get shownOrigin() {
+      return this.dataItem[SHOWN_ORIGIN_FIELD];
     },
-    set shownUrl(shownUrl) {
-      this.dataItem[SHOWN_URL_FIELD] = shownUrl;
+    set shownOrigin(shownOrigin) {
+      this.dataItem[SHOWN_ORIGIN_FIELD] = shownOrigin;
     },
 
     /**
@@ -295,6 +300,17 @@ cr.define('options.passwordManager', function() {
     },
 
     /**
+     * Get and set whether the origin is clickable.
+     * @type {boolean}
+     */
+    get isClickable() {
+      return this.dataItem[IS_CLICKABLE_FIELD];
+    },
+    set isClickable(isClickable) {
+      this.dataItem[IS_CLICKABLE_FIELD] = isClickable;
+    },
+
+    /**
      * Get and set whether the origin uses secure scheme.
      * @type {boolean}
      */
@@ -402,21 +418,21 @@ cr.define('options.passwordManager', function() {
      * @type {string}
      */
     get url() {
-      return this.dataItem[ORIGIN_FIELD];
+      return this.dataItem[URL_FIELD];
     },
     set url(url) {
-      this.dataItem[ORIGIN_FIELD] = url;
+      this.dataItem[URL_FIELD] = url;
     },
 
     /**
-     * Get and set the shown url for the entry.
+     * Get and set the shown origin for the entry.
      * @type {string}
      */
-    get shownUrl() {
-      return this.dataItem[SHOWN_URL_FIELD];
+    get shownOrigin() {
+      return this.dataItem[SHOWN_ORIGIN_FIELD];
     },
-    set shownUrl(shownUrl) {
-      this.dataItem[SHOWN_URL_FIELD] = shownUrl;
+    set shownOrigin(shownOrigin) {
+      this.dataItem[SHOWN_ORIGIN_FIELD] = shownOrigin;
     },
 
     /**
@@ -431,6 +447,17 @@ cr.define('options.passwordManager', function() {
     },
 
     /**
+     * Get and set whether the origin is clickable.
+     * @type {boolean}
+     */
+    get isClickable() {
+      return this.dataItem[IS_CLICKABLE_FIELD];
+    },
+    set isClickable(isClickable) {
+      this.dataItem[IS_CLICKABLE_FIELD] = isClickable;
+    },
+
+    /**
      * Get and set whether the origin uses secure scheme.
      * @type {boolean}
      */
@@ -556,8 +583,10 @@ cr.define('options.passwordManager', function() {
     PasswordExceptionsListItem: PasswordExceptionsListItem,
     PasswordsList: PasswordsList,
     PasswordExceptionsList: PasswordExceptionsList,
-    ORIGIN_FIELD: ORIGIN_FIELD,
-    SHOWN_URL_FIELD: SHOWN_URL_FIELD,
+    URL_FIELD: URL_FIELD,
+    SHOWN_ORIGIN_FIELD: SHOWN_ORIGIN_FIELD,
+    IS_ANDROID_URI_FIELD: IS_ANDROID_URI_FIELD,
+    IS_CLICKABLE_FIELD: IS_CLICKABLE_FIELD,
     IS_SECURE_FIELD: IS_SECURE_FIELD,
     USERNAME_FIELD: USERNAME_FIELD,
     PASSWORD_FIELD: PASSWORD_FIELD,