MD Settings: About: Show connectionTypes message

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..57e7d29df99a31c6d156d5f46c3ace86dc364c75 100644
--- a/chrome/browser/resources/settings/about_page/about_page.html
+++ b/chrome/browser/resources/settings/about_page/about_page.html
@@ -99,16 +99,22 @@
                       obsoleteSystemInfo_, currentUpdateStatusEvent_)]]">
               </iron-icon>
               <div class="start">
-                <div id="updateStatusMessage" hidden="[[!showUpdateStatus_]]"
 <if expr="not chromeos">
+                <div id="updateStatusMessage" hidden="[[!showUpdateStatus_]]"
                     inner-h-t-m-l="[[getUpdateStatusMessage_(
                         currentUpdateStatusEvent_)]]">
+                </div>
 </if>
 <if expr="chromeos">
+                <div id="updateStatusMessage" hidden="[[!showUpdateStatus_]]"
                     inner-h-t-m-l="[[getUpdateStatusMessage_(
                         currentUpdateStatusEvent_, targetChannel_)]]">
-</if>
                 </div>
+                <div id="updateStatusMessage" hidden="[[!showConnectionType_]]"

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

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:06:01]

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]

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:51]

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]

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

+                    inner-h-t-m-l="[[
+                        currentUpdateStatusEvent_.connectionTypes]]">
+                </div>
+</if>
                 <span id="deprecationWarning"
                     hidden="[[!obsoleteSystemInfo_.obsolete]]">
                   $i18n{aboutObsoleteSystem}