chrome://snippets-internals page for debugging NTP snippets.

chrome/browser/resources/snippets_internals.js

+// Copyright 2015 The Chromium Authors. All rights reserved.

[Marc Treib, 2016/04/14 15:43:44]

2016

[jkrcal, 2016/04/14 16:41:01]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+cr.define('chrome.snippets_internals', function() {

[Bernhard Bauer, 2016/04/14 16:50:24]

I would probably call this SnippetsInternals, to make it more like a static
class than a namespace.

[jkrcal, 2016/04/15 14:11:50]

Done.

+  'use strict';
+
+  function initialize() {
+    function submitDownload(event) {
+      chrome.send('download', [$('hosts-input').value]);
+      event.preventDefault();
+    }
+
+    $('submit-download').addEventListener('click', submitDownload);
+
+    function submitClear(event) {
+      chrome.send('clear');
+      event.preventDefault();
+    }
+
+    $('submit-clear').addEventListener('click', submitClear);
+
+    function submitClearDiscarded(event) {
+      chrome.send('clearDiscarded');
+      event.preventDefault();
+    }
+
+    $('discarded-snippets-clear').addEventListener('click',
+                                                   submitClearDiscarded);
+
+    chrome.send('loaded');
+  }
+
+  function receiveProperty(propertyId, value) {
+    if ($(propertyId)) $(propertyId).textContent = value;

[Bernhard Bauer, 2016/04/14 16:50:24]

In contrast to C++, the Javascript style guide does always require braces for
if-statements. And in contrast to Java (but similarly to C++), the body does not
go on the same line.

Also, why do you even check whether the node exists? It would be a bug to
receive a property for a non-existing node.

[jkrcal, 2016/04/15 14:11:50]

Done (removed the check).

+  }
+
+  function receiveHosts(hosts) {
+    displayList(hosts, 'hosts');
+
+    $('hosts-input').value = hosts.list.map(host => host.url).join(' ');
+  }
+
+  function receiveSnippets(snippets) {
+    displayList(snippets, 'snippets', 'snippet-title');
+  }
+
+  function receiveDiscardedSnippets(discardedSnippets) {
+    displayList(discardedSnippets, 'discarded-snippets',
+                'discarded-snippet-title');
+  }
+
+  function trigger(event) {

[Bernhard Bauer, 2016/04/14 16:50:24]

I would move this function closer to where it is used, and maybe even inline it?

[jkrcal, 2016/04/15 14:11:50]

I have moved it inside the displayList.
I have not inlined it to be consistent with all the listeners above.

If you think it is really worth to move all the listeners inline, let me know.

+    // The id of the div to (un)hide is stored to 'myid' attribute of the link.
+    var id = event.currentTarget.getAttribute('myid');
+    if ($(id)) $(id).classList.toggle('snippet-hidden');

[Bernhard Bauer, 2016/04/14 16:50:24]

This would also work nicer with a Polymer object. But in absence of that,
instead of going through the global namespace of node IDs, you could just find
the element by navigating locally, like this:

  event.currentTarget.parentElement.getElementsByClassName('foo')[0]

[jkrcal, 2016/04/15 14:11:50]

I am not sure about this suggestion. To me the code is harder to read and more
error-prone when changing the html.
Is it some common policy to avoid using ids for data elements?

+    event.preventDefault();
+  }
+
+  function displayList(object, domId, titleClass) {
+    jstProcess(new JsEvalContext(object), $(domId));
+
+    var text;
+    var display;
+
+    if (object.list.length > 0) {
+      text = '';
+      display = 'inline';
+    } else {
+      text = 'The list is empty.';
+      display = 'none';
+    }
+
+    if ($(domId + '-empty')) $(domId + '-empty').textContent = text;
+    if ($(domId + '-clear')) $(domId + '-clear').style.display = display;
+
+    var links = document.getElementsByClassName(titleClass);
+    for (var link of links) {
+      link.addEventListener('click', trigger);
+    }
+  }
+
+  // Return an object with all of the exports.
+  return {
+    initialize: initialize,
+    receiveProperty: receiveProperty,
+    receiveHosts: receiveHosts,
+    receiveSnippets: receiveSnippets,
+    receiveDiscardedSnippets: receiveDiscardedSnippets,
+  };
+});
+
+document.addEventListener('DOMContentLoaded',
+                          chrome.snippets_internals.initialize);