Bug 107561 - Make BackgroundHTMLParser track line/column numbers
Summary: Make BackgroundHTMLParser track line/column numbers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks: 106127
  Show dependency treegraph
 
Reported: 2013-01-22 10:35 PST by Tony Gentilcore
Modified: 2013-01-22 14:14 PST (History)
3 users (show)

See Also:


Attachments
Patch (6.18 KB, patch)
2013-01-22 10:36 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch for landing (6.16 KB, patch)
2013-01-22 12:21 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch for landing (6.32 KB, patch)
2013-01-22 13:39 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2013-01-22 10:35:43 PST
Make BackgroundHTMLParser track line/column numbers
Comment 1 Tony Gentilcore 2013-01-22 10:36:37 PST
Created attachment 184010 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-01-22 11:01:26 PST
Comment on attachment 184010 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184010&action=review

> Source/WebCore/html/parser/BackgroundHTMLParser.cpp:128
> +        m_pendingTokens.append(CompactHTMLToken(m_token, TextPosition(m_input.current().currentLine(), m_input.current().currentColumn())));

That's unfortunate.  I guess tokens are less compact now. :)
Comment 3 Eric Seidel (no email) 2013-01-22 11:02:10 PST
I guess we need lines for each token to handle event handler lines properly?
Comment 4 Eric Seidel (no email) 2013-01-22 11:05:39 PST
This is going to conflict with bug 107317.

I think we're also probably going to change compact token to just carry the deltas from the previous token.  Those will take up less than 32-bits each, and allow compact token to be smaller again.
Comment 5 Eric Seidel (no email) 2013-01-22 11:06:39 PST
Then we would send the real line number separately in the "Delivery" object, and only the deltas in each token.  That means that if for some reason we ever got off, we'd only be off for at max tokenLimit tokens.
Comment 6 WebKit Review Bot 2013-01-22 11:30:19 PST
Comment on attachment 184010 [details]
Patch

Rejecting attachment 184010 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
rce/WebKit/chromium/webkit/media/crypto/ppapi/cdm --revision 173055 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
56>At revision 173055.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/16043681
Comment 7 Eric Seidel (no email) 2013-01-22 11:30:51 PST
The question of where to draw the line between line number accuracy and parser speed is a separate one we can solve after this.

If we only stored per-token-deltas, then we could yield after any token where the delta would be too large. (trading speed for accuracy) or simply ignore the error, or use a solution like you did (which trades some potential speed for accuracy on all documents).

I'll follow-up with a separate bug after we get perf data.
Comment 8 Tony Gentilcore 2013-01-22 12:21:59 PST
Created attachment 184033 [details]
Patch for landing
Comment 9 WebKit Review Bot 2013-01-22 13:12:42 PST
Comment on attachment 184033 [details]
Patch for landing

Rejecting attachment 184033 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
z 2 (offset 8 lines).
patching file Source/WebCore/html/parser/CompactHTMLToken.h
Hunk #3 succeeded at 65 with fuzz 2.
Hunk #4 FAILED at 75.
1 out of 4 hunks FAILED -- saving rejects to file Source/WebCore/html/parser/CompactHTMLToken.h.rej
patching file Source/WebCore/html/parser/HTMLDocumentParser.cpp
patching file Source/WebCore/html/parser/HTMLDocumentParser.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16044730
Comment 10 Eric Seidel (no email) 2013-01-22 13:14:16 PST
It appears my patch won the commit-queue race. :(

We'll need to add this to "SameSizeAsCompactToken", which I can do if you'd like:
http://trac.webkit.org/changeset/140453/trunk/Source/WebCore/html/parser/CompactHTMLToken.cpp
Comment 11 Tony Gentilcore 2013-01-22 13:39:07 PST
Created attachment 184041 [details]
Patch for landing
Comment 12 WebKit Review Bot 2013-01-22 14:14:20 PST
Comment on attachment 184041 [details]
Patch for landing

Clearing flags on attachment: 184041

Committed r140467: <http://trac.webkit.org/changeset/140467>
Comment 13 WebKit Review Bot 2013-01-22 14:14:24 PST
All reviewed patches have been landed.  Closing bug.