Closed Bug 1582042 Opened 2 years ago Closed 2 years ago

BrowserChild should use DocShell::SetIsActive rather than PresShell::SetIsActive


(Core :: DOM: Core & HTML, defect, P2)




Tracking Status
firefox71 --- fixed


(Reporter: emilio, Assigned: emilio)




(3 files)

The later is not supposed to be called from anywhere else that isn't presShell / docshell.

I noticed the same thing in bug 1577084. Looks like splitting the docshell and presshell notion of Active happened in from bug 1397426. And then that has morphed into what we have today I guess?

Priority: -- → P2

I have a patch that on top of bug 1578379 is ~green. Causes some autoplay failures probably related to bug 1572798...

Assignee: nobody → emilio

The autoplay failures were because we were calling this from tab warming, which turns our document "visible", and thus we unblock media playback. Was a fun one to dig.

That is, for fission iframes. For top level stuff we rely on the tab switcher
going through SetDocShellIsActive and such.

See the expanded comment as for why. Ideally we could simplify this further by
not making RecvRenderLayers update the visibility (which spins the refresh
driver and such).

It's a bit suspect because it's very easy to get to an inconsistent state if the
browser chrome does something wrong.

I'll try to do that, but for now this should improve the fission situation

aForceRepaint wasn't doing what it claimed to do at all, as we've recently
learned. In current mozilla-central:

  • All those arguments ended up in a RecvRenderLayers with aForceRepaint = true
    (so far so good, that's expected).

  • But it was ignored (so that aForceRepaint is always true to calls to
    MakeVisible) from UpdateVisibility:

  • Plus that argument only does anything useful on current central if we get to
    the end of MakeVisible(true). And MakeVisible() early returns if already

So all in all this seems somewhat useless, and nobody has complained about such
a thing in a long time.

It seemed to do what it promised when it was introduced in, but it seems the
refactoring in broke it.

I think the new setup is somewhat easier to reason about, and nobody seems to be
missing that.

I'll try to remove the forceRepaint() call itself on a follow-up.

It's useless if the tab is already visible (i.e., has renderLayers=true), per
the previous patches, and that's the only point at which it gets called.

Pushed by
Use DocShell::SetIsActive from BrowserChild::MakeVisible for non top-level browsers. r=mconley,rhunt
Pushed by
Remove useless threading of aForceRepaint around various IPC messages. r=mconley
Pushed by
Remove nsIRemoteTab.forceRepaint(). r=mconley
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.