Bug 40393 - HTML5 Parser: Fix fast/profiler tests that depend on event handler line numbers
Summary: HTML5 Parser: Fix fast/profiler tests that depend on event handler line numbers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks: 39259
  Show dependency treegraph
 
Reported: 2010-06-09 16:09 PDT by Tony Gentilcore
Modified: 2010-06-10 11:18 PDT (History)
2 users (show)

See Also:


Attachments
Patch (1.84 KB, patch)
2010-06-09 16:15 PDT, Tony Gentilcore
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2010-06-09 16:09:56 PDT
HTML5 Parser: Fix fast/profiler tests that depend on event handler line numbers
Comment 1 Tony Gentilcore 2010-06-09 16:15:39 PDT
Created attachment 58306 [details]
Patch
Comment 2 Adam Barth 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(...)
Comment 3 Adam Barth 2010-06-09 23:14:04 PDT
I'm going to spoil you, but I'm going to merge your patch to TOT.  :)
Comment 4 Adam Barth 2010-06-09 23:35:01 PDT
Committed r60940: <http://trac.webkit.org/changeset/60940>
Comment 5 Eric Seidel (no email) 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?
Comment 6 Eric Seidel (no email) 2010-06-10 03:12:42 PDT
Why do we need to reset the line number to 0 after each token is processed?
Comment 7 Tony Gentilcore 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.
Comment 8 Tony Gentilcore 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.
Comment 9 Tony Gentilcore 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?
Comment 10 Adam Barth 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
Comment 11 Eric Seidel (no email) 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.