Really fix crash when dragging and dropping in chrome://settings/startup

Really fix crash when dragging and dropping in chrome://settings/startup

The root cause of this crash was that dropping in the <input> field
dispatches a DragDropStartupPage event to the C++ handler. Since the
<input> field doesn't have a corresponding index in the C++ model,
CustomHomePagesModel would attempt to dereference an index past the end
of |entries_|. The fix is to suppress drop events on the <input> field
from triggering the C++ handler, since you can't move the <input> field
anyway.

Also clean up the code to use ints (and remove conversions to/from
strings) where appropriate.

BUG=132974
TBR=jhawkins

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

[dcheng, 2013-03-06 22:32:07]

The original fix didn't actually work because the model might not actually be
empty. Oops.

Since we can't avoid getting drop events in this case (input boxes accept drops
by default), we explicitly check that this isn't a drop on the input element.

[Dan Beam, 2013-03-07 01:20:02]

can you explain how this fixes the crash?  I don't know what pageinfo nor
modelindex are

https://codereview.chromium.org/12548008/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/browser_options_startup_page_list.js
(right):

https://codereview.chromium.org/12548008/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/browser_options_startup_page_list.js:48: if
(pageInfo.modelIndex == '-1') {
why is this comparing to a string?

https://codereview.chromium.org/12548008/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/browser_options_startup_page_list.js:238:
dropTarget.pageInfo_.modelIndex == -1)
nit: curlies

[dcheng, 2013-03-07 06:16:32]

The problem is that when you drag and drop over the input box, we dispatch a
DragDropStartupPage to the C++ handler. The input box is a StartupPageListItem
so it has an index in the Javascript. However, it has no corresponding
representation in the C++ code. As a result, we pass an invalid index in
selected_indexes. CustomHomePagesModel::MoveURLs tries to use the index past the
end of entries_ at
http://git.chromium.org/gitweb/?p=chromium/chromium.git;a=blob;f=chrome/brows....

This also addresses the original bug report at http://crbug.com/136576, so I've
reverted the checks to DCHECKs since we shouldn't send any more invalid drops.

https://codereview.chromium.org/12548008/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/browser_options_startup_page_list.js
(right):

https://codereview.chromium.org/12548008/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/browser_options_startup_page_list.js:48: if
(pageInfo.modelIndex == '-1') {
On 2013/03/07 01:20:02, Dan Beam wrote:
> why is this comparing to a string?

Hm. I think that's just the way it was written originally. I've converted all
instances to just use ints (and removed the corresponding int to string to int
conversions in the .cc).

https://codereview.chromium.org/12548008/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/browser_options_startup_page_list.js:238:
dropTarget.pageInfo_.modelIndex == -1)
On 2013/03/07 01:20:02, Dan Beam wrote:
> nit: curlies

Done.

But out of curiosity, why? Is it because the if () spans multiple lines?

[Dan Beam, 2013-03-07 16:56:28]

https://codereview.chromium.org/12548008/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/browser_options_startup_page_list.js
(right):

https://codereview.chromium.org/12548008/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/browser_options_startup_page_list.js:238:
dropTarget.pageInfo_.modelIndex == -1)
On 2013/03/07 06:16:32, dcheng wrote:
> On 2013/03/07 01:20:02, Dan Beam wrote:
> > nit: curlies
> 
> Done.
> 
> But out of curiosity, why? Is it because the if () spans multiple lines?

Yes. This is relatively similar in Chrome C++ but it really depends on who you
ask (I'd say it's about a 80%/20% split *for* curlies when conditional body is
more than a line)

[Dan Beam, 2013-03-09 00:22:04]

lgtm w/nits

https://chromiumcodereview.appspot.com/12548008/diff/9001/chrome/browser/cust...
File chrome/browser/custom_home_pages_table_model.cc (right):

https://chromiumcodereview.appspot.com/12548008/diff/9001/chrome/browser/cust...
chrome/browser/custom_home_pages_table_model.cc:101: << "Index out of range: "
<< insert_before;
is this meant to stay here or debugging code?  I'd recommend removing the
message if it's meant to stay.  also, if insert_before == RowCount() what
happens?  I know I wrote this code but maybe I was off-by-one.

https://chromiumcodereview.appspot.com/12548008/diff/9001/chrome/browser/reso...
File chrome/browser/resources/options/browser_options_startup_page_list.js
(right):

https://chromiumcodereview.appspot.com/12548008/diff/9001/chrome/browser/reso...
chrome/browser/resources/options/browser_options_startup_page_list.js:250:
[newIndex, this.selectionModel.selectedIndexes]);
nit: plural of index is indices, gar! </vent> (don't need to change)

https://chromiumcodereview.appspot.com/12548008/diff/9001/chrome/browser/reso...
File chrome/browser/resources/options/startup_overlay.js (right):

https://chromiumcodereview.appspot.com/12548008/diff/9001/chrome/browser/reso...
chrome/browser/resources/options/startup_overlay.js:128: model.push({
nit: make 1 line, IMO

  model.push({modelIndex: -1});

[dcheng, 2013-03-09 01:06:22]

TBRing jhawkins.

https://codereview.chromium.org/12548008/diff/9001/chrome/browser/custom_home...
File chrome/browser/custom_home_pages_table_model.cc (right):

https://codereview.chromium.org/12548008/diff/9001/chrome/browser/custom_home...
chrome/browser/custom_home_pages_table_model.cc:101: << "Index out of range: "
<< insert_before;
On 2013/03/09 00:22:04, Dan Beam wrote:
> is this meant to stay here or debugging code?  I'd recommend removing the
> message if it's meant to stay.

I figured it didn't matter since it was only going to be included in debugging
builds, but removed.

> also, if insert_before == RowCount() what
> happens?  I know I wrote this code but maybe I was off-by-one.

We need to support this case; otherwise, we wouldn't be able to drag an item to
the end. So the code Just Works (tm) though I haven't traced through why.

https://codereview.chromium.org/12548008/diff/9001/chrome/browser/resources/o...
File chrome/browser/resources/options/startup_overlay.js (right):

https://codereview.chromium.org/12548008/diff/9001/chrome/browser/resources/o...
chrome/browser/resources/options/startup_overlay.js:128: model.push({
On 2013/03/09 00:22:04, Dan Beam wrote:
> nit: make 1 line, IMO
> 
>   model.push({modelIndex: -1});

Done.

[Dan Beam, 2013-03-09 01:21:00]

slgtm

[commit-bot: I haz the power, 2013-03-09 04:49:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/12548008/21002

[commit-bot: I haz the power, 2013-03-09 14:46:44]

Change committed as 187153