Implement most of the startup page controls in DOMUI prefs

Implement most of the startup page controls in DOMUI prefs

Hooks up display of startup pages, removal of pages, and setting the startup pages to the currently open tabs.
Uses a select for now, but will eventually be replaced with a List so the favicons can be displayed.

BUG=48713
TEST=Startup pages can be viewed, removed, and set to current in DOMUI prefs.

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

[arv (Not doing code reviews), 2010-07-27 16:42:51]

http://codereview.chromium.org/3046025/diff/4001/5001
File chrome/browser/custom_home_pages_table_model.cc (right):

http://codereview.chromium.org/3046025/diff/4001/5001#newcode89
chrome/browser/custom_home_pages_table_model.cc:89: !StartsWithASCII(url.spec(),
"chrome://options", false))
We have constants for these in url_constatns.h

url.SchemeIs(chrome::kChromeUIScheme) && url.host() ==
chrome::kChromeUIOptionsHost

http://codereview.chromium.org/3046025/diff/4001/5002
File chrome/browser/dom_ui/browser_options_handler.cc (right):

http://codereview.chromium.org/3046025/diff/4001/5002#newcode238
chrome/browser/dom_ui/browser_options_handler.cc:238: // TODO(stuartmorgan): Add
support for showing favicons.
This is easy. Include the URL in the response and then on the client you can use
chrome://favicon/URL as the source of an image or as background image.

http://codereview.chromium.org/3046025/diff/4001/5005
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/3046025/diff/4001/5005#newcode28
chrome/browser/resources/options/browser_options.js:28:
BrowserOptions.getInstance().updateRemoveButtonState_();
Can you do?

var self = this;
$('startupPages').onchange = function(event) {
  self.updateRemoveButtonState_();
};

or

$('startupPages').onchange = cr.bind(this.updateRemoveButtonState_, this);

http://codereview.chromium.org/3046025/diff/4001/5005#newcode173
chrome/browser/resources/options/browser_options.js:173: }
missing semi colon here and the other two

[stuartmorgan, 2010-07-27 17:55:53]

http://codereview.chromium.org/3046025/diff/4001/5001
File chrome/browser/custom_home_pages_table_model.cc (right):

http://codereview.chromium.org/3046025/diff/4001/5001#newcode89
chrome/browser/custom_home_pages_table_model.cc:89: !StartsWithASCII(url.spec(),
"chrome://options", false))
On 2010/07/27 16:42:51, arv wrote:
> We have constants for these in url_constatns.h
> 
> url.SchemeIs(chrome::kChromeUIScheme) && url.host() ==
> chrome::kChromeUIOptionsHost

Done.

http://codereview.chromium.org/3046025/diff/4001/5002
File chrome/browser/dom_ui/browser_options_handler.cc (right):

http://codereview.chromium.org/3046025/diff/4001/5002#newcode238
chrome/browser/dom_ui/browser_options_handler.cc:238: // TODO(stuartmorgan): Add
support for showing favicons.
On 2010/07/27 16:42:51, arv wrote:
> This is easy. Include the URL in the response and then on the client you can
use
> chrome://favicon/URL as the source of an image or as background image.

Getting the URL to the DOMUI isn't the problem, it's displaying the icon. I need
to replace the <select> with a List (since as far as I've been able to determine
it's impossible to show an icon in a <select> in WebKit), which is much lower
priority than getting the rest of the functionality in place.

I could send the URL and throw it away on the other side for now, but I thought
it would make more sense to add that when I actually start using it.

http://codereview.chromium.org/3046025/diff/4001/5005
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/3046025/diff/4001/5005#newcode28
chrome/browser/resources/options/browser_options.js:28:
BrowserOptions.getInstance().updateRemoveButtonState_();
On 2010/07/27 16:42:51, arv wrote:
> Can you do?
> 
> var self = this;
> $('startupPages').onchange = function(event) {
>   self.updateRemoveButtonState_();
> };
> 
> or
> 
> $('startupPages').onchange = cr.bind(this.updateRemoveButtonState_, this);

Done.

http://codereview.chromium.org/3046025/diff/4001/5005#newcode173
chrome/browser/resources/options/browser_options.js:173: }
On 2010/07/27 16:42:51, arv wrote:
> missing semi colon here and the other two

Done.

[zel, 2010-07-27 18:05:05]

http://codereview.chromium.org/3046025/diff/11001/12002
File chrome/browser/dom_ui/browser_options_handler.cc (right):

http://codereview.chromium.org/3046025/diff/11001/12002#newcode230
chrome/browser/dom_ui/browser_options_handler.cc:230:
startup_custom_pages_table_model_->SetObserver(this);
Where do you unregister this observer? Should this be added to
~BrowserOptionsHandler()?

[stuartmorgan, 2010-07-27 18:18:25]

http://codereview.chromium.org/3046025/diff/11001/12002
File chrome/browser/dom_ui/browser_options_handler.cc (right):

http://codereview.chromium.org/3046025/diff/11001/12002#newcode230
chrome/browser/dom_ui/browser_options_handler.cc:230:
startup_custom_pages_table_model_->SetObserver(this);
On 2010/07/27 18:05:05, zel wrote:
> Where do you unregister this observer? Should this be added to
> ~BrowserOptionsHandler()?

BrowserOptionsHandler owns the table model, so the table model (and thus the
observer pointer it has) can't outlive the BOH; given that removing the observer
in the destructor seemed redundant.

[zel, 2010-07-27 18:20:44]

LGTM

http://codereview.chromium.org/3046025/diff/11001/12002
File chrome/browser/dom_ui/browser_options_handler.cc (right):

http://codereview.chromium.org/3046025/diff/11001/12002#newcode230
chrome/browser/dom_ui/browser_options_handler.cc:230:
startup_custom_pages_table_model_->SetObserver(this);
On 2010/07/27 18:18:26, stuartmorgan wrote:
> On 2010/07/27 18:05:05, zel wrote:
> > Where do you unregister this observer? Should this be added to
> > ~BrowserOptionsHandler()?
> 
> BrowserOptionsHandler owns the table model, so the table model (and thus the
> observer pointer it has) can't outlive the BOH; given that removing the
observer
> in the destructor seemed redundant.

Fair enough... as long as CustomHomePagesTableModel does not make a callback to
the observer while being destructed.

[arv (Not doing code reviews), 2010-07-27 19:02:31]

LGTM

http://codereview.chromium.org/3046025/diff/4001/5002
File chrome/browser/dom_ui/browser_options_handler.cc (right):

http://codereview.chromium.org/3046025/diff/4001/5002#newcode238
chrome/browser/dom_ui/browser_options_handler.cc:238: // TODO(stuartmorgan): Add
support for showing favicons.
On 2010/07/27 17:55:54, stuartmorgan wrote:
> On 2010/07/27 16:42:51, arv wrote:
> > This is easy. Include the URL in the response and then on the client you can
> use
> > chrome://favicon/URL as the source of an image or as background image.
> 
> Getting the URL to the DOMUI isn't the problem, it's displaying the icon. I
need
> to replace the <select> with a List (since as far as I've been able to
determine
> it's impossible to show an icon in a <select> in WebKit), which is much lower
> priority than getting the rest of the functionality in place.
> 
> I could send the URL and throw it away on the other side for now, but I
thought
> it would make more sense to add that when I actually start using it.

Fair enough

http://codereview.chromium.org/3046025/diff/11001/12003
File chrome/browser/dom_ui/browser_options_handler.h (right):

http://codereview.chromium.org/3046025/diff/11001/12003#newcode7
chrome/browser/dom_ui/browser_options_handler.h:7: #pragma once
Can you do this in a separate patch?

[stuartmorgan, 2010-07-27 20:07:23]

http://codereview.chromium.org/3046025/diff/11001/12003
File chrome/browser/dom_ui/browser_options_handler.h (right):

http://codereview.chromium.org/3046025/diff/11001/12003#newcode7
chrome/browser/dom_ui/browser_options_handler.h:7: #pragma once
On 2010/07/27 19:02:31, arv wrote:
> Can you do this in a separate patch?

I'm not doing that; you're looking at the diff between snapshots, rather than
the patch, and trunk has changed.