[webkit-reviews] review granted: [Bug 84522] Page::setDefersLoading() should have a call count : [Attachment 139714] Patch v2

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has granted Jon Honeycutt
<jhoneycutt at apple.com>'s request for review:
Bug 84522: Page::setDefersLoading() should have a call count
https://bugs.webkit.org/show_bug.cgi?id=84522

Attachment 139714: Patch v2
https://bugs.webkit.org/attachment.cgi?id=139714&action=review

------- Additional Comments from Andy Estes <aestes at apple.com>
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.