Typecheck chrome://bookmarks using Closure Compiler

chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js

Index: chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js
diff --git a/chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js b/chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js
index a678d379e4116695cf56fe9ce3f6fd3896c1a7aa..96e804f39a4b8574e0db4c28cae7db373965fed7 100644
--- a/chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js
+++ b/chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js
@@ -5,12 +5,31 @@
 // TODO(arv): Now that this is driven by a data model, implement a data model
 //            that handles the loading and the events from the bookmark backend.
 
+/**
+ * @typedef {{childIds: Array.<string>}}
+ *
+ * @see chrome/common/extensions/api/bookmarks.json
+ */
+var ReorderInfo;
+
+/**
+ * @typedef {{parentId: string, index: number, oldParentId: string,
+ *            oldIndex: number}}
+ *
+ * @see chrome/common/extensions/api/bookmarks.json
+ */
+var MoveInfo;
+
 cr.define('bmm', function() {
   var List = cr.ui.List;
   var ListItem = cr.ui.ListItem;
   var ArrayDataModel = cr.ui.ArrayDataModel;
   var ContextMenuButton = cr.ui.ContextMenuButton;
 
+  /**
+   * This variable is fake and used only to declare exported bmm.list name.
+   * @suppress {suspiciousCode}

[Dan Beam, 2014/09/23 02:46:55]

why can't we simply change the module return statement to:

  return {
    ...
    list: $('list'),
  };

instead?

[Vitaly Pavlenko, 2014/09/23 22:20:54]

Done, fixed handling in compiler pass. But I don't think it's a good idea: at
the point when return object is created DOM is not processed, so $('list')
returns null.

As long as it's not used before it's assigned externally:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
it's ok, but it could mislead future code readers.

[Dan Beam, 2014/09/24 21:38:58]

see below

[Vitaly Pavlenko, 2014/09/24 22:18:23]

Done.

+   */
   var list;
 
   /**
@@ -57,7 +76,7 @@ cr.define('bmm', function() {
    * Creates a new bookmark list.
    * @param {Object=} opt_propertyBag Optional properties.
    * @constructor
-   * @extends {HTMLButtonElement}
+   * @extends {cr.ui.List}
    */
   var BookmarkList = cr.ui.define('list');
 
@@ -80,7 +99,8 @@ cr.define('bmm', function() {
     },
 
     createItem: function(bookmarkNode) {
-      return new BookmarkListItem(bookmarkNode);
+      return new BookmarkListItem(/** @type {BookmarkTreeNode} */(
+          bookmarkNode));
     },
 
     /** @private {string} */
@@ -198,9 +218,10 @@ cr.define('bmm', function() {
      * Handles mousedown events so that we can prevent the auto scroll as
      * necessary.
      * @private
-     * @param {!MouseEvent} e The mousedown event object.
+     * @param {!Event} e The mousedown event object.
      */
     handleMouseDown_: function(e) {
+      e = /** @type {!MouseEvent} */(e);
       if (e.button == 1) {
         // WebKit no longer fires click events for middle clicks so we manually
         // listen to mouse up to dispatch a click event.
@@ -220,9 +241,10 @@ cr.define('bmm', function() {
      * WebKit no longer dispatches click events for middle clicks so we need
      * to emulate it.
      * @private
-     * @param {!MouseEvent} e The mouse up event object.
+     * @param {!Event} e The mouse up event object.
      */
     handleMiddleMouseUp_: function(e) {
+      e = /** @type {!MouseEvent} */(e);
       this.removeEventListener('mouseup', this.handleMiddleMouseUp_);
       if (e.button == 1) {
         var el = e.target;
@@ -250,6 +272,10 @@ cr.define('bmm', function() {
       }
     },
 
+    /**
+     * @param {string} id
+     * @param {ReorderInfo} reorderInfo
+     */
     handleChildrenReordered: function(id, reorderInfo) {
       if (this.parentId == id) {
         // We create a new data model with updated items in the right order.
@@ -274,6 +300,10 @@ cr.define('bmm', function() {
         this.dataModel.splice(bookmarkNode.index, 0, bookmarkNode);
     },
 
+    /**
+     * @param {string} id
+     * @param {MoveInfo} moveInfo
+     */
     handleMoved: function(id, moveInfo) {
       if (moveInfo.parentId == this.parentId ||
           moveInfo.oldParentId == this.parentId) {
@@ -344,7 +374,6 @@ cr.define('bmm', function() {
 
   /**
    * The ID of the bookmark folder we are displaying.
-   * @type {string}
    */
   cr.defineProperty(BookmarkList, 'parentId', cr.PropertyKind.JS,
                     function() {
@@ -353,9 +382,10 @@ cr.define('bmm', function() {
 
   /**
    * The contextMenu property.
-   * @type {cr.ui.Menu}
    */
   cr.ui.contextMenuHandler.addContextMenuProperty(BookmarkList);
+  /** @type {cr.ui.Menu} */
+  BookmarkList.prototype.contextMenu;
 
   /**
    * Creates a new bookmark list item.
@@ -507,14 +537,14 @@ cr.define('bmm', function() {
         this.setAttribute('editing', '');
         this.draggable = false;
 
-        labelInput = doc.createElement('input');
+        labelInput = /** @type {HTMLElement} */(doc.createElement('input'));
         labelInput.placeholder =
             loadTimeData.getString('name_input_placeholder');
         replaceAllChildren(labelEl, labelInput);
         labelInput.value = title;
 
         if (!isFolder) {
-          urlInput = doc.createElement('input');
+          urlInput = /** @type {HTMLElement} */(doc.createElement('input'));
           urlInput.type = 'url';
           urlInput.required = true;
           urlInput.placeholder =
@@ -522,13 +552,13 @@ cr.define('bmm', function() {
 
           // We also need a name for the input for the CSS to work.
           urlInput.name = '-url-input-' + cr.createUid();
-          replaceAllChildren(urlEl, urlInput);
+          replaceAllChildren(assert(urlEl), urlInput);
           urlInput.value = url;
         }
 
-        function stopPropagation(e) {
+        var stopPropagation = function(e) {
           e.stopPropagation();
-        }
+        };

[Dan Beam, 2014/09/23 02:46:55]

why did you make this change?

[Vitaly Pavlenko, 2014/09/23 22:20:54]

##
/usr/local/google/home/vitalyp/src/chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:562:
ERROR - functions can only be declared at top level or immediately within
another function in ES5 strict mode
##         function stopPropagation(e) {
##         ^
## 

 
         var eventsToStop =
             ['mousedown', 'mouseup', 'contextmenu', 'dblclick', 'paste'];