Closed Bug 1607366 Opened 3 months ago Closed 1 month ago

Show an infobar warning if a user runs Fission without WebRender enabled

Categories

(Core :: DOM: Content Processes, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla75
Fission Milestone M5
Tracking Status
firefox75 --- fixed

People

(Reporter: cpeterson, Assigned: Yoric)

References

Details

Attachments

(2 files)

That would reduce bug reports about non-WebRender Fission issues (like bug 1606316), but still allow people to test Fission without WebRender.

We should show the infobar warning on every launch. That will annoy users, but we don't want them to forget they are testing an unsupported configuration.

Are any tests running Fission without WebRender?

Assignee: nobody → dteller

So, we can detect whether the window is running WebRender using nsIGfxInfo and nsIDocShell's or tabbrowser's useRemoteSubframes.

I'll try and implement this as an add-on, if I can find how to access xpcom from a system add-on.

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #2)

I'll try and implement this as an add-on, if I can find how to access xpcom from a system add-on.

I think other people have said this, but please do not use system add-ons to implement this infobar - we're trying to get rid of them as much as possible as they have performance as well as other issues, and aren't normally necessary unless we're updating them regularly, away from the usual release cycle, which I don't think will be the case here.

@Gijs: Noted.

@cpeterson: Is this something we'll want localized? Also, should this be Nightly-only?

Flags: needinfo?(cpeterson)

Attaching a first prototype. It eyeball-works but I haven't managed to CI test it. For some reason, aBrowser.getNotificationWithValue doesn't ever return a notification, maybe I'm running the code from the wrong process.

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #4)

@cpeterson: Is this something we'll want localized? Also, should this be Nightly-only?

This info bar can be Nightly only. Fission cannot be enabled in Beta or Release channels, at this time.

Localizing the message is not a high priority. This will be a temporary, Nightly-only warning for people who have manually enabled the fission.autostart pref. Those people likely learned about the pref from reading English comments in Bugzilla or email.

Flags: needinfo?(cpeterson)
Priority: P2 → P1
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f52616a34d3
Display a warning when we're running Fission without WebRender;r=Gijs

Florian, is there a way to somehow whitelist this warning for browser_startup_flicker.js and/or to remove browser_startup_flicker.js when we're running with Fission and without WebRender (which is an unsupported combo)?

Flags: needinfo?(dteller) → needinfo?(florian)
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f03b006038b5
Backed out changeset 3f52616a34d3 for causing bc failures in browser_startup_flicker.js CLOSED TREE

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #10)

Florian, is there a way to somehow whitelist this warning for browser_startup_flicker.js and/or to remove browser_startup_flicker.js when we're running with Fission and without WebRender (which is an unsupported combo)?

If this is an unsupported combo, should we disable the whole test job?

But otherwise, yes, you can add an early return at the beginning of the browser_startup_flicker.js test if fission is enabled and webrender is not.

Flags: needinfo?(florian)

(In reply to Florian Quèze [:florian] from comment #12)

If this is an unsupported combo, should we disable the whole test job?

I don't really know why we run it. Maybe ask Nika?

Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7bdea74790de
Deactivate browser_startup_flicker when we have Fission without WebRender;r=florian
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Attachment #9124734 - Attachment is obsolete: true

David, you marked the "real" patch of adding the notification bar obsolete, but AFAICT that patch is not in current central/autoland - shouldn't it be relanded now that the startup_flicker thing has landed?

Flags: needinfo?(dteller)
Attachment #9124734 - Attachment is obsolete: false

Oops, thanks!

Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/22f74b61de35
Display a warning when we're running Fission without WebRender;r=Gijs

Looks like I need to remove a few more tests when we're running with fission and without webrender.

Flags: needinfo?(dteller)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

If this notification bar is only there for users, perhaps an easier fix would be to just avoid showing the notification in automation? You can test for this using Cu.isInAutomation.

Flags: needinfo?(dteller)

(In reply to :Gijs (he/him) from comment #22)

If this notification bar is only there for users, perhaps an easier fix would be to just avoid showing the notification in automation? You can test for this using Cu.isInAutomation.

I guess I could get rid of my tests and do that. Since you're my reviewer, what's the best option in your eye?

Flags: needinfo?(dteller) → needinfo?(gijskruitbosch+bugs)

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #23)

(In reply to :Gijs (he/him) from comment #22)

If this notification bar is only there for users, perhaps an easier fix would be to just avoid showing the notification in automation? You can test for this using Cu.isInAutomation.

I guess I could get rid of my tests and do that. Since you're my reviewer, what's the best option in your eye?

Factor out the actual creation of the notification box into a function, only call it when Cu.isInAutomation is false, and then in your test, manually call that function? That way we can have our cake and eat it.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dteller)

I fear that that starts sounding a bit complicated to test what is essentially a one-liner, no?

Where would you like this function/method published? As a member of gBrowser?

Flags: needinfo?(dteller) → needinfo?(gijskruitbosch+bugs)

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #25)

I fear that that starts sounding a bit complicated to test what is essentially a one-liner, no?

I'd also be OK with removing the test, if you prefer.

Where would you like this function/method published? As a member of gBrowser?

It'd be on FissionTestingUI.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dteller)
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/10a8c9467b67
Display a warning when we're running Fission without WebRender;r=Gijs
Status: REOPENED → RESOLVED
Closed: 1 month ago1 month ago
Resolution: --- → FIXED
Flags: needinfo?(dteller)

This is a bit annoying on configurations where WebRender is enabled by default.

(In reply to Laurențiu Nicola from comment #29)

This is a bit annoying on configurations where WebRender is enabled by default.

Wait, we have configurations where WebRender is enabled and this shows up by error? If so, could you file a bug and Cc me?

Flags: needinfo?(grayshade)

i can't file a new bug right now, but I've seen it on Linux. I think the WR pref is also disabled on qualified Windows systems.

Flags: needinfo?(grayshade)
Regressions: 1620613
You need to log in before you can comment on or make changes to this bug.