[NTP4] Fix empty apps page crash.

[NTP4] Fix empty apps page crash.

R=estade@chromium.org,csharp@chromium.org
BUG=122214
TEST=Drag, switch to new pane, drop on original nav dot. Drop on newly created
pane. Nothing should asplode.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132468

[Dan Beam, 2012-04-12 02:11:50]

tell me what you guys think of this fix for bug 122214

[Dan Beam, 2012-04-12 04:51:01]

https://chromiumcodereview.appspot.com/10068001/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_sorting.cc (left):

https://chromiumcodereview.appspot.com/10068001/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_sorting.cc:85: <=
static_cast<size_t>(old_page_index)) {
I think this might've been off by 1 (index is 0-based so page 1 is at index 0)
unless I'm misunderstanding.

[csharp, 2012-04-12 13:19:54]

https://chromiumcodereview.appspot.com/10068001/diff/3002/chrome/browser/exte...
File chrome/browser/extensions/extension_sorting.cc (right):

https://chromiumcodereview.appspot.com/10068001/diff/3002/chrome/browser/exte...
chrome/browser/extensions/extension_sorting.cc:56: while
(ntp_ordinal_map_.size() <= required_size) {
If required_size=1 this makes ntp_ordinal_map.size()=2, don't we only want it to
be 1?

[Dan Beam, 2012-04-12 18:46:18]

https://chromiumcodereview.appspot.com/10068001/diff/3002/chrome/browser/exte...
File chrome/browser/extensions/extension_sorting.cc (right):

https://chromiumcodereview.appspot.com/10068001/diff/3002/chrome/browser/exte...
chrome/browser/extensions/extension_sorting.cc:56: while
(ntp_ordinal_map_.size() <= required_size) {
On 2012/04/12 13:19:54, csharp wrote:
> If required_size=1 this makes ntp_ordinal_map.size()=2, don't we only want it
to
> be 1?

Ah, that's why it worked with an index. OK, cool, fixed.

https://chromiumcodereview.appspot.com/10068001/diff/3002/chrome/browser/exte...
chrome/browser/extensions/extension_sorting.cc:411:
CreateOrdinalsIfNecessary(page_index);
this was also off by one, which was masked by this

[Evan Stade, 2012-04-13 01:21:14]

lgtm

http://codereview.chromium.org/10068001/diff/1006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_sorting.h (right):

http://codereview.chromium.org/10068001/diff/1006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sorting.h:133: void
CreateOrdinalsIfNecessary(size_t required_size);
s/required_size/minimum_size/

[Dan Beam, 2012-04-13 01:37:46]

http://codereview.chromium.org/10068001/diff/1006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_sorting.h (right):

http://codereview.chromium.org/10068001/diff/1006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sorting.h:133: void
CreateOrdinalsIfNecessary(size_t required_size);
On 2012/04/13 01:21:14, Evan Stade wrote:
> s/required_size/minimum_size/

Done.

[Dan Beam, 2012-04-13 03:15:02]

csharp: re-review when you get a chance as I hope to merge this to M19.

[csharp, 2012-04-13 13:46:37]

lgtm

[commit-bot: I haz the power, 2012-04-16 18:26:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/10068001/4008

[commit-bot: I haz the power, 2012-04-16 18:56:24]

Try job failure for 10068001-4008 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-04-16 20:09:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/10068001/4008

[commit-bot: I haz the power, 2012-04-16 21:50:05]

Change committed as 132468