737359 - Add the Allow button for a non-whitelisted site to API

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Remus Pop (:RemusPop)]

This is the button that appears if we try to install an addon from a website that is not whitelisted.

We will need this for the AMO tests.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Keep in mind that this is a doorhanger and should use the appropriate API from the toolbars module.

Comment 2

[Remus Pop (:RemusPop)]

Attachment: patch v1 (all branches)

Defined ADDON_BLOCKED_NOTIFICATION and added the allow button as a case in getElement method of locationBar in toolbars.js.

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 607894 [details] [diff] [review]
patch v1 (all branches)


>+const ADDON_BLOCKED_NOTIFICATION = NOTIFICATION_POPUP +
>+                                   '/id("addon-install-blocked-notification")';

The indentation above does not match with the correct one below
Use below even if its a bit longer than the standard, its consistent with the API indentation 

const ADDON_BLOCKED_NOTIFICATION = NOTIFICATION_POPUP + '/id("addon-install-blocked-notification")'; 

> const URLBAR_CONTAINER = '/id("main-window")/id("tab-view-deck")/[0]' +
>                          '/id("navigator-toolbox")/id("nav-bar")/id("urlbar-container")';
> const URLBAR_INPUTBOX = URLBAR_CONTAINER + '/id("urlbar")' +
>                                            '/anon({"anonid":"textbox-container"})' + 
>                                            '/anon({"anonid":"textbox-input-box"})';
> const CONTEXT_MENU = URLBAR_INPUTBOX + '/anon({"anonid":"input-box-contextmenu"})';
> 
> /**
>@@ -494,16 +496,23 @@ locationBar.prototype = {
>   getElement : function locationBar_getElement(spec) {
>     var elem = null;
> 
>     switch(spec.type) {
>       /**
>        * subtype: subtype to match
>        * value: value to match
>        */
>+      case "allowButton":
>+        elem = new elementslib.Lookup(this._controller.window.document,
>+                                      ADDON_BLOCKED_NOTIFICATION +
>+                                      '/anon([1])' +
>+                                      '/{"class":"popup-notification-button-container",' +
>+                                      '"pack":"end","align":"center"}/anon({"anonid":"button"})');

Same here 

>+        break;
>       case "contextMenu":
>         elem = new elementslib.Lookup(this._controller.window.document, CONTEXT_MENU);
>         break;
>       case "contextMenu_entry":
>         elem = new elementslib.Lookup(this._controller.window.document, CONTEXT_MENU +
>                                       '/{"cmd":"cmd_' + spec.subtype + '"}');
>         break;
>       case "favicon":

With those changes you have my + and you can continue the review process by passing along to Henrik

Comment 4

[Remus Pop (:RemusPop)]

Attachment: patch v2 (all branches)

Addressed all requests.

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 607950 [details] [diff] [review]
patch v2 (all branches)

Looks good to me. Over to Henrik

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 607950 [details] [diff] [review]
patch v2 (all branches)

> const NOTIFICATION_POPUP = '/id("main-window")/id("mainPopupSet")/id("notification-popup")';
>+const ADDON_BLOCKED_NOTIFICATION = NOTIFICATION_POPUP + '/id("addon-install-blocked-notification")';

We do not want to have this as a constant here. Please remove it.

>+      case "allowButton":
>+        elem = new elementslib.Lookup(this._controller.window.document, ADDON_BLOCKED_NOTIFICATION +
>+                                      '/anon([1])/{"class":"popup-notification-button-container",' +
>+                                      '"pack":"end","align":"center"}/anon({"anonid":"button"})');
>+        break;

There is no need for a new entry in the elements definition. For now we do not want to support special elements in this class. Instead make use of the 'getNotificationElement' method on the locationBar class. If you want to wrap that, add a new function to the add-ons module.

Beside this, I would propose that we always whitelist the target site for our tests instead of handling this notification. A single test which handles this notification should be enough, but it shouldn't be one of the tests you are creating here for the AMO site. It can be a functional restart test. So I wouldn't see the necessity to create an API method at all.

Comment 7

[Remus Pop (:RemusPop)]

Henrik, we have several tests for AMO that would benefit from this change.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Which tests? As said, it should be enough to have a single test which really works with the notification bar.

Comment 9

[Remus Pop (:RemusPop)]

The tests are in work. I have 2 of them and Vlad has 1. There are more which the WebQA needs them, but I think it's ok to have one that uses this button.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Vlad, can you please check if the patch still applies and test with one of the tests we have?

Comment 11

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Vlad, can you please check if the patch still applies and test with one of
> the tests we have?

Yes well comment 8 makes lots of sense to me. 
We have been whitelisting the target site so far, and working with the notification in just one test seems really decent because all the tests are in the same manner, at least ones which are installing stuff from AMO. 

IMHO this bug is WONTFIX. We don't want an API method if we use this only once, we can just use getNotificationElement with a lookup string to get the 'Allow' button

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Alright. So all are in agreement here. Lets close it as Wontfix.