Closed Bug 1029982 Opened 7 years ago Closed 7 years ago

Don't change mNeedLayoutFlush and mNeedStyleFlush when nsPresShell::FlushPendingNotification failed

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: mtseng, Assigned: mtseng)

References

Details

Attachments

(1 file, 2 obsolete files)

Per bug 9870404 comment 86, we shouldn't change those values if the flush failed.
Should be bug 987040....not 9870404
Attached patch layout_not_flush v1 (obsolete) — Splinter Review
Let FlushPendingNotification return a boolean value which indicate whether flush success or not and not change mNeedLayoutFlush/mNeedStyleFlush if the flush failed.
Attachment #8445707 - Flags: review?(roc)
Comment on attachment 8445707 [details] [diff] [review]
layout_not_flush v1

>@@ -7816,24 +7816,25 @@ nsDocument::FlushPendingNotifications(mo
>   // already but the presshell hasn't actually done the corresponding work yet.
>   // So if mInFlush and reentering this code, we need to flush the presshell.
>   if (mNeedStyleFlush ||
>       (mNeedLayoutFlush && aType >= Flush_InterruptibleLayout) ||
>       aType >= Flush_Display ||
>       mInFlush) {
>     nsCOMPtr<nsIPresShell> shell = GetShell();
>     if (shell) {
>-      mNeedStyleFlush = false;
>-      mNeedLayoutFlush = mNeedLayoutFlush && (aType < Flush_InterruptibleLayout);
>       // mInFlush is a bitfield, so can't us AutoRestore here.  But we
>       // need to keep track of multi-level reentry correctly, so need
>       // to restore the old mInFlush value.
>       bool oldInFlush = mInFlush;
>       mInFlush = true;
>-      shell->FlushPendingNotifications(aType);
>+      if (shell->FlushPendingNotifications(aType)) {
>+        mNeedStyleFlush = false;
>+        mNeedLayoutFlush = mNeedLayoutFlush && (aType < Flush_InterruptibleLayout);
>+      }
>       mInFlush = oldInFlush;
>     }
>   }
> }

This code correctly handles only clearing mNeedLayoutFlush when aType requests flushing layout.

>     if (flushType >= (mSuppressInterruptibleReflows ? Flush_Layout : Flush_InterruptibleLayout) &&
>         !mIsDestroying) {
>+      isFlushed = true;

But this change looks like it means that in the normal codepath, you'll return false for Flush_Style, and thus not clear mNeedStyleFlush for repeated Flush_Style flushes.


This also breaks the fact that FlushPendingNotifications sometimes calls SetNeedLayoutFlush by moving the clearing of mNeedLayoutFlush to after the call to FlushPendingNotifications; the resulting code probably still works because you return false in that case, but it's extremely confusing.


It might be better to restructure this in line with the existing code in PresShell::FlushPendingNotifications, and instead of taking this approach, call mDocument->SetNeedLayoutFlush() and/or mDocument->SetNeedStyleFlush() where needed.  Though that might also have the problem of setting mNeedLayoutFlush/mNeedStyleFlush where they weren't set before.
Attachment #8445707 - Flags: review-
Attached patch layout_not_flush v2 (obsolete) — Splinter Review
Hi dbaron, roc
I updated patch base on dbaron's comment. How about this patch? Thanks
Attachment #8445707 - Attachment is obsolete: true
Attachment #8446403 - Flags: review?(roc)
Attachment #8446403 - Flags: review?(dbaron)
Comment on attachment 8446403 [details] [diff] [review]
layout_not_flush v2

>+  bool isStyleFlushed = false;
>+  bool isLayoutFlushed = false;

Please call these variables didStyleFlush and didLayoutFlush.




>+  if (!isStyleFlushed && flushType >= Flush_Style) {
>+    mDocument->SetNeedStyleFlush();
>+  }
>+
>+  if (!isLayoutFlushed &&
>+      (flushType >=
>+       (mSuppressInterruptibleReflows ? Flush_Layout : Flush_InterruptibleLayout))) {
>+    mDocument->SetNeedLayoutFlush();

Please add && !mIsDestroying to both of these tests.


Also please remove this existing chunk of code:

>    } else if (!mIsDestroying && mSuppressInterruptibleReflows &&
>               flushType == Flush_InterruptibleLayout) {
>      // We suppressed this flush, but the document thinks it doesn't
>      // need to flush anymore.  Let it know what's really going on.
>      mDocument->SetNeedLayoutFlush();

since your new code replaces it, but move the comment from this chunk to your new code.  Except you need to adjust the comment to reflect that suppressed could now mean either due to mSuppressInterruptibleReflows or isSafeToFlush (where previously it was only about mSuppressInterruptibleReflows).

r=dbaron with those changes
Attachment #8446403 - Flags: review?(dbaron) → review+
Comment on attachment 8446403 [details] [diff] [review]
layout_not_flush v2

>Bug 1029982 - Don't change mNeedLayoutFlush and mNeedStyleFlush when flush failed.

Also please adjust your commit message to reflect that it's no longer "don't change" but instead now setting them back to true.
https://hg.mozilla.org/mozilla-central/rev/ac07609c5d20
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.