Add a chrome://nacl page to show if NaCl is enabled and if PNaCl is installed.

Add a chrome://nacl page to show if NaCl is enabled and if
PNaCl is installed.

How it looks with pnacl enabled:
http://www.corp.google.com/~jvoung/with-pnacl.png

and without pnacl enabled:
http://www.corp.google.com/~jvoung/without-pnacl.png

BUG=114444
BUG= http://code.google.com/p/nativeclient/issues/detail?id=2813

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151739

[jvoung (off chromium), 2012-08-07 21:43:30]

robertm and sehr -- would something like this make sense?

[robertm, 2012-08-07 22:13:22]

On 2012/08/07 21:43:30, jvoung (chromium) wrote:
> robertm and sehr -- would something like this make sense?

 I would love to have this but a little less code duplication would make it even
better.

[jvoung - send to chromium..., 2012-08-07 23:13:49]

On 2012/08/07 22:13:22, robertm wrote:
> On 2012/08/07 21:43:30, jvoung (chromium) wrote:
> > robertm and sehr -- would something like this make sense?
> 
>  I would love to have this but a little less code duplication would make it
even
> better.

A lot of the about pages use this async pattern between the .js/.html and the
browser-internal-querying with custom DOMHandler code, so I'm not sure that part
can be slimmed down.

I can get rid of the timer stuff, since we aren't showing the GPU info  here.

I can factor out the detailed windows version labeling, if they will accept it,
into base/win/windows_version.

Would that be enough de-duplication?

[jvoung - send to chromium..., 2012-08-07 23:52:06]

On 2012/08/07 23:13:49, jvoung wrote:
> On 2012/08/07 22:13:22, robertm wrote:
> > On 2012/08/07 21:43:30, jvoung (chromium) wrote:
> > > robertm and sehr -- would something like this make sense?
> > 
> >  I would love to have this but a little less code duplication would make it
> even
> > better.
> 
> A lot of the about pages use this async pattern between the .js/.html and the
> browser-internal-querying with custom DOMHandler code, so I'm not sure that
part
> can be slimmed down.
> 
> I can get rid of the timer stuff, since we aren't showing the GPU info  here.
> 
> I can factor out the detailed windows version labeling, if they will accept
it,
> into base/win/windows_version.
> 
> Would that be enough de-duplication?

Took out the timer stuff.  Also added sample images to the CL description.

[Robert Muth (chromium), 2012-08-08 17:57:58]

http://codereview.chromium.org/10823214/diff/3002/chrome/browser/resources/ab...
File chrome/browser/resources/about_nacl.html (right):

http://codereview.chromium.org/10823214/diff/3002/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:17: <div id="header"><h1
i18n-content="naclLongTitle">ABOUT_NACL</h1></div>
if we ever want to merge the code using the simple key/value
template it might be best to not use the "nacl" prefix for the ids. On the other
hand this could make it a little harder
to trace the flow of information. I do not have a good feeling for what the best
approach is, though.

http://codereview.chromium.org/10823214/diff/3002/chrome/browser/ui/webui/nac...
File chrome/browser/ui/webui/nacl_ui.cc (right):

http://codereview.chromium.org/10823214/diff/3002/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:159: // Store Key-Value pairs of
about-information.
I think the code would be clearer if everything
which populate list is moved to a separate function.

Note sure whether this function should also allocate "list".

http://codereview.chromium.org/10823214/diff/3002/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:160: ListValue* list = new ListValue();
indicate whose responsibility it is to free "list"

[jvoung - send to chromium..., 2012-08-08 19:03:11]

http://codereview.chromium.org/10823214/diff/3002/chrome/browser/resources/ab...
File chrome/browser/resources/about_nacl.html (right):

http://codereview.chromium.org/10823214/diff/3002/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:17: <div id="header"><h1
i18n-content="naclLongTitle">ABOUT_NACL</h1></div>
On 2012/08/08 17:57:58, Robert Muth (chromium) wrote:
> if we ever want to merge the code using the simple key/value
> template it might be best to not use the "nacl" prefix for the ids. On the
other
> hand this could make it a little harder
> to trace the flow of information. I do not have a good feeling for what the
best
> approach is, though.

Yeah I'm not sure either, but we can wait till someone actually tries to unify
the key-value templates to do that as a separate change.  The way it is now, at
least it's easy to search-and-replace nacl -> prefix-less, or something.

http://codereview.chromium.org/10823214/diff/3002/chrome/browser/ui/webui/nac...
File chrome/browser/ui/webui/nacl_ui.cc (right):

http://codereview.chromium.org/10823214/diff/3002/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:159: // Store Key-Value pairs of
about-information.
On 2012/08/08 17:57:58, Robert Muth (chromium) wrote:
> I think the code would be clearer if everything
> which populate list is moved to a separate function.
> 
> Note sure whether this function should also allocate "list".
> 

Done -- made that separate function allocate list also.

http://codereview.chromium.org/10823214/diff/3002/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:160: ListValue* list = new ListValue();
On 2012/08/08 17:57:58, Robert Muth (chromium) wrote:
> indicate whose responsibility it is to free "list"

Done.

[robertm, 2012-08-09 14:59:38]

LGTM

http://codereview.chromium.org/10823214/diff/7002/chrome/browser/ui/webui/nac...
File chrome/browser/ui/webui/nacl_ui.cc (right):

http://codereview.chromium.org/10823214/diff/7002/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:127: void AddPair(ListValue* list, const
string16& key, const std::string& value) {
what about adding another variant
where the first argument is as "const char*"

http://codereview.chromium.org/10823214/diff/7002/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:158: scoped_ptr<ListValue> list(new
ListValue());
the scoped_ptr seems a little overkill'ish

[jvoung (off chromium), 2012-08-09 17:00:22]

+jhawkins for OWNERS review

http://codereview.chromium.org/10823214/diff/7002/chrome/browser/ui/webui/nac...
File chrome/browser/ui/webui/nacl_ui.cc (right):

http://codereview.chromium.org/10823214/diff/7002/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:127: void AddPair(ListValue* list, const
string16& key, const std::string& value) {
On 2012/08/09 14:59:38, robertm wrote:
> what about adding another variant
> where the first argument is as "const char*" 

Done.

http://codereview.chromium.org/10823214/diff/7002/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:158: scoped_ptr<ListValue> list(new
ListValue());
On 2012/08/09 14:59:38, robertm wrote:
> the scoped_ptr seems a little overkill'ish

Well, just wanted to make it clear how its memory is managed, in case there is
an error while gathering the data or what-not.

[James Hawkins, 2012-08-09 17:16:55]

http://codereview.chromium.org/10823214/diff/7003/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10823214/diff/7003/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6329: +      Loading...
nit: Indentation is off by two spaces, here and below.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
File chrome/browser/resources/about_nacl.html (right):

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:4: <style>
No styles in the html.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:15: <div id="loading-message"
i18n-content="loadingMessage">LOADING_MESSAGE</div>
s/LOADING_MESSAGE//

Remove all inline text content.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:16: <div id="body-container"
style="visibility:hidden">
Use the hidden attribute instead.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:17: <div id="header"><h1
i18n-content="naclLongTitle">ABOUT_NACL</h1></div>
nit: <h1> must be dropped down and indented.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:18: 
nit: Remove blank line.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:20: <table cellpadding="2"
cellspacing="0" border="0">
Don't use table for non-tabular data.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
File chrome/browser/resources/about_nacl.js (right):

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.js:2: * Takes the |moduleListData| input
argument which represents data about
Indentation is off, should be two more spaces.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.js:7: function
renderTemplate(moduleListData) {
These need to be wrapped in a namespace.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.js:8: // This is the javascript code that
processes the template:
nit: // Process the template.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
File chrome/browser/ui/webui/nacl_ui.cc (right):

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:84: // Called when we think we might have
enough information to return data back
nit: Don't use pronouns (we) in comments; pronouns are ambiguous.

Here and elsewhere.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:115:
web_ui()->RegisterMessageCallback("requestNaClInfo",
nit: Start of parameter rows must align on the same column.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:120: void AddPair(ListValue* list, const
string16& key, const string16& value) {
nit: These methods need to be documented.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:122: results->SetString("key", key);
results->SetString(key, value)?

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:127: void AddPair(ListValue* list, const
string16& key, const std::string& value) {
Why do we need all of these permutations of AddPair?

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:260: if (!page_has_requested_data_ ||
!has_plugin_info_) {
Optional nit: Save a line by removing braces for single-line blocks.

[jvoung (off chromium), 2012-08-09 20:03:34]

Thanks for the review -- new patches uploaded.

http://codereview.chromium.org/10823214/diff/7003/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10823214/diff/7003/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6329: +      Loading...
On 2012/08/09 17:16:55, James Hawkins wrote:
> nit: Indentation is off by two spaces, here and below.

Done.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
File chrome/browser/resources/about_nacl.html (right):

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:4: <style>
On 2012/08/09 17:16:55, James Hawkins wrote:
> No styles in the html.

Sorry, was just following the pattern of other about_*.html.  Moved to a .css
file.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:15: <div id="loading-message"
i18n-content="loadingMessage">LOADING_MESSAGE</div>
On 2012/08/09 17:16:55, James Hawkins wrote:
> s/LOADING_MESSAGE//
> 
> Remove all inline text content.

Done.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:16: <div id="body-container"
style="visibility:hidden">
On 2012/08/09 17:16:55, James Hawkins wrote:
> Use the hidden attribute instead.

Done.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:17: <div id="header"><h1
i18n-content="naclLongTitle">ABOUT_NACL</h1></div>
On 2012/08/09 17:16:55, James Hawkins wrote:
> nit: <h1> must be dropped down and indented.

Done.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:18: 
On 2012/08/09 17:16:55, James Hawkins wrote:
> nit: Remove blank line.

Done.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:20: <table cellpadding="2"
cellspacing="0" border="0">
On 2012/08/09 17:16:55, James Hawkins wrote:
> Don't use table for non-tabular data.

Done.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
File chrome/browser/resources/about_nacl.js (right):

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.js:2: * Takes the |moduleListData| input
argument which represents data about
On 2012/08/09 17:16:55, James Hawkins wrote:
> Indentation is off, should be two more spaces.

double checking on two more spaces:

/**
  *
  */

or one more space?

/**
 *
 */

went with one for now.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.js:7: function
renderTemplate(moduleListData) {
On 2012/08/09 17:16:55, James Hawkins wrote:
> These need to be wrapped in a namespace.

Done.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.js:8: // This is the javascript code that
processes the template:
On 2012/08/09 17:16:55, James Hawkins wrote:
> nit: // Process the template.

Done.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
File chrome/browser/ui/webui/nacl_ui.cc (right):

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:84: // Called when we think we might have
enough information to return data back
On 2012/08/09 17:16:55, James Hawkins wrote:
> nit: Don't use pronouns (we) in comments; pronouns are ambiguous.
> 
> Here and elsewhere.

Done.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:115:
web_ui()->RegisterMessageCallback("requestNaClInfo",
On 2012/08/09 17:16:55, James Hawkins wrote:
> nit: Start of parameter rows must align on the same column.

Done.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:120: void AddPair(ListValue* list, const
string16& key, const string16& value) {
On 2012/08/09 17:16:55, James Hawkins wrote:
> nit: These methods need to be documented.

Done.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:122: results->SetString("key", key);
On 2012/08/09 17:16:55, James Hawkins wrote:
> results->SetString(key, value)?

Wouldn't that mess up the templating?

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:127: void AddPair(ListValue* list, const
string16& key, const std::string& value) {
On 2012/08/09 17:16:55, James Hawkins wrote:
> Why do we need all of these permutations of AddPair?

Hmm, some of the "values" are string16, coming from webkit.  

Some of the keys are string16, coming from the GetStringUTF16(IDS_*.)

A lot of the rest is unfortunately ascii, so it was for convenience (and cargo
culted from flash_ui.cc).  Do you prefer taking out the AddPair() variants and
being more explicit about which parts are ascii?

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:260: if (!page_has_requested_data_ ||
!has_plugin_info_) {
On 2012/08/09 17:16:55, James Hawkins wrote:
> Optional nit: Save a line by removing braces for single-line blocks.

Done.

[James Hawkins, 2012-08-11 19:21:07]

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
File chrome/browser/resources/about_nacl.css (right):

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.css:2: float: left;
Do we care about RTL, or is this a purely-developer UI?

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.css:7: .value {
nit: Remove extra space at beginning of line.

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
File chrome/browser/resources/about_nacl.html (right):

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:8: <div id="body-container"
hidden="hidden">
s/hidden="hidden"/hidden/

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:14: <span dir="ltr" jscontent="key"
class="key"></span>
Why are you forcing ltr here?

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
File chrome/browser/resources/about_nacl.js (right):

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.js:1: var nacl = nacl || {};
This is still dangerous in that these methods may overwrite anything in the
previously-existing nacl namespace.  This should be an anonymous namespace.

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/ui/webui/nac...
File chrome/browser/ui/webui/nacl_ui.cc (right):

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:7: #include <map>
Where is this used?

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:97: // Whether the plugin information is
ready.
nit: Add a blank line above this.

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:152: AddPair(list, flag_label, "On");
Optional nit: Remove braces to save a line.

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:235: nacl_version += ASCIIToUTF16(" (Disabled
in profile prefs)");
Optional nit: Remove braces to save a line.

[James Hawkins, 2012-08-11 19:24:28]

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
File chrome/browser/resources/about_nacl.js (right):

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.js:2: * Takes the |moduleListData| input
argument which represents data about
On 2012/08/09 20:03:35, jvoung (chromium) wrote:
> On 2012/08/09 17:16:55, James Hawkins wrote:
> > Indentation is off, should be two more spaces.
> 
> double checking on two more spaces:
> 
> /**
>   *
>   */
> 
> or one more space?
> 
> /**
>  *
>  */
> 
> went with one for now.

Sorry, the latter.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
File chrome/browser/ui/webui/nacl_ui.cc (right):

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:122: results->SetString("key", key);
On 2012/08/09 20:03:35, jvoung (chromium) wrote:
> On 2012/08/09 17:16:55, James Hawkins wrote:
> > results->SetString(key, value)?
> 
> Wouldn't that mess up the templating?

I don't know.

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:127: void AddPair(ListValue* list, const
string16& key, const std::string& value) {
On 2012/08/09 20:03:35, jvoung (chromium) wrote:
> On 2012/08/09 17:16:55, James Hawkins wrote:
> > Why do we need all of these permutations of AddPair?
> 
> Hmm, some of the "values" are string16, coming from webkit.  
> 
> Some of the keys are string16, coming from the GetStringUTF16(IDS_*.)
> 
> A lot of the rest is unfortunately ascii, so it was for convenience (and cargo
> culted from flash_ui.cc).  Do you prefer taking out the AddPair() variants and
> being more explicit about which parts are ascii?

Yes.

[jvoung (off chromium), 2012-08-13 22:15:43]

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
File chrome/browser/ui/webui/nacl_ui.cc (right):

http://codereview.chromium.org/10823214/diff/7003/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:122: results->SetString("key", key);
On 2012/08/11 19:24:28, James Hawkins wrote:
> On 2012/08/09 20:03:35, jvoung (chromium) wrote:
> > On 2012/08/09 17:16:55, James Hawkins wrote:
> > > results->SetString(key, value)?
> > 
> > Wouldn't that mess up the templating?
> 
> I don't know.

Yeah that simplification would have messed up the templating.  The key "key",
and key "value" must match the jscontent template variable names.  So, I left
this alone.

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
File chrome/browser/resources/about_nacl.css (right):

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.css:2: float: left;
On 2012/08/11 19:21:07, James Hawkins wrote:
> Do we care about RTL, or is this a purely-developer UI?

This is mostly a developer UI, so I haven't thought about internationalization
much.  It might be the case that users reporting bugs will copy-paste the info
displayed here, into bug reports (bug reports to Chrome devs or possibly app
developers too).  Usage could grow though in the future...  Removed LTR
assumption for now.

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.css:7: .value {
On 2012/08/11 19:21:07, James Hawkins wrote:
> nit: Remove extra space at beginning of line.

Done.

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
File chrome/browser/resources/about_nacl.html (right):

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:8: <div id="body-container"
hidden="hidden">
On 2012/08/11 19:21:07, James Hawkins wrote:
> s/hidden="hidden"/hidden/

Done.

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.html:14: <span dir="ltr" jscontent="key"
class="key"></span>
On 2012/08/11 19:21:07, James Hawkins wrote:
> Why are you forcing ltr here?

No real reason that I know of -- was cargo culted =(

Removed.

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
File chrome/browser/resources/about_nacl.js (right):

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/resources/ab...
chrome/browser/resources/about_nacl.js:1: var nacl = nacl || {};
On 2012/08/11 19:21:07, James Hawkins wrote:
> This is still dangerous in that these methods may overwrite anything in the
> previously-existing nacl namespace.  This should be an anonymous namespace.

Sorry a bit of a JS noob, so may be doing it wrong.  Did what seemed closest to
an anonymous namespace for most of the functions.  I need to expose the
returnNaClInfo function as a callback in the nacl_ui.cc code, so left that
alone.

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/ui/webui/nac...
File chrome/browser/ui/webui/nacl_ui.cc (right):

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:7: #include <map>
On 2012/08/11 19:21:07, James Hawkins wrote:
> Where is this used?

Oops not used anymore.  Removed.

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:97: // Whether the plugin information is
ready.
On 2012/08/11 19:21:07, James Hawkins wrote:
> nit: Add a blank line above this.

Done.

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:152: AddPair(list, flag_label, "On");
On 2012/08/11 19:21:07, James Hawkins wrote:
> Optional nit: Remove braces to save a line.

Done.

http://codereview.chromium.org/10823214/diff/6004/chrome/browser/ui/webui/nac...
chrome/browser/ui/webui/nacl_ui.cc:235: nacl_version += ASCIIToUTF16(" (Disabled
in profile prefs)");
On 2012/08/11 19:21:07, James Hawkins wrote:
> Optional nit: Remove braces to save a line.

Done.

[James Hawkins, 2012-08-15 17:56:59]

lgtm

[commit-bot: I haz the power, 2012-08-15 18:06:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/10823214/5019

[commit-bot: I haz the power, 2012-08-15 20:44:30]

Change committed as 151739