MD Settings: About: Show connectionTypes message

MD Settings: About: Show connectionTypes message

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

Review-Url: https://codereview.chromium.org/2698133003
Cr-Commit-Position: refs/heads/master@{#451826}
Committed: https://chromium.googlesource.com/chromium/src/+/875d1fb2901159381fb28c278148432c8da568b5

[stevenjb, 2017-02-16 20:51:19]

Description was changed from

==========
MD Settings: About: Show connectionTypes message

BUG=689715
==========

to

==========
MD Settings: About: Show connectionTypes message

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

[stevenjb, 2017-02-16 20:52:00]

stevenjb@chromium.org changed reviewers:
+ dpapad@chromium.org

[dpapad, 2017-02-16 21:22:27]

https://codereview.chromium.org/2698133003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/about_page/about_page.html (right):

https://codereview.chromium.org/2698133003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/about_page/about_page.html:113: <div
id="updateStatusMessage" hidden="[[!showConnectionType_]]"
Having two elements with the same ID in the DOM does not seem right. For example
which one is returned by this.$.updateStatusMessage?

Can you instead update the getUpdateStatusMessage_ function to return the
correct string? It already has ChromeOS specific logic inside it, and it already
depends on the currentUpdateStatusEvent_.

Also should we be updating the tests at
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/about_pa...

[stevenjb, 2017-02-17 01:03:11]

Patchset #2 (id:20001) has been deleted

[stevenjb, 2017-02-17 01:06:01]

https://codereview.chromium.org/2698133003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/about_page/about_page.html (right):

https://codereview.chromium.org/2698133003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/about_page/about_page.html:113: <div
id="updateStatusMessage" hidden="[[!showConnectionType_]]"
On 2017/02/16 21:22:27, dpapad wrote:
> Having two elements with the same ID in the DOM does not seem right. For
example
> which one is returned by this.$.updateStatusMessage?
> 
> Can you instead update the getUpdateStatusMessage_ function to return the
> correct string? It already has ChromeOS specific logic inside it, and it
already
> depends on the currentUpdateStatusEvent_.
> 
> Also should we be updating the tests at
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/about_pa...

Oops. Copy/paste bug. I'll change the id and add a test.

I don't understand your "Can you instead " suggestion. Are you suggesting
embedding multiple divs in the result? I wouldn't be super fond of that
approach, and testing becomes more fragile.

[dpapad, 2017-02-17 01:14:38]

https://codereview.chromium.org/2698133003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/about_page/about_page.html (right):

https://codereview.chromium.org/2698133003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/about_page/about_page.html:113: <div
id="updateStatusMessage" hidden="[[!showConnectionType_]]"
On 2017/02/17 at 01:06:01, stevenjb wrote:
> On 2017/02/16 21:22:27, dpapad wrote:
> > Having two elements with the same ID in the DOM does not seem right. For
example
> > which one is returned by this.$.updateStatusMessage?
> > 
> > Can you instead update the getUpdateStatusMessage_ function to return the
> > correct string? It already has ChromeOS specific logic inside it, and it
already
> > depends on the currentUpdateStatusEvent_.
> > 
> > Also should we be updating the tests at
> >
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/about_pa...
> 
> Oops. Copy/paste bug. I'll change the id and add a test.
> 
> I don't understand your "Can you instead " suggestion. Are you suggesting
embedding multiple divs in the result? I wouldn't be super fond of that
approach, and testing becomes more fragile.

Conceptually the connection types error message is part of the "update status
message" and getUpdateStatusMessage_ already returns HTML code and interpreted
as innerHTML. What justifies having the connection types related message as a
separate message instead of including it in the return value of
getUpdateStatusMessage_()?
Regarding testing, asserting that this.$.updateStatusMessage.textContent
contains some substring expected to exist in the new error message seems solid
enough, no?

[stevenjb, 2017-02-17 01:28:52]

https://codereview.chromium.org/2698133003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/about_page/about_page.html (right):

https://codereview.chromium.org/2698133003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/about_page/about_page.html:113: <div
id="updateStatusMessage" hidden="[[!showConnectionType_]]"
On 2017/02/17 01:14:38, dpapad wrote:
> On 2017/02/17 at 01:06:01, stevenjb wrote:
> > On 2017/02/16 21:22:27, dpapad wrote:
> > > Having two elements with the same ID in the DOM does not seem right. For
> example
> > > which one is returned by this.$.updateStatusMessage?
> > > 
> > > Can you instead update the getUpdateStatusMessage_ function to return the
> > > correct string? It already has ChromeOS specific logic inside it, and it
> already
> > > depends on the currentUpdateStatusEvent_.
> > > 
> > > Also should we be updating the tests at
> > >
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/about_pa...
> > 
> > Oops. Copy/paste bug. I'll change the id and add a test.
> > 
> > I don't understand your "Can you instead " suggestion. Are you suggesting
> embedding multiple divs in the result? I wouldn't be super fond of that
> approach, and testing becomes more fragile.
> 
> Conceptually the connection types error message is part of the "update status
> message" and getUpdateStatusMessage_ already returns HTML code and interpreted
> as innerHTML. What justifies having the connection types related message as a
> separate message instead of including it in the return value of
> getUpdateStatusMessage_()?
> Regarding testing, asserting that this.$.updateStatusMessage.textContent
> contains some substring expected to exist in the new error message seems solid
> enough, no?

I feel like there is a difference between embedding strings that may include <b>
tags and generating divs, but it's hardly worth depating as far as I am
concerned so I will just change it.

[dpapad, 2017-02-17 02:44:28]

LGTM

https://codereview.chromium.org/2698133003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/about_page/about_page.html (right):

https://codereview.chromium.org/2698133003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/about_page/about_page.html:113: <div
id="updateStatusMessage" hidden="[[!showConnectionType_]]"
On 2017/02/17 at 01:28:51, stevenjb wrote:
> On 2017/02/17 01:14:38, dpapad wrote:
> > On 2017/02/17 at 01:06:01, stevenjb wrote:
> > > On 2017/02/16 21:22:27, dpapad wrote:
> > > > Having two elements with the same ID in the DOM does not seem right. For
> > example
> > > > which one is returned by this.$.updateStatusMessage?
> > > > 
> > > > Can you instead update the getUpdateStatusMessage_ function to return
the
> > > > correct string? It already has ChromeOS specific logic inside it, and it
> > already
> > > > depends on the currentUpdateStatusEvent_.
> > > > 
> > > > Also should we be updating the tests at
> > > >
> >
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/about_pa...
> > > 
> > > Oops. Copy/paste bug. I'll change the id and add a test.
> > > 
> > > I don't understand your "Can you instead " suggestion. Are you suggesting
> > embedding multiple divs in the result? I wouldn't be super fond of that
> > approach, and testing becomes more fragile.
> > 
> > Conceptually the connection types error message is part of the "update
status
> > message" and getUpdateStatusMessage_ already returns HTML code and
interpreted
> > as innerHTML. What justifies having the connection types related message as
a
> > separate message instead of including it in the return value of
> > getUpdateStatusMessage_()?
> > Regarding testing, asserting that this.$.updateStatusMessage.textContent
> > contains some substring expected to exist in the new error message seems
solid
> > enough, no?
> 
> I feel like there is a difference between embedding strings that may include
<b> tags and generating divs, but it's hardly worth depating as far as I am
concerned so I will just change it.

FWIW, the <b> is just a trick. We don't actually include it in the DOM
(firstChild.innerHTML is getting the contents of <b>).

https://codereview.chromium.org/2698133003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2698133003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:293: return result;
Nit(optional): We can avoid duplicate logic as follows

default:
  function generateErrorHtml(message) {
    return '<div>' +
        parseHtmlSubset('<b>' + message + '</b>').firstChild.innerHTML +
        '</div>';
  }   

  var result = '';                                                              
                                                                                
                                                             
  var message = this.currentUpdateStatusEvent_.message;
  if (!!message)
    result += generateErrorHtml(message);
  var connectMessage = this.currentUpdateStatusEvent_.connectionTypes;
  if (!!connectMessage)
    result += generateErrorHtml(connectMessage);

  return result;

https://codereview.chromium.org/2698133003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/about_page_tests.js (right):

https://codereview.chromium.org/2698133003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:66: status: 'failed',
UpdateStatus.FAILED

https://codereview.chromium.org/2698133003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:362: -1,
page.$.updateStatusMessage.innerHTML.indexOf('no internet'));
Nit (optional): You can use String#includes instead,
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje....

[dpapad, 2017-02-17 02:48:14]

https://codereview.chromium.org/2698133003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2698133003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:289:
parseHtmlSubset('<b>' + connectMessage + '</b>')
It is also worth mentioning that this is only necessary if |connectMessage| can
potentially include HTML code (like a link). From the screenshot you provided at
https://bugs.chromium.org/p/chromium/issues/detail?id=689715#c16, I don't see a
link, so this parseHtmlSubset() call could be unnecessary.

[stevenjb, 2017-02-17 20:02:41]

https://codereview.chromium.org/2698133003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2698133003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:289:
parseHtmlSubset('<b>' + connectMessage + '</b>')
On 2017/02/17 02:48:14, dpapad wrote:
> It is also worth mentioning that this is only necessary if |connectMessage|
can
> potentially include HTML code (like a link). From the screenshot you provided
at
> https://bugs.chromium.org/p/chromium/issues/detail?id=689715#c16, I don't see
a
> link, so this parseHtmlSubset() call could be unnecessary.

I think that in principal we prefer to call this for inserting HTML, e.g. to
protect the code from a typo lower down the stack.

https://codereview.chromium.org/2698133003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:293: return result;
On 2017/02/17 02:44:28, dpapad wrote:
> Nit(optional): We can avoid duplicate logic as follows
> 
> default:
>   function generateErrorHtml(message) {
>     return '<div>' +
>         parseHtmlSubset('<b>' + message + '</b>').firstChild.innerHTML +
>         '</div>';
>   }   
> 
>   var result = '';                                                            
 
>                                                                               
 
>                                                              
>   var message = this.currentUpdateStatusEvent_.message;
>   if (!!message)
>     result += generateErrorHtml(message);
>   var connectMessage = this.currentUpdateStatusEvent_.connectionTypes;
>   if (!!connectMessage)
>     result += generateErrorHtml(connectMessage);
> 
>   return result;

Sure. Done.

https://codereview.chromium.org/2698133003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/about_page_tests.js (right):

https://codereview.chromium.org/2698133003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:66: status: 'failed',
On 2017/02/17 02:44:28, dpapad wrote:
> UpdateStatus.FAILED

Done.

https://codereview.chromium.org/2698133003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:362: -1,
page.$.updateStatusMessage.innerHTML.indexOf('no internet'));
On 2017/02/17 02:44:28, dpapad wrote:
> Nit (optional): You can use String#includes instead,
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje....

Huh. Good old JS, why have one method when you can have half a dozen? :)

[stevenjb, 2017-02-17 20:03:07]

PTAL since I changed the padding.

[dpapad, 2017-02-17 20:27:03]

On 2017/02/17 at 20:03:07, stevenjb wrote:
> PTAL since I changed the padding.

Can you share a screenshot for the error and non-error case?

[stevenjb, 2017-02-17 20:33:40]

On 2017/02/17 20:27:03, dpapad wrote:
> On 2017/02/17 at 20:03:07, stevenjb wrote:
> > PTAL since I changed the padding.
> 
> Can you share a screenshot for the error and non-error case?

See the issue :P

[dpapad, 2017-02-17 21:51:18]

LGTM with comment.

https://codereview.chromium.org/2698133003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.html (right):

https://codereview.chromium.org/2698133003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.html:36: .block {
This rule does not have any effect, can we remove it?. It is being overriden by
the equivalent
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...
anyway (as revealed by the devtools).

[stevenjb, 2017-02-18 19:46:17]

https://codereview.chromium.org/2698133003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.html (right):

https://codereview.chromium.org/2698133003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.html:36: .block {
On 2017/02/17 21:51:17, dpapad wrote:
> This rule does not have any effect, can we remove it?. It is being overriden
by
> the equivalent
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...
> anyway (as revealed by the devtools).

Huh, I didn't realize we had that, thanks. Done.

[stevenjb, 2017-02-18 19:46:21]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-02-18 19:46:21]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2698133003/#ps120001
(title: "Elim unneeded CSS")

[commit-bot: I haz the power, 2017-02-18 19:46:32]

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-02-18 21:08:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-18 21:08:44]

Failed to apply patch for
chrome/browser/resources/settings/about_page/about_page.html:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/resources/settings/about_page/about_page.html:98
  error: chrome/browser/resources/settings/about_page/about_page.html: patch
does not apply

Patch:       chrome/browser/resources/settings/about_page/about_page.html
Index: chrome/browser/resources/settings/about_page/about_page.html
diff --git a/chrome/browser/resources/settings/about_page/about_page.html
b/chrome/browser/resources/settings/about_page/about_page.html
index
3e4f02952002eceba0941e2c5fd70f26d49ca3e6..4a8dc47c851e502678ee7221af1a1513fc233153
100644
--- a/chrome/browser/resources/settings/about_page/about_page.html
+++ b/chrome/browser/resources/settings/about_page/about_page.html
@@ -37,6 +37,15 @@
         -webkit-user-select: text;
       }
 
+      .info-section {
+        margin-bottom: 12px;
+      }
+
+      .padded {
+        padding-bottom: 10px;
+        padding-top: 10px;
+      }
+
       .product-title {
         font-size: 153.85%;  /* 20px / 13px */
         margin-bottom: auto;
@@ -60,16 +69,6 @@
         fill: var(--paper-red-600);
       }
 
-      .product-info {
-        display: block;
-        padding-bottom: 10px;
-        padding-top: 10px;
-      }
-
-      .info-section {
-        margin-bottom: 12px;
-      }
-
       #regulatoryInfo img {
         width: 330px;
       }
@@ -98,7 +97,7 @@
                   src="[[getIconSrc_(
                       obsoleteSystemInfo_, currentUpdateStatusEvent_)]]">
               </iron-icon>
-              <div class="start">
+              <div class="start padded">
                 <div id="updateStatusMessage" hidden="[[!showUpdateStatus_]]"
 <if expr="not chromeos">
                     inner-h-t-m-l="[[getUpdateStatusMessage_(
@@ -182,7 +181,7 @@
               </button>
             </div>
 </if>
-            <div class="settings-box product-info copyable">
+            <div class="settings-box padded block copyable">
               <div class="info-section">
                 <div class="secondary">$i18n{aboutProductTitle}</div>
                 <div class="secondary">$i18n{aboutProductCopyright}</div>
@@ -199,7 +198,7 @@
 </if>
             </div>
 <if expr="chromeos">
-            <div class="settings-box product-info" id="regulatoryInfo"
+            <div class="settings-box padded block" id="regulatoryInfo"
                 hidden$="[[!shouldShowRegulatoryInfo_(regulatoryInfo_)]]">
               <img src="[[regulatoryInfo_.url]]"
alt="[[regulatoryInfo_.text]]">
             </div>

[stevenjb, 2017-02-21 17:52:30]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-02-21 17:52:30]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2698133003/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-21 17:53:05]

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-02-21 20:08:39]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487699550458430,
"parent_rev": "df7c768a829d848bc7ff934d6ed3b20bdb9529d9", "commit_rev":
"875d1fb2901159381fb28c278148432c8da568b5"}

[commit-bot: I haz the power, 2017-02-21 20:10:05]

Description was changed from

==========
MD Settings: About: Show connectionTypes message

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

to

==========
MD Settings: About: Show connectionTypes message

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

Review-Url: https://codereview.chromium.org/2698133003
Cr-Commit-Position: refs/heads/master@{#451826}
Committed:
https://chromium.googlesource.com/chromium/src/+/875d1fb2901159381fb28c278148...
==========

[commit-bot: I haz the power, 2017-02-21 20:10:09]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/875d1fb2901159381fb28c278148...

[Dan Beam, 2017-02-24 00:50:57]

https://codereview.chromium.org/2698133003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2698133003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:286: if (!!message)
this is the same as if (message)