Closed Bug 483015 Opened 11 years ago Closed 10 years ago

[HTML5] Expose HTML line number to JS and CSS parsers

Categories

(Core :: DOM: HTML Parser, defect, P1)

Other Branch
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

The tokenizer needs to keep count of the line number in order to avoid regressing CSS and JS error reporting.
Avoid regressing bug 190576.
Summary: Expose HTML line number to JS and CSS parsers → [HTML5] Expose HTML line number to JS and CSS parsers
Priority: -- → P1
Fixed in rev cbd95aa84038 in the HTML5 repo.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Forgot the case where doc.write leaves data in the input stream when returning.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file Testcase
This doesn't seem to work at all for <script> or <style>, in my testing.  Testcase attached
Attached patch Fix (obsolete) — Splinter Review
This patch should give sensible line numbers for content in the network stream and for content in the root context of document.open()ed documents.

Line numbers in other document.written content won't necessarily match the line numbers reported by the old HTML parser.
Attachment #411180 - Attachment is obsolete: true
Attached patch Mochitest (obsolete) — Splinter Review
Per Boris' comments in .platform, I've retained approximate line numbers even when they are know to be bogus.

Moving the line number in and out of the tokenizer in nsHtml5Parser is quite ugly, but I didn't want the tokenizer to know about all this.
Attachment #411666 - Flags: review?(bnewman)
Attached patch Refreshed mochitest (obsolete) — Splinter Review
Attachment #411400 - Attachment is obsolete: true
Attachment #411667 - Flags: review?(bnewman)
Comment on attachment 411666 [details] [diff] [review]
Right line numbers for network and document.opened root context, approximate for other document.writes

Looks good, and playing around with the testcase produced all the right behaviors, as far as I can see.
Attachment #411666 - Flags: review?(bnewman) → review+
Comment on attachment 411667 [details] [diff] [review]
Refreshed mochitest

I'd love to see a few more tests here.  What happens with multiple unknown properties? Multiple <style> tags?
(In reply to comment #9)
> (From update of attachment 411666 [details] [diff] [review])
> Looks good, and playing around with the testcase produced all the right
> behaviors, as far as I can see.

Thanks.

(In reply to comment #10)
> (From update of attachment 411667 [details] [diff] [review])
> I'd love to see a few more tests here.  What happens with multiple unknown
> properties? Multiple <style> tags?

Attached mochitests that have two <style> tags each and also test cases where the error isn't on the first line of the <style> element.
Attachment #411667 - Attachment is obsolete: true
Attachment #414031 - Flags: review?(bnewman)
Attachment #411667 - Flags: review?(bnewman)
Comment on attachment 414031 [details] [diff] [review]
Test two <style> tags, two bogus properties

r=me provided you add the code to set/reset the html5.enable pref
Attachment #414031 - Flags: review?(bnewman) → review+
(In reply to comment #12)
> (From update of attachment 414031 [details] [diff] [review])
> r=me provided you add the code to set/reset the html5.enable pref

Thanks.

(I haven't toggled the pref on test cases that are supposed to pass with both parsers, but I'll change it here. To get good test coverage anyway, it's necessary to push to try with html5.enable set to true.)
a=beltzner to land on mozilla-central as this is pref'd off by default and so I'm happy to consider it NPOTB for now.
Thanks for the a. Pushed:
http://hg.mozilla.org/mozilla-central/rev/60fea11d79cd

(I didn't push the test yet, since it depends on bug 529544.)
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Depends on: 529544
Flags: in-testsuite?
Resolution: --- → FIXED
Comment on attachment 414031 [details] [diff] [review]
Test two <style> tags, two bogus properties

Random nits:

>+      ok(this.lineNumbers.length > 0, "Seen too many errors!");

Maybe this was copied from another test, but s/Seen/Saw/ for better English, or just "Too many errors!".

>+      
>+      var scriptErr = message.QueryInterface(Ci.nsIScriptError);
>+
>+      is(scriptErr.lineNumber, this.lineNumbers.shift(),
>+         "Wrong line");
>+    },
>+    
>+    finish: function() {
>+      ok(this.lineNumbers.length == 0 , "Didn't the right number of messages.");

Missing "get"? Same error later.

Great to have these line numbering bugs fixed!

/be
(In reply to comment #16)
> (From update of attachment 414031 [details] [diff] [review])
> Random nits:

Nits fixed locally. Thanks.

I haven't pushed this fix, because flipping the html5.enable pref here makes some document that lives across tests (the harness itself presumably) cache a fragment parser and then the innerHTML setting fails in a subsequent test because the pref and the cached fragment parser are out of sync.

bnewman, is it OK if I make this test not flip the pref? sicking says making the document remember what kind of fragment parser it has is a waste of time.

(I don't have an explanation why this is a problem with this test case but not with other test cases that flip the pref.)
You need to log in before you can comment on or make changes to this bug.