Closed Bug 577607 Opened 9 years ago Closed 9 years ago

Make subframes use their parent frame's refresh driver

Categories

(Core :: Layout, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla2.0b4

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(4 files, 3 obsolete files)

Subframes (and resource documents) in content should just use the refresh driver of the root content document (which is the same thing as the root document in e10s-land).
We need this so that the external resource docs won't keep holding on to the main doc refresh driver after the main doc presshell goes away.
Attachment #456530 - Flags: review?(roc)
Attachment #456538 - Flags: review?(dbaron)
Comment on attachment 456538 [details] [diff] [review]
Part 2 with a tweak to Thaw/Freeze

No, this is wrong.
Attachment #456538 - Attachment is obsolete: true
Attachment #456538 - Flags: review?(dbaron)
Attachment #456531 - Attachment is obsolete: true
Attachment #456531 - Flags: review?(dbaron)
Attached patch Part 2 for real (obsolete) — Splinter Review
Attachment #456542 - Flags: review?(dbaron)
Comment on attachment 456542 [details] [diff] [review]
Part 2 for real

This is still failing some tests.  Need to sort that out.
Attachment #456542 - Flags: review?(dbaron) → review-
And the reason it's wrong is that the refresh driver only flushes the prescontext it's initialized with.
Priority: -- → P1
Summary: Make subframes use their parent frame's refresh driver → [FIX]Make subframes use their parent frame's refresh driver
Summary: [FIX]Make subframes use their parent frame's refresh driver → Make subframes use their parent frame's refresh driver
Whiteboard: [need review]
The patches in bug 569520 depend on this bug at the moment.  It could be detangled, with some pain...
Blocks: 569520
Attachment #456666 - Flags: review?(dbaron) → review?(roc)
Attachment #456667 - Flags: review?(dbaron) → review?(roc)
Comment on attachment 456666 [details] [diff] [review]
Part 2.  Make it possible for refresh driver to flush more than one presshell

+  PRPackedBool      mReflowScheduled; // If true, we have a reflow scheduled.
+                                      // Guaranteed to be false if
+                                      // mReflowContinueTimer is non-null.
+
+  PRPackedBool      mSuppressInterruptibleReflows;

Fix indent
Attachment #456666 - Flags: review?(roc) → review+
Comment on attachment 456667 [details] [diff] [review]
Part 3.  Make subframes use the parent's refresh driver

+        ourItem->GetSameTypeRootTreeItem(getter_AddRefs(root));
+        if (root != ourItem) {
+          mRefreshDriver = parent->GetShell()->GetPresContext()->RefreshDriver();

Couldn't you just use GetSameTypeParent here and check for null?

I think we really need an HasSameTypeParent method on nsIDocument for situations like this...
Attachment #456667 - Flags: review?(roc) → review+
> Fix indent

Done.

> Couldn't you just use GetSameTypeParent here and check for null?

Yes.  Done.

> I think we really need an HasSameTypeParent method on nsIDocument for
> situations like this...

Or perhaps IsRootOfType or something... Bug 586178 filed.
Whiteboard: [need review] → [need approval]
Risk:  Moderate.  I'm pretty sure things should work fine, and I've done a bunch of testing on sites with subframes, and our tests pass, but there's a bunch of rejiggering of how we trigger reflow and restyling in here.  Should be high-quality rejiggering, of course!

Benefit: lays the groundwork for some performance-related improvements we want to make.  Should somewhat improve performance on pages with subframes, and should definitely reduce the number of wakeups on such pages (which is good for mobile).
Attachment #464706 - Flags: approval2.0?
Comment on attachment 464706 [details] [diff] [review]
Roll-up patch for approval.  DO NOT CHECK IN

Approved with a short leash.
Attachment #464706 - Flags: approval2.0? → approval2.0+
Whiteboard: [need approval] → [need landing]
Depends on: 564652
Part 1 got pushed as http://hg.mozilla.org/mozilla-central/rev/d07f35b7a38f

The others got pushed as well, but then backed out for now.
Part 2: http://hg.mozilla.org/mozilla-central/rev/7d133a2a7d20
Part 3: http://hg.mozilla.org/mozilla-central/rev/4e326cd7bac6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [need landing]
Flags: in-testsuite-
Target Milestone: --- → mozilla2.0b4
Depends on: 587064
Depends on: 587494
Depends on: 590883
You need to log in before you can comment on or make changes to this bug.