ONC import option to chromeos tab in chrome://net-internals

ONC import option to chromeos tab in chrome://net-internals

Add chromeos tab to chrome://net-internals.
Move ONC file import input field to chromeos tab, from network options.
Support for encrypted onc files.
Fix net_internals browser test.

BUG=chromium-os:23472, chromium-os:19397
TEST=Go to chrome://net-internals, click on chromeos tab. Should be able to import an onc file from there. If the file is encrypted, should be prompted for a passcode.

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

[achuithb, 2011-12-01 10:00:08]

Charlie, please review. Thanks!

[Charlie Lee, 2011-12-01 19:03:15]

Does this stuff only show up on ChromeOS? I can't seem to tell from the code.

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/ui/webui/net...
File chrome/browser/ui/webui/net_internals_ui.cc (right):

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_internals_ui.cc:1302:
Value::CreateStringValue(success ? "ONC file successfully parsed" :
Should this text be localized? Does it make sense to just return true/false and
have the page show the correct text depending on the bool value?

[mmenke, 2011-12-01 19:12:24]

Drive by comments:  You're going to have to update the net-internals unit tests,
too.  One test checks visibility of all tabs on initialization (And switched to
them all, in order, to make sure nothing goes horribly wrong), and another
checks visibility after loading a log file.

Relevant files are in src/chrome/test/data/webui/net_internals/

You'll also need to add the new tab to the hashToTabHandleIdMap (Also in the
unit test files).

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/ui/webui/net...
File chrome/browser/ui/webui/net_internals_ui.cc (right):

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_internals_ui.cc:1302:
Value::CreateStringValue(success ? "ONC file successfully parsed" :
On 2011/12/01 19:03:16, Charlie Lee wrote:
> Should this text be localized? Does it make sense to just return true/false
and
> have the page show the correct text depending on the bool value?

about:net-internals is currently not internationalized.  It's meant for
debugging Chrome itself, so refers to internal structures and constants by their
actual names, so probably doesn't make much sense to just internationalize one
small part of it.

[achuithb, 2011-12-01 19:14:55]

Matt: I'll take a look at the unit tests. Thanks!

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/main.js (right):

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.js:106: false, cr.isChromeOS);
This is the code that ensures that this tab only shows up in ChromeOS

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/ui/webui/net...
File chrome/browser/ui/webui/net_internals_ui.cc (right):

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_internals_ui.cc:1302:
Value::CreateStringValue(success ? "ONC file successfully parsed" :
On 2011/12/01 19:03:16, Charlie Lee wrote:
> Should this text be localized? Does it make sense to just return true/false
and
> have the page show the correct text depending on the bool value?

It looks like the pages on net-internal are not localized.

I thought about putting the text in the page instead of in C++, but ended up
doing it this way. I'll switch to the other way as it probably makes more sense.

[mmenke, 2011-12-01 19:33:35]

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/browser_bridge.js (right):

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/browser_bridge.js:230: 
nit:  Blank line not needed.

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.html (right):

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.html:3: <p><input
type="file" id="import-onc"/>
Could you name all of these to have a prefix of "chromeos-view-".  The other
tabs all do it, to avoid conflicts.  Conflicts with "import-onc" are pretty
unlikely, but when you have things like "password-div", it's much more likely.

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:30:
$('onc-password').select();
Could you declare all these at the top, like all the other tabs do, rather than
inlining the names everwhere?  It makes refactoring and unit testing a little
easier.

[achuithb, 2011-12-02 05:25:41]

Matt, thanks for the feedback. PTAL.

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/browser_bridge.js (right):

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/browser_bridge.js:230: 
On 2011/12/01 19:33:35, Matt Menke wrote:
> nit:  Blank line not needed.

Done.

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.html (right):

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.html:3: <p><input
type="file" id="import-onc"/>
On 2011/12/01 19:33:35, Matt Menke wrote:
> Could you name all of these to have a prefix of "chromeos-view-".  The other
> tabs all do it, to avoid conflicts.  Conflicts with "import-onc" are pretty
> unlikely, but when you have things like "password-div", it's much more likely.

Done.

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8741009/diff/12001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:30:
$('onc-password').select();
On 2011/12/01 19:33:35, Matt Menke wrote:
> Could you declare all these at the top, like all the other tabs do, rather
than
> inlining the names everwhere?  It makes refactoring and unit testing a little
> easier.

Done.

[mmenke, 2011-12-02 15:07:54]

LGTM.  Should be fine as-is, but I do have one suggestion.

http://codereview.chromium.org/8741009/diff/14004/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8741009/diff/14004/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:49: reader.onloadend =
function(e) {
You might want to check for read errors here, and display some sort of feedback.
 I'm quite not sure what happens when passing an undefined through chrome.send. 
My guess is that it's treated as a NULL, and nothing unfortunate will happen,
but think it's best to play it safe and not depend on just what happens in that
case.

To be completely robust in the case of funky input, races with loading files and
someone entering a passcode, etc, I'd suggest putting a check for undefined
fileContent in importONCFile.

http://codereview.chromium.org/8741009/diff/14004/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/main.css (right):

http://codereview.chromium.org/8741009/diff/14004/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.css:62: #logs-view-tab-content {
While you're modifying this file, mind putting logs-view-tab-content just before
prerender-view-tab-content, to keep the order consistent?  Thanks.

[achuithb, 2011-12-02 19:49:10]

That's 3 suggestions :)

I'm going to defer handling of file read errors as it's more involved. I've
handled the other issue and fixed the ordering nit.

Thanks for the review, Matt!

Charlie?

http://codereview.chromium.org/8741009/diff/14004/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8741009/diff/14004/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:49: reader.onloadend =
function(e) {
On 2011/12/02 15:07:55, Matt Menke wrote:
> You might want to check for read errors here, and display some sort of
feedback.
>  I'm quite not sure what happens when passing an undefined through
chrome.send. 
> My guess is that it's treated as a NULL, and nothing unfortunate will happen,
> but think it's best to play it safe and not depend on just what happens in
that
> case.
> 
> To be completely robust in the case of funky input, races with loading files
and
> someone entering a passcode, etc, I'd suggest putting a check for undefined
> fileContent in importONCFile.

Handling read errors is not trivial (probably need to add a div with the error
message, figure out the html4 FileReader error handling code, and test), so I'm
going to defer to a subsequent CL if you don't mind.

I've added the test to ensure fileContent is not null, undefined, empty, etc to
importONCFile().

http://codereview.chromium.org/8741009/diff/14004/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/main.css (right):

http://codereview.chromium.org/8741009/diff/14004/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.css:62: #logs-view-tab-content {
On 2011/12/02 15:07:55, Matt Menke wrote:
> While you're modifying this file, mind putting logs-view-tab-content just
before
> prerender-view-tab-content, to keep the order consistent?  Thanks.

Done.

[mmenke, 2011-12-02 19:55:05]

On 2011/12/02 19:49:10, achuith.bhandarkar wrote:
> That's 3 suggestions :)
> 
> I'm going to defer handling of file read errors as it's more involved. I've
> handled the other issue and fixed the ordering nit.

I was just counting oddly - I didn't count the nit, and the importONCFile change
takes care of my concern about read failures.  

LGTM, once again, assuming you don't mind ignoring empty files - empty strings
act like false in Javascript.

[Charlie Lee, 2011-12-02 20:19:21]

lgtm

[commit-bot: I haz the power, 2011-12-02 21:29:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/8741009/17005

[commit-bot: I haz the power, 2011-12-02 21:30:23]

Presubmit check for 8741009-17005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/resources/net_internals/index.js,chrome/browser/resources/net_internals/main.js,chrome/browser/resources/net_internals/category_tabs.html,chrome/browser/resources/net_internals/chromeos_view.js,chrome/browser/resources/net_internals/chromeos_view.html,chrome/browser/resources/net_internals/browser_bridge.js,chrome/browser/resources/net_internals/index.html,chrome/browser/resources/net_internals/main.css

Presubmit checks took 3.1s to calculate.

[achuithb, 2011-12-02 22:01:32]

James: Need owner lgtm. Thanks!

[commit-bot: I haz the power, 2011-12-02 22:02:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/8741009/17005

[mmenke, 2011-12-02 22:03:25]

On 2011/12/02 22:02:12, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/achuith%40chromium.org/8741009/17005

I just committed an owners file (With review, of course).  Let's see if my
newfound LGTM power can not be abused.

[mmenke, 2011-12-02 22:03:41]

Now be abused, rather.

[James Hawkins, 2011-12-02 22:08:56]

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/browser_bridge.js (right):

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/browser_bridge.js:320: for (var i = 0; i
< this.crosONCFileParseObservers_.length; ++i)
i++

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.html (right):

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.html:2: <h3>Import ONC
file</h3>
Does this string need to be localized?

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.html:3: <p><input
type="file" id="chromeos-view-import-onc"/>
Why are you using a paragraph?  It's not closed btw.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.html:3: <p><input
type="file" id="chromeos-view-import-onc"/>
Don't close self-closing tags, here and elsewhere.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.html:6: <br><input
type="password" id="chromeos-view-onc-password"/>
Don't use <br>. Use divs.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:11: // We inherit from
DivView.
Don't use 'we' in comments; it's ambiguous.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:12: var superClass =
DivView;
Just use DivView directly.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:14: var fileContent;
Are these 'private' vars?

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:17: if (fileContent) {
No braces for single-line blocks.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:46: function
addEventListeners() {
Document all of these methods.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:69: *  @constructor
@extends

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/main.css (right):

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.css:65: #chromeos-view-tab-content,
Wha? you have two selectors, then a comment, then a selector. Please clear this
up.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.css:71: border-collapse:collapse;
Space after colon.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/ui/webui/net...
File chrome/browser/ui/webui/net_internals_ui.cc (right):

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_internals_ui.cc:1296: return;
Don't handle NOTREACHEDs by returning.

http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

[James Hawkins, 2011-12-02 22:10:03]

On 2011/12/02 22:03:25, Matt Menke wrote:
> On 2011/12/02 22:02:12, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >
https://chromium-status.appspot.com/cq/achuith%2540chromium.org/8741009/17005
> 
> I just committed an owners file (With review, of course).  Let's see if my
> newfound LGTM power can not be abused.

Please make sure whoever is in the owners file understand the Web Development
Style Guide and enforces the reviews.

http://www.chromium.org/developers/web-development-style-guide

[achuithb, 2011-12-03 00:23:32]

James: Thanks for the feedback. PTAL. Thanks!

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/browser_bridge.js (right):

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/browser_bridge.js:320: for (var i = 0; i
< this.crosONCFileParseObservers_.length; ++i)
On 2011/12/02 22:08:57, James Hawkins wrote:
> i++

Done.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.html (right):

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.html:2: <h3>Import ONC
file</h3>
On 2011/12/02 22:08:57, James Hawkins wrote:
> Does this string need to be localized?

Nope. Nothing in net-internals is.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.html:3: <p><input
type="file" id="chromeos-view-import-onc"/>
On 2011/12/02 22:08:57, James Hawkins wrote:
> Why are you using a paragraph?  It's not closed btw.

Done.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.html:3: <p><input
type="file" id="chromeos-view-import-onc"/>
On 2011/12/02 22:08:57, James Hawkins wrote:
> Don't close self-closing tags, here and elsewhere.

Done.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.html:6: <br><input
type="password" id="chromeos-view-onc-password"/>
On 2011/12/02 22:08:57, James Hawkins wrote:
> Don't use <br>. Use divs.

Done.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:11: // We inherit from
DivView.
On 2011/12/02 22:08:57, James Hawkins wrote:
> Don't use 'we' in comments; it's ambiguous.

Done.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:12: var superClass =
DivView;
On 2011/12/02 22:08:57, James Hawkins wrote:
> Just use DivView directly.

Done.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:14: var fileContent;
On 2011/12/02 22:08:57, James Hawkins wrote:
> Are these 'private' vars?

Yes - they are in anonymous function scope.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:17: if (fileContent) {
On 2011/12/02 22:08:57, James Hawkins wrote:
> No braces for single-line blocks.

Done.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:46: function
addEventListeners() {
On 2011/12/02 22:08:57, James Hawkins wrote:
> Document all of these methods.

Done.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:69: *  @constructor
On 2011/12/02 22:08:57, James Hawkins wrote:
> @extends

Done.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/main.css (right):

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.css:65: #chromeos-view-tab-content,
On 2011/12/02 22:08:57, James Hawkins wrote:
> Wha? you have two selectors, then a comment, then a selector. Please clear
this
> up.

Done.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.css:71: border-collapse:collapse;
On 2011/12/02 22:08:57, James Hawkins wrote:
> Space after colon.

Done.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/ui/webui/net...
File chrome/browser/ui/webui/net_internals_ui.cc (right):

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_internals_ui.cc:1296: return;
On 2011/12/02 22:08:57, James Hawkins wrote:
> Don't handle NOTREACHEDs by returning.
> 
>
http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

Done.

[James Hawkins, 2011-12-04 18:13:07]

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:14: var fileContent;
On 2011/12/03 00:23:32, achuith.bhandarkar wrote:
> On 2011/12/02 22:08:57, James Hawkins wrote:
> > Are these 'private' vars?
> 
> Yes - they are in anonymous function scope.

OK. Append '_' to the name, document the vars, make sure to add @private to doc.

http://codereview.chromium.org/8741009/diff/27005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.html (right):

http://codereview.chromium.org/8741009/diff/27005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.html:1: <style>
Move this out to either chromeos_view.css or the appropriate CSS file.

http://codereview.chromium.org/8741009/diff/27005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8741009/diff/27005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:18: if (fileContent) {
No braces for single-line logic blocks.

[achuithb, 2011-12-04 21:56:41]

James, PTAL.

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8741009/diff/17005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:14: var fileContent;
On 2011/12/04 18:13:07, James Hawkins wrote:
> On 2011/12/03 00:23:32, achuith.bhandarkar wrote:
> > On 2011/12/02 22:08:57, James Hawkins wrote:
> > > Are these 'private' vars?
> > 
> > Yes - they are in anonymous function scope.
> 
> OK. Append '_' to the name, document the vars, make sure to add @private to
doc.

Done.

http://codereview.chromium.org/8741009/diff/27005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.html (right):

http://codereview.chromium.org/8741009/diff/27005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.html:1: <style>
On 2011/12/04 18:13:07, James Hawkins wrote:
> Move this out to either chromeos_view.css or the appropriate CSS file.

Done.

http://codereview.chromium.org/8741009/diff/27005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8741009/diff/27005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:18: if (fileContent) {
On 2011/12/04 18:13:07, James Hawkins wrote:
> No braces for single-line logic blocks.

In C++, you need braces for single line blocks with else statements. So it's
different in js? Done.

[James Hawkins, 2011-12-04 22:11:54]

http://codereview.chromium.org/8741009/diff/27005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8741009/diff/27005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:18: if (fileContent) {
On 2011/12/04 21:56:42, achuith.bhandarkar wrote:
> On 2011/12/04 18:13:07, James Hawkins wrote:
> > No braces for single-line logic blocks.
> 
> In C++, you need braces for single line blocks with else statements. So it's
> different in js? Done.

From the C++ style guide:

"In general, curly braces are not required for single-line statements..."

You may be confused about this line:

"Curly braces around both IF and ELSE required because one of the clauses used
braces."

The key part is that braces are only required if one of the blocks requires
braces. If they're both single line, no braces is fine.

[James Hawkins, 2011-12-04 22:12:09]

LGTM

[commit-bot: I haz the power, 2011-12-05 00:23:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/8741009/31002

[commit-bot: I haz the power, 2011-12-05 01:34:35]

Change committed as 112952

[achuithb, 2011-12-07 18:49:27]

Thanks for the review, James!

http://codereview.chromium.org/8741009/diff/27005/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8741009/diff/27005/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:18: if (fileContent) {
On 2011/12/04 22:11:54, James Hawkins wrote:
> On 2011/12/04 21:56:42, achuith.bhandarkar wrote:
> > On 2011/12/04 18:13:07, James Hawkins wrote:
> > > No braces for single-line logic blocks.
> > 
> > In C++, you need braces for single line blocks with else statements. So it's
> > different in js? Done.
> 
> From the C++ style guide:
> 
> "In general, curly braces are not required for single-line statements..."
> 
> You may be confused about this line:
> 
> "Curly braces around both IF and ELSE required because one of the clauses used
> braces."
> 
> The key part is that braces are only required if one of the blocks requires
> braces. If they're both single line, no braces is fine.

Ah, thanks for the explanation. Yes, I was indeed confused about this.