Bug 40393

Summary:

HTML5 Parser: Fix fast/profiler tests that depend on event handler line numbers

Product:

WebKit

Reporter:

Tony Gentilcore <tonyg>

Component:

New Bugs

Assignee:

Tony Gentilcore <tonyg>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, eric

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Bug Depends on:

Bug Blocks:

39259

Attachments:

Description	Flags
Patch	abarth: review-

Tony Gentilcore

Reported 2010-06-09 16:09:56 PDT

HTML5 Parser: Fix fast/profiler tests that depend on event handler line numbers

Attachments
Patch (1.84 KB, patch) 2010-06-09 16:15 PDT, Tony Gentilcore	abarth: review-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Tony Gentilcore

Comment 1 2010-06-09 16:15:39 PDT

Created attachment 58306 [details] Patch

Adam Barth

Comment 2 2010-06-09 16:33:08 PDT

Comment on attachment 58306 [details] Patch WebCore/html/HTML5Tokenizer.cpp:105 + if (scriptController) I'm worried that constructTreeFromToken can cause script to run synchronously, which could invalidate the |frame|, which would invalidate scriptController. Maybe we need a private helper function to get the script controller? if (ScriptController* scriptController = script()) scriptController->setEventHandlerLineNumber(...)

Adam Barth

Comment 3 2010-06-09 23:14:04 PDT

I'm going to spoil you, but I'm going to merge your patch to TOT. :)

Adam Barth

Comment 4 2010-06-09 23:35:01 PDT

Committed r60940: <http://trac.webkit.org/changeset/60940>

Eric Seidel (no email)

Comment 5 2010-06-10 03:11:00 PDT

Woh. Wasn't this a huge slowdown on the benchmark? We're adding two branches and a virtual function call to the per-token loop, no?

Eric Seidel (no email)

Comment 6 2010-06-10 03:12:42 PDT

Why do we need to reset the line number to 0 after each token is processed?

Tony Gentilcore

Comment 7 2010-06-10 07:52:25 PDT

(In reply to comment #3) > I'm going to spoil you, but I'm going to merge your patch to TOT. :) Thanks! Man you're fast. I had the same patch ready but it didn't finish building before the get together last night. There was one more place in Tokenizer that the script() fn could be used. I'll mail a patch for that.

Tony Gentilcore

Comment 8 2010-06-10 07:54:42 PDT

(In reply to comment #6) > Why do we need to reset the line number to 0 after each token is processed? I haven't confirmed, but I'm nearly certain that is for the case when a script dynamically adds an element with an event handler (it shouldn't be associated w/ the line number of the parser). In any case, it seemed safest to match the old Tokenizer's behavior for now.

Tony Gentilcore

Comment 9 2010-06-10 07:55:12 PDT

(In reply to comment #5) > Woh. Wasn't this a huge slowdown on the benchmark? We're adding two branches and a virtual function call to the per-token loop, no? Can you point me to the benchmark to run?

Adam Barth

Comment 10 2010-06-10 10:12:51 PDT

> Can you point me to the benchmark to run? http://trac.webkit.org/browser/trunk/WebCore/benchmarks/parser/html-parser.html

Eric Seidel (no email)

Comment 11 2010-06-10 11:18:14 PDT

(In reply to comment #8) > (In reply to comment #6) > > Why do we need to reset the line number to 0 after each token is processed? > > I haven't confirmed, but I'm nearly certain that is for the case when a script dynamically adds an element with an event handler (it shouldn't be associated w/ the line number of the parser). In any case, it seemed safest to match the old Tokenizer's behavior for now. Some elements carry a createdByParser flag. But I guess not all. Otherwise that would be easy to check after the fact.

Note You need to log in before you can comment on or make changes to this bug.