Bug 154636 - A function named canTakeNextToken executing blocking scripts is misleading
Summary: A function named canTakeNextToken executing blocking scripts is misleading
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-24 00:10 PST by Ryosuke Niwa
Modified: 2016-02-24 12:25 PST (History)
7 users (show)

See Also:


Attachments
Cleanup (13.03 KB, patch)
2016-02-24 00:34 PST, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-02-24 00:10:55 PST
A function named "canTakeNextToken" shouldn't implicitly execute blocking scripts
Comment 1 Ryosuke Niwa 2016-02-24 00:34:53 PST
Created attachment 272104 [details]
Cleanup
Comment 2 Darin Adler 2016-02-24 08:27:37 PST
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;
Comment 3 Ryosuke Niwa 2016-02-24 12:18:20 PST
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.
Comment 4 Ryosuke Niwa 2016-02-24 12:25:55 PST
Committed r197040: <http://trac.webkit.org/changeset/197040>