Bug 1357142: Kill PresShell::RecreateFramesFor. r=bz
It's not only inefficient, but also prone to buggyness. Since styles may not be
up-to-date when it happens.
Post a reconstruct instead, which ensures a style flush happens before running
frame construction.
MozReview-Commit-ID: DrakHsJv5fY
Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>
--- a/dom/base/nsObjectLoadingContent.cpp
+++ b/dom/base/nsObjectLoadingContent.cpp
@@ -2669,38 +2669,42 @@ nsObjectLoadingContent::NotifyStateChang
nsIDocument* doc = thisContent->GetComposedDoc();
if (!doc) {
return; // Nothing to do
}
EventStates newState = ObjectState();
+ if (newState == aOldState && mType == aOldType) {
+ return; // Also done.
+ }
+
if (newState != aOldState) {
NS_ASSERTION(thisContent->IsInComposedDoc(), "Something is confused");
// This will trigger frame construction
EventStates changedBits = aOldState ^ newState;
-
{
nsAutoScriptBlocker scriptBlocker;
doc->ContentStateChanged(thisContent, changedBits);
}
- if (aSync) {
- NS_ASSERTION(InActiveDocument(thisContent), "Something is confused");
- // Make sure that frames are actually constructed immediately.
- doc->FlushPendingNotifications(FlushType::Frames);
- }
} else if (aOldType != mType) {
// If our state changed, then we already recreated frames
// Otherwise, need to do that here
nsCOMPtr<nsIPresShell> shell = doc->GetShell();
if (shell) {
- shell->RecreateFramesFor(thisContent);
+ shell->PostRecreateFramesFor(thisContent->AsElement());
}
}
+
+ if (aSync) {
+ NS_ASSERTION(InActiveDocument(thisContent), "Something is confused");
+ // Make sure that frames are actually constructed immediately.
+ doc->FlushPendingNotifications(FlushType::Frames);
+ }
}
nsObjectLoadingContent::ObjectType
nsObjectLoadingContent::GetTypeOfContent(const nsCString& aMIMEType)
{
nsCOMPtr<nsIContent> thisContent =
do_QueryInterface(static_cast<nsIImageLoadingContent*>(this));
NS_ASSERTION(thisContent, "must be a content");
--- a/dom/xbl/nsBindingManager.cpp
+++ b/dom/xbl/nsBindingManager.cpp
@@ -250,52 +250,53 @@ nsBindingManager::GetAnonymousNodesFor(n
nsINodeList*
nsBindingManager::GetAnonymousNodesFor(nsIContent* aContent)
{
nsXBLBinding* binding = GetBindingWithContent(aContent);
return binding ? binding->GetAnonymousNodeList() : nullptr;
}
nsresult
-nsBindingManager::ClearBinding(nsIContent* aContent)
+nsBindingManager::ClearBinding(Element* aElement)
{
// Hold a ref to the binding so it won't die when we remove it from our table
RefPtr<nsXBLBinding> binding =
- aContent ? aContent->GetXBLBinding() : nullptr;
+ aElement ? aElement->GetXBLBinding() : nullptr;
if (!binding) {
return NS_OK;
}
// For now we can only handle removing a binding if it's the only one
NS_ENSURE_FALSE(binding->GetBaseBinding(), NS_ERROR_FAILURE);
// Hold strong ref in case removing the binding tries to close the
// window or something.
// XXXbz should that be ownerdoc? Wouldn't we need a ref to the
// currentdoc too? What's the one that should be passed to
// ChangeDocument?
- nsCOMPtr<nsIDocument> doc = aContent->OwnerDoc();
+ nsCOMPtr<nsIDocument> doc = aElement->OwnerDoc();
// Finally remove the binding...
// XXXbz this doesn't remove the implementation! Should fix! Until
// then we need the explicit UnhookEventHandlers here.
binding->UnhookEventHandlers();
binding->ChangeDocument(doc, nullptr);
- aContent->SetXBLBinding(nullptr, this);
+ aElement->SetXBLBinding(nullptr, this);
binding->MarkForDeath();
// ...and recreate its frames. We need to do this since the frames may have
// been removed and style may have changed due to the removal of the
// anonymous children.
// XXXbz this should be using the current doc (if any), not the owner doc.
nsIPresShell *presShell = doc->GetShell();
NS_ENSURE_TRUE(presShell, NS_ERROR_FAILURE);
- return presShell->RecreateFramesFor(aContent);;
+ presShell->PostRecreateFramesFor(aElement);
+ return NS_OK;
}
nsresult
nsBindingManager::LoadBindingDocument(nsIDocument* aBoundDoc,
nsIURI* aURL,
nsIPrincipal* aOriginPrincipal)
{
NS_PRECONDITION(aURL, "Must have a URI to load!");
--- a/dom/xbl/nsBindingManager.h
+++ b/dom/xbl/nsBindingManager.h
@@ -85,17 +85,17 @@ public:
* Return the nodelist of "anonymous" kids for this node. This might
* actually include some of the nodes actual DOM kids, if there are
* <children> tags directly as kids of <content>. This will only end up
* returning a non-null list for nodes which have a binding attached.
*/
nsresult GetAnonymousNodesFor(nsIContent* aContent, nsIDOMNodeList** aResult);
nsINodeList* GetAnonymousNodesFor(nsIContent* aContent);
- nsresult ClearBinding(nsIContent* aContent);
+ nsresult ClearBinding(mozilla::dom::Element* aElement);
nsresult LoadBindingDocument(nsIDocument* aBoundDoc, nsIURI* aURL,
nsIPrincipal* aOriginPrincipal);
nsresult AddToAttachedQueue(nsXBLBinding* aBinding);
void RemoveFromAttachedQueue(nsXBLBinding* aBinding);
void ProcessAttachedQueue(uint32_t aSkipSize = 0)
{
if (mProcessingAttachedStack || mAttachedStack.Length() <= aSkipSize) {
--- a/dom/xbl/nsXBLResourceLoader.cpp
+++ b/dom/xbl/nsXBLResourceLoader.cpp
@@ -216,25 +216,26 @@ nsXBLResourceLoader::NotifyBoundElements
if (!xblService)
return;
nsIURI* bindingURI = mBinding->BindingURI();
uint32_t eltCount = mBoundElements.Count();
for (uint32_t j = 0; j < eltCount; j++) {
nsCOMPtr<nsIContent> content = mBoundElements.ObjectAt(j);
-
+ MOZ_ASSERT(content->IsElement());
+
bool ready = false;
xblService->BindingReady(content, bindingURI, &ready);
if (ready) {
// We need the document to flush out frame construction and
// such, so we want to use the current document.
nsIDocument* doc = content->GetUncomposedDoc();
-
+
if (doc) {
// Flush first to make sure we can get the frame for content
doc->FlushPendingNotifications(FlushType::Frames);
// If |content| is (in addition to having binding |mBinding|)
// also a descendant of another element with binding |mBinding|,
// then we might have just constructed it due to the
// notification of its parent. (We can know about both if the
@@ -243,21 +244,24 @@ nsXBLResourceLoader::NotifyBoundElements
// has a primary frame and whether it's in the undisplayed map
// before sending a ContentInserted notification, or bad things
// will happen.
nsIPresShell *shell = doc->GetShell();
if (shell) {
nsIFrame* childFrame = content->GetPrimaryFrame();
if (!childFrame) {
// Check to see if it's in the undisplayed content map.
+ //
+ // FIXME(emilio, bug 1359384): What about display: contents stuff?
+ // Looks like this would be inefficient in that case?
nsStyleContext* sc =
shell->FrameManager()->GetUndisplayedContent(content);
if (!sc) {
- shell->RecreateFramesFor(content);
+ shell->PostRecreateFramesFor(content->AsElement());
}
}
}
// Flush again
// XXXbz why is this needed?
doc->FlushPendingNotifications(FlushType::ContentAndNotify);
}
--- a/editor/libeditor/HTMLAnonymousNodeEditor.cpp
+++ b/editor/libeditor/HTMLAnonymousNodeEditor.cpp
@@ -228,26 +228,26 @@ HTMLEditor::CreateAnonymousElement(nsIAt
ElementDeletionObserver* observer =
new ElementDeletionObserver(newContent, parentContent);
NS_ADDREF(observer); // NodeWillBeDestroyed releases.
parentContent->AddMutationObserver(observer);
newContent->AddMutationObserver(observer);
#ifdef DEBUG
- // Editor anonymous content gets passed to RecreateFramesFor... which can't
- // _really_ deal with anonymous content (because it can't get the frame tree
- // ordering right). But for us the ordering doesn't matter so this is sort of
- // ok.
+ // Editor anonymous content gets passed to PostRecreateFramesFor... which
+ // can't _really_ deal with anonymous content (because it can't get the frame
+ // tree ordering right). But for us the ordering doesn't matter so this is
+ // sort of ok.
newContent->SetProperty(nsGkAtoms::restylableAnonymousNode,
reinterpret_cast<void*>(true));
#endif // DEBUG
// display the element
- ps->RecreateFramesFor(newContent);
+ ps->PostRecreateFramesFor(newContent);
return newContent.forget();
}
// Removes event listener and calls DeleteRefToAnonymousNode.
void
HTMLEditor::RemoveListenerAndDeleteRef(const nsAString& aEvent,
nsIDOMEventListener* aListener,
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -2945,54 +2945,16 @@ PresShell::CreateFramesFor(nsIContent* a
nsILayoutHistoryState* layoutState = fc->GetLastCapturedLayoutHistoryState();
fc->BeginUpdate();
fc->ContentInserted(aContent->GetParent(), aContent, layoutState, false);
fc->EndUpdate();
--mChangeNestCount;
}
-nsresult
-PresShell::RecreateFramesFor(nsIContent* aContent)
-{
- NS_ENSURE_TRUE(mPresContext, NS_ERROR_FAILURE);
- if (!mDidInitialize) {
- // Nothing to do here. In fact, if we proceed and aContent is the
- // root we will crash.
- return NS_OK;
- }
-
- // Don't call RecreateFramesForContent since that is not exported and we want
- // to keep the number of entrypoints down.
-
- NS_ASSERTION(mViewManager, "Should have view manager");
-
- // Have to make sure that the content notifications are flushed before we
- // start messing with the frame model; otherwise we can get content doubling.
- mDocument->FlushPendingNotifications(FlushType::ContentAndNotify);
-
- nsAutoScriptBlocker scriptBlocker;
-
- nsStyleChangeList changeList(mPresContext->StyleSet()->BackendType());
- changeList.AppendChange(nullptr, aContent, nsChangeHint_ReconstructFrame);
-
- // We might have restyles pending when we're asked to recreate frames.
- // Record that we're OK with stale styles being returned, to avoid assertions.
- ServoStyleSet::AutoAllowStaleStyles guard(mStyleSet->GetAsServo());
-
- // Mark ourselves as not safe to flush while we're doing frame construction.
- ++mChangeNestCount;
- RestyleManager* restyleManager = mPresContext->RestyleManager();
- restyleManager->ProcessRestyledFrames(changeList);
- restyleManager->FlushOverflowChangedTracker();
- --mChangeNestCount;
-
- return NS_OK;
-}
-
void
nsIPresShell::PostRecreateFramesFor(Element* aElement)
{
mPresContext->RestyleManager()->PostRestyleEvent(aElement, nsRestyleHint(0),
nsChangeHint_ReconstructFrame);
}
void
--- a/layout/base/PresShell.h
+++ b/layout/base/PresShell.h
@@ -51,16 +51,20 @@ struct nsCallbackEventRequest;
class ReflowCountMgr;
#endif
class nsPresShellEventCB;
class nsAutoCauseReflowNotifier;
namespace mozilla {
+namespace dom {
+class Element;
+} // namespace dom
+
class EventDispatchingCallback;
// A set type for tracking visible frames, for use by the visibility code in
// PresShell. The set contains nsIFrame* pointers.
typedef nsTHashtable<nsPtrHashKey<nsIFrame>> VisibleFrames;
// A hash table type for tracking visible regions, for use by the visibility
// code in PresShell. The mapping is from view IDs to regions in the
@@ -128,21 +132,16 @@ public:
virtual bool IsSafeToFlush() const override;
virtual void DoFlushPendingNotifications(mozilla::FlushType aType) override;
virtual void DoFlushPendingNotifications(mozilla::ChangesToFlush aType) override;
virtual void DestroyFramesFor(nsIContent* aContent,
nsIContent** aDestroyedFramesFor) override;
virtual void CreateFramesFor(nsIContent* aContent) override;
/**
- * Recreates the frames for a node
- */
- virtual nsresult RecreateFramesFor(nsIContent* aContent) override;
-
- /**
* Post a callback that should be handled after reflow has finished.
*/
virtual nsresult PostReflowCallback(nsIReflowCallback* aCallback) override;
virtual void CancelReflowCallback(nsIReflowCallback* aCallback) override;
virtual void ClearFrameRefs(nsIFrame* aFrame) override;
virtual already_AddRefed<gfxContext> CreateReferenceRenderingContext() override;
virtual nsresult GoToAnchor(const nsAString& aAnchorName, bool aScroll,
--- a/layout/base/nsIPresShell.h
+++ b/layout/base/nsIPresShell.h
@@ -548,21 +548,16 @@ public:
nsIContent** aDestroyedFramesFor) = 0;
/**
* Create new frames for aContent. It will use the last captured layout
* history state captured in the frame constructor to restore the state
* in the new frame tree.
*/
virtual void CreateFramesFor(nsIContent* aContent) = 0;
- /**
- * Recreates the frames for a node
- */
- virtual nsresult RecreateFramesFor(nsIContent* aContent) = 0;
-
void PostRecreateFramesFor(mozilla::dom::Element* aElement);
void RestyleForAnimation(mozilla::dom::Element* aElement,
nsRestyleHint aHint);
// ShadowRoot has APIs that can change styles so we only
// want to restyle elements in the ShadowRoot and not the whole
// document.
virtual void RecordShadowStyleChange(mozilla::dom::ShadowRoot* aShadowRoot) = 0;