Support drag&drop for startup pages

Support drag&drop for startup pages

BUG=69686
TEST=Drag & drop individual or multiple startup pages, confirm D&D works


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

[csilv, 2011-06-13 22:57:02]

LGTM, see comments below.

http://codereview.chromium.org/7044136/diff/1/chrome/browser/custom_home_page...
File chrome/browser/custom_home_pages_table_model.cc (right):

http://codereview.chromium.org/7044136/diff/1/chrome/browser/custom_home_page...
chrome/browser/custom_home_pages_table_model.cc:76: const std::vector<int>&
index_list)
Should we be concerned about this not working properly if index_list is not in
sorted order?  This won't happen in the current implementation, just thinking
defensively against future changes.

http://codereview.chromium.org/7044136/diff/1/chrome/browser/custom_home_page...
chrome/browser/custom_home_pages_table_model.cc:80: // The range of elements
that needs to be reshuffled is [ |first|, |last| ).
nit picky: the end of the comment opens with a bracket and closes with a
parenthesis.

http://codereview.chromium.org/7044136/diff/1/chrome/browser/custom_home_page...
chrome/browser/custom_home_pages_table_model.cc:81: int first =
std::min(insert_before,index_list.front());
style nit: space after comma

http://codereview.chromium.org/7044136/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/browser_options_page.css (right):

http://codereview.chromium.org/7044136/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options_page.css:19:
#startupPagesListDropmarker {
style nit: arrange elements alphabetically unless there is an ordering
dependency.

http://codereview.chromium.org/7044136/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/browser_options_startup_page_list.js
(right):

http://codereview.chromium.org/7044136/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options_startup_page_list.js:124:
dropPos: null,
Seems like it might be cleaner to use a boolean value instead, say:
  dropAbove = false,

[groby-ooo-7-16, 2011-06-13 23:45:27]

http://codereview.chromium.org/7044136/diff/1/chrome/browser/custom_home_page...
File chrome/browser/custom_home_pages_table_model.cc (right):

http://codereview.chromium.org/7044136/diff/1/chrome/browser/custom_home_page...
chrome/browser/custom_home_pages_table_model.cc:76: const std::vector<int>&
index_list)
Since the underlying SelectionModel uses Object.keys it's guaranteed to return
an ordered list. And it seems all other JS referring to selected items also
relies on the selection being ordered.

I've added a comment to that effect so people will at least be aware of this.

http://codereview.chromium.org/7044136/diff/1/chrome/browser/custom_home_page...
chrome/browser/custom_home_pages_table_model.cc:80: // The range of elements
that needs to be reshuffled is [ |first|, |last| ).
Actually intentional - that's the standard range notation STL uses. In this
case, indicating a half-open range including first, excluding last.

http://codereview.chromium.org/7044136/diff/1/chrome/browser/custom_home_page...
chrome/browser/custom_home_pages_table_model.cc:81: int first =
std::min(insert_before,index_list.front());
On 2011/06/13 22:57:02, csilv wrote:
> style nit: space after comma

Done.

http://codereview.chromium.org/7044136/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/browser_options_page.css (right):

http://codereview.chromium.org/7044136/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options_page.css:19:
#startupPagesListDropmarker {
On 2011/06/13 22:57:02, csilv wrote:
> style nit: arrange elements alphabetically unless there is an ordering
> dependency.

Done.

http://codereview.chromium.org/7044136/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/browser_options_startup_page_list.js
(right):

http://codereview.chromium.org/7044136/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options_startup_page_list.js:124:
dropPos: null,
Following other d&d implementations in language list and bookmarks manager here.
(Also, it's a result of style guides "prefer enums over bools"). 

If you still think it should be fixed, please let me know so I can clean it up
consistently across all implementations.

[csilv, 2011-06-14 00:06:12]

http://codereview.chromium.org/7044136/diff/1/chrome/browser/custom_home_page...
File chrome/browser/custom_home_pages_table_model.cc (right):

http://codereview.chromium.org/7044136/diff/1/chrome/browser/custom_home_page...
chrome/browser/custom_home_pages_table_model.cc:76: const std::vector<int>&
index_list)
A comment is prefect, thanks.

On 2011/06/13 23:45:27, groby wrote:
> Since the underlying SelectionModel uses Object.keys it's guaranteed to return
> an ordered list. And it seems all other JS referring to selected items also
> relies on the selection being ordered.
> 
> I've added a comment to that effect so people will at least be aware of this.

http://codereview.chromium.org/7044136/diff/1/chrome/browser/custom_home_page...
chrome/browser/custom_home_pages_table_model.cc:80: // The range of elements
that needs to be reshuffled is [ |first|, |last| ).
Ah, that thought crossed my mind.  (I'm not familiar w/ STL notation as you can
see.)

On 2011/06/13 23:45:27, groby wrote:
> Actually intentional - that's the standard range notation STL uses. In this
> case, indicating a half-open range including first, excluding last.

http://codereview.chromium.org/7044136/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/browser_options_startup_page_list.js
(right):

http://codereview.chromium.org/7044136/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options_startup_page_list.js:124:
dropPos: null,
You won me over w/ your style guide reference.  :-)

On 2011/06/13 23:45:27, groby wrote:
> Following other d&d implementations in language list and bookmarks manager
here.
> (Also, it's a result of style guides "prefer enums over bools"). 
> 
> If you still think it should be fixed, please let me know so I can clean it up
> consistently across all implementations.

[commit-bot: I haz the power, 2011-06-14 01:00:50]

Change committed as 88936