Closed
Bug 534226
Opened 13 years ago
Closed 13 years ago
Remove support for multiple presshells
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: smaug, Assigned: smaug)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
41.82 KB,
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
.
Assignee | ||
Comment 1•13 years ago
|
||
I can do this, after bug 500882 is fixed.
Assignee: nobody → Olli.Pettay
Depends on: 500882
Assignee | ||
Comment 2•13 years ago
|
||
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 3•13 years ago
|
||
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+
Assignee | ||
Comment 4•13 years ago
|
||
(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.
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #420393 -
Flags: superreview?(roc)
Assignee | ||
Updated•13 years ago
|
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.
Assignee | ||
Comment 8•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/69409debef05
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
![]() |
||
Comment 9•13 years ago
|
||
> 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?
Assignee | ||
Comment 10•13 years ago
|
||
Seems like SetShellHidden(PR_FALSE) is called just before dropping contentviewer from bfcache.
![]() |
||
Comment 11•13 years ago
|
||
OK. Let's keep things this way, then.
Updated•3 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•