Diagnostics UI: more UI implementation.

Diagnostics UI: more UI implementation.

Implement encapsulated JS handling of the diagnostics page.

Adjust the page layout and element styles to match the Chrome OS Diagnostics Implementation Plan V1 document.

Handle i18n for all strings.

BUG=139442
TEST=lumpy device.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152044

[hshi1, 2012-08-13 23:51:34]

Please take a look at this patch; it mostly deals with the JS layer for
encapsulating the diagnostic page, keeping the adapter status, etc.

Sorry but I have to add jhawkins@ (owner of chrome/) due to the change in
browser_resources.grd, for the addition of three PNG icon resources (icon
credit: sgabriel@)

[hshi1, 2012-08-14 02:18:19]

Question about landing new assets (PNG image): it looks like it is causing make
failures for linux_chromeos and linux_chromeos_clang (no rules to make the png
files).

Do I need to check in these PNG files first in a separate CL? Thanks.

On 2012/08/13 23:51:34, hshi1 wrote:
> Please take a look at this patch; it mostly deals with the JS layer for
> encapsulating the diagnostic page, keeping the adapter status, etc.
> 
> Sorry but I have to add jhawkins@ (owner of chrome/) due to the change in
> browser_resources.grd, for the addition of three PNG icon resources (icon
> credit: sgabriel@)

[xiyuan, 2012-08-14 16:49:57]

On 2012/08/14 02:18:19, hshi1 wrote:
> Question about landing new assets (PNG image): it looks like it is causing
make
> failures for linux_chromeos and linux_chromeos_clang (no rules to make the png
> files).
> 
> Do I need to check in these PNG files first in a separate CL? Thanks.
> 

Yes, you need to check in PNG files first (via dcommit, not via CQ). Currently,
the patch generated would not contain any binary files.

[xiyuan, 2012-08-14 17:00:01]

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/diagnostics/main.html (right):

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/diagnostics/main.html:23: <div
id="loading">Loading...</div>
We should not have text directly in a html file for i18n's purpose.

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/diagnostics/main.js (right):

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/diagnostics/main.js:20: {adapter: 'wwan0',
name: '3G'},
Do we need to worry about i18n for the names?

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/diagnostics/main.js:56:
$('loading').setAttribute('style', 'display:none');
$('loading').hidden = true;

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/diagnostics/main.js:120:
$('adapter-selection').innerHTML = adapterSelectionHTML;
Is it possible to use our js list instead of doing this?

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/diagnostics/main.js:180:
$('connectivity-status').innerHTML = statusHTML;
i18n?

And in general, we should avoid manipulating innerHTML directly.

[hshi1, 2012-08-14 20:09:28]

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/diagnostics/main.html (right):

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/diagnostics/main.html:23: <div
id="loading">Loading...</div>
On 2012/08/14 17:00:01, xiyuan wrote:
> We should not have text directly in a html file for i18n's purpose.

Done.

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/diagnostics/main.js (right):

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/diagnostics/main.js:20: {adapter: 'wwan0',
name: '3G'},
Yes I think all strings should be localized.

Can you recommend an example that shows how best to get localized strings from
within the JS? Thanks.

On 2012/08/14 17:00:01, xiyuan wrote:
> Do we need to worry about i18n for the names?

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/diagnostics/main.js:56:
$('loading').setAttribute('style', 'display:none');
On 2012/08/14 17:00:01, xiyuan wrote:
> $('loading').hidden = true;

Done.

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/diagnostics/main.js:120:
$('adapter-selection').innerHTML = adapterSelectionHTML;
On 2012/08/14 17:00:01, xiyuan wrote:
> Is it possible to use our js list instead of doing this?

Can you please elaborate what does it mean to use the JS list? Do you mean to
explicitly create <input> elements and then append them to the DOM?

[xiyuan, 2012-08-14 23:00:38]

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/diagnostics/main.js (right):

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/diagnostics/main.js:20: {adapter: 'wwan0',
name: '3G'},
On 2012/08/14 20:09:28, hshi1 wrote:
> Yes I think all strings should be localized.
> 
> Can you recommend an example that shows how best to get localized strings from
> within the JS? Thanks.
> 

Include local_strings.js and you can get i18n strings via LocalStrings.

e.g.
- Define your string in one of the grd file;
- Export the string in C++ code, using AddLocalizedString etc;
- In JS, you can get the string 
  var localStrings = new LocalStrings();
  localStrings.getString('your_exported_string_name');

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/diagnostics/main.js:120:
$('adapter-selection').innerHTML = adapterSelectionHTML;
On 2012/08/14 20:09:28, hshi1 wrote:
> Can you please elaborate what does it mean to use the JS list? Do you mean to
> explicitly create <input> elements and then append them to the DOM?

Search for "cr.ui.List" in js files and you could see a bunch of places where it
is used. 
e.g.
https://cs.corp.google.com/#chrome/src/chrome/browser/resources/options2/chro...

Basically, you derive your js class from cr.ui.List, bind it with DOM node (i.e.
cr.ui.List.decorate(domElement)) or create it then appendChild to DOM tree. And
in your list class, you an override createListItem and provide your own item
there.

If you don't want go this approach, then we should explicitly create <input> and
appendChild like you mentioned.

[hshi1, 2012-08-15 00:44:32]

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/diagnostics/main.js (right):

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/diagnostics/main.js:20: {adapter: 'wwan0',
name: '3G'},
On 2012/08/14 23:00:39, xiyuan wrote:
> On 2012/08/14 20:09:28, hshi1 wrote:
> > Yes I think all strings should be localized.
> > 
> > Can you recommend an example that shows how best to get localized strings
from
> > within the JS? Thanks.
> > 
> 
> Include local_strings.js and you can get i18n strings via LocalStrings.
> 
> e.g.
> - Define your string in one of the grd file;
> - Export the string in C++ code, using AddLocalizedString etc;
> - In JS, you can get the string 
>   var localStrings = new LocalStrings();
>   localStrings.getString('your_exported_string_name');

Done.

http://codereview.chromium.org/10825291/diff/7004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/diagnostics/main.js:120:
$('adapter-selection').innerHTML = adapterSelectionHTML;
I'll explicitly create the DOM elements and appendChild.

On 2012/08/14 23:00:39, xiyuan wrote:
> On 2012/08/14 20:09:28, hshi1 wrote:
> > Can you please elaborate what does it mean to use the JS list? Do you mean
to
> > explicitly create <input> elements and then append them to the DOM?
> 
> Search for "cr.ui.List" in js files and you could see a bunch of places where
it
> is used. 
> e.g.
>
https://cs.corp.google.com/#chrome/src/chrome/browser/resources/options2/chro...
> 
> Basically, you derive your js class from cr.ui.List, bind it with DOM node
(i.e.
> cr.ui.List.decorate(domElement)) or create it then appendChild to DOM tree.
And
> in your list class, you an override createListItem and provide your own item
> there.
> 
> If you don't want go this approach, then we should explicitly create <input>
and
> appendChild like you mentioned.

http://codereview.chromium.org/10825291/diff/15001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/diagnostics/main.js (right):

http://codereview.chromium.org/10825291/diff/15001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:223: '2) reboot your
router<br /></div>';
How would you suggest to handle i18n strings like this one? It is not only a
very long paragraph but also contains formatting (such as <i> and <br>). I have
looked at the GRD file and haven't found anything like this.

[xiyuan, 2012-08-15 16:36:07]

http://codereview.chromium.org/10825291/diff/15001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/diagnostics/main.js (right):

http://codereview.chromium.org/10825291/diff/15001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:223: '2) reboot your
router<br /></div>';
On 2012/08/15 00:44:33, hshi1 wrote:
> How would you suggest to handle i18n strings like this one? It is not only a
> very long paragraph but also contains formatting (such as <i> and <br>). I
have
> looked at the GRD file and haven't found anything like this.

Sorry that I was not clear enough. What I suggested to do is something like
this:
- Define a template in grd file.
  e.g.
  This shows a <ph name="BEGIN_BOLD">&lt;b&gt;</ph>bold text<ph
name="END_BOLD">&lt;/b&gt;</ph>, and a replacable <ph name="NAME">$1<ex>a
name</ex></ph>
  The above example template has a bold tag to mark "bold text" and has a "$1"
placeholder which you could replace with the string you needed.
- Export the template via addLocalizedString etc;
- Use localString.getStringF to format the string in js; (or format the string
in C++ then export it).

The following is another example:

https://cs.corp.google.com/#chrome/src/net/base/dir_header.html&sq=package:ch...

where listingParsingErrorBoxText is a formatted string using
IDS_DIRECTORY_LISTING_PARSING_ERROR_BOX_TEXT as a template.

Ping me if you have more questions. Think you need to use this method for all
the text in this function. "appendChild" is not the right way to do i18n.

[hshi1, 2012-08-15 22:59:25]

http://codereview.chromium.org/10825291/diff/15001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/diagnostics/main.js (right):

http://codereview.chromium.org/10825291/diff/15001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:223: '2) reboot your
router<br /></div>';
On 2012/08/15 16:36:07, xiyuan wrote:
> On 2012/08/15 00:44:33, hshi1 wrote:
> > How would you suggest to handle i18n strings like this one? It is not only a
> > very long paragraph but also contains formatting (such as <i> and <br>). I
> have
> > looked at the GRD file and haven't found anything like this.
> 
> Sorry that I was not clear enough. What I suggested to do is something like
> this:
> - Define a template in grd file.
>   e.g.
>   This shows a <ph name="BEGIN_BOLD">&lt;b&gt;</ph>bold text<ph
> name="END_BOLD">&lt;/b&gt;</ph>, and a replacable <ph name="NAME">$1<ex>a
> name</ex></ph>
>   The above example template has a bold tag to mark "bold text" and has a "$1"
> placeholder which you could replace with the string you needed.
> - Export the template via addLocalizedString etc;
> - Use localString.getStringF to format the string in js; (or format the string
> in C++ then export it).
> 
> The following is another example:
> 
>
https://cs.corp.google.com/#chrome/src/net/base/dir_header.html&sq=package:ch...
> 
> where listingParsingErrorBoxText is a formatted string using
> IDS_DIRECTORY_LISTING_PARSING_ERROR_BOX_TEXT as a template.
> 
> Ping me if you have more questions. Think you need to use this method for all
> the text in this function. "appendChild" is not the right way to do i18n.

Done.

[xiyuan, 2012-08-15 23:41:28]

LGTM with nits

http://codereview.chromium.org/10825291/diff/13009/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/diagnostics/main.js (right):

http://codereview.chromium.org/10825291/diff/13009/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:11: function DiagPage() {}
nit: Move this down to be with cr.addSingletonGetter

http://codereview.chromium.org/10825291/diff/13009/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:152:
document.createTextNode(DiagPage.AdapterType[i].name));
nit: Maybe use a <label for="radio_element_id">xxx</label> for easier access for
the radio button? Otherwise, user has to click precisely on the little radio
button.

http://codereview.chromium.org/10825291/diff/13009/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:179:
localStrings.getStringF('testing-connection-to-internet', 3);
nit: It seems to be that your cuold make step number 1, 2 and 3 as part of the
string and just use localStrings.getString when you can.

http://codereview.chromium.org/10825291/diff/13009/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:186: var tickIconElement =
document.createElement('img');
nit: Since we are cloneNode of the two image element, could we move it out of
this function so that we don't createElement every time when this function is
called.

http://codereview.chromium.org/10825291/diff/13009/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:187:
errorElement.setAttribute('class', 'test-error');
nit: errorElement.className = 'test-error';
or errorElement.classList.add('test-error');

[hshi1, 2012-08-16 00:29:17]

http://codereview.chromium.org/10825291/diff/13009/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/diagnostics/main.js (right):

http://codereview.chromium.org/10825291/diff/13009/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:11: function DiagPage() {}
On 2012/08/15 23:41:28, xiyuan wrote:
> nit: Move this down to be with cr.addSingletonGetter

Done.

http://codereview.chromium.org/10825291/diff/13009/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:152:
document.createTextNode(DiagPage.AdapterType[i].name));
On 2012/08/15 23:41:28, xiyuan wrote:
> nit: Maybe use a <label for="radio_element_id">xxx</label> for easier access
for
> the radio button? Otherwise, user has to click precisely on the little radio
> button.

Done.

http://codereview.chromium.org/10825291/diff/13009/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:179:
localStrings.getStringF('testing-connection-to-internet', 3);
On 2012/08/15 23:41:28, xiyuan wrote:
> nit: It seems to be that your cuold make step number 1, 2 and 3 as part of the
> string and just use localStrings.getString when you can.

Done.

http://codereview.chromium.org/10825291/diff/13009/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:186: var tickIconElement =
document.createElement('img');
On 2012/08/15 23:41:28, xiyuan wrote:
> nit: Since we are cloneNode of the two image element, could we move it out of
> this function so that we don't createElement every time when this function is
> called.

Done.

http://codereview.chromium.org/10825291/diff/13009/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:187:
errorElement.setAttribute('class', 'test-error');
On 2012/08/15 23:41:28, xiyuan wrote:
> nit: errorElement.className = 'test-error';
> or errorElement.classList.add('test-error');

Done.

[xiyuan, 2012-08-16 17:13:31]

still LGTM

http://codereview.chromium.org/10825291/diff/12012/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/diagnostics/main.js (right):

http://codereview.chromium.org/10825291/diff/12012/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:29: function
createElementFromText(elementName, text) {
nit: This function seems no longer used.

http://codereview.chromium.org/10825291/diff/12012/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:72:
this.tickIconElement_.setAttribute('src', 'tick.png');
nit: Prefer to move this even further out of this class, in the same scope of
where DiagPage is defined.

[hshi1, 2012-08-16 18:40:41]

http://codereview.chromium.org/10825291/diff/12012/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/diagnostics/main.js (right):

http://codereview.chromium.org/10825291/diff/12012/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:29: function
createElementFromText(elementName, text) {
On 2012/08/16 17:13:31, xiyuan wrote:
> nit: This function seems no longer used.

Done.

http://codereview.chromium.org/10825291/diff/12012/chrome/browser/resources/c...
chrome/browser/resources/chromeos/diagnostics/main.js:72:
this.tickIconElement_.setAttribute('src', 'tick.png');
On 2012/08/16 17:13:31, xiyuan wrote:
> nit: Prefer to move this even further out of this class, in the same scope of
> where DiagPage is defined.

Done.

[hshi1, 2012-08-16 20:06:14]

Add cpu@ for generated_resources.grd.

[hshi1, 2012-08-16 21:26:23]

I just pinged James and found that he is OOO till 8/21.

Adding darin@ and sky@ to review (specifically, changes in
generated_resources.grd and browser_resources.grd), thanks!

[hshi1, 2012-08-16 21:54:55]

I'm afriad Darin is also OOO.

Add thakis@ to review the grd file changes, thanks!

[sky, 2012-08-16 23:06:22]

LGTM

[commit-bot: I haz the power, 2012-08-16 23:07:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10825291/9018

[commit-bot: I haz the power, 2012-08-17 01:59:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10825291/9018

[commit-bot: I haz the power, 2012-08-17 03:29:06]

Try job failure for 10825291-9018 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...