Fix "Set pages" drop crash by changing outside drags to simply add a new URL

chrome/browser/ui/webui/options/startup_page_list_browsertest.js

Index: chrome/browser/ui/webui/options/startup_page_list_browsertest.js
diff --git a/chrome/browser/ui/webui/options/startup_page_list_browsertest.js b/chrome/browser/ui/webui/options/startup_page_list_browsertest.js
new file mode 100644
index 0000000000000000000000000000000000000000..e53b490884e1f5e706550882b68fd11b8cfc8b14
--- /dev/null
+++ b/chrome/browser/ui/webui/options/startup_page_list_browsertest.js
@@ -0,0 +1,139 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * Fixture for startup pages WebUI tests.
+ * @extends {testing.Test}
+ * @constructor
+ */
+function StartupPageListWebUITest() {}
+
+StartupPageListWebUITest.prototype = {
+  __proto__: testing.Test.prototype,
+
+  /**
+   * Browse to the options page & call our preLoad().
+   * @override
+   */
+  browsePreload: 'chrome://settings-frame/startup',
+
+  /** @override */
+  setUp: function() {
+    StartupOverlay.updateStartupPages(this.fakeStartupList);
+    // 1 item for entering data, 1+ from |this.fakeStartupList|.
+    assertGE(this.getList().items.length, 2);
+  },
+
+  /** @protected */

[arv (Not doing code reviews), 2013/10/29 13:48:03]

protected seems strange but ok

[Dan Beam, 2013/10/29 17:31:31]

Ack.

+  getList: function() {
+    return $('startupPagesList');
+  },
+
+  /**
+   * Register a mock handler to ensure expectations are met and options pages
+   * behave correctly.
+   * @override
+   */
+  preLoad: function() {
+    this.makeAndRegisterMockHandler(['addStartupPage',
+                                     'dragDropStartupPage']);
+  },
+
+  /**
+   * A fake list of startup pages to send to the overlay.
+   * @protected
+   */
+  fakeStartupList: [{title: 'Yahoo!',
+                     url: 'http://yahoo.com',

[arv (Not doing code reviews), 2013/10/29 13:48:03]

{
  key: value,
  ...
}

If the object literal does not fit on one line

[Dan Beam, 2013/10/29 17:31:31]

Done.

+                     tooltip: 'Yahoo! homepage',
+                     modelIndex: 0},
+                    {title: 'Facebook',
+                     url: 'http://facebook.com',
+                     tooltip: 'Facebook :: Sign In',
+                     modelIndex: 1}],
+};
+
+(function() {
+
+/**
+ * A mock data transfer object for drag/drop events.
+ * @constructor
+ */
+function MockDataTransfer() {}
+
+/**
+ * Installs a lazily created MockDataTransfer on event#dataTransfer.
+ * @param {!Event} event An event to modify.
+ */
+MockDataTransfer.install = function(event) {
+  event.__defineGetter__('dataTransfer', function() {
+    event.dataTransfer_ = event.dataTransfer_ || new MockDataTransfer;
+    return event.dataTransfer_;
+  });
+};
+
+MockDataTransfer.prototype = {
+  /**
+   * @type {!Object.<string, string>}
+   * @private
+   */
+  data_: {},

[arv (Not doing code reviews), 2013/10/29 13:48:03]

No storage on the prototype object please

[Dan Beam, 2013/10/29 17:31:31]

Done.

+
+  /**
+   * The URL data in this mock drop event.
+   * @param {string} type The text of data being set.
+   * @param {string} val The data to set.
+   * @private

[arv (Not doing code reviews), 2013/10/29 13:48:03]

private does not seem correct. It is never called from within this "class". Is
it dead code?

If private use trailing underscore

[Dan Beam, 2013/10/29 17:31:31]

spurious annotation removed

+   */
+  setData: function(type, val) {
+    this.data_[type] = String(val);
+  },
+
+  /**
+   * Gets data associated with this fake data transfer.
+   * @param {string} type The type of data to get.
+   * @returns {string} The requested type of data or '' if not set.
+   */
+  getData: function(type) {
+    return this.data_[type] || '';
+  },
+};
+
+/**
+ * Creates a fake bubbling, cancelable mouse event with a mock data transfer
+ * installed.
+ * @param {string} type A type of mouse event (e.g. 'drop').
+ * @return {!Event} A fake mouse event.
+ */
+function createMouseEvent(type) {
+  var event = document.createEvent('MouseEvent');

[arv (Not doing code reviews), 2013/10/29 13:48:03]

Use 

new MouseEvent(type, {bubbles: true, cancelable: true})

[Dan Beam, 2013/10/29 17:31:31]

Done.

+  event.initMouseEvent(type, true, true);

[arv (Not doing code reviews), 2013/10/29 13:48:03]

initMouseEvent has 15 (or some ridiculous number like that) parameters and they
are not optional.

[Dan Beam, 2013/10/29 17:31:31]

Ack.

+  MockDataTransfer.install(event);
+  return event;
+}
+
+TEST_F('StartupPageListWebUITest', 'testDropFromOutsideSource', function() {
+  /** @const */ var newPage = 'http://google.com';

[arv (Not doing code reviews), 2013/10/29 13:48:03]

useless anotation

[Dan Beam, 2013/10/29 17:31:31]

technically all of these are useless without a compiler ;) but we do this quite
a few places

[arv (Not doing code reviews), 2013/10/29 19:51:21]

Generally I do not see the point in consts for local variables but some people
like them.

+
+  var mockDropEvent = createMouseEvent('drop');
+  mockDropEvent.dataTransfer.setData('url', newPage);
+
+  this.mockHandler.expects(once()).addStartupPage([newPage, 0]);
+
+  this.getList().items[0].dispatchEvent(mockDropEvent);
+
+  expectTrue(mockDropEvent.defaultPrevented);
+});
+
+TEST_F('StartupPageListWebUITest', 'testDropToReorder', function() {
+  // TODO(dbeam): mock4js doesn't handle complex arguments well. Fix this.
+  this.mockHandler.expects(once()).dragDropStartupPage([0, [1].join()]);
+
+  this.getList().selectionModel.selectedIndex = 1;
+  expectEquals(1, this.getList().selectionModel.selectedIndexes.length);
+
+  this.getList().items[0].dispatchEvent(createMouseEvent('drop'));
+});
+
+}());