Closed Bug 792290 Opened 7 years ago Closed 7 years ago

Change TimeSetting to TimeZoneSettingObserver

Categories

(Core :: DOM: Core & HTML, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: slee, Assigned: slee)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Depends on: 783021
Summary: Change TimeSetting to TimezoneSettingObserver → Change TimeSetting to TimeZoneSettingObserver
Attached patch patch (obsolete) — Splinter Review
Attachment #674954 - Flags: review?(justin.lebar+bug)
Attached patch rename (obsolete) — Splinter Review
Attachment #674955 - Flags: review?(justin.lebar+bug)
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.
Attachment #674954 - Flags: review?(justin.lebar+bug) → review+
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.
Attachment #674955 - Flags: review?(justin.lebar+bug)
Attached patch patch v2Splinter Review
address comment 6.
Attachment #674954 - Attachment is obsolete: true
Attachment #675426 - Flags: review+
Attached patch rename v2Splinter Review
Use |hg rename| to generate the patch.
Attachment #674955 - Attachment is obsolete: true
Attachment #675428 - Flags: review?(justin.lebar+bug)
Comment on attachment 675428 [details] [diff] [review]
rename v2

Oh, that's much easier to review!  :)
Attachment #675428 - Flags: review?(justin.lebar+bug) → 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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/233981f90736
https://hg.mozilla.org/mozilla-central/rev/ff4f0988d156
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
blocking-basecamp: --- → ?
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
blocking-basecamp: ? → +
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.