MD Settings: About page, implementing detailed build info.

MD Settings: About page, implementing detailed build info.

BUG=546841
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/552dbdc4c66ae49753f84c892d5e3ff54daeb1ab
Cr-Commit-Position: refs/heads/master@{#393638}

[dpapad, 2016-05-12 22:23:20]

Description was changed from

==========
MD Settings: About page, implementing detailed build info.

BUG=546841
==========

to

==========
MD Settings: About page, implementing detailed build info.

BUG=546841
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-05-13 18:01:20]

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

[dpapad, 2016-05-13 18:01:20]

Screenshot at http://imgur.com/zG03r49.
"Change channel" mechanism will be implemented in follow up CLs. Also I am
planning to have a separate CL for localizing all hardcoded strings.

[tommycli, 2016-05-13 18:24:26]

lgtm nits only below

https://codereview.chromium.org/1975003002/diff/60001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1975003002/diff/60001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:16: Currently on <ph
name="CHANNEL_NAME">$1<ex>Stable</ex></ph>
nit: since the channel names are all lowercase, can this example be lowercase
too?

https://codereview.chromium.org/1975003002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/detailed_build_info.html
(right):

https://codereview.chromium.org/1975003002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.html:7: 
Add TODO(s) for the strings need i18n?

https://codereview.chromium.org/1975003002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/detailed_build_info.js
(right):

https://codereview.chromium.org/1975003002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.js:16: function
browserChannelToI18nId(channel) {
I would suggest if it's only used once, to just inline it. Then you could also
get rid of the (function() { business

https://codereview.chromium.org/1975003002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/1975003002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:493: function
CrSettingsAboutPageTest() {}
I realize we don't have a good existing ordering here. Maybe we should slowly
try to alphabetize it. As such, perhaps this belongs nearer to the top like how
it's organized in the resources file.

[dpapad, 2016-05-13 19:28:02]

Thanks Tommy.

https://codereview.chromium.org/1975003002/diff/60001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1975003002/diff/60001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:16: Currently on <ph
name="CHANNEL_NAME">$1<ex>Stable</ex></ph>
On 2016/05/13 at 18:24:26, tommycli wrote:
> nit: since the channel names are all lowercase, can this example be lowercase
too?

Done.

https://codereview.chromium.org/1975003002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/detailed_build_info.html
(right):

https://codereview.chromium.org/1975003002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.html:7: 
On 2016/05/13 at 18:24:26, tommycli wrote:
> Add TODO(s) for the strings need i18n?

Done, added TODOs, except for 'V8', which is a name and therefore not really
translatable (this is what old Options did too see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
where the string is hardcoded).

https://codereview.chromium.org/1975003002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/detailed_build_info.js
(right):

https://codereview.chromium.org/1975003002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.js:16: function
browserChannelToI18nId(channel) {
On 2016/05/13 at 18:24:26, tommycli wrote:
> I would suggest if it's only used once, to just inline it. Then you could also
get rid of the (function() { business

Thought about this. If I inline it then I can no longer use the
assertNotReached(), or I have to inline but still keep it as a function (as
follows), 

browserProxy.getCurrentChannel().then(function(channel) {
  var browserChannelToI18nId = function(...) {
    ...
    assertNotReached();
  };
 
  this.currentlyOnChannelText_ = this.i18n(
     'aboutCurrentlyOnChannel',
      this.i18n(browserChannelToI18nId(channel)));
}.bind(this));

Decided to keep it as is instead, which also makes the ready() method shorter
and slightly more readable.

https://codereview.chromium.org/1975003002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/1975003002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:493: function
CrSettingsAboutPageTest() {}
On 2016/05/13 at 18:24:26, tommycli wrote:
> I realize we don't have a good existing ordering here. Maybe we should slowly
try to alphabetize it. As such, perhaps this belongs nearer to the top like how
it's organized in the resources file.

Done.

[dpapad, 2016-05-13 19:32:47]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-05-13 19:32:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1975003002/#ps80001
(title: "Addressing comments.")

[commit-bot: I haz the power, 2016-05-13 19:33:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1975003002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1975003002/80001

[commit-bot: I haz the power, 2016-05-13 21:00:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-13 21:02:40]

Description was changed from

==========
MD Settings: About page, implementing detailed build info.

BUG=546841
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: About page, implementing detailed build info.

BUG=546841
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/552dbdc4c66ae49753f84c892d5e3ff54daeb1ab
Cr-Commit-Position: refs/heads/master@{#393638}
==========

[commit-bot: I haz the power, 2016-05-13 21:02:41]

Patchset 5 (id:??) landed as
https://crrev.com/552dbdc4c66ae49753f84c892d5e3ff54daeb1ab
Cr-Commit-Position: refs/heads/master@{#393638}

[Evan Stade, 2016-05-13 22:05:00]

On 2016/05/13 21:02:41, commit-bot: I haz the power wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/552dbdc4c66ae49753f84c892d5e3ff54daeb1ab
> Cr-Commit-Position: refs/heads/master@{#393638}

could this change have caused:

[ RUN      ] SettingsFormatWebUITest.CheckboxIdOrPrefCheck

[5008:296:0513/140640:WARNING:CONSOLE(0)] "/deep/ combinator is deprecated. See
https://www.chromestatus.com/features/6750456638341120 for more details.",
source:  (0)

[5008:296:0513/140641:INFO:CONSOLE(1196)] "Running TestCase
SettingsFormatWebUITest.CheckboxIdOrPrefCheck", source: test_api.js (1196)

[5008:296:0513/140641:ERROR:web_ui_test_handler.cc(76)] Failed:
RUN_TEST_F("SettingsFormatWebUITest","CheckboxIdOrPrefCheck")

Error: 

Accessibility issues found on chrome://settings-frame/

*** Begin accessibility audit results ***

An accessibility audit found 

Errors:

Error: AX_ARIA_08 (Elements with ARIA roles must ensure required owned elements
are present) failed on the following elements (1 - 2 of 2):

#default-search-engine-list

#other-search-engine-list

See
https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rule...
for more information.





*** End accessibility audit results ***

    at Object.runAccessibilityAudit (test_api.js:371:17)

    at Object.runAccessibilityAudit (test_api.js:501:22)

    at test_api.js:995:20

    at testDone (test_api.js:783:31)

    at runTest (test_api.js:1042:7)

    at <anonymous>:1:1

c:\b\build\slave\win_x64_builder\build\src\out\release_x64\gen\chrome\browser\ui\webui\options\settings_format_browsertest-gen.cc(38):
error: Value of: RunJavascriptTestF( false, "SettingsFormatWebUITest",
"CheckboxIdOrPrefCheck")

  Actual: false

Expected: true

[dpapad, 2016-05-13 22:10:04]

On 2016/05/13 at 22:05:00, estade wrote:
> On 2016/05/13 21:02:41, commit-bot: I haz the power wrote:
> > Patchset 5 (id:??) landed as
> > https://crrev.com/552dbdc4c66ae49753f84c892d5e3ff54daeb1ab
> > Cr-Commit-Position: refs/heads/master@{#393638}
> 
> could this change have caused:
> 
> [ RUN      ] SettingsFormatWebUITest.CheckboxIdOrPrefCheck
> 
> [5008:296:0513/140640:WARNING:CONSOLE(0)] "/deep/ combinator is deprecated.
See https://www.chromestatus.com/features/6750456638341120 for more details.",
source:  (0)
> 
> [5008:296:0513/140641:INFO:CONSOLE(1196)] "Running TestCase
SettingsFormatWebUITest.CheckboxIdOrPrefCheck", source: test_api.js (1196)
> 
> [5008:296:0513/140641:ERROR:web_ui_test_handler.cc(76)] Failed:
RUN_TEST_F("SettingsFormatWebUITest","CheckboxIdOrPrefCheck")
> 
> Error: 
> 
> Accessibility issues found on chrome://settings-frame/
> 
> *** Begin accessibility audit results ***
> 
> An accessibility audit found 
> 
> Errors:
> 
> Error: AX_ARIA_08 (Elements with ARIA roles must ensure required owned
elements are present) failed on the following elements (1 - 2 of 2):
> 
> #default-search-engine-list
> 
> #other-search-engine-list
> 
> See
https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rule...
for more information.
> 
> 
> 
> 
> 
> *** End accessibility audit results ***
> 
>     at Object.runAccessibilityAudit (test_api.js:371:17)
> 
>     at Object.runAccessibilityAudit (test_api.js:501:22)
> 
>     at test_api.js:995:20
> 
>     at testDone (test_api.js:783:31)
> 
>     at runTest (test_api.js:1042:7)
> 
>     at <anonymous>:1:1
> 
>
c:\b\build\slave\win_x64_builder\build\src\out\release_x64\gen\chrome\browser\ui\webui\options\settings_format_browsertest-gen.cc(38):
error: Value of: RunJavascriptTestF( false, "SettingsFormatWebUITest",
"CheckboxIdOrPrefCheck")
> 
>   Actual: false
> 
> Expected: true

This change should only affect chrome://md-settings. I don't see how it could
have caused an issue on the old Options (chrome://settings).