908978 - new Notification API events onclick and onclose aren't being handled

Status: RESOLVED DUPLICATE of bug 910915

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Caio Lima(:caiolima)]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

Using the UITest case "new Notification()", showing a notification and after clicking on it, neither the onclick and onclose aren't handled.

The same happens if we click the button to close the notification.



Expected results:

These methods should be executed.

Comment 1

[Caio Lima(:caiolima)]

Could you confirm this bug please?

Comment 2

[Julien Wajsberg [:julienw]]

I saw that already too, I said that in bug 901856 comment 1.

Thanks for filing the bug :)

Comment 3

[Caio Lima(:caiolima)]

I identified the problem, but I'm a little lost to solve it.

When I use the "nsIAppsService->ShowAppNotification(...)" the observer code is executed normally, but the close operation doesn't work because it's based on "nsIAlertService->ShowAlertNotification"

I guess the correct way to fix it is to make the oberver be called, but I don't have a lot of knowledge how it works.

Could anyone give me a direction?

Comment 4

[Caio Lima(:caiolima)]

Attachment: bug-908978-fix.patch

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 795197 [details] [diff] [review]
bug-908978-fix.patch

Review of attachment 795197 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know this code very well.

Gregor, do you feel like you could review this ? Otherwise William Chen or waiting until Vivien is back ?

::: b2g/chrome/content/shell.js
@@ +722,5 @@
>            Services.io.newURI(listener.target, null, null),
>            Services.io.newURI(listener.manifestURL, null, null)
>          );
>        }
> +    } else if (uid.startsWith("alert") || detail.type.startsWith("desktop-notification")) {

you need to check if detail.type is a String first, this comes from content.

Comment 6

[Caio Lima(:caiolima)]

(In reply to Julien Wajsberg [:julienw] from comment #5)
> you need to check if detail.type is a String first, this comes from content.

Do you think it's really necessary? There is a case where the type is not a string?

Comment 7

[Julien Wajsberg [:julienw]]

I think this can be null/undefined...

Comment 8

[Caio Lima(:caiolima)]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> I think this can be null/undefined...

I understand, you are right.

Comment 9

[Caio Lima(:caiolima)]

Attachment: bug-908978-fix2.patch

Added a verification if type is null

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Note that this may be fixed by the new logic in Bug 910915. Instead of checking detail type, I moved the alert check first, then do a global fall-through otherwise, because as far as I can tell, if it's not an alert, all we care about is either shipping to the message manager or making a system message regardless of detail info.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Checked on UI test, bug 910915 does fix this. Will wait until reviews are in before taking status actions though.

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, this did end up being a dupe of bug 910915. It'll be fixed when that lands (waiting for tree to unburn).