| Summary: | Pull script line number from DocumentParser instead of pushing it to ScriptController | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tony Gentilcore <tonyg> | ||||||||||||||||||
| Component: | New Bugs | Assignee: | Tony Gentilcore <tonyg> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | abarth, commit-queue, eric, webkit-ews, webkit.review.bot | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Other | ||||||||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||
| Bug Blocks: | 39259 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Tony Gentilcore
2010-06-15 19:36:32 PDT
Created attachment 58848 [details]
Patch
Attachment 58848 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3337186 Comment on attachment 58848 [details]
Patch
Nits below:
WebCore/bindings/js/ScriptEventListener.cpp:74
+ if (DocumentParser* parser = frame->document()->parser())
Wow, so much prettier with the new names.
WebCore/ChangeLog:10
+ cleaner IMHO.
Can you measure how much this helps us? I'm curious what kind of scale change we need per token to move the benchmark. I'm pretty sure the benchmark moves per-branch per-character.
WebCore/bindings/js/ScriptEventListener.cpp:75
+ lineNumber = parser->lineNumber() + 1;
Can you make this a static inline function just so we can give this wacky conversion a name?
WebCore/bindings/v8/ScriptEventListener.cpp:68
+ lineNumber = parser->lineNumber();
No +1 here?
How did it end up as push in the first place? Can we CC the folks involved in that change in case there is knowledge to be shared? Wow, nevermind. This backwards design has been with us since back in the KJSProxy days. Let's torch it. :) I went back to r11588 and still found this stuff here. Comment on attachment 58848 [details]
Patch
I'm changing this to a r-. I'd like to see perf numbers from this change (from running our parser benchmark) and I would like to see the lineNumber+1 hack encapsulated into some sort of function somewhere. Probably just moved over into the ScriptController code. If it had a eventHandlerlineNumber() accessor which knew how to call the parser and do the +1, that seems simplest.
Oh, and if we're fixing a bug with event handler line numbers and the xml parser we should be sure to test that. I can help you write the xhtml or svg if needed. Created attachment 59014 [details]
Patch
Attachment 59014 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #3) > (From update of attachment 58848 [details]) > Nits below: > > WebCore/bindings/js/ScriptEventListener.cpp:74 > + if (DocumentParser* parser = frame->document()->parser()) > Wow, so much prettier with the new names. Yeah, nice work. > > WebCore/ChangeLog:10 > + cleaner IMHO. > Can you measure how much this helps us? I'm curious what kind of scale change we need per token to move the benchmark. I'm pretty sure the benchmark moves per-branch per-character. > Here are my results: Before patch: avg 3948.5 stdev 30.445853576472445 avg 3946.25 stdev 28.035468606748843 After patch: avg 3861.7 stdev 28.241990014869703 avg 3859.4 stdev 28.793054718108667 > WebCore/bindings/js/ScriptEventListener.cpp:75 > + lineNumber = parser->lineNumber() + 1; > Can you make this a static inline function just so we can give this wacky conversion a name? > Done. > WebCore/bindings/v8/ScriptEventListener.cpp:68 > + lineNumber = parser->lineNumber(); > No +1 here? This matches the existing code. Adding the +1 makes the line number expectations in existing LayoutTests fail. Created attachment 59017 [details]
Fix tab in ChangeLog
(In reply to comment #10) > > WebCore/bindings/v8/ScriptEventListener.cpp:68 > > + lineNumber = parser->lineNumber(); > > No +1 here? > > This matches the existing code. Adding the +1 makes the line number expectations in existing LayoutTests fail. Current LayoutTest expectations aren't necessarily right, especially when it comes to line numbers for scripts. if this is a V8 vs. JSC difference we should abstract it away using ScriptController somehow. Parser line numbers need to be translated to "script engine" line numbers in some way, and that shouldn't be by manual +1's in random places. :) Comment on attachment 59017 [details]
Fix tab in ChangeLog
Wow, this patch is GORGEOUS.
I would kinda like a comment here to explain the +1. At least we only have one +1 now though:
+ if (DocumentParser* parser = m_frame->document()->parser())
+ return parser->lineNumber() + 1;
Also, do we need some sort of XML-based event handler test to test the fixed line number handling for the XML parser?
Created attachment 59067 [details]
Patch
Created attachment 59069 [details]
Patch
Comment on attachment 59069 [details]
Patch
Fantastic!
We should file a follow-up bug about passing your new test.
Thanks!
(In reply to comment #16) > (From update of attachment 59069 [details]) > Fantastic! > > We should file a follow-up bug about passing your new test. https://bugs.webkit.org/show_bug.cgi?id=40809 > > Thanks! Comment on attachment 59069 [details] Patch Rejecting patch 59069 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Last 500 characters of output: rtree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found Testing 19160 test cases. svg/dom/script-line-number.xhtml -> failed Exiting early after 1 failures. 16783 tests run. 326.66s total testing time 16782 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 15 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/3301412 (In reply to comment #18) > (From update of attachment 59069 [details]) > Rejecting patch 59069 from commit-queue. > > Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 > Last 500 characters of output: > rtree > Compiling Java tests > make: Nothing to be done for `default'. > Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests > Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found > Testing 19160 test cases. > svg/dom/script-line-number.xhtml -> failed > > Exiting early after 1 failures. 16783 tests run. > 326.66s total testing time > > 16782 test cases (99%) succeeded > 1 test case (<1%) had incorrect layout > 15 test cases (<1%) had stderr output > > Full output: http://webkit-commit-queue.appspot.com/results/3301412 Is it possible to know which platform generated the failure? I'm having trouble reproducing locally. The commit-queue runs on Mac Leopard. (In reply to comment #20) > The commit-queue runs on Mac Leopard. I finally figured it out. It reports different line numbers when run after another test vs. being run by itself. Something is really wacky with the line number from libxml2. I have two options: 1. Disable this new test in this patch 2. Fix the XML line numbers in this patch We're probablyl not resetting libxml's internal state in between uses. You should feel free to skip the test (or make it simply not print the line numbers in the expectations). Or you could also figure out how to reset libxml's internal state. Thank you for chasing the goose, but don't feel obligated to run too far. :) We seem to use:
int XMLDocumentParser::lineNumber() const
{
return context() ? context()->input->line : 1;
}
int XMLDocumentParser::columnNumber() const
{
return context() ? context()->input->col : 1;
}
instead of:
http://xmlsoft.org/html/libxml-SAX2.html#xmlSAX2GetLineNumber
Not sure why.
It's possible that context->input gets re-used between contexts. I kinda doubt it though.
I suggest just skipping on libxml platforms. This stuff is complicated and broken. If you'd like to dive into libXML2 feel free, but I've been there. It's a trail of tears. Mostly we end up having to be careful because libxml2 keeps a lot of global state, and clients of WebKit may also be clients of libxml. When we've talked with DV (the author of libxml2) he's expressly discouraged using libxml2 from a library like we do. So we have to hack around various bits of libxml2, like its encoding detection, because we only use it for its xml parsing, and not for all the http fetching, encoding detection, tree building, etc. that it has built into it. (In reply to comment #24) > I suggest just skipping on libxml platforms. This stuff is complicated and broken. If you'd like to dive into libXML2 feel free, but I've been there. It's a trail of tears. Mostly we end up having to be careful because libxml2 keeps a lot of global state, and clients of WebKit may also be clients of libxml. When we've talked with DV (the author of libxml2) he's expressly discouraged using libxml2 from a library like we do. So we have to hack around various bits of libxml2, like its encoding detection, because we only use it for its xml parsing, and not for all the http fetching, encoding detection, tree building, etc. that it has built into it. Well context->input->line, context->node->line, and xmlSAX2GetLineNumber(context) all seem to behave identically. I also tried setting context->linenumbers = 1 and lineNumbersDefault(1) during initialization. So I'm giving up on chasing this goose. Since libxml2 is used everywhere except QT, this would have to be skipped everywhere except there. I don't have a convenient way to build and test QT to see if the test works there, so I just commented out the expectations in the layout test and linked to https://bugs.webkit.org/show_bug.cgi?id=40809. Created attachment 59204 [details]
Patch
Comment on attachment 59204 [details]
Patch
This si fine. Seems silly land LayoutTests/svg/dom/script-line-number.xhtml which does nothing. would be better to just attach it to the bug which is going to fix it instead. :)
Created attachment 59377 [details]
Patch
(In reply to comment #27) > (From update of attachment 59204 [details]) > This si fine. Seems silly land LayoutTests/svg/dom/script-line-number.xhtml which does nothing. would be better to just attach it to the bug which is going to fix it instead. :) Okay, I've removed the SVG test. I'm attaching it to https://bugs.webkit.org/show_bug.cgi?id=40809 now. Comment on attachment 59377 [details]
Patch
You can just re-upload with "land-safely" which will queue it for the commit-queue (or you can mark the patch yourself). If the reviewer field is filled in, you don't need another review, just cq+ (which now that you're a committer you can give yourself).
Committed r61640: <http://trac.webkit.org/changeset/61640> http://trac.webkit.org/changeset/61640 might have broken Qt Linux Release The following changes are on the blame list: http://trac.webkit.org/changeset/61640 http://trac.webkit.org/changeset/61638 http://trac.webkit.org/changeset/61639 Created attachment 59453 [details]
Patch
|