Closed
Bug 1146900
Opened 9 years ago
Closed 9 years ago
[Shinano][Aries] Use timekeep instead of time_daemon
Categories
(Firefox OS Graveyard :: GonkIntegration, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: gerard-majax, Assigned: gerard-majax)
References
Details
(Keywords: foxfood, Whiteboard: [systemsfe][spark])
Attachments
(3 files, 3 obsolete files)
+++ This bug was initially created as a clone of Bug #1110010 +++ As documented in bug 1110010 comment 33, let's drop this blob.
Assignee | ||
Comment 1•9 years ago
|
||
Hm, QC's RIL code depends on libtime_genoff.so:
> 03-24 15:26:14.410 1397 1397 E RILD : dlopen failed: dlopen failed: could not load library "libtime_genoff.so" needed by "libril-qc-qmi-1.so"; caused by library "libtime_genoff.so" not found
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
> 04-26 00:04:43.470 262 262 E TimeKeep: Will execute: restore
> 03-24 17:36:37.000 262 262 E TimeKeep: Time restored!
> 03-24 17:36:37.990 295 295 I : Starting bootanimation with (1) format framebuffer
> 03-24 17:36:39.010 295 295 I Gecko : -*- TimeKeepService: B2G time service enabled
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8582638 -
Flags: review?(mwu)
Assignee | ||
Comment 5•9 years ago
|
||
Should we fork this repo to our mozilla-b2g account, also, or is it fine like this?
Attachment #8582639 -
Flags: review?(mwu)
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8582484 [details] [diff] [review] Support timekeep daemon r=... So this is implementing something similar to what was done in bug 1110010. I stole the time observer fix from there too. I'm just wondering whether we should: - try to detect the case where there is also qc-time-service present, and let it handle. We cannot rely on libtime_genoff being there since it's needed anyway by libril-qc stuff - maybe hide this service behing a build flag that would be set for the platforms that will support it Also, please note this should work on Flame too, but I have not yet tested it.
Attachment #8582484 -
Flags: feedback?(fabrice)
Attachment #8582484 -
Flags: feedback?(aosmond)
Assignee | ||
Comment 7•9 years ago
|
||
And FYI the original Java logic provided in the repository is visible at: https://github.com/sonyxperiadev/timekeep/blob/master/src/com/sony/timekeep/TimeKeep.java#L44 We are just doing the same but in Gecko :)
Comment 8•9 years ago
|
||
The service needs to be stored outside of the gecko repo since it's a device specific feature/hack. Only the devices that need it should have it installed, so there should be no need to detect whether it needs to be run.
Assignee | ||
Updated•9 years ago
|
Attachment #8582484 -
Attachment is obsolete: true
Attachment #8582484 -
Flags: feedback?(fabrice)
Attachment #8582484 -
Flags: feedback?(aosmond)
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #4) > Created attachment 8582638 [details] [review] > Shinano common PR So this one now includes the component, as suggested in comment 8.
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8582996 [details] [diff] [review] Fix time change observers r=... So that's the same as in bug 1110010 for just the time observer changes, stole from Andrew.
Flags: needinfo?(aosmond)
Attachment #8582996 -
Flags: review?(fabrice)
Assignee | ||
Comment 12•9 years ago
|
||
Bevis, we noticed that libgenoff_time used to somehow notify the modem. Do you know how we could check that we broke nothing on this side?
Flags: needinfo?(btseng)
Comment 13•9 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #12) > Bevis, we noticed that libgenoff_time used to somehow notify the modem. Do > you know how we could check that we broke nothing on this side? Sorry, I have no idea about this. :(
Flags: needinfo?(btseng)
Comment 14•9 years ago
|
||
Comment on attachment 8582638 [details] [review] Shinano common PR The manifest is under one license and the xpcom component is under another license. r=me with everything under apache 2.
Attachment #8582638 -
Flags: review?(mwu) → review+
Comment 15•9 years ago
|
||
Comment on attachment 8582639 [details] [review] Manifest PR Going to be a little more work to get a new remote in, but I'm ok with it. Also r=me if you decide to fork this to mozilla-b2g. Up to you.
Attachment #8582639 -
Flags: review?(mwu) → review+
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Michael Wu [:mwu] from comment #15) > Comment on attachment 8582639 [details] [review] > Manifest PR > > Going to be a little more work to get a new remote in, but I'm ok with it. > Also r=me if you decide to fork this to mozilla-b2g. Up to you. I've forked it so that we do not get breakages because of further changes that could happen in the future.
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Michael Wu [:mwu] from comment #14) > Comment on attachment 8582638 [details] [review] > Shinano common PR > > The manifest is under one license and the xpcom component is under another > license. r=me with everything under apache 2. Set the proper license in the XPCOM, thanks :)
Assignee | ||
Comment 18•9 years ago
|
||
https://github.com/mozilla-b2g/b2g-manifest/commit/e07d5a4158c216cc73c26e9934669ff1fd6cc680 https://github.com/mozilla-b2g/device-shinano-common/commit/9f104ade7020ef26ff3a914bbff26e68281ccfa7
Comment 19•9 years ago
|
||
Comment on attachment 8582996 [details] [diff] [review] Fix time change observers r=... Review of attachment 8582996 [details] [diff] [review]: ----------------------------------------------------------------- redirecting to bent since he was looking at the c++ changes in bug 1110010
Attachment #8582996 -
Flags: review?(fabrice) → review?(bent.mozilla)
Comment 20•9 years ago
|
||
Comment on attachment 8582996 [details] [diff] [review] Fix time change observers r=... Review of attachment 8582996 [details] [diff] [review]: ----------------------------------------------------------------- My only reservation with the patch (being the author :)) was perhaps making this generic would be better; for example, one could register for notifications when observers for a specific topic transition between empty and non-empty, and use the same mechanism for EventListenerManager::AddEventListenerInternal. It was very frustrating to realize observers could miss events due to there being no window event listeners, despite them being separate notification mechanisms.
Attachment #8582996 -
Flags: review+
Updated•9 years ago
|
Flags: needinfo?(aosmond)
Updated•9 years ago
|
Whiteboard: [systemsfe]
Comment on attachment 8582996 [details] [diff] [review] Fix time change observers r=... Review of attachment 8582996 [details] [diff] [review]: ----------------------------------------------------------------- I'd love to get this fixed ASAP but this patch is kind of backwards... We don't want to add any special code to nsObserverService, it's just a generic notification system. Instead we should just register for time notifications at startup, and whenever we get a notification we should call nsObserverService::NotifyObservers with the "system-clock-change" topic.
Attachment #8582996 -
Flags: review?(bent.mozilla) → review-
Assignee | ||
Comment 22•9 years ago
|
||
Thanks Ben, that indeed mostly matches Andrew's comment 20. I'll dig into this and try to fix. Andrew, in the meantime, if you have brain availability to fix it, feel free :)
Flags: needinfo?(aosmond)
Assignee | ||
Comment 23•9 years ago
|
||
The window dependency comes from bug 795328
Assignee | ||
Comment 24•9 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #23) > The window dependency comes from bug 795328 Never mind, it was there before.
Assignee | ||
Comment 25•9 years ago
|
||
So, just a couple of stupid questions to make sure I understand correctly. (In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #21) > Comment on attachment 8582996 [details] [diff] [review] > Fix time change observers r=... > > Review of attachment 8582996 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'd love to get this fixed ASAP but this patch is kind of backwards... > > We don't want to add any special code to nsObserverService, it's just a > generic notification system. What you dislike is: - in nsObserverService we had hard-coded specific handling for "system-clock-change" - the matching changes in the TimeChangeObserver that does |return nsSystemTimeChangeObserver::GetInstance()->EnableObserverServiceNotificationsImpl();| > > Instead we should just register for time notifications at startup, and > whenever we get a notification we should call > nsObserverService::NotifyObservers with the "system-clock-change" topic. I'm not sure to get exactly what you mean by "register for time notifications at startup". Are you referring to |RegisterSystemClockChangeObserver(sObserver);| ? Which startup are we referring about ? TimeChangeObserver, or device ?
Flags: needinfo?(bent.mozilla)
(In reply to Alexandre LISSY :gerard-majax from comment #25) > What you dislike is: > - in nsObserverService we had hard-coded specific handling for > "system-clock-change" Yep! > - the matching changes in the TimeChangeObserver that does |return > nsSystemTimeChangeObserver::GetInstance()- > >EnableObserverServiceNotificationsImpl();| I don't care about this really, whatever works! > I'm not sure to get exactly what you mean by "register for time > notifications at startup". I'm saying that we need a component that gets instantiated at xpcom-startup that calls registers an always-on listener. That listener then forwards to nsObserverList whenever it needs to. I guess we only need this with MOZ_WIDGET_GONK?
Flags: needinfo?(bent.mozilla)
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #26) > nsObserverList Oops, meant nsObserverService.
Assignee | ||
Comment 28•9 years ago
|
||
To ease further understanding, one symptom of the issue is: > $ adb logcat -v threadtime| grep -i 'hal::' > 04-09 13:10:22.000 328 328 I Gecko : hal::NotifySystemClockChange(1416949594700) > 04-09 13:10:25.160 328 328 I Gecko : hal::RegisterSystemClockChangeObserver() This is what is explained in bug 1110010 comment 31: the |RegisterSystemClockChangeObserver| call is only done once there is some window.
Comment 29•9 years ago
|
||
Updated as per bent's feedback. I think this is what you meant in combination with: https://bugzilla.mozilla.org/attachment.cgi?id=8590343&action=diff The QTimeService XPCOM component calls the register/deregister functions of nsITimeService to enable the notifications.
Attachment #8582996 -
Attachment is obsolete: true
Flags: needinfo?(aosmond)
Attachment #8590348 -
Flags: review?(bent.mozilla)
Comment 30•9 years ago
|
||
Minor fix, I was a little too fast on the trigger and left in some code that was used but was replaced.
Attachment #8590348 -
Attachment is obsolete: true
Attachment #8590348 -
Flags: review?(bent.mozilla)
Attachment #8590351 -
Flags: review?(bent.mozilla)
Comment on attachment 8590351 [details] [diff] [review] Fix time change observers, v2.1 Review of attachment 8590351 [details] [diff] [review]: ----------------------------------------------------------------- This looks mostly fine, but I think we can simplify this a bit: ::: dom/time/TimeService.cpp @@ +30,5 @@ > > NS_IMETHODIMP > +TimeService::Register() > +{ > + return mozilla::time::AddObserver(); Just call this in the constructor? @@ +36,5 @@ > + > +NS_IMETHODIMP > +TimeService::Deregister() > +{ > + return mozilla::time::RemoveObserver(); And call this in the destructor? ::: dom/time/nsITimeService.idl @@ +15,5 @@ > + /* Enable system clock change notifications. */ > + void register(); > + > + /* Disable system clock change notifications. */ > + void deregister(); I don't think these should be needed. Instead, just add an 'app-startup' entry to the category list here for TimeService: http://mxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutModule.cpp#1303 That way it will always be available, no need to manually register/deregister.
Attachment #8590351 -
Flags: review?(bent.mozilla) → review-
Updated•9 years ago
|
Priority: -- → P1
Comment 32•9 years ago
|
||
Alex, do we want to keep working on this?
Flags: needinfo?(lissyx+mozillians)
Whiteboard: [systemsfe] → [systemsfe][spark]
Assignee | ||
Comment 33•9 years ago
|
||
Well according to Andrew we would still need it, but I have not been able to prove any cases where not having it was blocking ; not even as in bug 1110010. Andrew, do you have some time?
Flags: needinfo?(lissyx+mozillians) → needinfo?(aosmond)
Comment 35•9 years ago
|
||
[Extending summary to mention the main symptom here, to increase discoverability for people who are about to file dupes of this]
Summary: [Shinano][Aries] Use timekeep instead of time_daemon → [Shinano][Aries] Use timekeep instead of time_daemon (aka timestamps shown for files over MTP are incorrect)
Assignee | ||
Comment 39•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #35) > [Extending summary to mention the main symptom here, to increase > discoverability for people who are about to file dupes of this] I disagree: filesystem dates are good, EXIF infos dates are good. I think it's a MTP-specific issue.
Summary: [Shinano][Aries] Use timekeep instead of time_daemon (aka timestamps shown for files over MTP are incorrect) → [Shinano][Aries] Use timekeep instead of time_daemon
Comment 40•9 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #39) > I disagree: filesystem dates are good, EXIF infos dates are good. I think > it's a MTP-specific issue. (in other words: recent dupes of this bug were actually about a different issue, & were incorrectly duped. That issue is now covered by bug 1167090.)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(aosmond)
Assignee | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•