From https://bugzilla.mozilla.org/show_bug.cgi?id=790499#c5, we should do the followings to make this class clearer. 1. Change the class name 2. Hide TimeSetting class since it is not meant to be called by others. 3. Add some documents about this class.

Thanks a lot for taking this bug from my rant, Steven!

According to my dictionary, the canonical spelling is "time zone", so this should be TimeZoneSettingObserver. :)

Thanks Justin and Steven for addressing this! Please feel free to let me know if I can take this one to refine TimeSetting since I was the original designer of that. Btw, the original TimeSetting design is following the AutoMounterSetting, which could be another bad naming or class if it is also served as an observer and should not be exposed to public.

Comment on attachment 674954 [details] [diff] [review] patch ># HG changeset patch ># Parent 93cc1ee9429165ad859ac031ade8fde49eceeeaa ># User Steven Lee <slee@mozilla.com> > >Bug 792290 : Change TimeSetting to TimeZoneSettingObserver. r=jlebar Both patches have the same checkin comment, which isn't good. The checkin comment should explain what this particular patch does. (Unless you're going to fold the patches together when you check them in?) >diff --git a/dom/system/gonk/TimeSetting.h b/dom/system/gonk/TimeSetting.h > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this file, > * You can obtain one at http://mozilla.org/MPL/2.0/. */ >- >+ Nit: Adding trailing whitespace here. >+class InitTimeZoneCb MOZ_FINAL : public nsISettingsServiceCallback I can't figure out where the "Init" comes from in this name. Would TimeZoneSettingCallback or TimeZoneSettingCb be a better name? >+static mozilla::StaticRefPtr<TimeZoneSettingObserver> sTimeZoneSettingObserver; Everything in this file which shouldn't be accessible to other files (which I think means everything above this line) should be in an anonymous namespace, so it doesn't get exported into mozilla::system.

Comment on attachment 674955 [details] [diff] [review] rename Can you please attach a patch which has a proper rename in the diff? This is important not only for readability during review, but also for keeping our hg log sane. If you're in git, this can be accomplished with |git diff -M -C| (or maybe git diff -M25 -C25, tweaking the numbers until it detects a rename). If you're in hg, you need to explicitly hg rename the file, or I think hg addremove -s might help.

address comment 6.

Use |hg rename| to generate the patch.

Comment on attachment 675428 [details] [diff] [review] rename v2 Oh, that's much easier to review! :)

try server results: * try: -b do -p all -u none ** https://tbpl.mozilla.org/?tree=Try&rev=a032537cdb30 * try: -b d -p linux64 -u all -t none ** https://tbpl.mozilla.org/?tree=Try&rev=4038d31c6077

https://hg.mozilla.org/integration/mozilla-inbound/rev/233981f90736 https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4f0988d156

https://hg.mozilla.org/mozilla-central/rev/233981f90736 https://hg.mozilla.org/mozilla-central/rev/ff4f0988d156

a=gal. He has a bad wifi connection and can't set the flag.

We need this to land bug 811414 which blocks bug 809674

https://hg.mozilla.org/releases/mozilla-aurora/rev/38c831cc9b9e https://hg.mozilla.org/releases/mozilla-aurora/rev/8e625d17e98f (Assuming gal will properly mark this bb+ eventually)