Closed
Bug 787131
Opened 12 years ago
Closed 12 years ago
Move handling of dynamic type attr changes on <xul:browser> out of nsSubDocumentFrame and into nsFrameLoader
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file)
The goal is for this to work even when the <browser> is display:none.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #656941 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Updated•12 years ago
|
Updated•12 years ago
|
Summary: Move handling of dynamic type attr changes on <xul:browser> our of nsSubDocumentFrame and into nsFrameLoader → Move handling of dynamic type attr changes on <xul:browser> out of nsSubDocumentFrame and into nsFrameLoader
Comment 2•12 years ago
|
||
Comment on attachment 656941 [details] [diff] [review] Move management of primary content state stuff from the nsSubDocumentFrame into the nsFrameLoader, so it works on display:none elements. > void > nsFrameLoader::SetOwnerContent(Element* aContent) > { >+ if (!aContent && mObservingOwnerContent) { >+ mObservingOwnerContent = false; >+ mOwnerContent->RemoveMutationObserver(this); >+ } > mOwnerContent = aContent; The code above is correct only if !!aContent implies !mObservingOwnerContent, right? Otherwise we could leave an observer on the old mOwnerContent. It happens that the invariant above is satisfied in the instances where SetOwnerContent is called, but it's not clear that's the only correct way to call SetOwnerContent. I guess you're working around the complexity that arises during swapping due to the possibility of aContent == mOwnerContent? Still, it's quite abstruse. Perhaps the easiest way to simplify this would be to assert(!mObservingOwnerContent) at the top of SetOwnerContent and change the one relevant call to SetOwnerContent(nullptr) to remove the observer first? This looks fine to me otherwise, although I have to admit that I don't understand how this change affects bug 776928.
Attachment #656941 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 3•12 years ago
|
||
> The code above is correct only if !!aContent implies !mObservingOwnerContent, > right? No, it's also correct if "!!aContent && mObservingOwnerContent" means we're actually already observing aContent. Which happens to be true in the swap case, because we call AddTreeItemToTreeOwner before we call SetOwnerContent. I could try to reorder those, but I was trying to go for a minimally invasive fix. > Perhaps the easiest way to simplify this would be to > assert(!mObservingOwnerContent) at the top of SetOwnerContent Per above, that would assert on swaps. > although I have to admit that I don't understand how this change affects bug 776928. With the patch in that bug, we end up with situations where when a tab is coming to foreground its type changes to content-primary in the same function as its style display value, but before the display change is processed and creates a frame. So we were failing to tell the treeowner about the new content-primary docshell.
Comment 4•12 years ago
|
||
> Per above, that would assert on swaps. Ah, right, because mObservingOwnerContent, at that point, means mObservingMyNewOwnerContent. :-/ > I could try to reorder those, but I was trying to go for a minimally invasive fix. Reordering them looks safe to me, and would probably be much saner.
Assignee | ||
Comment 5•12 years ago
|
||
With the reordering: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a014a515052
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla18
Comment 6•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9a014a515052
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•