Bug 154636 – A function named canTakeNextToken executing blocking scripts is misleading

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Ryosuke Niwa]

A function named "canTakeNextToken" shouldn't implicitly execute blocking scripts

[Ryosuke Niwa]

Created attachment 272104 [details]
Cleanup

[Darin Adler]

Comment on attachment 272104 [details]
Cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=272104&action=review

> Source/WebCore/ChangeLog:3
> +        HTMLDocumentParser::canTakeNextToken shouldn't execute blocking scripts

Why no new test case?

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:222
> +        // happen instead  is that assigning window.location causes the parser to stop parsing cleanly.

Extra space here between "instead" and "is".

> Source/WebCore/html/parser/HTMLParserScheduler.h:104
> +        // monotonicallyIncreasingTime() can be expensive. By delaying, we avoided calling
> +        // monotonicallyIncreasingTime() when constructing non-yielding PumpSessions.
> +        if (!session.startTime)
> +            session.startTime = monotonicallyIncreasingTime();
> +
> +        session.processedTokens = 1;
> +        session.didSeeScript = false;
> +
> +        double elapsedTime = monotonicallyIncreasingTime() - session.startTime;
> +        return elapsedTime > m_parserTimeLimit;

Would be nice to refactor this so it only calls monotonicallyIncreasingTime() *once* the first time it’s called. Instead of calling it twice and subtracting the values. Maybe like this:

    if (!session.startTime) {
        session.startTime = monotonicallyIncreasingTime();
        return false;
    }
    return monotonicallyIncreasingTime() - session.startTime > m_parserTimeLimit;

[Ryosuke Niwa]

Comment on attachment 272104 [details]
Cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=272104&action=review

>> Source/WebCore/ChangeLog:3
>> +        HTMLDocumentParser::canTakeNextToken shouldn't execute blocking scripts
> 
> Why no new test case?

Sorry, maybe this bug title is misleading.
This patch is pure refactoring and there should be no behavioral change.
I've renamed the bug and clarified it in the change log.

>> Source/WebCore/html/parser/HTMLDocumentParser.cpp:222
>> +        // happen instead  is that assigning window.location causes the parser to stop parsing cleanly.
> 
> Extra space here between "instead" and "is".

Removed.

>> Source/WebCore/html/parser/HTMLParserScheduler.h:104
>> +        return elapsedTime > m_parserTimeLimit;
> 
> Would be nice to refactor this so it only calls monotonicallyIncreasingTime() *once* the first time it’s called. Instead of calling it twice and subtracting the values. Maybe like this:
> 
>     if (!session.startTime) {
>         session.startTime = monotonicallyIncreasingTime();
>         return false;
>     }
>     return monotonicallyIncreasingTime() - session.startTime > m_parserTimeLimit;

Fixed.

[Ryosuke Niwa]

Committed r197040: <http://trac.webkit.org/changeset/197040>