1268619 - Add a mozscreenshots configuration for site permission notifications

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[Matthew N. [:MattN]]

Since we'll be touching site notification popups and anchors, it would help with ui-review and catching regressions if we add a configuration to mozscreenshots to capture images of the UI.

This just requires adding a file to https://mxr.mozilla.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/ and writing a function for each image which returns a promise when the UI is ready to be captured.

See https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots for more documentation.

Comment 1

[Johann Hofmann [:johannh]]

Attachment: MozReview Request: Bug 1268619 - Add a mozscreenshots configuration for site permission notifications

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

Comment 2

[Johann Hofmann [:johannh]]

Comment on attachment 8747098 [details]
MozReview Request: Bug 1268619 - Add a mozscreenshots configuration for site permission notifications

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

Comment 3

[Johann Hofmann [:johannh]]

I hope this covers what you had in mind :)

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8747098 [details]
MozReview Request: Bug 1268619 - Add a mozscreenshots configuration for site permission notifications

https://reviewboard.mozilla.org/r/49735/#review46945

(In reply to Johann Hofmann [:johannh] from comment #3)
> I hope this covers what you had in mind :)

Unfortunately it doesn't as I think this is too specific and probably only useful for bug 1147981. I was thinking of a page like the various test pages that we have around which actually goes through the real code to trigger the prompts. That way we are actually capturing the proper buttons, messages and actually used icons. It's probably useful to have a test page like that in the tree anyways.

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8747098 [details]
MozReview Request: Bug 1268619 - Add a mozscreenshots configuration for site permission notifications

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

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8747098 [details]
MozReview Request: Bug 1268619 - Add a mozscreenshots configuration for site permission notifications

This is a WIP that covers what I consider the easiest notifications. I also did a try push for that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04460b3b5ce5c55ccfdb5d1a84cc7b22a6558744

One problem I had with earlier trys is that Windows doesn't display the webcam notification. No idea if that's because of our machines or not, but I'll investigate. Another thing that doesn't work is pointerLock. It works fine if you manually click on it, but not with automation.  That might be because it only works in a user-initiated action. Is there any way to turn that security policy off?

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8747098 [details]
MozReview Request: Bug 1268619 - Add a mozscreenshots configuration for site permission notifications

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

Comment 8

[Johann Hofmann [:johannh]]

So my previous push was VERY WIP, as in it was completely dysfunctional. Could you take a look at this one? :)

Produces these screenshots: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=25d777f7efb3&newProject=try&newRev=2725488cd319275db5d8ab65ce04ee7a2402e35b

I'm looking into the Windows camera issue which currently mostly means setting up a Windows 7 VM and Visual Studio. :/

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8747098 [details]
MozReview Request: Bug 1268619 - Add a mozscreenshots configuration for site permission notifications

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49735/diff/4-5/

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8747098 [details]
MozReview Request: Bug 1268619 - Add a mozscreenshots configuration for site permission notifications

Updated so that pointerLock actually works now.

Comment 11

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/49735/#review49191

Good job figuring this out. We mostly just need to fix the pointer lock prompt.

::: browser/tools/mozscreenshots/browser.ini:6
(Diff revision 5)
>  [DEFAULT]
>  subsuite = screenshots
>  support-files =
>    head.js
> +  mozscreenshots/extension/lib/notifications.html
> +  mozscreenshots/extension/lib/borderify.xpi

Can we just use one of the existing extensions in the tree that have an icon? Then we can have the support-files reference it?

https://mxr.mozilla.org/mozilla-central/find?string=.xpi&tree=mozilla-central

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PopupNotifications.jsm:13
(Diff revision 5)
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/Timer.jsm");

Nit: remove unused imports when you're done.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PopupNotifications.jsm:18
(Diff revision 5)
> +// TODO not tested yet:
> +//"default",
> +//"indexedDB",
> +//"login-fill",
> +//"plugins",
> +//"servicesInstall",
> +//"translate",
> +//"translated",
> +//"eme",

Don't worry about these for this initial landing. We can always add others later and if we add them at the end then it won't interfere with existing comparisons. I also would ignore the translation and login fill ones since we don't ship them.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PopupNotifications.jsm:54
(Diff revision 5)
> +        // XXX how to use SpecialPowers here?
> +        Services.prefs.setCharPref("media.getusermedia.screensharing.allowed_domains", "example.com");

Instead of using SpecialPowers (which I don't know how to use here), you can just set this in the init function above and also register a cleanup function there too.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PopupNotifications.jsm:73
(Diff revision 5)
> +          E10SUtils.wrapHandlingUserInput(content, true, function() {
> +            canvas.click();
> +          });

This didn't seem to work on my 10.10 machine as the prompt wasn't shown. To simplify the code, I think we should just wrapHandlingUserInput for every click in `clickOn`. If we do that then every applyConfig will just be a clickOn call so we can build the confgigurations dynamically in `init` and shorten this file up.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PopupNotifications.jsm:92
(Diff revision 5)
> +    addons: {
> +      applyConfig: Task.async(function*() {
> +        yield clickOn("#addons");

This shows when the site doesn't have the xpiinstall permission but I think we may show a different one when you do. Ideally we would test both if they are different.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PopupNotifications.jsm:102
(Diff revision 5)
> +  let tab = yield BrowserTestUtils.openNewForegroundTab(browserWindow.gBrowser, URL);
> +  return BrowserTestUtils.synthesizeMouseAtCenter(selector, {}, tab.linkedBrowser);

Nit: I think you should use BrowserTestUtils.withNewTab so the tab gets closed after

::: browser/tools/mozscreenshots/mozscreenshots/extension/lib/notifications.html:3
(Diff revision 5)
> +<head>
> +  <title>Notifications Test</title>

Include the `<meta charset="utf-8">` before the <title> to avoid a console warning.

::: browser/tools/mozscreenshots/mozscreenshots/extension/lib/notifications.html:6
(Diff revision 5)
> +<body>
> +  <div>

Nit: why the <div>?

::: browser/tools/mozscreenshots/mozscreenshots/extension/lib/notifications.html:9
(Diff revision 5)
> +    <button id="webRTC-shareDevices" onclick="shareDevice({video: true});" type="button">Video</button>
> +    <button id="webRTC-shareMicrophone" onclick="shareDevice({audio: true});" type="button">Audio</button>

Do we need the @type=button outside of a <form>?

::: browser/tools/mozscreenshots/mozscreenshots/extension/lib/notifications.html:18
(Diff revision 5)
> +    <canvas width="100" height="100" id="pointerLock"></canvas>
> +    <button id="web-notifications" type="button">web-notifications</button>
> +    <form>
> +      <input type="email" id="email" value="[e-mail]" />
> +      <input type="password" id="password" value="123456" />
> +      <input type="submit" id="login-fill" />

Why not <button> since you're using that elsewhere?

This doesn't show the login-fill doorhanger, it's the capture one so rename this and the configuration "login-capture" and "loginCapture" respectively.

::: browser/tools/mozscreenshots/mozscreenshots/extension/lib/notifications.html:23
(Diff revision 5)
> +  <script type="application/javascript">
> +    document.getElementById("geo").addEventListener("click", function(){
> +      navigator.geolocation.getCurrentPosition(() => {});
> +    });
> +    document.getElementById("web-notifications").addEventListener("click", function(){
> +      Notification.requestPermission();

Why do some buttons use addEventListener while others use @onclick? I would be fine with them all using @onclick.

::: browser/tools/mozscreenshots/mozscreenshots/extension/lib/notifications.html:31
(Diff revision 5)
> +    });
> +    document.getElementById("web-notifications").addEventListener("click", function(){
> +      Notification.requestPermission();
> +    });
> +    let canvas = document.getElementById("pointerLock");
> +    canvas.addEventListener("click", function(e){

Nit: space before `{`

Comment 12

[Johann Hofmann [:johannh]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #11)
> https://reviewboard.mozilla.org/r/49735/#review49191
> 
> Good job figuring this out. We mostly just need to fix the pointer lock
> prompt.
> 
> ::: browser/tools/mozscreenshots/browser.ini:6
> (Diff revision 5)
> >  [DEFAULT]
> >  subsuite = screenshots
> >  support-files =
> >    head.js
> > +  mozscreenshots/extension/lib/notifications.html
> > +  mozscreenshots/extension/lib/borderify.xpi
> 
> Can we just use one of the existing extensions in the tree that have an
> icon? Then we can have the support-files reference it?
> 

Mmh the borderify file is really small and using some other extension would make the test prone to failures if that file was renamed/removed, no?

> 
> :::
> browser/tools/mozscreenshots/mozscreenshots/extension/configurations/
> PopupNotifications.jsm:73
> (Diff revision 5)
> > +          E10SUtils.wrapHandlingUserInput(content, true, function() {
> > +            canvas.click();
> > +          });
> 
> This didn't seem to work on my 10.10 machine as the prompt wasn't shown. To
> simplify the code, I think we should just wrapHandlingUserInput for every
> click in `clickOn`. If we do that then every applyConfig will just be a
> clickOn call so we can build the confgigurations dynamically in `init` and
> shorten this file up.
> 

Huh

> :::
> browser/tools/mozscreenshots/mozscreenshots/extension/lib/notifications.html:
> 9
> (Diff revision 5)
> > +    <button id="webRTC-shareDevices" onclick="shareDevice({video: true});" type="button">Video</button>
> > +    <button id="webRTC-shareMicrophone" onclick="shareDevice({audio: true});" type="button">Audio</button>
> 
> Do we need the @type=button outside of a <form>?
> 

Well it's as good as any element and makes it kinda semantically clear that this is supposed to be clicked on.

> :::
> browser/tools/mozscreenshots/mozscreenshots/extension/lib/notifications.html:
> 23
> (Diff revision 5)
> > +  <script type="application/javascript">
> > +    document.getElementById("geo").addEventListener("click", function(){
> > +      navigator.geolocation.getCurrentPosition(() => {});
> > +    });
> > +    document.getElementById("web-notifications").addEventListener("click", function(){
> > +      Notification.requestPermission();
> 
> Why do some buttons use addEventListener while others use @onclick? I would
> be fine with them all using @onclick.
> 

Because WIP :D

I was actually thinking to make all use addEventListener but I guess onclick works.

Comment 13

[Matthew N. [:MattN]]

(In reply to Johann Hofmann [:johannh] from comment #12)
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #11)
> > https://reviewboard.mozilla.org/r/49735/#review49191
> > 
> > Good job figuring this out. We mostly just need to fix the pointer lock
> > prompt.
> > 
> > ::: browser/tools/mozscreenshots/browser.ini:6
> > (Diff revision 5)
> > >  [DEFAULT]
> > >  subsuite = screenshots
> > >  support-files =
> > >    head.js
> > > +  mozscreenshots/extension/lib/notifications.html
> > > +  mozscreenshots/extension/lib/borderify.xpi
> > 
> > Can we just use one of the existing extensions in the tree that have an
> > icon? Then we can have the support-files reference it?
> > 
> 
> Mmh the borderify file is really small and using some other extension would
> make the test prone to failures if that file was renamed/removed, no?

If you reference the other XPI from support-files then the build will fail if it's removed.

> > :::
> > browser/tools/mozscreenshots/mozscreenshots/extension/lib/notifications.html:
> > 9
> > (Diff revision 5)
> > > +    <button id="webRTC-shareDevices" onclick="shareDevice({video: true});" type="button">Video</button>
> > > +    <button id="webRTC-shareMicrophone" onclick="shareDevice({audio: true});" type="button">Audio</button>
> > 
> > Do we need the @type=button outside of a <form>?
> > 
> 
> Well it's as good as any element and makes it kinda semantically clear that
> this is supposed to be clicked on.

I was talking about the `type=button` attribute, not that fact that it's <button> which I agree makes sense.

Comment 14

[Johann Hofmann [:johannh]]

Comment on attachment 8747098 [details]
MozReview Request: Bug 1268619 - Add a mozscreenshots configuration for site permission notifications

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49735/diff/5-6/

Comment 15

[Johann Hofmann [:johannh]]

This change introduces a "cleanup" function in mozscreenshots that can be used to clean up after a test.

It's just an idea, we can change how it works. Without the general concept I wouldn't know how to close tabs or restore prefs after a config has run. BrowserTestUtils.withNewTab won't work because that will close the tab as soon as it returns the Promise and mozscreenshot is taking the screenshot. So we need something to run after the screenshot has been taken to clean up.

As for the prefs, putting them all in init won't work since we test some configurations with opposing preferences.

I think a generic cleanup function will also be useful in the future as we might want even more complex interaction in follow-up bugs.

I also found a nice pref that can simulate the existence of media devices and added that, hopefully fixing non-appearing prompts on Windows.

As we talked about in person, it might make sense to defer the pointerLock troubles to a follow up bug (which doesn't mean we're not gonna do it, just that this bug has enough useful functionality on its own).

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #13)
> > 
> > Mmh the borderify file is really small and using some other extension would
> > make the test prone to failures if that file was renamed/removed, no?
> 
> If you reference the other XPI from support-files then the build will fail
> if it's removed.
> 

Unless there is some sort of general purpose test addon for use by all sorts of tests, that doesn't sound like great OS project comradeship to me. Whoever decides that addon isn't useful anymore would have to fix up our test as well. :)

> I was talking about the `type=button` attribute, not that fact that it's
> <button> which I agree makes sense.

Oh, sorry, I misread. You're right, it's not needed.

Comment 16

[Johann Hofmann [:johannh]]

Screenshots at https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=5511d54a3f17&newProject=try&newRev=afbae638ea16

As I feared, the add-ons notifications take a bit too long too load, I'll need to listen to the right dialogue to show up. Will do that tomorrow and update the patch.

Comment 17

[Johann Hofmann [:johannh]]

Comment on attachment 8747098 [details]
MozReview Request: Bug 1268619 - Add a mozscreenshots configuration for site permission notifications

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49735/diff/6-7/

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8747098 [details]
MozReview Request: Bug 1268619 - Add a mozscreenshots configuration for site permission notifications

https://reviewboard.mozilla.org/r/49735/#review52104

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PopupNotifications.jsm:108
(Diff revisions 6 - 7)
> +        yield BrowserTestUtils.waitForCondition(
> +          () => !notification.hasAttribute("hidden"), "addon popup did not show", 200);

Nit: we normally try to avoid wrapping after a `(` so get the first argument on the same line and wrap after the comma.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:206
(Diff revision 7)
> -    yield this._onConfigurationReady(combo);
> +    yield this._onConfigurationReady(combo)
> +      // For all configurations with a cleanup function, run the cleanup.
> +      .then(() => Promise.all(combo.map(({cleanup}) => cleanup && cleanup())));

IIUC, this is incorrect for when multiple sets are specified. The cleanup would need to run only when switching to another configuration in the same JSM.
e.g. consider two JSMs with configurations [A, B] and [1, 2].

I think this is what we would want:
Capture A1
Cleanup 1
Capture A2
Cleanup 2
Cleanup A
Capture B1
Cleanup 1
Capture B2
…

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PopupNotifications.jsm:7
(Diff revision 7)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["PopupNotifications"];

Rename this to PermissionPrompts.jsm as it's not really testing features of PopupNotifications in general, only permission-related things.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PopupNotifications.jsm:94
(Diff revision 7)
> +      applyConfig: Task.async(function*() {
> +        Services.prefs.setBoolPref("extensions.install.requireBuiltInCerts", false);
> +        yield clickOn("#addons", URL);
> +      }),
> +      cleanup: Task.async(function*() {
> +        Services.prefs.clearUserPref("extensions.install.requireBuiltInCerts");

Note that I'm really not worried about cleaning up the prefs as each dir. is run independently so leaving prefs around won't break other jobs. This is another reason why I'm fine not adding cleanup yet since that may be more complicated.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PopupNotifications.jsm:102
(Diff revision 7)
> +    },
> +
> +    addonsNoWhitelist: {
> +      applyConfig: Task.async(function*() {
> +        let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +        let notification = browserWindow.document.getElementById("addon-progress-notification");

Sorry, I just realized this isn't a good candidate for screenshots as the progress bar and progress nubers will cause intermittents as-is. Let's just remove this for now.

We'll want to compare two try pushes before landing to see if there are other intermittents.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PopupNotifications.jsm:120
(Diff revision 7)
> +function* closeLastTab(selector) {
> +  let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +  yield BrowserTestUtils.removeTab(browserWindow.gBrowser.selectedTab);
> +}

Don't assume that the selected tab is the last one we opened and then you can just call this at the beginning of each applyConfig instead of solving the cleanup method if you want. i.e. store the reference to the last tab and if it's truthy, close it before opening the new tab.

::: browser/tools/mozscreenshots/mozscreenshots/extension/lib/notifications.html:19
(Diff revision 7)
> +    <button id="web-notifications" onclick="toggleNotification()">web-notifications</button>
> +    <a id="addons" href="borderify.xpi">Install Add-On</a>
> +    <form>
> +      <input type="email" id="email" value="[e-mail]" />
> +      <input type="password" id="password" value="123456" />
> +      <button type="submit" id="login-capture" />

Nit: This empty tiny button bothers me:
<button type="submit" id="login-capture">Login</button>

Comment 19

[Johann Hofmann [:johannh]]

Comment on attachment 8747098 [details]
MozReview Request: Bug 1268619 - Add a mozscreenshots configuration for site permission notifications

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49735/diff/7-8/

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 8747098 [details]
MozReview Request: Bug 1268619 - Add a mozscreenshots configuration for site permission notifications

https://reviewboard.mozilla.org/r/49735/#review52294

I'll address these two nits and push today. Thanks for your patience learning this stuff.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PermissionPrompts.jsm:105
(Diff revision 8)
> +function* closeLastTab(selector) {
> +  if (lastTab) {
> +    let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");

Nit: Early return style is preferred to reduce nesting:
```
if (!lastTab) {
  return;
}
```

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PermissionPrompts.jsm:116
(Diff revision 8)
> +  let tab = yield BrowserTestUtils.openNewForegroundTab(browserWindow.gBrowser, URL);
> +
> +  // save the tab so we can close it later
> +  lastTab = tab;
> +
> +  yield ContentTask.spawn(tab.linkedBrowser, selector, function* (arg) {

Nit: The `tab` local variable isn't needed. Just use lastTab.

Comment 21

[Pulsebot]

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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/32a5b0f3bf99