Closed
Bug 1029982
Opened 10 years ago
Closed 10 years ago
Don't change mNeedLayoutFlush and mNeedStyleFlush when nsPresShell::FlushPendingNotification failed
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: mtseng, Assigned: mtseng)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
Per bug 9870404 comment 86, we shouldn't change those values if the flush failed.
Assignee | ||
Comment 2•10 years ago
|
||
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)
Attachment #8445707 -
Flags: review?(roc) → review+
Comment 4•10 years ago
|
||
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-
Assignee | ||
Comment 5•10 years ago
|
||
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)
Attachment #8446403 -
Flags: review?(roc) → review+
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
Update to address comment. try push: https://tbpl.mozilla.org/?tree=Try&rev=162015a63561
Attachment #8446403 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac07609c5d20
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ac07609c5d20
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•