Reenabled ChromeOS system setting for timezone selection....

Reenabled ChromeOS system setting for timezone selection.

BUG=chromium-os:5646
TEST=option chrome:options on chromeos build, go to system page and make timezone is back there


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

[zel, 2010-08-13 00:24:29]

still missing change notification handling from libcros, trying to figure out a
neat way to do this part...

[dhg, 2010-08-13 15:36:44]

http://codereview.chromium.org/3143009/diff/15002/29009
File chrome/browser/chromeos/dom_ui/cros_options_page_ui_handler.cc (right):

http://codereview.chromium.org/3143009/diff/15002/29009#newcode20
chrome/browser/chromeos/dom_ui/cros_options_page_ui_handler.cc:20:
CrosSettings::Get()->RemoveProvider(settings_provider_);
hmm... I think this means its  never cleaned up (so there's a leak on exit)

[xiyuan, 2010-08-13 17:16:40]

http://codereview.chromium.org/3143009/diff/15002/29002
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/3143009/diff/15002/29002#newcode124
chrome/browser/chromeos/cros_settings.cc:124: delete providers_[i];
think we probably want the handlers to own and manage the lifetime of the
providers after this change.

http://codereview.chromium.org/3143009/diff/15002/29009
File chrome/browser/chromeos/dom_ui/cros_options_page_ui_handler.cc (right):

http://codereview.chromium.org/3143009/diff/15002/29009#newcode20
chrome/browser/chromeos/dom_ui/cros_options_page_ui_handler.cc:20:
CrosSettings::Get()->RemoveProvider(settings_provider_);
On 2010/08/13 15:36:45, dhg wrote:
> hmm... I think this means its  never cleaned up (so there's a leak on exit)

yeah, we might want to move the "delete" in CrosSettings dtor to here.

http://codereview.chromium.org/3143009/diff/15002/29011
File chrome/browser/chromeos/dom_ui/system_options_handler.cc (right):

http://codereview.chromium.org/3143009/diff/15002/29011#newcode24
chrome/browser/chromeos/dom_ui/system_options_handler.cc:24:
reinterpret_cast<chromeos::CrosSettingsProvider*>(
do we need to reinterpret_cast here? Since SystemSettingsProvider derives from
CrosSettingsProvider, we could use its pointer in places for
CrosSettingsProvider*, right?

http://codereview.chromium.org/3143009/diff/15002/29013
File chrome/browser/chromeos/dom_ui/system_settings_provider.cc (right):

http://codereview.chromium.org/3143009/diff/15002/29013#newcode138
chrome/browser/chromeos/dom_ui/system_settings_provider.cc:138: }
nit: empty line before ending of namespace.

[zel, 2010-08-13 20:59:34]

Note that the update also contains the notification mechanism as well. Please
review that part.

http://codereview.chromium.org/3143009/diff/15002/29002
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/3143009/diff/15002/29002#newcode124
chrome/browser/chromeos/cros_settings.cc:124: delete providers_[i];
On 2010/08/13 17:16:40, xiyuan wrote:
> think we probably want the handlers to own and manage the lifetime of the
> providers after this change.

Done.

http://codereview.chromium.org/3143009/diff/15002/29009
File chrome/browser/chromeos/dom_ui/cros_options_page_ui_handler.cc (right):

http://codereview.chromium.org/3143009/diff/15002/29009#newcode20
chrome/browser/chromeos/dom_ui/cros_options_page_ui_handler.cc:20:
CrosSettings::Get()->RemoveProvider(settings_provider_);
On 2010/08/13 15:36:45, dhg wrote:
> hmm... I think this means its  never cleaned up (so there's a leak on exit)

Done.

http://codereview.chromium.org/3143009/diff/15002/29009#newcode20
chrome/browser/chromeos/dom_ui/cros_options_page_ui_handler.cc:20:
CrosSettings::Get()->RemoveProvider(settings_provider_);
On 2010/08/13 17:16:40, xiyuan wrote:
> On 2010/08/13 15:36:45, dhg wrote:
> > hmm... I think this means its  never cleaned up (so there's a leak on exit)
> 
> yeah, we might want to move the "delete" in CrosSettings dtor to here. 
> 

Done. It was cleaned somewhere else, but as Xiyuan pointed out - this is a
better place for sure. settings_provider_ is now scoped_ptr<>

http://codereview.chromium.org/3143009/diff/15002/29011
File chrome/browser/chromeos/dom_ui/system_options_handler.cc (right):

http://codereview.chromium.org/3143009/diff/15002/29011#newcode24
chrome/browser/chromeos/dom_ui/system_options_handler.cc:24:
reinterpret_cast<chromeos::CrosSettingsProvider*>(
On 2010/08/13 17:16:40, xiyuan wrote:
> do we need to reinterpret_cast here? Since SystemSettingsProvider derives from
> CrosSettingsProvider, we could use its pointer in places for
> CrosSettingsProvider*, right?

Done.

http://codereview.chromium.org/3143009/diff/15002/29013
File chrome/browser/chromeos/dom_ui/system_settings_provider.cc (right):

http://codereview.chromium.org/3143009/diff/15002/29013#newcode138
chrome/browser/chromeos/dom_ui/system_settings_provider.cc:138: }
On 2010/08/13 17:16:40, xiyuan wrote:
> nit: empty line before ending of namespace.

Done.

[dhg, 2010-08-16 15:57:21]

I'm a little concerned about the lifetime of the new providers.  Not sure this
is great for things which have async activities, etc which might take a bit to
populate. We'd be constantly having to message dbus to get state, etc on every
new view.  Not sure if thats desirable or not.

http://codereview.chromium.org/3143009/diff/51003/67009
File chrome/browser/chromeos/dom_ui/core_chromeos_options_handler.cc (right):

http://codereview.chromium.org/3143009/diff/51003/67009#newcode63
chrome/browser/chromeos/dom_ui/core_chromeos_options_handler.cc:63: const
NotificationSource& source,
indent

[dhg, 2010-08-16 16:17:57]

We could just say, submit this one as is, then work on pulling it out in a
subsequent CL.

On 2010/08/16 15:57:21, dhg wrote:
> I'm a little concerned about the lifetime of the new providers.  Not sure this
> is great for things which have async activities, etc which might take a bit to
> populate. We'd be constantly having to message dbus to get state, etc on every
> new view.  Not sure if thats desirable or not.
> 
> http://codereview.chromium.org/3143009/diff/51003/67009
> File chrome/browser/chromeos/dom_ui/core_chromeos_options_handler.cc (right):
> 
> http://codereview.chromium.org/3143009/diff/51003/67009#newcode63
> chrome/browser/chromeos/dom_ui/core_chromeos_options_handler.cc:63: const
> NotificationSource& source,
> indent

[xiyuan, 2010-08-16 16:19:08]

LGTM

Per discussion, we might need to decouple settings provider from UI. But I think
this CL is good for now.