1285196 - WindowsPreviewPerTab should not do anything when the feature is disabled

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[:Gijs (he/him)]

There's evidence this causes pretty significant perf impact (bug 1241537, bug 1000946) and it really shouldn't need to do anything unless the feature is turned on.

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1285196 - make WindowsPreviewPerTab.jsm do nothing if disabled,

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

Comment 3

[:Gijs (he/him)]

In case this is helpful for the review: the reason I'm using _prefenabled rather than 'enabled' to keep track of this is because the code already uses 'enabled' to turn the feature half-off when the maximum number of previews is reached, and I didn't want to interfere with that code (so I've tried not to, but it would be good if you checked I actually managed :-) ).

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8768859 [details]
Bug 1285196 - make WindowsPreviewPerTab.jsm do nothing if disabled,

https://reviewboard.mozilla.org/r/62876/#review60656

Doesn't look like it would be too hard/scary to merge the 2 enable paths. But I understand why you did it, and since I don't know how expensive is to destroy/regenerate previews everytime we cross the limit, I'll just ignore my gut and accept we'll have 2 code paths doing similar stuff :)

::: browser/modules/WindowsPreviewPerTab.jsm:630
(Diff revision 1)
>          // will have finished fetching 'in order'.
>          if (index != -1) {
>            let tab = this.tabbrowser.tabs[index];
> -          if (tab.getAttribute("image") == aIconURL) {
> -            this.previews.get(tab).icon = img;
> +          let preview = this.previews.get(tab);
> +          if (tab.getAttribute("image") == aIconURL ||
> +              (!preview.icon && !requestURL)) {

to make this condition more readable, I'd add somewhere a:
let isDefaultFavicon = !requestURL;
Otherwise it's quite unclear why here one should check requestURL.

::: browser/modules/WindowsPreviewPerTab.jsm:742
(Diff revision 1)
> +    // (rather than this code running on startup because the pref was
> +    // already set to true), we must initialize previews for open windows:
> +    if (this.initialized) {
> +      let browserWindows = Services.wm.getEnumerator("navigator:browser");
> +      while (browserWindows.hasMoreElements()) {
> +        let win = browserWindows.getNext();

wm.getEnumerator may also return closed windows (bug 528706), thus I'd add an if (!win.closed)

::: browser/modules/WindowsPreviewPerTab.jsm:772
(Diff revision 1)
> +      return;
> +    }
>      if (this.previews.length > this.maxpreviews)
>        this.enabled = false;
>      else
>        this.enabled = this._prefenabled;

so now this can only be true, or we bailout earlier, that means you can rewrite it as
this.enabled = this.previews.length <= this.maxpreviews;

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/b6edea2fa690
make WindowsPreviewPerTab.jsm do nothing if disabled, r=mak

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/016e4fdb5a52
followup: fix leaking of taskbartabgroup object on windows when disabling and fix test issues that assume previews persist when the pref disables us, rs=bustage

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6edea2fa690
https://hg.mozilla.org/mozilla-central/rev/016e4fdb5a52

Comment 8

[:Gijs (he/him)]

Joel, I'm kind of curious if this led to talos wins. But the [talos] links on hg.m.o for the revisions seem to all be quite... confusing... e.g. https://hg.mozilla.org/integration/fx-team/rev/016e4fdb5a52 links to this: https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=016e4fdb5a52374d139e3553fc564a7ae0f9ea74&newProject=fx-team&newRevision=016e4fdb5a52374d139e3553fc564a7ae0f9ea74&framework=1

which doesn't make a whole lot of sense. Known issue?

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

that link is comparing the same revision to itself, also there is a single data point for the revision, that will not be useful to determine the real effect here.  

I kicked off more jobs to collect additional data points- you will be able to see it all here in a few hours:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=b6edea2fa690&newProject=fx-team&newRevision=016e4fdb5a52&framework=1&showOnlyImportant=0

^ assuming the buildbot db issues do not affect launching new jobs.

Comment 10

[:Gijs (he/him)]

(In reply to Joel Maher ( :jmaher ) from comment #9)
> that link is comparing the same revision to itself

Yes, I'm aware of that - it was kind of my point: that's the link hg.m.o gives me on https://hg.mozilla.org/integration/fx-team/rev/016e4fdb5a52 . Is there a bug on file to fix the links it provides?

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

I think that is a random bug.  Compare view link doesn't default to the previous revision, you would need to provide the base branch/revision.

oh wait- when I click on your link there are links in the hg view for talos/perfherder-  i had no idea.  Let me file a bug for that.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

filed in bug 1287096