Closed
Bug 1299775
Opened 8 years ago
Closed 8 years ago
Implement chrome.idle.onStateChanged
Categories
(WebExtensions :: Compatibility, defect, P2)
WebExtensions
Compatibility
Tracking
(firefox51 fixed, firefox52 fixed)
People
(Reporter: bsilverberg, Assigned: bsilverberg)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [idle] triaged)
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
aswan
:
review+
gchang
:
approval-mozilla-aurora+
|
Details |
The chrome.idle API [1] has been partially implemented as a stub which simply returns "active" whenever `queryState` is called.
This bug is to investigate the feasibility of implementing it more fully, and, if feasible, to track that implementation.
There are two methods and one event to potentially support:
- `queryState` method [2] which returns the current state of the system, which in Chrome is either "idle", "active" or "locked".
- `setDetectionInterval` method [3] which is used to set the detection interval which is in turn used by the onStateChanged event
- `onStateChanged` event [4] which is fired whenever the state changes, and uses the detection interval as set by `setDetectionInterval` to determine the state
It looks like we can use nsIIdleService [5], specifically the `idleTime` property, to implement `queryState`, although we can only provide either "active" or "idle" as "locked" does not seem to be supported. I have put together a proof of concept and it seems to work.
Implementing `onStateChanged` to work along with `setDetectionInterval` seems a bit more tricky. We can use the nsIIdleService `addIdleObserver` [6] method to implement `onStateChanged`, which would be fairly straightforward, but it would not behave the same as Chrome. If we used an observer created via that method then it would fire whenever the nsIIdleService decides that the system has changed from "idle" to "active" and back again, but it would not take the detection interval as set via `setDetectionInterval` into account. It would solely be the nsIIdleService deciding the state of the system. I think this would likely be acceptable for the purposes of delivering an `onStateChanged` event to extension developers, but I'd like to hear other opinions.
We could implement our own polling system, using nsIIdleService.idleTime, but this does not sound like an ideal solution to me.
As mentioned above, it also looks like we will be unable to support the "locked" state.
[1] https://developer.chrome.com/extensions/idle
[2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/idle/queryState
[3] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/idle/setDetectionInterval
[4] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/idle/onStateChanged
[5] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIIdleService
[6] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIIdleService#addIdleObserver()
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Flags: needinfo?(kmaglione+bmo)
Whiteboard: [triaged idle] → [idle] triaged
Assignee | ||
Comment 1•8 years ago
|
||
I suppose another option, if we want to support `onStateChanged` with `setDetectionInterval` would be to enhance the nsIIdleService to allow one to set a detection interval that is then used to determine the idle state, which would then be used by the observer.
Assignee | ||
Comment 2•8 years ago
|
||
Updating this bug to be only for setDetectionInterval and onStateChanged. Bug 1299846 has been opened for queryState.
Priority: -- → P2
Summary: Complete the implementation of chrome.idle → Implement chrome.idle.onStateChanged
Assignee | ||
Comment 3•8 years ago
|
||
Having looked at `addIdleObserver` again, it looks like it will actually work in a manner similar to Chrome, in that it does accept a time interval and the observer is only notified when a user has been idle for that many seconds. I'm not sure how I missed that the first time.
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
I have tested this manually to be sure that it works, but I'm not sure how to write an automated test for it. I've added some tests using a mock IdleService, to verify that we are in fact interacting with the service as the code expects, but it doesn't test the actual events. It neither tests that listeners would be fired when the observer is informed of events, nor that the data passed to the listener is accurate.
I suppose the lack of coverage for the observer/events might not be an issue as all we'd really be testing is stuff outside of the API code (i.e., things like EventEmitter and SingletonEventManager) which should be covered by tests themselves. It might be nice to test the data aspect, but I fear that is impossible without actually creating an idle state, which may also not be possible and in any case is highly undesirable. Even if I could figure out how to mock an event to trigger the listener, the test would still be using mock data, and not real data.
If you have any ideas of how to create additional coverage for this, I'd be happy to give them a try.
Also, I'm not in love with the code for the API. It feels kind of ugly to me, but I wasn't sure what a better version would look like, so feel free to make suggestions for that as well.
Comment 6•8 years ago
|
||
The idle service has its own tests. If you want to test this with the actual idle service in addition to a mock, you can do what those tests do, and check the current idle time, and then add an observer for a slightly longer time than that.
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #6)
> The idle service has its own tests.
Where are they? I couldn't find anything via searchfox.org.
Comment 8•8 years ago
|
||
(In reply to Bob Silverberg [:bsilverberg] from comment #7)
> (In reply to Kris Maglione [:kmag] from comment #6)
> > The idle service has its own tests.
>
> Where are they? I couldn't find anything via searchfox.org.
http://searchfox.org/mozilla-central/source/dom/base/test/test_bug715041.xul
http://searchfox.org/mozilla-central/source/widget/tests/test_bug343416.xul
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review75752
::: toolkit/components/extensions/ext-idle.js:22
(Diff revision 1)
> +var observersMap = new WeakMap();
> +
> +function getObserver(extension) {
> + let observerInfo = observersMap.get(extension);
> + let {observer, detectionInterval} = observerInfo;
> + detectionInterval = detectionInterval || 60;
You can move this into the body of the if below. Or, I think it would actually be cleaner to just install the default value when you first create the object in the map. (And initializing observer to null at that time would be nice too)
::: toolkit/components/extensions/ext-idle.js:26
(Diff revision 1)
> + let {observer, detectionInterval} = observerInfo;
> + detectionInterval = detectionInterval || 60;
> + if (!observer) {
> + observer = {
> + observe: function(subject, topic, data) {
> + this.emit("stateChanged", topic);
It looks like there's an "idle-daily" topic that can arrive here that isn't part of the documented webextensions idle api:
http://searchfox.org/mozilla-central/source/widget/nsIIdleService.idl#77
::: toolkit/components/extensions/ext-idle.js:37
(Diff revision 1)
> + observerInfo.detectionInterval = detectionInterval;
> + }
> + return observer;
> +}
> +
> +function setDetectionInterval(extension, detectionIntervalInSeconds) {
Can you call the second parameter something like newInterval instead?
::: toolkit/components/extensions/ext-idle.js:47
(Diff revision 1)
> + idleService.addIdleObserver(observer, detectionIntervalInSeconds);
> + }
> + observerInfo.detectionInterval = detectionIntervalInSeconds;
> +}
> +
> +/* eslint-disable mozilla/balanced-listeners */
I think you could avoid this by generating entries in this map on the fly as needed (this would also mean no entries in this map for extensions that don't use this api). And the shutdown handler can simply be replaced by a call to context.callOnClose()
::: toolkit/components/extensions/schemas/idle.json:60
(Diff revision 1)
> "events": [
> {
> "name": "onStateChanged",
> - "unsupported": true,
> "type": "function",
> - "description": "Fired when the system changes to an active, idle or locked state. The event fires with \"locked\" if the screen is locked or the screensaver activates, \"idle\" if the system is unlocked and the user has not generated any input for a specified number of seconds, and \"active\" when the user generates input on an idle system.",
> + "description": "Fired when the system changes to an active or idle. The event fires with \"idle\" if the the user has not generated any input for a specified number of seconds, and \"active\" when the user generates input on an idle system.",
nit: put the world "state" back at the end of the first sentence.
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review75754
Pretty minor comments, the main implementation looks good, but I think tests with the real idle service as discussed over in bugzilla would be good before landing.
Attachment #8789022 -
Flags: review?(aswan)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review75752
> I think you could avoid this by generating entries in this map on the fly as needed (this would also mean no entries in this map for extensions that don't use this api). And the shutdown handler can simply be replaced by a call to context.callOnClose()
I removed this section and am now only creating a map entry as needed. I tried to figure out where to add the call to `context.callOnClose()` but failed - it didn't seem to want to go anywhere. Where do you think it belongs?
Assignee | ||
Comment 13•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review75754
I enhanced the mock to fire events so we can test the plubming of the event handling.
Comment 14•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review75752
> I removed this section and am now only creating a map entry as needed. I tried to figure out where to add the call to `context.callOnClose()` but failed - it didn't seem to want to go anywhere. Where do you think it belongs?
The logical place would be the inner clause in `getObserverInfo()` where the entry in the map is created. You'll need to pass the context all the way down to enable that though of course.
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review76662
Are you plannning to add a test that uses the real idle service or go with the existing test only?
::: toolkit/components/extensions/ext-idle.js:37
(Diff revisions 1 - 2)
> let {observer, detectionInterval} = observerInfo;
> - detectionInterval = detectionInterval || 60;
> if (!observer) {
> observer = {
> observe: function(subject, topic, data) {
> + if (topic != "idle-daily") {
Since there are only two values you care about here, it seems safer to just check if topic is one of those.
Attachment #8789022 -
Flags: review?(aswan)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review76662
I wasn't planning on adding a test with the real idle service, as the tests I added in the last commit do cover the API code, imo. Tests which use the real idle service would really only test the real idle service, and I think we can assume that it works as advertised as it has its own tests. I could add a test using `setTimeout()` as is done for the service itself [1], but I'd rather not do that, which would add a delay into the running of the test. If you think it's really important to do so, I can.
[1] http://searchfox.org/mozilla-central/source/widget/tests/test_bug343416.xul
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review77326
Can you add a test for setting the detection interval and then adding a listener?
Regarding testing with the real service vs mocks, the things this mocked test won't catch are mismatches in things like values being seconds vs milliseconds, a misspelled topic name, etc. But if you're satisfied with this, its okay by me.
::: toolkit/components/extensions/test/xpcshell/test_ext_idle.js:44
(Diff revision 3)
> + let {expectedAdd, expectedRemove, expectedFires} = expectedActivity;
> + let {addCalls, removeCalls, observerFires} = idleService._activity;
> + equal(expectedAdd.length, addCalls.length, "idleService.addIdleObserver was called the expected number of times");
> + equal(expectedRemove.length, removeCalls.length, "idleService.removeIdleObserver was called the expected number of times");
> + equal(expectedFires.length, observerFires.length, "idle observer was fired the expected number of times");
> + for (let [i, interval] of expectedAdd.entries()) {
Can't you just use `deepEqual()` here (and below)?
Attachment #8789022 -
Flags: review?(aswan) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review77326
I added the test you suggested. Having tested it manually I am fine with having that missing coverage. Looking at the API spec, if we wanted to do a "real" test then we'd need to wait at least 15 seconds in the test as that's the minimum a developer is allowed to pass into `setDetectionInterval`, and that seems like a long time to wait in a single test.
> Can't you just use `deepEqual()` here (and below)?
Yes, I can. Fixed!
Comment 21•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review77326
I'm not so sure about that. We should just be able to get the current idle time (which should almost certainly be longer than 15s when running on infra) and just add ~1s to it (but `Math.max(secs+1s, 15s)`).
Assignee | ||
Comment 22•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review77326
Ah, so because the tests have been running for awhile, the system is already idle. But, if that's the case then we'd need to trigger it to change from idle to active, in order for the listener to fire, correct? How would we do that?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
I added a test that uses the real idle service (which needed to be a mochitest).
Attachment #8789022 -
Flags: review+ → review?(aswan)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•8 years ago
|
||
Hmm, this failed on all platforms on try, although it was perfectly happy on my local machine. I made one tweak and added some debugging code and have resubmitted it to try.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 30•8 years ago
|
||
mozreview-review |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review79114
::: toolkit/components/extensions/test/mochitest/test_chrome_ext_idle.html:22
(Diff revision 9)
> +const idleService = Cc["@mozilla.org/widget/idleservice;1"].getService(Ci.nsIIdleService);
> +
> +add_task(function* testWithRealIdleService() {
> + function background() {
> + browser.test.onMessage.addListener(function(msg) {
> + let detectionInterval = arguments[1];
Make this function take spread args rather than using `arguments`
::: toolkit/components/extensions/test/mochitest/test_chrome_ext_idle.html:50
(Diff revision 9)
> + },
> + });
> +
> + yield extension.startup();
> + let idleTime = idleService.idleTime;
> + let detectionInterval = Math.max(Math.ceil(idleTime / 1000) + 2, 15);
What's the logic here? The call to Math.ceil() doesn't seem right, if we're already idle for longer that 15 seconds, then the listener below will never fire...
Attachment #8789022 -
Flags: review?(aswan)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 32•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review79114
> What's the logic here? The call to Math.ceil() doesn't seem right, if we're already idle for longer that 15 seconds, then the listener below will never fire...
`Math.ceil` is just used to convert the result of `idleTime / 1000` to an integer, as only integers are allowed for detectionInterval. It's not applied to the entire expression. `Math.max` is applied to the entire expression so that we use the maximum of either 15, or the number arrived at by dividing `idleTime` by 1000 and then adding 2. When run on infra the idle time is already over 15 seconds when the test is run and it passes, so there's no concern that the listener will never fire.
Comment 33•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review79114
> `Math.ceil` is just used to convert the result of `idleTime / 1000` to an integer, as only integers are allowed for detectionInterval. It's not applied to the entire expression. `Math.max` is applied to the entire expression so that we use the maximum of either 15, or the number arrived at by dividing `idleTime` by 1000 and then adding 2. When run on infra the idle time is already over 15 seconds when the test is run and it passes, so there's no concern that the listener will never fire.
Sorry typo, I meant max, not ceil. But now I'm confused, if this passes if the interval is larger than the idle time (ie, if idleTime+2 is < 15) and it also passes if the interval is smaller than the idle time (ie, if idleTime > 15), then what exactly are we testing here?
Assignee | ||
Comment 34•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review79114
> Sorry typo, I meant max, not ceil. But now I'm confused, if this passes if the interval is larger than the idle time (ie, if idleTime+2 is < 15) and it also passes if the interval is smaller than the idle time (ie, if idleTime > 15), then what exactly are we testing here?
I am confused by your confusion. ;) We are testing that the listener will fire when the idle state changes (and that state-change is based on the detection interval that we set).
On my local machine, when I run the test, idleTime is less than 15 seconds, so I see the test wait until the detectionInterval (15 seconds) has passed, and then the listener fires and the test completes. On infra, when the test runs, idleTime is already above 15 seconds, so we set the detectionInterval to a higher amount, so that the listener will fire when the interval is met. The test also passes there.
The 15 second thing is required because that is the minimum that the API will allow us to set the detectionInterval to.
Comment 35•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review79114
> I am confused by your confusion. ;) We are testing that the listener will fire when the idle state changes (and that state-change is based on the detection interval that we set).
>
> On my local machine, when I run the test, idleTime is less than 15 seconds, so I see the test wait until the detectionInterval (15 seconds) has passed, and then the listener fires and the test completes. On infra, when the test runs, idleTime is already above 15 seconds, so we set the detectionInterval to a higher amount, so that the listener will fire when the interval is met. The test also passes there.
>
> The 15 second thing is required because that is the minimum that the API will allow us to set the detectionInterval to.
Ah, got it, sorry for the confusion.
Comment 36•8 years ago
|
||
mozreview-review |
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
https://reviewboard.mozilla.org/r/77304/#review79222
Attachment #8789022 -
Flags: review?(aswan) → review+
Comment 38•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a4404f79adb8
Implement chrome.idle.onStateChanged, r=aswan
Keywords: checkin-needed
Comment 39•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Comment 40•8 years ago
|
||
Assignee | ||
Comment 41•8 years ago
|
||
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
Approval Request Comment
[Feature/regressing bug #]: 1299775
[User impact if declined]: Part of the WebExtensions idle API is landing in 51, but this feature didn't make the merge and hence won't land until 52. Ideally all parts of the API would become available to web extensions developers in 51.
[Describe test coverage new/current, TreeHerder]: There are tests for the new feature in the patch. I have also submitted a try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c00b8c54434f2db3a04a85ac2167c77f023f726.
[Risks and why]: This is a single API method and is covered by tests, so the risk is low.
[String/UUID change made/needed]: none
Attachment #8789022 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
status-firefox51:
--- → affected
Comment 42•8 years ago
|
||
Comment on attachment 8789022 [details]
Bug 1299775 - Implement chrome.idle.onStateChanged,
Webextension API will be shipped in 51 and tests are included. Take this in 51 aurora.
Hi :bsilverberg,
For the "Feature/regressing bug" in template, this should not be the bug itself. Is bug 1252215/bug 1269342 the feature you mentioned?
Flags: needinfo?(bob.silverberg)
Attachment #8789022 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 43•8 years ago
|
||
bugherder uplift |
Assignee | ||
Comment 44•8 years ago
|
||
(In reply to Gerry Chang [:gchang] from comment #42)
> Comment on attachment 8789022 [details]
> Bug 1299775 - Implement chrome.idle.onStateChanged,
>
> Webextension API will be shipped in 51 and tests are included. Take this in
> 51 aurora.
>
> Hi :bsilverberg,
> For the "Feature/regressing bug" in template, this should not be the bug
> itself. Is bug 1252215/bug 1269342 the feature you mentioned?
No, this has nothing to do with either of those bugs. The bug that landed associated functionality in 51 is bug 1299846, so maybe that is the correct bug to reference?
Flags: needinfo?(bob.silverberg)
Assignee | ||
Comment 45•8 years ago
|
||
Adding dev-doc-needed as the version in which this feature lands has changed.
Keywords: dev-doc-needed
Comment 46•8 years ago
|
||
Bob, I've updated the compat data to indicate that this API (and setDetectionInterval()) are supported in Firefox 51:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/idle#Browser_compatibility
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/idle/onStateChanged
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/idle/setDetectionInterval
Please let me know if you need anything else.
Flags: needinfo?(bob.silverberg)
Updated•8 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•