Closed Bug 534226 Opened 10 years ago Closed 10 years ago

Remove support for multiple presshells

Categories

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

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

.
I can do this, after bug 500882 is fixed.
Assignee: nobody → Olli.Pettay
Depends on: 500882
Attached patch patch (obsolete) — Splinter Review
Tried to keep the changes pretty minimal (even in nsDocument), and made presshell a raw ref only if I was sure that nothing bad could happen.

Didn't rename GetPrimaryShell to GetShell to keep the patch size reasonable.
Attachment #420318 - Flags: superreview?(roc)
Attachment #420318 - Flags: review?(bzbarsky)
Comment on attachment 420318 [details] [diff] [review]
patch

>+++ b/content/base/public/nsIDocument.h
>+  void DeleteShell(nsIPresShell* aShell)

Did we just keep the argument so we can assert about it?  If so, do we want to nix that argument altogether in a followup and move the assert into the presshell code or something?

>+  nsIPresShell* GetPrimaryShell() const

Followup bug on whatever renaming we want here, right?

>+++ b/content/base/src/nsContentSink.cpp
> nsContentSink::ScrollToRef()
>-  if (mRef.IsEmpty()) {
>+  if (mRef.IsEmpty() || !mDocument) {


Why is this change needed?

>+  // Make sure we don't call InitialReflow() for a shell that has
>+  // already called it. This can happen when the layout frame for
>+  // an iframe is constructed *between* the Embed() call for the
>+  // docshell in the iframe, and the content sink's call to OpenBody().
>+  // (Bug 153815)

I _think_ that now that we're guaranteed to only have one shell at a time this might in fact be impossible.  Maybe a followup bug to investigate and simplify as needed if we care?

We should probably audit the iterator callsites you're changing and see which of them actually need a strong presshell ref...  The iterator forces all consumers to have one.

>+++ b/layout/svg/base/src/nsSVGOuterSVGFrame.cpp
>@@ -87,33 +86,29 @@ nsSVGMutationObserver::AttributeChanged(

Good catch here. Oops.  ;)

r=bzbarsky.
Attachment #420318 - Flags: review?(bzbarsky) → review+
(In reply to comment #3)
> Did we just keep the argument so we can assert about it?
Yes.
>  If so, do we want to
> nix that argument altogether in a followup and move the assert into the
> presshell code or something?
Or I just move the assertion to presshell in this bug. Simple thing.

> Followup bug on whatever renaming we want here, right?
Ok

> Why is this change needed?
Ah, right, it isn't needed after all.
I was just trying to be over-careful with null checks.

> I _think_ that now that we're guaranteed to only have one shell at a time this
> might in fact be impossible.  Maybe a followup bug to investigate and simplify
> as needed if we care?
Ok

> 
> We should probably audit the iterator callsites you're changing and see which
> of them actually need a strong presshell ref...  The iterator forces all
> consumers to have one.
Yeah. I didn't want to do that in this patch.
Attached patch +commentSplinter Review
Attachment #420393 - Flags: superreview?(roc)
Attachment #420318 - Attachment is obsolete: true
Attachment #420318 - Flags: superreview?(roc)
Comment on attachment 420393 [details] [diff] [review]
+comment

We need to rename GetPrimaryShell to GetShell, but that doesn't have to be this patch.
Attachment #420393 - Flags: superreview?(roc) → superreview+
> > We should probably audit the iterator callsites you're changing and see which
> > of them actually need a strong presshell ref...  The iterator forces all
> > consumers to have one.
> Yeah. I didn't want to do that in this patch.

We don't have to do the audit in this patch, but it would make sense to do it in this bug IMHO, or otherwise soon, since otherwise we'll lose track of which sites need to be audited.
http://hg.mozilla.org/mozilla-central/rev/69409debef05
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 538360
Blocks: 538362
>    NS_ASSERTION(mDocument->GetPrimaryShell() == this, "Wrong shell?");

Wouldn't that fire any time a presshell for a bfcached document (which returns null for GetPrimaryShell()) is destroyed?
Seems like SetShellHidden(PR_FALSE) is called just before dropping
contentviewer from bfcache.
OK.  Let's keep things this way, then.
Depends on: 539531
No longer depends on: 539531
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.