Closed Bug 967475 Opened 11 years ago Closed 11 years ago

Implement navigator.mozResendAllNotifications

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Version:
29 Branch
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
2.0 S2 (23may)
Project Flags:
tracking-b2g backlog
Tracking Flags:
Tracking Status
b2g-v1.3T --- ?
b2g-v1.4 --- ?

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 40 obsolete files)

Link to Github https://github.com/mozilla-b2g/gaia/pull/16396
46 bytes, text/x-github-pull-request
qdot
: review+
timdream
: review+
arthurcc
: review+
timdream
: feedback+
Details | Review
0001-Bug-967475-Implement-mozChromeNotifications-API-r-mh_hg.patch
41.55 KB, patch
gerard-majax
: review+
Details | Diff | Splinter Review
In order to fix bug 874364, we need a special API for the system app that allows to retrieve all the stored notification. Proposal has been discussed in bug 874364, we implement a navigator.mozGetAllNotifications() that will return a Promise of Notification object for all the stored notifications.
Attached patch WIP patch (obsolete) — — Splinter Review
Plumbery patch ready: added the API, able to return an empty Promise.
Attached patch WIP patch (obsolete) — — Splinter Review
Fetching from NotificationDB works
Attachment #8370748 - Attachment is obsolete: true
Attached patch WIP patch (obsolete) — — Splinter Review
Do not export the whole NotificationDB object
Attachment #8370779 - Attachment is obsolete: true
Attached patch WIP patch (obsolete) — — Splinter Review
Objects are now getting correctly back to system app: E/GeckoConsole( 4131): Content JS DEBUG at app://system.gaiamobile.org/js/bootstrap.js:72 in onSuccess: Got notifications: 3 E/GeckoConsole( 4131): Content JS DEBUG at app://system.gaiamobile.org/js/bootstrap.js:73 in onSuccess: Got notifications: => [{},{},{}] E/GeckoConsole( 4131): Content JS DEBUG at app://system.gaiamobile.org/js/bootstrap.js:76 in onSuccess: Got one notification: {1298062c-b16c-4ba3-ad1c-7f62d21caec2} E/GeckoConsole( 4131): Content JS DEBUG at app://system.gaiamobile.org/js/bootstrap.js:76 in onSuccess: Got one notification: {1f441f29-e70d-4cf9-8c85-0bd5d6076c76} E/GeckoConsole( 4131): Content JS DEBUG at app://system.gaiamobile.org/js/bootstrap.js:76 in onSuccess: Got one notification: {0cd29186-12e2-4135-bccd-2158fa2adad8}
Attachment #8370846 - Attachment is obsolete: true
Attached patch WIP patch for Gecko (obsolete) — — Splinter Review
Sending events to System app with origin included
Attachment #8371328 - Attachment is obsolete: true
Attached patch WIP patch for Gaia System app (obsolete) — — Splinter Review
Retrieving notifications, sending events to populate notification tray.
Gaia version with just mozResendAllNotifications().
Attachment #8371368 - Attachment is obsolete: true
Attached patch [deprecated] WIP patch for Gecko (obsolete) — — Splinter Review
Implementing mozResendAllNotifications(), everything done on Gecko side.
Attachment #8371367 - Attachment is obsolete: true
Attachment #8371595 - Attachment description: WIP patch for Gaia System app → [deprecated] WIP patch for Gaia System app
Attachment #8371596 - Attachment description: WIP patch for Gecko → [deprecated] WIP patch for Gecko
Gecko part working well with SMS app
Gaia part, asking for notifications to be resent at boot time.
Fixing some coding style issues
Attachment #8371678 - Attachment is obsolete: true
Blocks: 968955
Fixing behavior and correctly opening apps
Attachment #8372083 - Attachment is obsolete: true
Depends on: 970873
Whiteboard: [systemsfe]
Target Milestone: --- → 1.4 S2 (28feb)
Update patch: limit scope to Certified Apps, and add a 'desktop-notification-resend' permission.
Attachment #8372342 - Attachment is obsolete: true
Update: adding the desktop-notification-resend permission to the manifest.
Attachment #8371679 - Attachment is obsolete: true
Augmenting the API to add a callback notifying the number of resent notifications, and preparing mochitests.
Attachment #8377029 - Attachment is obsolete: true
Making use of the newly introduced callback, sending a desktop-notification-resend event with the amount of notifications that will be resent, and inhibits sound/vibration for all those notifications. Also adding unit tests.
Attachment #8377031 - Attachment is obsolete: true
Please find attached the link to the pull request for the Gaia part. Travis for this is green: https://travis-ci.org/mozilla-b2g/gaia/builds/19105333 We will wait for review on the Gecko API before reviewing and landing this.
Comment on attachment 8377508 [details] [diff] [review] 0001-Bug-967475-Implement-mozChromeNotifications-API.patch Meanwhile I finish writing tests, would you mind giving a first feedback?
Attachment #8377508 - Flags: feedback?(mhenretty)
Comment on attachment 8377509 [details] [diff] [review] 0001-Bug-967475-Request-all-stored-notifications-on-boot.patch Meanwhile I finish writing tests, would you mind giving a first feedback?
Attachment #8377509 - Flags: review?(mhenretty)
Comment on attachment 8377508 [details] [diff] [review] 0001-Bug-967475-Implement-mozChromeNotifications-API.patch Review of attachment 8377508 [details] [diff] [review]: ----------------------------------------------------------------- Overall, I think the approach is right. Biggest thing I see is having two different places generating the alertName. ::: b2g/components/AlertsService.js @@ +111,4 @@ > // the notification so the app get a change to react. > if (data.target) { > gSystemMessenger.sendMessage("notification", { > + clicked: (topic === "alertclickcallback"), Why are we adding this for mozResend? ::: dom/src/notification/ChromeNotifications.js @@ +49,5 @@ > + let notificationCallback = (function(notifications) { > + for (let i = 0; i < notifications.length; i++) { > + let notification = notifications[i]; > + let tag = (notification.tag != '') ? > + ('#tag:' + notification.tag) : ('#notag:' + notification.id); This is extremely unfortunate. We now have two places where we construct the AlertName (one in c++, one in JS) which will need to be kept in sync. I'm wondering if there is a way around this, like perhaps storing the AlertName in the DB as well? Or even using AlertName as the ID in the DB? ::: dom/src/notification/NotificationDB.jsm @@ +228,4 @@ > this.taskGetAll(task.data, wrappedCallback); > break; > > + case "mozgetall": I think we need a more descriptive name here, perhaps 'getallacrossorigin'? @@ +254,4 @@ > callback(notifications); > }, > > + taskMozGetAll: function(callback) { ...perhaps 'taskGetAllAcrossOrigin' ::: dom/webidl/ChromeNotifications.webidl @@ +5,5 @@ > + */ > + > +[JSImplementation="@mozilla.org/mozChromeNotifications;1", > + NavigatorProperty="mozChromeNotifications", > + AvailableIn="CertifiedApps"] Do we need both the permission check above, and this AvailableIn scoping? It seems to me, after reading through https://bugzilla.mozilla.org/show_bug.cgi?id=958667 that we might just need this scoping attribute.
Attachment #8377508 - Flags: feedback?(mhenretty) → feedback+
Comment on attachment 8377509 [details] [diff] [review] 0001-Bug-967475-Request-all-stored-notifications-on-boot.patch I made some comments on the PR on github. I think I will need to do a further review of the Gaia changes once the Gecko change is finalized.
Attachment #8377509 - Flags: review?(mhenretty)
Also, I just talked to ehsan on IRC, and he says there is no reason to use both the AvailableIn attribute, and have a permission for 'desktop-notification-resend' if they both ensure the same thing (ie. the app is certified). For the gecko patch, I think we can remove the permission.
Summary: Implement navigator.mozGetAllNotifications → Implement navigator.mozResendAllNotifications
Depends on: 971766
Addressing previous remarks on Gaia part.
Attachment #8377509 - Attachment is obsolete: true
Of course the Github PR has been updated :)
Addressing the Gecko part remarks
Attachment #8377508 - Attachment is obsolete: true
(In reply to Michael Henretty [:mhenretty] from comment #20) > Comment on attachment 8377508 [details] [diff] [review] > 0001-Bug-967475-Implement-mozChromeNotifications-API.patch > > Review of attachment 8377508 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall, I think the approach is right. Biggest thing I see is having two > different places generating the alertName. > > ::: b2g/components/AlertsService.js > @@ +111,4 @@ > > // the notification so the app get a change to react. > > if (data.target) { > > gSystemMessenger.sendMessage("notification", { > > + clicked: (topic === "alertclickcallback"), > > Why are we adding this for mozResend? Some apps are relying on this and if we don't, bad things happens. > > ::: dom/src/notification/ChromeNotifications.js > @@ +49,5 @@ > > + let notificationCallback = (function(notifications) { > > + for (let i = 0; i < notifications.length; i++) { > > + let notification = notifications[i]; > > + let tag = (notification.tag != '') ? > > + ('#tag:' + notification.tag) : ('#notag:' + notification.id); > > This is extremely unfortunate. We now have two places where we construct the > AlertName (one in c++, one in JS) which will need to be kept in sync. I'm > wondering if there is a way around this, like perhaps storing the AlertName > in the DB as well? Or even using AlertName as the ID in the DB? I don't have a strong opinion on how to fix this, but I agree this is unfortunate. What about storing it in the DB ? Maybe not as the id, but as another field ? > > ::: dom/src/notification/NotificationDB.jsm > @@ +228,4 @@ > > this.taskGetAll(task.data, wrappedCallback); > > break; > > > > + case "mozgetall": > > I think we need a more descriptive name here, perhaps 'getallacrossorigin'? Done. > > @@ +254,4 @@ > > callback(notifications); > > }, > > > > + taskMozGetAll: function(callback) { > > ...perhaps 'taskGetAllAcrossOrigin' Done. > > ::: dom/webidl/ChromeNotifications.webidl > @@ +5,5 @@ > > + */ > > + > > +[JSImplementation="@mozilla.org/mozChromeNotifications;1", > > + NavigatorProperty="mozChromeNotifications", > > + AvailableIn="CertifiedApps"] > > Do we need both the permission check above, and this AvailableIn scoping? It > seems to me, after reading through > https://bugzilla.mozilla.org/show_bug.cgi?id=958667 that we might just need > this scoping attribute. Done also. Please note that this version includes tests.
Comment on attachment 8379065 [details] [diff] [review] 0001-Bug-967475-Request-all-stored-notifications-on-boot.patch Review request for the updated gaia part :)
Attachment #8379065 - Flags: review?(mhenretty)
Comment on attachment 8379066 [details] [diff] [review] 0001-Bug-967475-Implement-mozChromeNotifications-API.patch Review request for the updated gecko part
Attachment #8379066 - Flags: review?(mhenretty)
I've tested on my Inari device, and this is working as expected. A Try build is pending at: https://tbpl.mozilla.org/?tree=Try&rev=b7459a81f2f9
(In reply to Alexandre LISSY :gerard-majax from comment #29) > I've tested on my Inari device, and this is working as expected. A Try build > is pending at: https://tbpl.mozilla.org/?tree=Try&rev=b7459a81f2f9 That Try fails, probably due to the lack of clobbering. A new Try is available at: https://tbpl.mozilla.org/?tree=Try&rev=1a943b6e328e that includes CLOBBER changes and also packaging of mozChromeNotifications.
Updating, that should fix Try testing issues: https://tbpl.mozilla.org/?tree=Try&rev=83f30268c71c I still have to address the refactoring of notification name. As discussed on IRC, I will hack to save it in the database and thus get it back already nicely formatted.
Attachment #8379066 - Attachment is obsolete: true
Attachment #8379066 - Flags: review?(mhenretty)
Update: fixing latest tests on Try, and saving the alertName in the database.
Attachment #8379065 - Attachment is obsolete: true
Attachment #8379680 - Attachment is obsolete: true
Attachment #8379065 - Flags: review?(mhenretty)
Pending Try that matches uptodate patch: https://tbpl.mozilla.org/?tree=Try&rev=005571efc8ed
(In reply to Alexandre LISSY :gerard-majax from comment #33) > Pending Try that matches uptodate patch: > https://tbpl.mozilla.org/?tree=Try&rev=005571efc8ed All green, apart on B2G Emulator ICS Opt/Debug: https://tbpl.mozilla.org/php/getParsedLog.php?id=35060589&tree=Try#error0 It seems the system cannot load the JS I implemented. I see: 11:10:32 INFO - 555 INFO TEST-INFO | /tests/dom/tests/mochitest/notification/test_notification_resend.html | ::Notification Tests:: - Test that we have mozChromeNotifications API 11:10:32 INFO - [Child 718] WARNING: NS_ENSURE_TRUE(XRE_GetProcessType() == GeckoProcessType_Default) failed: file ../../../../gecko/content/base/src/nsFrameMessageManager.cpp, line 1902 11:10:32 INFO - System JS : ERROR resource://gre/modules/XPCOMUtils.jsm:202 - NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService] 11:10:32 INFO - [Child 718] WARNING: Failed to get JS implementation for contract: file ../../../gecko/dom/bindings/BindingUtils.cpp, line 2044 I'm investigating this last failure.
We need to stop importing NotificationDB.jsm and rather use message passing.
Updating, changing the way we communicate with NotificationDB to rather use the message passing interface. Locally, I have green on notifications mochitests for b2g-desktop and b2g emulator ics.
Attachment #8379798 - Attachment is obsolete: true
Fixing the tests for some last timeout on resend test on B2G ICS Emulator Debug.
Attachment #8380221 - Attachment is obsolete: true
Looks like everything is green for mochitests at: https://tbpl.mozilla.org/?tree=Try&rev=4f0b0bd12cd2 :)
Try green is stable after rebasing on uptodate master: https://tbpl.mozilla.org/?tree=Try&rev=59a94dd0e674
blocking-b2g: --- → backlog
Target Milestone: 1.4 S2 (28feb) → ---
Rebased.
Attachment #8380360 - Attachment is obsolete: true
Rebased.
Attachment #8386800 - Attachment is obsolete: true
Rebased.
Attachment #8388464 - Attachment is obsolete: true
Depends on: 930794
Rebased after bug 930794
Attachment #8400650 - Attachment is obsolete: true
Rebased.
Attachment #8400725 - Attachment is obsolete: true
What's left to do here? I think we can do another try run, get a review and finally land! \o/
Removing notifications that do not have an alertName.
Attachment #8404531 - Attachment is obsolete: true
Merging bug 968955.
Attachment #8404659 - Attachment is obsolete: true
Attachment #8404679 - Flags: review?(mhenretty)
Comment on attachment 8404679 [details] [diff] [review] 0002-Bug-967475-Implement-mozChromeNotifications-API.patch Review of attachment 8404679 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good. Just a couple of code comments below. Also, I ran the notification mochitests against the browser locally, and they seemed to fail. Perhaps I am doing something wrong, because they seemed fine in the try run. 0:55.94 51 INFO TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/notification/test_notification_resend.html | should have mozChromeNotifications API 0:55.94 52 INFO TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/notification/test_notification_resend.html | Test threw exception! 0:55.94 59 INFO TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/notification/test_notification_resend.html | Test threw exception! Also, I applied your gaia patch to a gecko build, and clicking on notifications did not result in bringing the originating app to the foreground. I also saw this error in logcat. I/GeckoDump( 1147): XXX FIXME : Got a mozContentEvent: desktop-notification-click I/Gecko ( 1587): [Child 1587] ###!!! ASSERTION: Uh, inner window set as event target!: '!win || !win->IsInnerWindow()', file /Volumes/firefoxos/mozilla-central/dom/events/Event.cpp, line 577 The funny thing is, I could reboot the device and then click on the notification, and the app would indeed be brought to the foreground. Something weird must be happening with the clickhandler of the running app vs a system message to a non-running app. I'm not convinced this is related to your patch. ::: dom/src/notification/ChromeNotifications.js @@ +3,5 @@ > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; > + > +const DEBUG = true; nit: false; @@ +16,5 @@ > +const Ci = Components.interfaces; > +const Cu = Components.utils; > + > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); Why do we need Services here? @@ +39,5 @@ > + > +ChromeNotifications.prototype = { > + > + performResend: function(notifications) { > + this.resentNotifications = 0; Why do we store resentNotifications on the object? It seems we just need it for the scope of this function. @@ +75,5 @@ > + this.performResend(message.data.notifications); > + break; > + > + default: > + debug("Unrecognized message: " + message.name); To prevent the string concat and extra function call, we usually do: if (DEBUG) { debug("Unrecognized message: " + message.name); } ::: dom/src/notification/Notification.cpp @@ +462,5 @@ > nsString id; > notification->GetID(id); > + > + nsString alertName; > + notification->GetAlertName(alertName); Now that we are always generating the alertName when we create the notification (rather than just on notification events), I think alertName belongs as a member variable so we don't have to keep calling this function. ::: dom/src/notification/NotificationDB.jsm @@ +65,5 @@ > for (var id in this.notifications[origin]) { > var curNotification = this.notifications[origin][id]; > + > + // Trash away saved notifications that do not have the > + // 'alertName' being recorded. We need more description here. Something like: "Notifications without the alertName field cannot be resent by mozResendAllNotifications, so we discard them to avoid conflicts between displayed notifications and those returned with Notification.get()." ::: dom/src/notification/NotificationStorage.js @@ +46,5 @@ > body: body, > tag: tag, > + icon: icon, > + alertName: alertName, > + timestamp: new Date().getTime() Just a note for the future: If we add createTime to the notification object, we will probably have to generate the timestamp in C++ rather than in the DB. But doing it here is fine for now. ::: dom/tests/mochitest/notification/NotificationTest.js @@ +57,5 @@ > + promise.then(function (notifications) { > + is(notifications.length, 0, "notifications are all cleaned"); > + done(); > + }); > + }, 1000); We shouldn't be doing this wait here. Instead we should trigger those check only after we have implicitly called close on all the notifications. In other words, this entire function should be moved into the .then callback above on line 49. ::: dom/tests/mochitest/notification/test_notification_resend.html @@ +56,5 @@ > + > + function (done) { > + info("Trying to resend the notification"); > + navigator.mozChromeNotifications.mozResendAllNotifications(function(number) { > + is(number, 1, "One notification resent"); I would love to make this test a little more intereseting. Maybe we send two notifications initially, fetch them, close 1, and the resend that one and make sure there is only still one. Something like that.
Attachment #8404679 - Flags: review?(mhenretty) → review+
Thanks, I've addressed most of the nits. I'm going to try to improve the tests as you suggested, it makes a lot of sense.
Comment on attachment 8377583 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/16396 I forgot to set review flag on the Gaia part too :)
Attachment #8377583 - Flags: review?(mhenretty)
Comment on attachment 8404679 [details] [diff] [review] 0002-Bug-967475-Implement-mozChromeNotifications-API.patch Review of attachment 8404679 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/src/notification/ChromeNotifications.js @@ +11,5 @@ > + dump("-*- ChromeNotifications.js: " + s + "\n"); > + } > +} > + > +const Cc = Components.classes; Not needed. @@ +33,5 @@ > +function ChromeNotifications() { > + this.resentNotifications = -1; > + this.resendCallback = undefined; > + > + cpmm.addMessageListener("Notification:GetAllAccrossOrigin:Return:OK", this); You also have to remove the Listener when we shut down otherwise we leak. Look at the SettingsManager.js. ::: dom/src/notification/NotificationDB.jsm @@ +45,4 @@ > ppmm.addMessageListener("Notification:Save", this); > ppmm.addMessageListener("Notification:Delete", this); > ppmm.addMessageListener("Notification:GetAll", this); > + ppmm.addMessageListener("Notification:GetAllAccrossOrigin", this); We have to unregister all of them. Please make an array with all the titles and iterate over them for register and unregister.
Thanks for your feedback, I've addressed them. A new try is running at https://tbpl.mozilla.org/?tree=Try&rev=917f270c464f with the changes.
Michael, I've addressed most of your remarks and also those from Gregor. Attached is a patch with the fixes, and also I've added some more testing regarding what is being passed to the showAppNotification() method. After relooking at the whole code, I think it would also be safer to add more integration testing on Gaia side.
Attachment #8404679 - Attachment is obsolete: true
Attachment #8406021 - Flags: review?(mhenretty)
Added more tests on Gecko side, especially against NotificationDB. Please also note that I've pushed new integration tests on the Gaia side, so that travis is red until we land the Gecko part.
Attachment #8406021 - Attachment is obsolete: true
Attachment #8406021 - Flags: review?(mhenretty)
Attachment #8406765 - Flags: review?(mhenretty)
Attachment #8371595 - Attachment is obsolete: true
Attachment #8371596 - Attachment is obsolete: true
Comment on attachment 8406765 [details] [diff] [review] 0001-Bug-967475-Implement-mozChromeNotifications-API.patch Review of attachment 8406765 [details] [diff] [review]: ----------------------------------------------------------------- This is still an r+, so long as the NotificationTest part is fixed to not rely on setTimeout. ::: dom/src/notification/Notification.cpp @@ +462,5 @@ > nsString id; > notification->GetID(id); > + > + nsString alertName; > + notification->GetAlertName(alertName); I still think we should make this a member variable. ::: dom/src/notification/NotificationDB.jsm @@ +292,5 @@ > + for (var i in this.notifications[origin]) { > + var notification = this.notifications[origin][i]; > + > + // Notifications without the alertName field cannot be resent by > + // mozResendAllNotifications, so we discard them to avoid Let's update this comment to not say we "discard" notifications, but rather that we just don't resend them. ::: dom/tests/mochitest/notification/NotificationTest.js @@ +62,5 @@ > + remaining.then(function(notifications) { > + is(notifications.length, 0, "no more notification"); > + done(); > + }); > + }, 1000); This needs to be reworked. I can imagine situations where we are calling done twice, which is not good. Also, using a 1 second setTimeout is the hopes gecko has enough time to finish other async tasks is just asking for trouble. Better would be to attach onclose handlers to each of the notifications retrieved, and then call done when all of those onclose handlers have been called. Somthing like. var closeCount = 0; var handleAll = function() { if (++closeCount === notifications.length) { done(); } }; notifications.forEach(function(n) { n.onclose = handleAll; n.close(); }); ...and then we shouldn't need the setTimeout.
Attachment #8406765 - Flags: review?(mhenretty) → review+
(In reply to Michael Henretty [:mhenretty] from comment #59) > Comment on attachment 8406765 [details] [diff] [review] > 0001-Bug-967475-Implement-mozChromeNotifications-API.patch > > Review of attachment 8406765 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is still an r+, so long as the NotificationTest part is fixed to not > rely on setTimeout. > > ::: dom/src/notification/Notification.cpp > @@ +462,5 @@ > > nsString id; > > notification->GetID(id); > > + > > + nsString alertName; > > + notification->GetAlertName(alertName); > > I still think we should make this a member variable. You're right, I missed to fix this. > > ::: dom/src/notification/NotificationDB.jsm > @@ +292,5 @@ > > + for (var i in this.notifications[origin]) { > > + var notification = this.notifications[origin][i]; > > + > > + // Notifications without the alertName field cannot be resent by > > + // mozResendAllNotifications, so we discard them to avoid > > Let's update this comment to not say we "discard" notifications, but rather > that we just don't resend them. Done, thanks. > > ::: dom/tests/mochitest/notification/NotificationTest.js > @@ +62,5 @@ > > + remaining.then(function(notifications) { > > + is(notifications.length, 0, "no more notification"); > > + done(); > > + }); > > + }, 1000); > > This needs to be reworked. I can imagine situations where we are calling > done twice, which is not good. Also, using a 1 second setTimeout is the > hopes gecko has enough time to finish other async tasks is just asking for > trouble. > > Better would be to attach onclose handlers to each of the notifications > retrieved, and then call done when all of those onclose handlers have been > called. Somthing like. > > var closeCount = 0; > var handleAll = function() { > if (++closeCount === notifications.length) { > done(); > } > }; > > notifications.forEach(function(n) { > n.onclose = handleAll; > n.close(); > }); > > ...and then we shouldn't need the setTimeout. Yep, this is what I tried, but for some reason, close event was never triggered. Inspecting this was my next point :)
Removed the use of closeAllPreviousNotifications and instead closing properly them.
Attachment #8406765 - Attachment is obsolete: true
Attachment #8407548 - Attachment is obsolete: true
(In reply to Michael Henretty [:mhenretty] from comment #51) > Also, I applied your gaia patch to a gecko build, and clicking on > notifications did not result in bringing the originating app to the > foreground. I also saw this error in logcat. > > I/GeckoDump( 1147): XXX FIXME : Got a mozContentEvent: > desktop-notification-click > I/Gecko ( 1587): [Child 1587] ###!!! ASSERTION: Uh, inner window set as > event target!: '!win || !win->IsInnerWindow()', file > /Volumes/firefoxos/mozilla-central/dom/events/Event.cpp, line 577 I just did a rebuild and flash of gecko and gaia with patches applied, and I am no longer seeing this issue. \o/
(In reply to Michael Henretty [:mhenretty] from comment #63) > (In reply to Michael Henretty [:mhenretty] from comment #51) > > Also, I applied your gaia patch to a gecko build, and clicking on > > notifications did not result in bringing the originating app to the > > foreground. I also saw this error in logcat. > > > > I/GeckoDump( 1147): XXX FIXME : Got a mozContentEvent: > > desktop-notification-click > > I/Gecko ( 1587): [Child 1587] ###!!! ASSERTION: Uh, inner window set as > > event target!: '!win || !win->IsInnerWindow()', file > > /Volumes/firefoxos/mozilla-central/dom/events/Event.cpp, line 577 > > I just did a rebuild and flash of gecko and gaia with patches applied, and I > am no longer seeing this issue. > \o/ Excellent news :)
Comment on attachment 8407591 [details] [diff] [review] 0001-Bug-967475-Implement-mozChromeNotifications-API.patch Ben, I'm flagging you for review per gregor suggestion, since this is code that will live in dom/ :).
Attachment #8407591 - Flags: review?(bent.mozilla)
Michael, I've added more tests on the gaia integration side, so we now cover: > mozChromeNotifications: > ✓ Checking mozResendAllNotifications API (126ms) > ✓ Sending no notification, resends none (2382ms) > ✓ Sending one notification, resends one (2481ms) > ✓ Sending two notifications, resends two (2334ms) > ✓ Sending two notifications, close one, resends one (2476ms) > ✓ Sending one notification, remove from tray, resend (4978ms) > ✓ desktop-notification-click starts application (10313ms) > ✓ desktop-notification-close removes notification (5315ms) I think we have enough coverage now :)
Flags: needinfo?(mhenretty)
Comment on attachment 8377583 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/16396 Looking good! I left a few nits on Github, and I think the tests need a few changes. But I think we can land the gecko stuff in the meantime.
Attachment #8377583 - Flags: review?(mhenretty)
Flags: needinfo?(mhenretty)
Gasp, I did some more extensive testing after addressing most of your github comments, and I found some issue: > I/Gecko ( 426): -*- NotificationStorage.js: GET: threadId:2 > I/Gecko ( 112): -*- NotificationDB component: Received message:Notification:GetAll > I/Gecko ( 112): -*- NotificationDB component: Queueing task: getall > I/Gecko ( 112): -*- NotificationDB component: Task, getting all > I/Gecko ( 112): -*- NotificationDB component: Finishing task: getall > I/Gecko ( 426): -*- NotificationStorage.js: DELETE: {b15888d0-698c-4fe2-b5fd-c6be845b1469} > I/Gecko ( 112): -*- NotificationDB component: Received message:Notification:Delete > I/Gecko ( 112): -*- NotificationDB component: Queueing task: delete > I/Gecko ( 112): -*- NotificationDB component: Task, deleting > I/GeckoDump( 112): XXX FIXME : Got a mozContentEvent: desktop-notification-close > I/Gecko ( 112): -*- NotificationStorage.js: DELETE: {b15888d0-698c-4fe2-b5fd-c6be845b1469} > I/Gecko ( 112): -*- NotificationDB component: Received message:Notification:Delete > I/Gecko ( 112): -*- NotificationDB component: Queueing task: delete > I/Gecko ( 112): -*- NotificationDB component: Finishing task: delete > I/Gecko ( 112): -*- NotificationDB component: Task, deleting > I/Gecko ( 112): -*- NotificationDB component: No notification found with id: {b15888d0-698c-4fe2-b5fd-c6be845b1469} > I/Gecko ( 426): -*- NotificationStorage.js: GET: threadId:1 > I/Gecko ( 426): -*- NotificationStorage.js: DELETE: {603012d8-42d6-482c-97c4-74175f57ec31} > I/Gecko ( 112): -*- NotificationDB component: Received message:Notification:Delete > I/Gecko ( 112): -*- NotificationDB component: Queueing task: delete > I/GeckoDump( 112): XXX FIXME : Got a mozContentEvent: desktop-notification-close > I/Gecko ( 112): -*- NotificationStorage.js: DELETE: {603012d8-42d6-482c-97c4-74175f57ec31} > I/Gecko ( 112): -*- NotificationDB component: Received message:Notification:Delete > I/Gecko ( 112): -*- NotificationDB component: Queueing task: delete So, STRs were: 0. Send SMS from one mobile phone 1. Send SMS from another mobile phone 2. Open Messages app, go to in thread of message sent in (0) 3. Then to go thread of message sent in (1) What happens is that: - on (2), we issue two requests to delete the notification, as documented in the copy/past above. obviously, the second one fails - when performing (3), the deletion is scheduled, but the task queue has been left in a dangling state previously
Updating: fixing some debug code in NotificationDB and making sure the queue will not get stopped.
Attachment #8407591 - Attachment is obsolete: true
Attachment #8407591 - Flags: review?(bent.mozilla)
Attachment #8408970 - Flags: review?(bent.mozilla)
Added some code to make sure we test that the task queue does not gets blocked when sending Delete.
Attachment #8408970 - Attachment is obsolete: true
Attachment #8408970 - Flags: review?(bent.mozilla)
Comment on attachment 8408977 [details] [diff] [review] 0001-Bug-967475-Implement-mozChromeNotifications-API.patch sorry for losing the review flag :(
Attachment #8408977 - Flags: review?(bent.mozilla)
Zac, I'd like that some people from testing can make sure this will not regress on their side. I'm asking because of bug 997767.
Flags: needinfo?(zcampbell)
What do you mean exactly Alexandre, do you want QA done with this patch or just automation checked? If you do a Try run that is a good start as there are some UI tests in Gu on TBPL.
Flags: needinfo?(zcampbell)
(In reply to Zac C (:zac) from comment #73) > What do you mean exactly Alexandre, do you want QA done with this patch or > just automation checked? > > If you do a Try run that is a good start as there are some UI tests in Gu on > TBPL. Try was green, but this does not cover all cases. If you could take the patch and do a run to check, that would be cool :)
OK, can you post the Try run so we can re-use the Hamachi build from that?
Depends on: 997949
Comment on attachment 8377583 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/16396 Tim, could you review this Gaia part ? I've updated it to add a setting in the Notification menu that allows us to switch off resending in case of issues.
Attachment #8377583 - Flags: review?(timdream)
Rebased patch. Since we added XPCShell tests for NotificationDB, I moved some tests for resending functionnality to this xpcshell test code.
Attachment #8408977 - Attachment is obsolete: true
Attachment #8408977 - Flags: review?(bent.mozilla)
Attachment #8415313 - Flags: review?(bent.mozilla)
Attachment #8415313 - Flags: feedback?(mhenretty)
Comment on attachment 8415313 [details] [diff] [review] 0001-Bug-967475-Implement-mozChromeNotifications-API.patch Asking a review is better.
Attachment #8415313 - Flags: feedback?(mhenretty) → review?(mhenretty)
Comment on attachment 8415313 [details] [diff] [review] 0001-Bug-967475-Implement-mozChromeNotifications-API.patch xpcshell tests look great. thanks for adding those.
Attachment #8415313 - Flags: review?(mhenretty) → review+
Comment on attachment 8415313 [details] [diff] [review] 0001-Bug-967475-Implement-mozChromeNotifications-API.patch Review of attachment 8415313 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me except for the leak, see below. ::: dom/interfaces/notification/nsINotificationStorage.idl @@ +64,5 @@ > in DOMString lang, > in DOMString body, > in DOMString tag, > + in DOMString icon, > + in DOMString alertName); Hrm, is this actually just 'origin + tag' as the comment says? If so then I don't see why we're passing this info twice... Otherwise the comment is wrong :) ::: dom/src/notification/ChromeNotifications.js @@ +13,5 @@ > +const Ci = Components.interfaces; > +const Cu = Components.utils; > + > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); XPCOMUtils.defineLazyModuleGetter maybe? @@ +27,5 @@ > +const CHROMENOTIFICATIONS_CID = "{74f94093-8b37-497e-824f-c3b250a911da}"; > +const CHROMENOTIFICATIONS_CONTRACTID = "@mozilla.org/mozChromeNotifications;1"; > + > +function ChromeNotifications() { > + this.resendCallback = undefined; I think null is the more correct thing here? @@ +28,5 @@ > +const CHROMENOTIFICATIONS_CONTRACTID = "@mozilla.org/mozChromeNotifications;1"; > + > +function ChromeNotifications() { > + this.resendCallback = undefined; > + Services.obs.addObserver(this, "xpcom-shutdown", false); This isn't right, this will hold the object alive until shutdown. You want to implement nsIDOMGlobalPropertyInitializer, grab the inner window id, watch for inner-window-destroyed, and do your unregister there. See DOMRequestHelper.jsm for an example. We need a bug filed to fix settings and find other things like this too... @@ +54,5 @@ > + } > + ); > + resentNotifications++; > + }); > + this.resendCallback && this.resendCallback(resentNotifications); This should probably go in a try/catch block to prevent content exceptions from messing you up. ::: dom/src/notification/Notification.cpp @@ +422,4 @@ > mID(aID), mTitle(aTitle), mBody(aBody), mDir(aDir), mLang(aLang), > mTag(aTag), mIconUrl(aIconUrl), mIsClosed(false) > { > + ComputeAlertName(mAlertName); It doesn't look like this function is actually necessary, you could just do the computation here. Passing the member where you're storing the result is a bit weird too. Also, ComputeAlertName() can currently fail. What should you do if it fails in the constructor like this? ::: dom/src/notification/Notification.h @@ +134,5 @@ > static nsresult GetOrigin(nsPIDOMWindow* aWindow, nsString& aOrigin); > > + nsresult ComputeAlertName(nsString& aAlertName); > + > + void GetAlertName(nsAString& aRetval) Does anyone call this now? I see you use 'mAlertName' in several places... I'd remove it if you can, otherwise make this a 'const' method. ::: dom/src/notification/NotificationDB.jsm @@ +15,4 @@ > > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/osfile.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); XPCOMUtils.defineLazyModuleGetter maybe? @@ +34,4 @@ > const NOTIFICATION_STORE_PATH = > OS.Path.join(NOTIFICATION_STORE_DIR, "notificationstore.json"); > > +let kMessages = [ Not const like above? @@ +37,5 @@ > +let kMessages = [ > + "Notification:Save", > + "Notification:Delete", > + "Notification:GetAll", > + "Notification:GetAllAccrossOrigin" Nit: 'Accross' is a typo. But normally we refer to this as just 'CrossOrigin' rather than 'AcrossOrigin'. There are lots of places with some variation of this below. @@ +69,5 @@ > + observe: function(aSubject, aTopic, aData) { > + if (DEBUG) debug("Topic: " + aTopic); > + if (aTopic == "xpcom-shutdown") { > + Services.obs.removeObserver(this, "xpcom-shutdown"); > + this.unregisterListeners(); Might be worth setting a flag here to make sure that 'init' cannot ever be called again after this. @@ +298,5 @@ > + if (!('alertName' in notification)) { > + continue; > + } > + > + notification.origin = origin; Hrm, is this going to replace the existing property on the notification object? Is that necessary? I would expect it to be the same...
Attachment #8415313 - Flags: review?(bent.mozilla) → review-
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #80) > We need a bug filed to fix settings and find other things like this too... Actually settings looks ok.
Depends on: 1005093
Attached file Github PR (obsolete) —
Created new PR to try and trigger gaia-try run. That was unsuccessful :/
Attachment #8377583 - Attachment is obsolete: true
Attachment #8377583 - Flags: review?(timdream)
Comment on attachment 8377583 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/16396 Whoops, sorry wrong bug.
Attachment #8377583 - Attachment is obsolete: false
Attachment #8377583 - Flags: review?(timdream)
Comment on attachment 8416769 [details] [review] Github PR too many bugs open at once.
Attachment #8416769 - Attachment is obsolete: true
A few replies below, addressing the rest now :) (In reply to ben turner [:bent] (use the needinfo? flag!) from comment #80) [...] > > ::: dom/interfaces/notification/nsINotificationStorage.idl > @@ +64,5 @@ > > in DOMString lang, > > in DOMString body, > > in DOMString tag, > > + in DOMString icon, > > + in DOMString alertName); > > Hrm, is this actually just 'origin + tag' as the comment says? If so then I > don't see why we're passing this info twice... Otherwise the comment is > wrong :) We need this to send it back to content, but Michael and myself were not very happy to have this computed at several places (Notification.cpp, here), hence introducing the alertName in the database. Maybe the comment is not clear: this alertName will either be origin + tag or a notag anchor with the id itself. > ::: dom/src/notification/Notification.cpp > @@ +422,4 @@ > > mID(aID), mTitle(aTitle), mBody(aBody), mDir(aDir), mLang(aLang), > > mTag(aTag), mIconUrl(aIconUrl), mIsClosed(false) > > { > > + ComputeAlertName(mAlertName); > > It doesn't look like this function is actually necessary, you could just do > the computation here. Passing the member where you're storing the result is > a bit weird too. Making it a member variable was to address comment 59. > > Also, ComputeAlertName() can currently fail. What should you do if it fails > in the constructor like this? > > ::: dom/src/notification/Notification.h > @@ +134,5 @@ > > static nsresult GetOrigin(nsPIDOMWindow* aWindow, nsString& aOrigin); > > > > + nsresult ComputeAlertName(nsString& aAlertName); > > + > > + void GetAlertName(nsAString& aRetval) > > Does anyone call this now? I see you use 'mAlertName' in several places... > I'd remove it if you can, otherwise make this a 'const' method. Yes, there's a call in Notification::Constructor, around line 468. > > @@ +298,5 @@ > > + if (!('alertName' in notification)) { > > + continue; > > + } > > + > > + notification.origin = origin; > > Hrm, is this going to replace the existing property on the notification > object? Is that necessary? I would expect it to be the same... No, this is just to set the origin for use as manifestURL in the callback of mozResendAllNotifications.
Address most of the remarks ; only thore regarding the code of Notification.cpp are to be addressed.
Attachment #8415313 - Attachment is obsolete: true
Attachment #8417418 - Flags: feedback?(mhenretty)
Attachment #8417418 - Flags: feedback?(bent.mozilla)
Comment on attachment 8417418 [details] [diff] [review] 0001-Bug-967475-Implement-mozChromeNotifications-API.patch Review of attachment 8417418 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Alexandre LISSY :gerard-majax from comment #85) > A few replies below, addressing the rest now :) > > (In reply to ben turner [:bent] (use the needinfo? flag!) from comment #80) > > [...] > > > ::: dom/src/notification/Notification.cpp > > @@ +422,4 @@ > > > mID(aID), mTitle(aTitle), mBody(aBody), mDir(aDir), mLang(aLang), > > > mTag(aTag), mIconUrl(aIconUrl), mIsClosed(false) > > > { > > > + ComputeAlertName(mAlertName); > > > > It doesn't look like this function is actually necessary, you could just do > > the computation here. Passing the member where you're storing the result is > > a bit weird too. > > Making it a member variable was to address comment 59. Thinking about it, I agree that if we only compute the alert name once we don't need a separate function for it (ie we can put that function inline). We still need it as a member variable since we will need this alert name over the lifetime of the notification, and it doesn't make sense to keep recalculating it. > > > > > Also, ComputeAlertName() can currently fail. What should you do if it fails > > in the constructor like this? > > > > ::: dom/src/notification/Notification.h > > @@ +134,5 @@ > > > static nsresult GetOrigin(nsPIDOMWindow* aWindow, nsString& aOrigin); > > > > > > + nsresult ComputeAlertName(nsString& aAlertName); > > > + > > > + void GetAlertName(nsAString& aRetval) > > > > Does anyone call this now? I see you use 'mAlertName' in several places... > > I'd remove it if you can, otherwise make this a 'const' method. > > Yes, there's a call in Notification::Constructor, around line 468. If it's in the constructor, seems we should just use the mAlertName itself. ---------- Overall, I'm still a fb+ on the approach.
Attachment #8417418 - Flags: feedback?(mhenretty) → feedback+
Depends on: 1006537
(In reply to Michael Henretty [:mhenretty] from comment #87) > Thinking about it, I agree that if we only compute the alert name once we > don't need a separate function for it (ie we can put that function inline). > We still need it as a member variable since we will need this alert name > over the lifetime of the notification, and it doesn't make sense to keep > recalculating it. Yep, this is what I was getting at.
Comment on attachment 8417418 [details] [diff] [review] 0001-Bug-967475-Implement-mozChromeNotifications-API.patch Review of attachment 8417418 [details] [diff] [review]: ----------------------------------------------------------------- Canceling review based on above, will happily stamp the next one :)
Attachment #8417418 - Flags: feedback?(bent.mozilla)
Need some time to read this.
Comment on attachment 8377583 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/16396 qDot, could you review the marionette tests? I am not familiar with that enough.
Attachment #8377583 - Flags: review?(kyle)
Comment on attachment 8377583 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/16396 I am setting f+ because we shouldn't be putting things in bootstrap.js anymore. I also worries we might be overloading notification.js too much though, so I think we should think of a good strategy to dismantle the script and use this bug as a starting point.
Attachment #8377583 - Flags: review?(timdream) → feedback+
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #92) > Comment on attachment 8377583 [details] [review] > Link to Github https://github.com/mozilla-b2g/gaia/pull/16396 > > I am setting f+ because we shouldn't be putting things in bootstrap.js > anymore. I also worries we might be overloading notification.js too much > though, so I think we should think of a good strategy to dismantle the > script and use this bug as a starting point. Where do you want this to be ? In another file, listening for an event (which one) to know when the system is booting but not yet showing UI to user ?
Flags: needinfo?(timdream)
(In reply to Alexandre LISSY :gerard-majax from comment #93) > Where do you want this to be ? In another file, listening for an event > (which one) to know when the system is booting but not yet showing UI to > user ? Just for the purpose of this bug, maybe notification.js again maybe? Let's figure out how to dismantle it in the next bug. I want to make sure you (and others in the SystemFE team) aware of what everyone can do to maintain code quality.
Flags: needinfo?(timdream)
Ben, with this updated patch all your remarks should now be correctly addressed. While testing this with Gaia integration tests, I've noticed that the inner-window-destroyed never gets called on the window that triggered the resend call. I suspect this is expected, but I was wondering if we would not like to remove the observer after having performed the resending, or if it's okay like this. I moved ComputeAlertName's body into the constructor, and added an assert in case of GetOrigin() failure.
Attachment #8417418 - Attachment is obsolete: true
Attachment #8418648 - Flags: review?(bent.mozilla)
Comment on attachment 8418648 [details] [diff] [review] 0001-Bug-967475-Implement-mozChromeNotifications-API.patch Review of attachment 8418648 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these fixed: ::: dom/src/notification/ChromeNotifications.js @@ +36,5 @@ > + > +ChromeNotifications.prototype = { > + > + init: function(aWindow) { > + if (aWindow) { This should always be non-null I believe, so no need to check. @@ +37,5 @@ > +ChromeNotifications.prototype = { > + > + init: function(aWindow) { > + if (aWindow) { > + // We don't use this.innerWindowID, but other classes rely on it. Nit: This comment doesn't make sense, copy-paste cruft I guess? I'd remove it. @@ +99,5 @@ > + let wId = aSubject.QueryInterface(Ci.nsISupportsPRUint64).data; > + if (wId != this.innerWindowID) { > + return; > + } > + this._window = null; More copy-paste junk, you don't have a _window. ::: dom/src/notification/Notification.cpp @@ +423,4 @@ > mID(aID), mTitle(aTitle), mBody(aBody), mDir(aDir), mLang(aLang), > mTag(aTag), mIconUrl(aIconUrl), mIsClosed(false) > { > + nsresult rv = GetOrigin(GetOwner(), mAlertName); This all looks great, but you should use a stack nsAutoString here to do all your appending and such. Then, once your string is fully constructed, assign it to mAlertName. That way you'll avoid several small mallocs.
Attachment #8418648 - Flags: review?(bent.mozilla) → review+
Try: https://tbpl.mozilla.org/?tree=Try&rev=dfa8991e0c54 Everything is green, except one test failing on checking the manifestURL value : this is actually the test that is buggy. I've fixed it and will locally that it's okay.
Updated, should fix the previous orange try on manifestURL test. Carrying r+ from bent and mhenretty. New try pending at https://tbpl.mozilla.org/?tree=Try&rev=ddad5dcad553
Attachment #8418648 - Attachment is obsolete: true
Attachment #8419514 - Flags: review+
This patch should be ready to land as soon as the pending try at https://tbpl.mozilla.org/?tree=Try&rev=ddad5dcad553 is all green.
Keywords: checkin-needed
latest Try is green :)
https://hg.mozilla.org/mozilla-central/rev/21d743a319c6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Ugh, didn't mean to get this merged over without the backout included. Backout merged over in https://hg.mozilla.org/mozilla-central/rev/e2106a5d1230
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla32 → ---
Preferences::* can GC as far as the analysis is concerned, so you have to either do those checks after you're done with the incoming JSObject* or you have to root across those checks. That said, those Preferences::* calls are being added to code that can run in workers, which seems pretty dubious to me.
(In reply to Boris Zbarsky [:bz] from comment #107) > Preferences::* can GC as far as the analysis is concerned, so you have to > either do those checks after you're done with the incoming JSObject* or you > have to root across those checks. > > That said, those Preferences::* calls are being added to code that can run > in workers, which seems pretty dubious to me. I stole those from bug 971766.
Flags: needinfo?(lissyx+mozillians) → needinfo?(bzbarsky)
Which has not them anymore :(. How can I make my code look like a certified app running, then?
One option is to use a Func that checks both the permissions and the pref. That's a bit annoying because it means you can't just use AvailableIn in the IDL. The other option is to do what you did but do the pref check last. So: return principal->GetAppStatus() == nsIPrincipal::APP_STATUS_CERTIFIED || Preferences::GetBool(...); That will make sure that it only runs if NS_IsMainThread, and after we're done with aObj.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #110) > One option is to use a Func that checks both the permissions and the pref. > That's a bit annoying because it means you can't just use AvailableIn in the > IDL. > > The other option is to do what you did but do the pref check last. So: > > return principal->GetAppStatus() == nsIPrincipal::APP_STATUS_CERTIFIED || > Preferences::GetBool(...); > > That will make sure that it only runs if NS_IsMainThread, and after we're > done with aObj. Thanks, I've done the second solution and running it to try now !
Try to check the BindingUtils changes: https://tbpl.mozilla.org/?tree=Try&rev=8ff18651ff90 Try to make sure mochitests still passes: https://tbpl.mozilla.org/?tree=Try&rev=be4323555811
Carrying r+ from bent and mhenretty. Moving the pref checking as suggested.
Attachment #8419514 - Attachment is obsolete: true
Attachment #8419932 - Flags: review+
Hazard Try since the previous one failed: https://tbpl.mozilla.org/?tree=Try&rev=dcc60da9dfb9 Mochitests try is green: https://tbpl.mozilla.org/?tree=Try&rev=be4323555811
Try is green for the hazard: https://tbpl.mozilla.org/?tree=Try&rev=dcc60da9dfb9
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba7c2ae77d55
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment on attachment 8377583 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/16396 Arthur, you're a peer on Settings, mind having a look at the Settings changes ? Thanks!
Attachment #8377583 - Flags: review?(arthur.chen)
Comment on attachment 8377583 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/16396 Tim, I need your approval for the system app part :)
Attachment #8377583 - Flags: review?(timdream)
Comment on attachment 8377583 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/16396 Thanks for the patch. Please add the newly added string to the localization file.
Attachment #8377583 - Flags: review?(arthur.chen)
Comment on attachment 8377583 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/16396 PR updated, fixing the missing notification string.
Attachment #8377583 - Flags: review?(arthur.chen)
Ryan, there is still a Gaia part to land to mark this as FIXED.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8377583 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/16396 r+ notification.js on the condition we would revisit that in the future.
Attachment #8377583 - Flags: review?(timdream) → review+
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #123) > Comment on attachment 8377583 [details] [review] > Link to Github https://github.com/mozilla-b2g/gaia/pull/16396 > > r+ notification.js on the condition we would revisit that in the future. Well, if you are talking about bug 1006990 and bug 1006993 I agree those should be the next targets :)
Attachment #8377583 - Flags: review?(arthur.chen) → review+
I ran a couple of travis builds at https://travis-ci.org/mozilla-b2g/gaia/builds/25072671 to make sure tests were stable. For now, 3 were completed all green.
Since you did the gaia patch in this bug, should you dupe bug 874364 here ?
(In reply to Julien Wajsberg [:julienw] from comment #126) > Since you did the gaia patch in this bug, should you dupe bug 874364 here ? This one has been more used as a tracking for the feature itself.
Travis green after rebasing and running quite a couple of times: https://travis-ci.org/mozilla-b2g/gaia/builds/25135122 Also just tested successfully on my Flame.
https://github.com/mozilla-b2g/gaia/commit/ad2b0cb926d073566fe7f3a5f2361001aa172f90
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Depends on: 1011158
Blocks: 958654
Target Milestone: mozilla32 → 2.0 S2 (23may)
Keywords: dev-doc-needed
Keywords: dev-doc-needed
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: