DOMUI Prefs: Implement inline editability for startup pages

DOMUI Prefs: Implement inline editability for startup pages

Also moves more common code into InlineEditableItem, and fixes some styling issues with inline-editable lists.

BUG=63817
TEST=Startup pages should be editable in DOMUI prefs. Also, the lead selection in an editable list should show top and bottom borders.

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

[Evan Stade, 2011-01-13 23:12:46]

looks ok to me

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/browser_options_handler.cc (right):

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/browser_options_handler.cc:368: NOTREACHED();
imo these should be CHECKs. Who actually builds and tests with debug?

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options_startup_page_list.js
(right):

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options_startup_page_list.js:82: [
this.pageInfo_['modelIndex'], this.urlField_.value ]);
are the spaces around the [] typical? you don't use them below, for example

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/inline_editable_list.js (right):

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:152: * @param {bool}
opt_alwaysEditable True if the cell always shows the input.
variable name is foo_opt but in this comment it's opt_foo and opt_foo_opt

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/search_engine_manager_engine_list.js
(right):

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_engine_manager_engine_list.js:256:
localStrings.getString('editSearchEngineInvalidTitleToolTip'));
templateData is shorter, i don't know what the difference is otherwise

[stuartmorgan, 2011-01-13 23:35:37]

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/browser_options_handler.cc (right):

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/browser_options_handler.cc:368: NOTREACHED();
On 2011/01/13 23:12:46, Evan Stade wrote:
> imo these should be CHECKs. Who actually builds and tests with debug?

Done for all the argument parsing. For the bounds checking I'm leaving
NOTREACHED because on the off chance that a race happens (a user deletes the
last row just as the model changes underneath them to remove that row, for
example) I don't think the whole browser should come down.

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options_startup_page_list.js
(right):

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options_startup_page_list.js:82: [
this.pageInfo_['modelIndex'], this.urlField_.value ]);
On 2011/01/13 23:12:46, Evan Stade wrote:
> are the spaces around the [] typical? you don't use them below, for example

Done.

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/inline_editable_list.js (right):

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:152: * @param {bool}
opt_alwaysEditable True if the cell always shows the input.
On 2011/01/13 23:12:46, Evan Stade wrote:
> variable name is foo_opt but in this comment it's opt_foo and opt_foo_opt

Done.

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/search_engine_manager_engine_list.js
(right):

http://codereview.chromium.org/6303003/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_engine_manager_engine_list.js:256:
localStrings.getString('editSearchEngineInvalidTitleToolTip'));
On 2011/01/13 23:12:46, Evan Stade wrote:
> templateData is shorter, i don't know what the difference is otherwise

templateData is fine here; Done.

[arv (Not doing code reviews), 2011-01-14 00:23:02]

LGTM

http://codereview.chromium.org/6303003/diff/8001/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options_page.css (right):

http://codereview.chromium.org/6303003/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options_page.css:13: color: #666666;
Use #666 }:-)

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

http://codereview.chromium.org/6303003/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:156:
createEditableTextCell: function(text, opt_alwaysEditable) {
I wish we could just use contentEditable but then we lose validation... which
might be worth it.

[stuartmorgan, 2011-01-14 00:37:09]

http://codereview.chromium.org/6303003/diff/8001/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options_page.css (right):

http://codereview.chromium.org/6303003/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options_page.css:13: color: #666666;
On 2011/01/14 00:23:03, arv wrote:
> Use #666 }:-)

Done.

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

http://codereview.chromium.org/6303003/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:156:
createEditableTextCell: function(text, opt_alwaysEditable) {
On 2011/01/14 00:23:03, arv wrote:
> I wish we could just use contentEditable but then we lose validation... which
> might be worth it.

Most of our validation is done with callbacks to native code, and we'll be using
custom UI, so we don't really benefit much from using the HTML5 validation
stuff. I think this is definitely worth exploring post-M10.