Bug 84522 – Page::setDefersLoading() should have a call count

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jon Honeycutt]

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.

[Jon Honeycutt]

Created attachment 138221 [details]
Patch

[WebKit Review Bot]

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.

[Andy Estes]

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.

[Jon Honeycutt]

Created attachment 139714 [details]
Patch v2

[Andy Estes]

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.

[Jon Honeycutt]

(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>.