DOMUI Prefs: Add a deletable item list type, and use it for startup pages.

DOMUI Prefs: Add a deletable item list type, and use it for startup pages.

This takes care of wrapping a ListItem to give it a delete button, per mocks.

Also cleans up some add/remove cruft from browser_options.js

BUG=63817
TEST=Startup page rows should have X's to delete them.

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

[arv (Not doing code reviews), 2010-12-13 20:18:41]

http://codereview.chromium.org/5685003/diff/4001/chrome/browser/resources/opt...
File chrome/browser/resources/options/deletable_item_list.js (right):

http://codereview.chromium.org/5685003/diff/4001/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:66: var closeButtonEl =
this.ownerDocument.createElement('button');
I think you can get away with only closeButtonEl here. it should not be
necessary to wrap the button in a div.

http://codereview.chromium.org/5685003/diff/4001/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:69:
closeButtonEl.onclick = function(event) {
I think you might be better of listening to click at the list level and checking
the target classList. That way you can also get rid of the delegate.

http://codereview.chromium.org/5685003/diff/4001/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:70:
self.delegate_.deleteItem(self.value_);
Another option might have been to dispatch a delete event or walk up the DOM
until you find the list.

[stuartmorgan, 2010-12-14 00:10:58]

http://codereview.chromium.org/5685003/diff/4001/chrome/browser/resources/opt...
File chrome/browser/resources/options/deletable_item_list.js (right):

http://codereview.chromium.org/5685003/diff/4001/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:66: var closeButtonEl =
this.ownerDocument.createElement('button');
On 2010/12/13 20:18:41, arv wrote:
> I think you can get away with only closeButtonEl here. it should not be
> necessary to wrap the button in a div.

Done.

http://codereview.chromium.org/5685003/diff/4001/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:69:
closeButtonEl.onclick = function(event) {
On 2010/12/13 20:18:41, arv wrote:
> I think you might be better of listening to click at the list level and
checking
> the target classList. That way you can also get rid of the delegate.

Done.

[arv (Not doing code reviews), 2010-12-14 19:07:41]

LGTM with nits

http://codereview.chromium.org/5685003/diff/8001/chrome/browser/resources/opt...
File chrome/browser/resources/options/deletable_item_list.js (right):

http://codereview.chromium.org/5685003/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:49: var self = this;
self is no longer used

http://codereview.chromium.org/5685003/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:102: var listItem =
this.getListItemContainingElement_(target);
Where is getListItemContainingElement_ defined?

Since it is private it needs to be in this class but I don't see it.

http://codereview.chromium.org/5685003/diff/8001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/5685003/diff/8001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/list.js:319:
getListItemContainingElement_: function(element) {
This should be public. Maybe rename it getListItemAncestor as well.

[stuartmorgan, 2010-12-14 22:09:50]

http://codereview.chromium.org/5685003/diff/8001/chrome/browser/resources/opt...
File chrome/browser/resources/options/deletable_item_list.js (right):

http://codereview.chromium.org/5685003/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:49: var self = this;
On 2010/12/14 19:07:41, arv wrote:
> self is no longer used

Done.

http://codereview.chromium.org/5685003/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:102: var listItem =
this.getListItemContainingElement_(target);
On 2010/12/14 19:07:41, arv wrote:
> Where is getListItemContainingElement_ defined?
> 
> Since it is private it needs to be in this class but I don't see it.

Oops, victim of refactoring. This part of the logic was in the superclass in an
earlier local iteration.

http://codereview.chromium.org/5685003/diff/8001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/5685003/diff/8001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/list.js:319:
getListItemContainingElement_: function(element) {
On 2010/12/14 19:07:41, arv wrote:
> This should be public. Maybe rename it getListItemAncestor as well.

Done.