1111663 - [JavaScript Error: "TypeError: button.onClicked is not a function" {file: "resource://gre/modules/Notifications.jsm" line: 236}]

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

I saw this error in the console. It looks like this line should use `onButtonClicked`:
http://hg.mozilla.org/mozilla-central/annotate/a25d603f8385/mobile/android/modules/Notifications.jsm#l236

fedepaol or wesj, is there a test for this, or a good way to test this?

Comment 1

[Federico Paolinelli]

Isn't it fixed by this
https://hg.mozilla.org/mozilla-central/rev/1ab5f84fa165#l3.13 ?

The problem was that the button object passed to the notification api does not have a onClicked callback anymore since click events are handled by those handled objects.

The way to test would be just to click on a button of the download notification (ie pause or resume).

Comment 2

[:Margaret Leibovic]

(In reply to Federico Paolinelli from comment #1)
> Isn't it fixed by this
> https://hg.mozilla.org/mozilla-central/rev/1ab5f84fa165#l3.13 ?

Not necessarily, if onClicked is defined, but not a function. I actually saw this while testing with that patch applied.

> The problem was that the button object passed to the notification api does
> not have a onClicked callback anymore since click events are handled by
> those handled objects.

Could you point me to the code that does this? I'm confused about how this ends up with an onClicked property in the first place, since I don't see us setting that anywhere in the tree.

Comment 3

[:Margaret Leibovic]

Ah, I did some digging, and it looks like this is a regression from bug 1004495.

https://hg.mozilla.org/mozilla-central/rev/6ee3c3ba17b5

Comment 4

[Wesley Johnston (:wesj)]

We probably just need a check for this method (in order to maintain backwards compat in this API. We should put some error logging in here to notify people of the deprecated API as well (and then we can deprecate it).

Comment 5

[Petr Marek]

Hi, can I take this as my first bug fix?

Comment 6

[:Margaret Leibovic]

(In reply to Petr Marek from comment #5)
> Hi, can I take this as my first bug fix?

Sure, go for it! If you don't have a build set up yet, you should follow instructions here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

Next, you should try to reproduce this error locally to understand what's going on

After that, you should write a patch with the fix suggested above, and attach it to this bug for review:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 7

[Petr Marek]

(In reply to :Margaret Leibovic from comment #6)
> (In reply to Petr Marek from comment #5)
> > Hi, can I take this as my first bug fix?
> 
> Sure, go for it! If you don't have a build set up yet, you should follow
> instructions here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Build_Instructions/Simple_Firefox_for_Android_build
> 
> Next, you should try to reproduce this error locally to understand what's
> going on
> 
> After that, you should write a patch with the fix suggested above, and
> attach it to this bug for review:
> https://developer.mozilla.org/en-US/docs/Mercurial/
> Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F

Ok I'm in. I'm setting it all up, I'll let know my progress. thx

Comment 8

[Petr Marek]

I've followed (frontend build) https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ but I have troubles to build it. Complete output of ./mach build, .mozconfig and .hgrc is here: http://pastebin.com/eTyj2r8q. 

Can you please navigate where I should start to resolve this issue (mozilla wiki, known issues...)?

Comment 9

[:Margaret Leibovic]

(In reply to Petr Marek from comment #8)
> I've followed (frontend build)
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ but I have
> troubles to build it. Complete output of ./mach build, .mozconfig and .hgrc
> is here: http://pastebin.com/eTyj2r8q. 
> 
> Can you please navigate where I should start to resolve this issue (mozilla
> wiki, known issues...)?

These are the docs I normally encourage people to follow:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

It's not immediately obvious to me what your build problem is. You should join #mobile on irc.mozilla.org for realtime help debugging your build issue.

Comment 10

[Petr Marek]

My issue with build was because of the exFAT drive I cloned the repo on. Main indicator that something wasn't right was repo's size 56Gi. There was a problem with symlink creation and build silently crashed. Now on OS X Extended drive it looks ok (4.5GiB), run Ok.

Comment 11

[Petr Marek]

Attachment: patch for Bug 1111663

I added suggested tests whether is it a function for both properties (button.onClicked, button.onButtonClicked). Adding log message was also suggested, but I only found this way to log, not sure that this is suitable for deprecation warning (or other type?): "Services.console.logStringMessage(msg)". Is there a convention for log messages format?
Thx for your review.

Comment 12

[:Margaret Leibovic]

Comment on attachment 8729984 [details] [diff] [review]
patch for Bug 1111663

You should request review from a reviewer, not set review+ yourself. In the case, I'm an appropriate reviewer.

Comment 13

[Petr Marek]

Yes, sorry, I did not fully understood that flag. So, will you give me a feedback?

Comment 14

[:Margaret Leibovic]

Comment on attachment 8729984 [details] [diff] [review]
patch for Bug 1111663

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

::: mobile/android/modules/Notifications.jsm
@@ +240,5 @@
> +          if(button.onClicked instanceof Function){ 
> +            button.onClicked(id, notification._cookie);
> +          }
> +          if(button.onButtonClicked instanceof Function) {
> +            button.onButtonClicked(id, notification._cookie);

It's been a while since I've looked at this code... can you remind me where this `onButtonClicked` method comes from? Is that something that add-on authors declare? I don't see this defined anywhere in the tree.

Comment 15

[Petr Marek]

Attachment: Screen Shot 2016-03-15 at 23.18.25.png

(In reply to :Margaret Leibovic from comment #14)
> Comment on attachment 8729984 [details] [diff] [review]
> patch for Bug 1111663
> 
> Review of attachment 8729984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/Notifications.jsm
> @@ +240,5 @@
> > +          if(button.onClicked instanceof Function){ 
> > +            button.onClicked(id, notification._cookie);
> > +          }
> > +          if(button.onButtonClicked instanceof Function) {
> > +            button.onButtonClicked(id, notification._cookie);
> 
> It's been a while since I've looked at this code... can you remind me where
> this `onButtonClicked` method comes from? Is that something that add-on
> authors declare? I don't see this defined anywhere in the tree.

Actually you mentioned it in the first comment of this bug so I supposed that should be a check for this method. As I looked at the code, it does not seems that this `onButtonClicked` method is used. 
Please see the attachment. In the screenshot there is an output of the 'Find in path..' for this method. I think that the relevant code here is only my check for method existence and line 'onButtonClicked: ignoreEvent(context, "notifications.onButtonClicked")' which does not make much sense to me as I don't know the code base. Maybe the method is actually ignored or left in the code base for compatibility reasons as mentioned Federico Paolinelli?

Comment 16

[:Margaret Leibovic]

(In reply to Petr Marek from comment #15)
> Created attachment 8730949 [details]
> Screen Shot 2016-03-15 at 23.18.25.png
> 
> (In reply to :Margaret Leibovic from comment #14)
> > Comment on attachment 8729984 [details] [diff] [review]
> > patch for Bug 1111663
> > 
> > Review of attachment 8729984 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/modules/Notifications.jsm
> > @@ +240,5 @@
> > > +          if(button.onClicked instanceof Function){ 
> > > +            button.onClicked(id, notification._cookie);
> > > +          }
> > > +          if(button.onButtonClicked instanceof Function) {
> > > +            button.onButtonClicked(id, notification._cookie);
> > 
> > It's been a while since I've looked at this code... can you remind me where
> > this `onButtonClicked` method comes from? Is that something that add-on
> > authors declare? I don't see this defined anywhere in the tree.
> 
> Actually you mentioned it in the first comment of this bug so I supposed
> that should be a check for this method. As I looked at the code, it does not
> seems that this `onButtonClicked` method is used. 
> Please see the attachment. In the screenshot there is an output of the 'Find
> in path..' for this method. I think that the relevant code here is only my
> check for method existence and line 'onButtonClicked: ignoreEvent(context,
> "notifications.onButtonClicked")' which does not make much sense to me as I
> don't know the code base. Maybe the method is actually ignored or left in
> the code base for compatibility reasons as mentioned Federico Paolinelli?

Looking at the code, it looks like I probably made a typo and meant to say `onButtonClick`.

I understand that you're not familiar with the codebase, but it's been a long time since I've looked at this myself. Part of the job of fixing a bug is diving in to take a look around and justify the change you made :)

Looking at the documentation, I see that `onClick` and `onButtonClick` are actually two different supported notification handler methods:
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/Notifications.jsm

And you can see how we use them in DownloadNotification:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/DownloadNotifications.jsm#148

Spending even more time thinking about this code and the documentation, I'm not even sure why we have this button.onClick call at all here... seems like it should be removed.

Looking in our add-ons MXR, I don't even see any add-ons using Notifications.jsm, so we don't need to worry about add-on compatibility, but should think about how this is being used in our app itself.

In conclusion, you should just remove this button click code from the bottom of this case statement, and you should help update the documentation on MDN to make it clearer how this module is actually used.

Comment 17

[:Margaret Leibovic]

Comment on attachment 8729984 [details] [diff] [review]
patch for Bug 1111663

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

::: mobile/android/modules/Notifications.jsm
@@ +231,4 @@
>            });
>          }
>  
>          if (notification && !notification._buttons) {

You can remove this and everything below it in this case statement.

Comment 18

[Petr Marek]

(In reply to :Margaret Leibovic from comment #17)
> Comment on attachment 8729984 [details] [diff] [review]
> patch for Bug 1111663
> 
> Review of attachment 8729984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/Notifications.jsm
> @@ +231,4 @@
> >            });
> >          }
> >  
> >          if (notification && !notification._buttons) {
> 
> You can remove this and everything below it in this case statement.

I looked at the code and documentation closely and for pre 33 there are 'aOptions.onCancel' and 'aOptions.onClick' methods. If I understand correctly these could be defined. In documentation I don't see precise format proposed; that id should 'onClick(id, cookie)' and 'onCancel(id, cookie)' as it is used like that (at least 'onCancel' in case "notification-closed"). 

So I proposed new patch considering this. But if I should rather remove that my last modification and update documentation that 'onCancel' and 'onClick' from options are not used anymore I will do it. But case "notification-closed" should be also modified imo.

Comment 19

[Petr Marek]

Attachment: bug_1111663_v2.patch

Comment 20

[:Margaret Leibovic]

(In reply to Petr Marek from comment #18)
> (In reply to :Margaret Leibovic from comment #17)
> > Comment on attachment 8729984 [details] [diff] [review]
> > patch for Bug 1111663
> > 
> > Review of attachment 8729984 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/modules/Notifications.jsm
> > @@ +231,4 @@
> > >            });
> > >          }
> > >  
> > >          if (notification && !notification._buttons) {
> > 
> > You can remove this and everything below it in this case statement.
> 
> I looked at the code and documentation closely and for pre 33 there are
> 'aOptions.onCancel' and 'aOptions.onClick' methods. If I understand
> correctly these could be defined. In documentation I don't see precise
> format proposed; that id should 'onClick(id, cookie)' and 'onCancel(id,
> cookie)' as it is used like that (at least 'onCancel' in case
> "notification-closed"). 
> 
> So I proposed new patch considering this. But if I should rather remove that
> my last modification and update documentation that 'onCancel' and 'onClick'
> from options are not used anymore I will do it. But case
> "notification-closed" should be also modified imo.

As the documentation says, this is for old versions of Firefox (before 33), so this is deprecated. Let's remove it.

Comment 21

[Petr Marek]

Attachment: onClick removed

Comment 22

[:Margaret Leibovic]

Comment on attachment 8731827 [details] [diff] [review]
onClick removed

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

Thanks for the patch! It looks like this builds on top of your previous patches, instead of making a new patch. Could you fold your patches together locally to generate a single commit to check in?

Also, in the future, you should set the review? flag on a patch, instead of the needinfo? flag on the bug, when you want someone to review a patch.

Thanks for your patience!

Comment 23

[Petr Marek]

Attachment: bug_1111663_v4.1.patch

Not sure how should I correctly combine my consequent (read bunch of mozilla docs), created with cmd: hg diff -r bug_1111663.patch -r bug_1111663_v2.patch -r bug_1111663_v3.patch > bug_1111663_v4.1.patch

Comment 24

[Petr Marek]

Attachment: bug_1111663_v4.2.patch

Not sure how should I correctly combine my consequent (read bunch of mozilla docs), created with cmd: hg export --output bug_1111663_v4.2.patch-r bug_1111663.patch -r bug_1111663_v2.patch -r bug_1111663_v3.patch

Comment 25

[:Margaret Leibovic]

The goal is to make a single diff file that shows the difference between the mozilla-repo tree you pulled, and the changes you want to make. As you can see from the files you attached, that's not the case for those files.

I'm not sure what your tree looks like, so it's hard to give exact steps for you to create this patch. Did you use hg commit to make commits? Or are you using mercurial queues? If the former, you can use the histedit extension to fold commits together. If the latter, you should look up the qfold command.

Comment 26

[:Margaret Leibovic]

Comment on attachment 8732162 [details] [diff] [review]
bug_1111663_v4.1.patch

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

::: mobile/android/modules/Notifications.jsm
@@ -242,5 @@
> -          }
> -          if(button.onButtonClicked instanceof Function) {
> -            button.onButtonClicked(id, notification._cookie);
> -          }
> -        }

Apologies, I only looked at the second commit, this one actually looks like it's doing the right thing.

Comment 27

[:Margaret Leibovic]

You should generate a patch with an appropriate commit message, then attach that to the patch to get checked in. Here are details about how to do that:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

You should also mark old versions of patches as obsolete when you upload new ones.

Comment 28

[Petr Marek]

Attachment: bug_1111663_v5.patch

I could not get it right so I cloned the repo again and made my changes. Then I used this cmd to make this patch: 'hg qnew bug_1111663.patch'

I hope this patch is finally correct. This is really long run :)
Thanks for your patience. 

Please, let me know how should I proceed with update of the documentation.

Comment 29

[:Margaret Leibovic]

Comment on attachment 8732338 [details] [diff] [review]
bug_1111663_v5.patch

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

Thanks, this is great.

Comment 30

[:Margaret Leibovic]

(In reply to Petr Marek from comment #28)

> Please, let me know how should I proceed with update of the documentation.

MDN is a wiki, so you can make an account and propose changes to this page:
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/Notifications.jsm

I would replace references to "NotificationHandler" with something like "handler object", since there isn't actually a NotificationHandler type defined anywhere. I would also explain what properties are expected or supported in that object.

Additionally, I think we can get rid of references to things that were deprecated in Firefox 33, that was a very long time ago, and it's not at all supported anymore.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/93955ff309b7

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93955ff309b7

Comment 33

[Petr Marek]

(In reply to :Margaret Leibovic from comment #30)
> (In reply to Petr Marek from comment #28)
> 
> > Please, let me know how should I proceed with update of the documentation.
> 
> MDN is a wiki, so you can make an account and propose changes to this page:
> https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/
> Notifications.jsm
> 
> I would replace references to "NotificationHandler" with something like
> "handler object", since there isn't actually a NotificationHandler type
> defined anywhere. I would also explain what properties are expected or
> supported in that object.
> 
> Additionally, I think we can get rid of references to things that were
> deprecated in Firefox 33, that was a very long time ago, and it's not at all
> supported anymore.

Thanks, because of the ongoing DOS attack, registration of new accounts is disabled so I suppose I should wait and check it time to time...

Comment 34

[:Margaret Leibovic]

Lonnen, who manages MDN? It seems bad that a new contributor can't make an account to update documentation (see above).

Comment 35

[Lonnen :lonnen]

MDN is under a sustained and sophisticated spam attack that has forced us to temporarily disable new registrations. All hands on the project have been diverted to mitigating this. Until we have it resolved, you can join #mdn and someone can create an account for you.