If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Update puppeteer notification class for webextension

RESOLVED FIXED in Firefox 56

Status

Testing
Marionette
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: aswan, Assigned: whimboo)

Tracking

Version 3
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 months ago
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.
(Reporter)

Updated

6 months ago
Blocks: 1352204
Comment hidden (mozreview-request)
(Reporter)

Updated

6 months ago
Assignee: nobody → aswan
(Reporter)

Updated

6 months ago
Attachment #8863064 - Flags: review?(hskupin)
(Assignee)

Comment 2

6 months ago
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
Flags: needinfo?(dave.hunt)

Comment 3

6 months ago
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.
Flags: needinfo?(dave.hunt)
(Assignee)

Comment 4

6 months ago
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.
Flags: needinfo?(aswan)
(Reporter)

Comment 5

6 months ago
(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.
Flags: needinfo?(aswan)
(Assignee)

Comment 6

5 months ago
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.
Flags: needinfo?(hskupin)
(Reporter)

Comment 7

5 months ago
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...
(Reporter)

Updated

5 months ago
Blocks: 1369517
(Assignee)

Comment 8

4 months ago
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
Flags: needinfo?(hskupin)
(Assignee)

Updated

4 months ago
Flags: needinfo?(hskupin)
(Assignee)

Comment 9

4 months ago
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.
Depends on: 1373772
Flags: needinfo?(hskupin)
(Reporter)

Updated

4 months ago
Assignee: aswan → nobody
(Assignee)

Updated

4 months ago
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
(Assignee)

Comment 10

4 months ago
mozreview-review
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.
Attachment #8863064 - Flags: review?(hskupin) → review-
(Assignee)

Comment 11

4 months ago
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.
Summary: Remove puppeteer code for addon install notifications → Update puppeteer notification class for webextension
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8863064 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8881942 - Flags: review?(dave.hunt)
Attachment #8881943 - Flags: review?(dave.hunt)

Comment 16

4 months ago
mozreview-review
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
Attachment #8881942 - Flags: review?(dave.hunt) → review-

Comment 17

4 months ago
mozreview-review
Comment on attachment 8881943 [details]
Bug 1360687 - Synchronize webextension from Puppeteer with Marionette harness.

https://reviewboard.mozilla.org/r/153018/#review158412
Attachment #8881943 - Flags: review?(dave.hunt) → review+
(Assignee)

Comment 18

4 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

4 months ago
mozreview-review
Comment on attachment 8881942 [details]
Bug 1360687 - Update puppeteer notification class for webextensions.

https://reviewboard.mozilla.org/r/153016/#review159112
Attachment #8881942 - Flags: review?(dave.hunt) → review+

Comment 22

4 months ago
Pushed by hskupin@mozilla.com:
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/02f8be2ae22b
https://hg.mozilla.org/mozilla-central/rev/ace24caed618
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.