1360687 - Update puppeteer notification class for webextension

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement)

Description

[Andrew Swan [:aswan]]

Puppeteer has some code for handling the notifications that appear during addon installation, but it is not up to date with recent changes for things like webextension permissions and it is tested with legacy extensions that will soon stop working on release.
None of this code is actually used in tree and we have browser chrome tests that cover install flows in toolkit/mozapps/extensions/test/xpinstall and browser/base/content/test/webextensions
Rather than trying to keep the (unused?) puppeteer code up to date, lets just remove it.

Comment 1

[Andrew Swan [:aswan]]

Attachment: Bug 1360687 Remove puppeteer addon install notifications code

Review commit: https://reviewboard.mozilla.org/r/134904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134904/

Comment 2

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

Puppeteer is a library which can be used by external code. There is also foxpuppet [1] which has a similar implementation, and will be used for testing extensions. I'm cc'ing Dave given that he is more familiar with webtesting and when those notifications will be used. Also he wrote the code you are trying to remove now.

[1] https://github.com/mozilla/FoxPuppet

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I believe we'll be using FoxPuppet in preference to Puppeteer for any web testing that needs to interact with the browser UI. That said, I believe these tests did recently detect a regression for one version of the patch for bug 1335778, so we should at least ensure we have this covered by a Marionette unit test.

Comment 4

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

I already wrote a unit test for that. So it's covered on mozilla-central already.

Andrew, maybe you can give us an overview what would have to be changed to make the notification class compatible? Which other notifications do we get? 

Please also keep in mind that the same extension which is used here, also is in use by Marionette test resources. So the extension itself would need to be updated anyway.

Comment 5

[Andrew Swan [:aswan]]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> I already wrote a unit test for that. So it's covered on mozilla-central
> already.
> 
> Andrew, maybe you can give us an overview what would have to be changed to
> make the notification class compatible? Which other notifications do we get? 

For webextensions (which will be the only supported extensions on release beginning in 57) the confirmation notification is replaced by one that shows the permissions the extension requires.  And then there is another post-install notification.  The notifications are defined here:
http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/browser/base/content/popup-notifications.inc#75-88

> Please also keep in mind that the same extension which is used here, also is
> in use by Marionette test resources. So the extension itself would need to
> be updated anyway.

Can you point me to where it is used?  Maybe you're thinking of the xpi in testing/marionette/harness/marionette_harness/tests/unit ?

I'm still unclear on what this code is used for.  Nothing in mozilla-central uses it and Dave says in comment 3 that using FoxPuppet is preferred to using this code anyway.

Comment 6

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

I think I will have a quick look at existent webextensions and which kind of notifications we get. As Andrew told me examples can be found here:

https://github.com/mdn/webextensions-examples
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/webextensions

Especially the latter URL has a couple of webextensions which request different permissions during installation.

Comment 7

[Andrew Swan [:aswan]]

Henrik, would you prefer that we change this bug into "support webextensions permissions notifications in puppeteer"?  I'm still confused about why we're maintaining this code since doesn't appear to actually be used by any tests (other than its own unit tests).
This is going to become a blocker for work on 57 relatively soon...

Comment 8

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

I hope to get to this bug soon. In the meantime I just want to reference a PR from FoxPuppet which adds support for the new notifications. So an update for Puppeteer should be also simple.

https://github.com/mozilla/FoxPuppet/pull/86

Comment 9

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

Lets wait on the Marionette unit test with the new webextension as worked on by Andrew on bug 1373772. Then it will be easy to update Puppeteer too.

Comment 10

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

Comment on attachment 8863064 [details]
Bug 1360687 Remove puppeteer addon install notifications code

https://reviewboard.mozilla.org/r/134904/#review158104

I would like to see this adapted to be used with webextensions. I will have a look at this.

Comment 11

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

I have a patch which replaces the currently restartless extension with a webextension, and at the same time only adds basic support for the new kind of notification. Once we have tests which require it to be installed, we have to add this code.

Comment 12

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

Attachment: Bug 1360687 - Update puppeteer notification class for webextensions.

To support webextensions in Puppeteer the current restartless
extension gets replaced by a webextension. It allow us to test
at least the permission notification. This patch only adds basic
support.

Review commit: https://reviewboard.mozilla.org/r/153016/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153016/

Comment 13

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

Attachment: Bug 1360687 - Synchronize webextension from Puppeteer with Marionette harness.

The current webextension in Marionette harness tests doesn't have an
addon id and as such will not be able to be installed outside of
about:debugging. This patch synchronizes the updated webextension
from Puppeteer with Marionette harness.

Review commit: https://reviewboard.mozilla.org/r/153018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153018/

Comment 14

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

Comment on attachment 8881942 [details]
Bug 1360687 - Update puppeteer notification class for webextensions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153016/diff/1-2/

Comment 15

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

Comment on attachment 8881943 [details]
Bug 1360687 - Synchronize webextension from Puppeteer with Marionette harness.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153018/diff/1-2/

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8881942 [details]
Bug 1360687 - Update puppeteer notification class for webextensions.

https://reviewboard.mozilla.org/r/153016/#review158408

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/browser/notifications.py:119
(Diff revision 2)
>      """Add-on progress notification."""
>  
>      pass
> +
> +
> +class AddOnWebextPermissionsNotification(BaseNotification):

For FoxPuppet we kept the user facing API the same, and do not expect the user to distinguish between the install prompt for legacy and web extension add-ons. As there will ultimately only be web extensions, I don't feel that it's necessary to provide a new class for this notification. See https://github.com/mozilla/FoxPuppet/commit/cdb3ff89ef166dcb727a7ea6c78110dbeabc1f99

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8881943 [details]
Bug 1360687 - Synchronize webextension from Puppeteer with Marionette harness.

https://reviewboard.mozilla.org/r/153018/#review158412

Comment 18

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

Comment on attachment 8881942 [details]
Bug 1360687 - Update puppeteer notification class for webextensions.

https://reviewboard.mozilla.org/r/153016/#review158408

> For FoxPuppet we kept the user facing API the same, and do not expect the user to distinguish between the install prompt for legacy and web extension add-ons. As there will ultimately only be web extensions, I don't feel that it's necessary to provide a new class for this notification. See https://github.com/mozilla/FoxPuppet/commit/cdb3ff89ef166dcb727a7ea6c78110dbeabc1f99

Ok, that make sense. I will do the same but in the same step will also remove the current property for `addon_name` and the `install` and `cancel` methods. Those are not used anywhere and I do not have the time right now to find out how to retrieve them in Puppeteer. The code as used in Foxpuppet is different, and as that I wasn't able to get it working in the last hour.

Comment 19

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

Comment on attachment 8881942 [details]
Bug 1360687 - Update puppeteer notification class for webextensions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153016/diff/2-3/

Comment 20

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

Comment on attachment 8881943 [details]
Bug 1360687 - Synchronize webextension from Puppeteer with Marionette harness.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153018/diff/2-3/

Comment 21

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8881942 [details]
Bug 1360687 - Update puppeteer notification class for webextensions.

https://reviewboard.mozilla.org/r/153016/#review159112

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02f8be2ae22b
Update puppeteer notification class for webextensions. r=davehunt
https://hg.mozilla.org/integration/autoland/rev/ace24caed618
Synchronize webextension from Puppeteer with Marionette harness. r=davehunt

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/02f8be2ae22b
https://hg.mozilla.org/mozilla-central/rev/ace24caed618