Page::setDefersLoading() should have a call count so that making several calls to defer loading must be balanced by an equal number of calls to resume loading.
Created attachment 138221 [details] Patch
Attachment 138221 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/page/Page.cpp:576: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 138221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138221&action=review As we discussed on IRC, as it stands this'll cause issues with WebKit nightly builds. > Source/WebCore/page/Page.cpp:575 > + ASSERT(defers || m_deferLoadingCallCount > 0); If you made m_deferLoadingCallCount unsigned you could simplify this expression. >> Source/WebCore/page/Page.cpp:576 >> + if (defers && ++m_deferLoadingCallCount > 1) > > An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Like StyleBot says... > Source/WebCore/page/Page.cpp:578 > + else if (!defers && --m_deferLoadingCallCount > 0) If you made m_deferLoadingCallCount unsigned you could simplify this expression. > Source/WebCore/page/Page.h:221 > + bool defersLoading() const { return m_deferLoadingCallCount > 0; } If you made m_deferLoadingCallCount unsigned you could simplify this expression. > Source/WebCore/page/Page.h:391 > + int m_deferLoadingCallCount; m_defersLoadingCallCount seems like a better name given the name of the method.
Created attachment 139714 [details] Patch v2
Comment on attachment 139714 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=139714&action=review r=me, although I like this new behavior so much that I find it unfortunate that it's opt-in rather than opt-out. If having the new behavior enabled by default causes bugs in other ports they can always opt out with a setting, or just fix their bug. > Source/WebCore/page/Page.cpp:585 > - if (defers == m_defersLoading) > + if (m_settings->wantsBalancedSetDefersLoadingBehavior()) { > + ASSERT(defers || m_defersLoadingCallCount); > + if (defers && ++m_defersLoadingCallCount > 1) > + return; > + if (!defers && --m_defersLoadingCallCount) > + return; > + } else if (defers == m_defersLoading) I'd assert that m_defersLoadingCallCount is 0 if wantsBalancedSetDefersLoadingBehavior() is false, which catches the case where the setting changes from true to false between deferring and resuming. Your other assert already catches the case where it changes from false to true.
(In reply to comment #5) > (From update of attachment 139714 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139714&action=review > > r=me, although I like this new behavior so much that I find it unfortunate that it's opt-in rather than opt-out. If having the new behavior enabled by default causes bugs in other ports they can always opt out with a setting, or just fix their bug. OK, I'll file a bug about making this opt-out. > > > Source/WebCore/page/Page.cpp:585 > > - if (defers == m_defersLoading) > > + if (m_settings->wantsBalancedSetDefersLoadingBehavior()) { > > + ASSERT(defers || m_defersLoadingCallCount); > > + if (defers && ++m_defersLoadingCallCount > 1) > > + return; > > + if (!defers && --m_defersLoadingCallCount) > > + return; > > + } else if (defers == m_defersLoading) > > I'd assert that m_defersLoadingCallCount is 0 if wantsBalancedSetDefersLoadingBehavior() is false, which catches the case where the setting changes from true to false between deferring and resuming. Your other assert already catches the case where it changes from false to true. Landed with this change in <http://trac.webkit.org/changeset/115925>.