1225563 - Log error in JS that Doorhangers only support one positive and one negative button if invalid buttons are provided

Status: RESOLVED FIXED

Description

[azrdev]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151103081951

Steps to reproduce:

In an addon, call NativeWindow.doorhanger.show("label", "const id", [button1, button2], ...); where the buttons are objects with `label` and `callback` members, but *not* a `positive:true`.
As per <https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/NativeWindow/doorhanger#show%28%29>, only one of the Buttons is shown.


Actual results:

There is no indication why the button(s) have been dropped from display.


Expected results:

A log message would be useful.

Comment 1

[Chenxia Liu [:liuche] - PTO + conference til 10/12]

We do log an error, but it goes to Logcat in Android. Since this mainly affects Addon authors, this notification should be in JS.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/DoorHangerPopup.java#131

Comment 2

[Chenxia Liu [:liuche] - PTO + conference til 10/12]

Updating these links:

In the Java code, we log an error if there are more than two buttons.
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/DoorHangerPopup.java#133

We should do this same check in JS when sending the message to create doorhanger buttons, and log an error if there are more than two buttons.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2300

Comment 3

[dlim]

Created attachment 8719689 [details] [diff] [review]
Log error in JS that Doorhangers only support one positive and one negative button if invalid buttons are provided

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8719689 [details] [diff] [review]
Log error in JS that Doorhangers only support one positive and one negative button if invalid buttons are provided

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

One nit, but otherwise looks good!  If you'll post an updated patch, we'll mark it checkin-needed.  (That's a Bugzilla keyword; I'm not sure you have the rights to set it just yet.)

::: mobile/android/chrome/content/browser.js
@@ +2288,5 @@
>        if (aButtons == null) {
>          aButtons = [];
>        }
>  
> +      if(aButtons.length > 2) {

nit: space between if and (.

Comment 5

[dlim]

Created attachment 8720973 [details] [diff] [review]
Log error in JS that Doorhangers only support one positive and one negative button if invalid buttons are provided

Comment 6

[dlim]

Created attachment 8722662 [details] [diff] [review]
Log error in JS that Doorhangers only support one positive and one negative button if invalid buttons are provided

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8722662 [details] [diff] [review]
Log error in JS that Doorhangers only support one positive and one negative button if invalid buttons are provided

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

Looks good.

Comment 8

[Pulsebot]

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e38a16b7d37e