Summary: | A function named canTakeNextToken executing blocking scripts is misleading | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||
Component: | Page Loading | Assignee: | Ryosuke Niwa <rniwa> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | beidson, cdumez, commit-queue, darin, esprehn+autocc, gyuyoung.kim, koivisto | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Ryosuke Niwa
2016-02-24 00:10:55 PST
Created attachment 272104 [details]
Cleanup
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 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. Committed r197040: <http://trac.webkit.org/changeset/197040> |