RESOLVED FIXED 40393
HTML5 Parser: Fix fast/profiler tests that depend on event handler line numbers 
https://bugs.webkit.org/show_bug.cgi?id=40393
Summary HTML5 Parser: Fix fast/profiler tests that depend on event handler line numbers
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-
DetailsFormatted DiffDiff
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.