Introduce a button to toggle hiding/showing of the details in the events tab.

Introduce a button to toggle hiding/showing of the details in the events tab.

BUG=314863
R=kjiwa@google.com, rlarocque@chromium.org

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

[lipalani1, 2013-11-04 22:52:14]

PTAL

[Nicolas Zea, 2013-11-04 22:58:07]

Richard is a better reviewer for javascript code than I so I'll leave that to
him. Could you file a bug for this and other refactoring of the events page
you're doing?

[rlarocque, 2013-11-04 23:21:49]

+JS expert Kamil for a second opinion.

https://codereview.chromium.org/53153010/diff/30001/chrome/browser/resources/...
File chrome/browser/resources/sync_internals/events.js (right):

https://codereview.chromium.org/53153010/diff/30001/chrome/browser/resources/...
chrome/browser/resources/sync_internals/events.js:9: detailsNode.style.display =
'none';
I think I'd prefer to have this manipulated via a CSS class, rather than
modifying the style directly.  That's probably a more "hygenic" way to do it. 
In theory, the JavaScript shouldn't be tied this closely to the display /
layout.

It probably doesn't matter in this case, though.

I'll defer to Kamil for style advice on this one.

https://codereview.chromium.org/53153010/diff/30001/chrome/browser/resources/...
chrome/browser/resources/sync_internals/events.js:15: function
makeLogEntryNode(entry) {
nit: I think the style

var makeLogEntryNode = function(entry) {
...
}

is preferred.  I realize this isn't your fault, but you're welcome to clean up
this (and other function definitions) while you're modifying this file anyway.

[kjiwa, 2013-11-04 23:47:25]

https://codereview.chromium.org/53153010/diff/30001/chrome/browser/resources/...
File chrome/browser/resources/sync_internals/events.js (right):

https://codereview.chromium.org/53153010/diff/30001/chrome/browser/resources/...
chrome/browser/resources/sync_internals/events.js:9: detailsNode.style.display =
'none';
A CSS class is preferable. With one you can implement this as:

/* in your CSS file */
.hidden {
  display: none;
}

// in your JS
if (detailsNode.classList.contains('hidden')) {
  detailsNode.classList.remove('hidden');
} else {
  detailsNode.classList.add('hidden');
}

[kjiwa, 2013-11-04 23:48:44]

https://codereview.chromium.org/53153010/diff/80001/chrome/browser/resources/...
File chrome/browser/resources/sync_internals/events.js (left):

https://codereview.chromium.org/53153010/diff/80001/chrome/browser/resources/...
chrome/browser/resources/sync_internals/events.js:6: function
makeLogEntryNode(entry) {
You can also add 'use strict' to enable strict mode:
http://ejohn.org/blog/ecmascript-5-strict-mode-json-and-more/.

[lipalani1, 2013-11-05 00:14:12]

PTAL

[kjiwa, 2013-11-05 00:21:55]

lgtm

https://codereview.chromium.org/53153010/diff/120001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/events.js (right):

https://codereview.chromium.org/53153010/diff/120001/chrome/browser/resources...
chrome/browser/resources/sync_internals/events.js:14: }
As a minor note this function can be generalized if you ever want to toggle
other classes. e.g.

var toggleClass = function(e, c) {
  if (e.classList.contains(c)) {
    e.classList.remove(c);
  } else {
    e.classList.add(c);
  }
}

// ...

detailsToggleButton.addEventListener('click', function() { toggleClass(details,
'hidden'); }, false);

https://codereview.chromium.org/53153010/diff/120001/chrome/browser/resources...
chrome/browser/resources/sync_internals/events.js:29:
details.classList.add('hidden');
For consistency you may want to use:

  details.classList.add("details");
  details.classList.add("hidden");

[rlarocque, 2013-11-05 00:22:58]

lgtm

[commit-bot: I haz the power, 2013-11-05 15:47:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/53153010/200001

[commit-bot: I haz the power, 2013-11-05 16:15:40]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-05 17:46:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/53153010/290001

[commit-bot: I haz the power, 2013-11-05 18:17:01]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[lipalani1, 2013-11-05 18:47:08]

Committed patchset #6 manually as r233031 (presubmit successful).