Closed Bug 1582042 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(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 https://hg.mozilla.org/mozilla-central/rev/b0c38584a789 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
anyway.

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:

https://searchfox.org/mozilla-central/rev/f43ae7e1c43a4a940b658381157a6ea6c5a185c1/dom/ipc/BrowserChild.cpp#2523

  • 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
    visible.

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
https://hg.mozilla.org/mozilla-central/rev/27f6f789b194, but it seems the
refactoring in https://hg.mozilla.org/mozilla-central/rev/4df5fa6fa785 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 ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08088ff7a8be
Use DocShell::SetIsActive from BrowserChild::MakeVisible for non top-level browsers. r=mconley,rhunt
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f2441f2f26e
Remove useless threading of aForceRepaint around various IPC messages. r=mconley
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9f12d9b4c6b5
Remove nsIRemoteTab.forceRepaint(). r=mconley
Status: NEW → RESOLVED
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.