JS style nits

a few more JS style nits...

BUG=122002
TEST=none

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

[Dan Beam, 2012-04-05 22:24:13]

it hurt... up to you what you want to fix, most of these were mainly for me for
later

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/about_stats.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/about_stats.js:45: if (showAll || name.indexOf(text) >=
0) {
nit: no curlies

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/about_stats.js:69: if (delta > 0) {
nor here

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/about_stats.js:85: if (value == 'null') {
:^( == 'null', nor here

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/about_stats.js:92: if (value_node.innerHTML == 'null')
{
:_( == 'null', nor here

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/about_stats.js:171: var sort_func = undefined;
remove "= undefined"

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/about_stats.js:184: }
nit: ;

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/about_stats.js:201:
document.addEventListener('DOMContentLoaded', onLoadWork);
nit: no \n

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/certificate_viewer.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/certificate_viewer.js:24: if (e.keyCode == 27) {  //
ESC
nit: no curlies

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/crashes.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/crashes.js:11: chrome.send('requestCrashList');
yay, it works!

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/crashes.js:62: crash['id'];
lern2encodeURIComponent or appendParam()
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/resources/s...

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/feedback.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/feedback.js:6: var FEEDBACK_LANDING_PAGE =
/** @const */

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/feedback.js:84: if ($('saved-screenshots'))
I'll let this double $('id') lookup slide.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/feedback.js:113: var userEmail =
$('user-email-text').textContent;
and this one, >:|

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/feedback.js:139: $('saved-screenshots').hidden = true;
and this one, >:|

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/feedback.js:142: selectImage('current-screenshots',
and this one, >:|

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/feedback.js:159: $('saved-screenshots').hidden = false;
and this one, >:|

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/feedback.js:204: if ($('sysinfo-url')) {
and this, :|

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/feedback.js:287: $('page-url-text').value =
defaults[0];
and this one, >:|

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/flags.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/flags.js:10: 'flagsExperiments': [
I think we generally don't put '' around the key names.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/flags.js:50: elements[i].onchange = function() {
STEPHEN! (jk, saw this when I reviewed his patch, didn't matter at the time
[started looking around, urge to kill rose])

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/flags.js:110: String(enable)]);
+1\s

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/flags.js:121: [String(node.internal_name) + '@' +
index, 'true']);
'true' vs. true?!

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/plugins.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/plugins.js:10: 'plugins': [
'pain'

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/plugins.js:110: * current state of installed plugins.
@param? :(

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/plugins.js:130: return false;
damn it, I just realized these are being used instead of preventDefault() :.(

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/plugins.js:222: * Determines whether a plugin's version
should be displayed.
neither of these have @return, :(

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/plugins.js:251: setInterval(requestPluginsData, 30000);
ughhhhhhhhh

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/plugins.js:259:
document.getElementById('details-link').onclick = toggleTmiMode;
nit: no \n

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/policy.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/policy.js:11: 'anyPoliciesSet': true,
'damnIt'

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/policy.js:50: Policy.prototype = {
nit: I don't think we usually have an \n here

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/policy.js:259: }
;

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/policy.js:278: // Get data and have it displayed upon
loading.
delicious copy pasta is not so delicious

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/sessions.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sessions.js:4: 
should probably be a

  cr.define('session', function() {

up here or something

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sessions.js:5: localStrings = new LocalStrings();
seems like these are all global assignments, :\

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sessions.js:8: kExpandedArrow = '\u25BE';
I don't see a reason to have these like this instead of just ▾ and ▸.

P.S.

▾ ▾ ▾ 
 ▾ ▾    amidoinitrite? 
  ▾

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sessions.js:34: * title for a list of windows or tabs.
no @returns

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sessions.js:40: div.onclick =
Function('toggleExpandedState(this)');
wat!

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sessions.js:58: * Transforms a tab into an HTML
element.
no @returns

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sessions.js:65: tabItem.style.backgroundImage =
url('chrome://favicon/' + tab.url);
was really confused until I figured out url() is a function in util.js

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sessions.js:70: * Transforms a window into an HTML
element.
no @returns

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sessions.js:81: * Transforms a session into an HTML
element.
no @returns

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/sync_setup_overlay.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sync_setup_overlay.js:86: 
none of these are doc'd?!

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sync_setup_overlay.js:94: return undefined;
wat?!

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sync_setup_overlay.js:105: return undefined;
ugh

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sync_setup_overlay.js:222: var result =
JSON.stringify({
this is needlessly encoded to JSON twice?! </vent>

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sync_setup_overlay.js:337: if (args['encryptAllData'])
{
better to use . instead of [], :(

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sync_setup_overlay.js:466: /** @private */
not enough doc!

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sync_setup_overlay.js:888: });
tl;dr too much pain

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/translate.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/translate.js:28: setTimeout(checkLibReady, 100);
not sure if gross or slick...

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/translate.js:30: 
it's possible we'd want to doc this as well... mainly for when we get to
compilation phase

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/translate.js:73: * @type {boolean}
wrong @type, I smell copy pasta

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/translate.js:88: *                   case the
translation is not started.  True otherwise.
wrong indent, should be 4\s

[Tyler Breisacher (Chromium), 2012-04-05 22:39:13]

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/about_stats.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/about_stats.js:45: if (showAll || name.indexOf(text) >=
0) {
On 2012/04/05 22:24:13, Dan Beam wrote:
> nit: no curlies

Done.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/about_stats.js:69: if (delta > 0) {
On 2012/04/05 22:24:13, Dan Beam wrote:
> nor here

Done.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/about_stats.js:85: if (value == 'null') {
On 2012/04/05 22:24:13, Dan Beam wrote:
> :^( == 'null', nor here

Done.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/about_stats.js:92: if (value_node.innerHTML == 'null')
{
On 2012/04/05 22:24:13, Dan Beam wrote:
> :_( == 'null', nor here

Done.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/about_stats.js:171: var sort_func = undefined;
On 2012/04/05 22:24:13, Dan Beam wrote:
> remove "= undefined"

Done.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/about_stats.js:184: }
On 2012/04/05 22:24:13, Dan Beam wrote:
> nit: ;

Done.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/about_stats.js:201:
document.addEventListener('DOMContentLoaded', onLoadWork);
On 2012/04/05 22:24:13, Dan Beam wrote:
> nit: no \n

Done.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/sessions.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sessions.js:8: kExpandedArrow = '\u25BE';
On 2012/04/05 22:24:13, Dan Beam wrote:
> I don't see a reason to have these like this instead of just ▾ and ▸.
> 
> P.S.
> 
> ▾ ▾ ▾ 
>  ▾ ▾    amidoinitrite? 
>   ▾ 

I would guess it's because someday, someone will open this with a text editor
that doesn't understand Unicode very well, and it will get totally messed up.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/sessions.js:40: div.onclick =
Function('toggleExpandedState(this)');
On 2012/04/05 22:24:13, Dan Beam wrote:
> wat!

Yeah, it should obviously be

  div.onclick = new Function('toggleExpandedState(this);');

=P

But seriously, I'll do another CL with this type of change in it, plus
documentation, and all the other stuff you mentioned in this file..

[Tyler Breisacher (Chromium), 2012-04-05 22:49:54]

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/certificate_viewer.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/certificate_viewer.js:24: if (e.keyCode == 27) {  //
ESC
On 2012/04/05 22:24:13, Dan Beam wrote:
> nit: no curlies

Done.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/crashes.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/crashes.js:11: chrome.send('requestCrashList');
On 2012/04/05 22:24:13, Dan Beam wrote:
> yay, it works!

It's like you don't have faith in my ability to write regular expressions or
something...

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/feedback.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/feedback.js:6: var FEEDBACK_LANDING_PAGE =
On 2012/04/05 22:24:13, Dan Beam wrote:
> /** @const */

Here, yes. The other constants are not actually constant -- it looks they're set
to '' here and then init'd to their real value later. So we should maybe
refactor in another CL...

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/flags.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/flags.js:110: String(enable)]);
On 2012/04/05 22:24:13, Dan Beam wrote:
> +1\s

Done.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/flags.js:121: [String(node.internal_name) + '@' +
index, 'true']);
On 2012/04/05 22:24:13, Dan Beam wrote:
> 'true' vs. true?!

The C++ is probably set up to only take strings. I don't want to mess with that
right now, but yes, any time I see a string literal "null" or "true" or
whatever, I'm suspicious of it...

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/plugins.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/plugins.js:110: * current state of installed plugins.
On 2012/04/05 22:24:13, Dan Beam wrote:
> @param? :(

This CL is only super mechanical stuff, but yes. I'll do another pass for
documentation and such.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/plugins.js:259:
document.getElementById('details-link').onclick = toggleTmiMode;
On 2012/04/05 22:24:13, Dan Beam wrote:
> nit: no \n

Done.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/policy.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/policy.js:50: Policy.prototype = {
On 2012/04/05 22:24:13, Dan Beam wrote:
> nit: I don't think we usually have an \n here

Done.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/policy.js:259: }
On 2012/04/05 22:24:13, Dan Beam wrote:
> ;

Done.

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/policy.js:278: // Get data and have it displayed upon
loading.
On 2012/04/05 22:24:13, Dan Beam wrote:
> delicious copy pasta is not so delicious

♨

[Dan Beam, 2012-04-06 02:58:29]

lgtm, but I'm going to think about this code in my nightmares (and about my
comments on this CL in my free time...)

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/crashes.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/crashes.js:11: chrome.send('requestCrashList');
On 2012/04/05 22:49:55, Tyler Breisacher wrote:
> On 2012/04/05 22:24:13, Dan Beam wrote:
> > yay, it works!
> 
> It's like you don't have faith in my ability to write regular expressions or
> something...

I do now! :P

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
File chrome/browser/resources/feedback.js (right):

https://chromiumcodereview.appspot.com/10010019/diff/3001/chrome/browser/reso...
chrome/browser/resources/feedback.js:6: var FEEDBACK_LANDING_PAGE =
On 2012/04/05 22:49:55, Tyler Breisacher wrote:
> On 2012/04/05 22:24:13, Dan Beam wrote:
> > /** @const */
> 
> Here, yes. The other constants are not actually constant -- it looks they're
set
> to '' here and then init'd to their real value later. So we should maybe
> refactor in another CL...

Only meant this one.