Closed
Bug 1148694
Opened 10 years ago
Closed 10 years ago
remove CSSCharsetRule
Categories
(Core :: DOM: CSS Object Model, defect)
Core
DOM: CSS Object Model
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: dbaron, Assigned: kevin.m.wern, Mentored)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [lang=c++])
Attachments
(3 files, 8 obsolete files)
940 bytes,
patch
|
Details | Diff | Splinter Review | |
8.07 KB,
patch
|
Details | Diff | Splinter Review | |
2.47 KB,
patch
|
Details | Diff | Splinter Review |
Per https://www.w3.org/Bugs/Public/show_bug.cgi?id=27422 we should be able to remove CSSCharsetRule. Chrome has removed it.
Reporter | ||
Updated•10 years ago
|
Mentor: dbaron
Whiteboard: [lang=c++]
Assignee | ||
Comment 1•10 years ago
|
||
Hey, I'm kind of new to this whole system. I want to work on this--is there a way to assign this bug to myself, or am I supposed to comment here and have someone change it? -Kevin
Flags: needinfo?(dbaron)
Reporter | ||
Comment 2•10 years ago
|
||
For now, I can change it, since you don't yet have the privileges described in https://developer.mozilla.org/en-US/docs/What_to_do_and_what_not_to_do_in_Bugzilla . As for what to do here: the first thing to do is probably to double-check what Chrome has done, which I *believe* is continuing to parse and support @charset rules, but just creating no representation in the object model for them. You'll then probably want to write a sequence of at least two, and maybe more, separate patches to fix this bug. The first would be to change the code that handles @charset rules to set the @charset without actually creating a charset rule object. You'd then want to adjust any existing mochitests to reflect that new behavior, and if there aren't any existing ones, write a mochitest for it. You can find this code starting from CSSParserImpl::ParseCharsetRule in layout/style/nsCSSParser.cpp . The second would be to remove the code related to CSSCharsetRule, which https://mxr.mozilla.org/mozilla-central/search?string=CSSCharsetRule should give you a good way to find. (I'm assuming you already know how to pull and build the source code.)
Assignee: nobody → kevin.m.wern
Flags: needinfo?(dbaron)
Comment 3•10 years ago
|
||
Correct, there was no observable change to the parsing, there's just no CSSCharsetRule created and exposed.
Comment 4•10 years ago
|
||
(Note that the CHARSET_RULE constant on CSSRule is not removed. This is consistent with the DOM spec.)
Assignee | ||
Comment 5•10 years ago
|
||
Hey, Sorry for the delay, I've had a really busy week, on top of the fact that a lot of the time in this bug went into the learning curve of building, mochitest, and other stuff (especially reading/double checking the code is being removed properly). Anyway, I finished all the functional patching, my only question really concerns testing. A few of the tests depend on CharsetRule being a part of the object model, which I've removed right now but am planning to reincorporate with the new functionality. An example is in layout/style/tests/test_css_eof_handling.html, which uses the cssRules array and requires it have at least one member. The behavior I'd expect for a replacement test would be to just use the determined charset of the test document (with the proper environment for the browser to choose to use the charset rule) as an indicator of whether or not the rule was parsed properly. Should I put these replacements in a new file for this bug because the behavior is so different? And then are the tests in layout/reftests/css-charset good enough for testing the basic functionality of @charset, or should I be writing more? I just want to make sure I'm being thorough with this.
Reporter | ||
Comment 6•10 years ago
|
||
(In reply to kevin.m.wern from comment #5) > reincorporate with the new functionality. An example is in > layout/style/tests/test_css_eof_handling.html, which uses the cssRules array > and requires it have at least one member. For that particular example, I think it's fine to just remove the two tests in question, since the behavior is now untestable from a Web standards perspective.
Assignee | ||
Comment 7•10 years ago
|
||
Patch for removing the generation of a rule object in ParseCharsetRule
Assignee | ||
Comment 8•10 years ago
|
||
Patch that removes code concerning the CharsetRule object
Assignee | ||
Comment 9•10 years ago
|
||
Remove test cases involving csscharsetrule objects
Assignee | ||
Comment 10•10 years ago
|
||
When I was first trying to build the code, I had to remove 'override' to get it to succeed. Not relevant to this bug but I figured it'd be easy to look into if it needs to be fixed.
Assignee | ||
Comment 11•10 years ago
|
||
The last patch is not relevant to this bug, but it is something I found when I was first trying to build the code. I figured I should at least mention it rather than omit the patch.
Reporter | ||
Updated•10 years ago
|
Attachment #8590697 -
Flags: review?(dbaron)
Reporter | ||
Comment 12•10 years ago
|
||
Comment on attachment 8590704 [details] [diff] [review] patch-fixbuildbug.patch Not sure why you needed this, but it hasn't been needed by anybody else, it's not relevant to this bug, and it looks like it ought to work fine (since ThenValue inherits from ThenValueBase which inherits from Consumer, which has a method with an identical signature).
Attachment #8590704 -
Attachment is obsolete: true
Reporter | ||
Comment 13•10 years ago
|
||
Comment on attachment 8590697 [details] [diff] [review] patch-parsecharsetrule.patch It would probably be good to add a comment that there is no rule object created for an @charset rule. Otherwise the code might be confusing. In addition to that, you probably want a commit message that says something like: Bug 1148694 patch 1 - Stop creating a CharsetRule object when parsing @charset rules. r=dbaron r=dbaron with that
Attachment #8590697 -
Flags: review?(dbaron) → review+
Reporter | ||
Comment 14•10 years ago
|
||
Comment on attachment 8590700 [details] [diff] [review] patch-removecharsetrule.patch I'd suggest a commit message like: Bug 1148694 patch 2 - Remove interface and implementation of CSSCharsetRule. r=dbaron r=dbaron with that
Attachment #8590700 -
Flags: review+
Reporter | ||
Comment 15•10 years ago
|
||
Comment on attachment 8590703 [details] [diff] [review] patch-fixtests.patch The change to test_css_eof_handling.html looks fine. I'm a little more worried about the change for test_rule_insertion.html. How did the test fail with the patch? Also, it's not clear to me what the rule insertion methods should do now. I think the change in the first patch will cause them to throw exceptions, since CSSParserImpl::ParseRule throws NS_ERROR_DOM_SYNTAX_ERR if no rule is produced. Is that what's supposed to happen? What do other browsers that have implemented this change do? Finally, I think you will also need to change dom/tests/mochitest/general/test_interfaces.html. (The second patch should have caused it to fail. Did it not?) (I can find a DOM peer to review that; :khuey is probably willing.) Sorry for not getting to this sooner.
Flags: needinfo?(kevin.m.wern)
Assignee | ||
Comment 16•10 years ago
|
||
In the final version, I will most likely combine this with patch-fixtests when I'm done looking at rule insertion
Flags: needinfo?(kevin.m.wern)
Assignee | ||
Comment 17•10 years ago
|
||
Based on chromium/blink, it seems as if they are only parsing @charset as part of a sheet. Other than that, they are setting up their parser to start from @-internal-rule in the hierarchy when ParseRule is called. They use a similar function to our Loader's to actually determine the charset. https://chromium.googlesource.com/chromium/blink/+/master/Source/core/html/parser/TextResourceDecoder.cpp And the parser changes here: http://src.chromium.org/viewvc/blink/trunk/Source/core/css/parser/CSSGrammar.y?r1=186292&r2=186291&pathrev=186292 https://src.chromium.org/viewvc/blink/trunk/Source/core/css/parser/BisonCSSParser-in.cpp?revision=185966&pathrev=186292 (not a change, but supports what I was mentioning) So I can definitely suppress the NS_ERROR_DOM_SYNTAX_ERR with a conditional, but as for the question of what insertRule should do, I have no idea. It seems basically unsupported for @charset, which makes sense I guess for a rule with strict positional requirements that actually has no position at all. Should I be acting as if it is unsupported? If so, should I remove CHARSET_RULE from conditionals? I just want to see if I'm on the right track before moving forward.
Comment 18•10 years ago
|
||
(In reply to kevin.m.wern from comment #17) > but as for the question of what insertRule should do, I have no idea. insertRule('@charset "utf-8";', 0) should throw SyntaxError per spec. Blink does this also AFAICT. http://dev.w3.org/csswg/cssom/#insert-a-css-rule step 1 invokes "parse a CSS rule" http://dev.w3.org/csswg/cssom/#parse-a-css-rule step 3 does a grammar check. http://dev.w3.org/csswg/css-syntax-3/#charset-rule - there is no grammar for @charset, so it is an invalid rule. http://dev.w3.org/csswg/cssom/#parse-a-css-rule step 3 returns syntax error. http://dev.w3.org/csswg/cssom/#insert-a-css-rule step 2 throws SyntaxError. > If so, should I remove > CHARSET_RULE from conditionals? I'm not certain what this refers to be see comment 4
Assignee | ||
Comment 19•10 years ago
|
||
I think blink actually parses the first charset rule based on Grammar.y, but based on the documentation Simon showed me it seems that all the charset instances should return a syntax error. So I did not modify the surrounding behavior of nsCSSParser.
Attachment #8590697 -
Attachment is obsolete: true
Assignee | ||
Comment 20•10 years ago
|
||
Nothing changed other than the commit message
Attachment #8590700 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8590703 -
Attachment is obsolete: true
Attachment #8594586 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8595220 -
Flags: review?(dbaron)
Assignee | ||
Updated•10 years ago
|
Attachment #8595216 -
Flags: review?(dbaron)
Assignee | ||
Updated•10 years ago
|
Attachment #8595214 -
Flags: review?(dbaron)
Reporter | ||
Comment 22•10 years ago
|
||
Comment on attachment 8595214 [details] [diff] [review] Parse charset changes with comments >From 37f7c51cae2390663cbab0107dbf58a883da73c0 Mon Sep 17 00:00:00 2001 >From: Kevin Wern <kwern@Kevins-MacBook-Pro.local> Oops, just noticed this. You should put a real email address here! >Subject: [PATCH] Bug 1148694 patch 1 - Stop creating a CharsetRule object > when parsing @charset rules. r=dbaron Hopefully this will get wrapped back to a single line when somebody imports the patch in order to land it. >+ // CharsetRule object no longer created from @charset - Bug 1148694 It's better for comments to describe the state of the code rather than a change that was previously made. The comment should say something like: // It's intentional that we don't create a rule object for @charset rules. r=dbaron with that
Attachment #8595214 -
Flags: review?(dbaron) → review+
Reporter | ||
Comment 23•10 years ago
|
||
Comment on attachment 8595216 [details] [diff] [review] Remove charset rule with correct commit message Same thing about the email address, but otherwise fine.
Attachment #8595216 -
Flags: review?(dbaron) → review+
Reporter | ||
Comment 24•10 years ago
|
||
I think everything looks good except for this question from comment 15: (In reply to David Baron [:dbaron] ⏰UTC-7 from comment #15) > I'm a little more worried about the change for test_rule_insertion.html. > How did the test fail with the patch?
Flags: needinfo?(kevin.m.wern)
Reporter | ||
Comment 25•10 years ago
|
||
Comment on attachment 8595220 [details] [diff] [review] Merge of two test patches Asking khuey to rubber-stamp the test_interfaces.html change.
Attachment #8595220 -
Flags: review?(khuey)
Attachment #8595220 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 26•10 years ago
|
||
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #24) > I think everything looks good except for this question from comment 15: > > (In reply to David Baron [:dbaron] ⏰UTC-7 from comment #15) > > I'm a little more worried about the change for test_rule_insertion.html. > > How did the test fail with the patch? The following test failures were outputted to the terminal: 671 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <0,0,0,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS 672 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <0,0,1,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS 673 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <0,0,2,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS 674 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <1,0,0,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS 675 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <1,0,1,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS 676 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <1,0,2,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS 677 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <2,0,0,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS 678 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <2,0,1,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS 679 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <2,0,2,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS SUITE-END | took 23s The GUI reflects this. For the test, @charset is inserted with every outer rule combination. The exception results from no rule object being created in ParseRule, which inherently returns a syntax error, and a failure result from InsertRule. This coincides with the CSS specs Simon posted.
Flags: needinfo?(kevin.m.wern)
Comment 27•10 years ago
|
||
(In reply to kevin.m.wern from comment #19) > I think blink actually parses the first charset rule based on Grammar.y, but > based on the documentation Simon showed me it seems that all the charset > instances should return a syntax error. So I did not modify the surrounding > behavior of nsCSSParser. When parsing a full stylesheet, a leading @charset rule is not a syntax error; see step 3 of http://dev.w3.org/csswg/css-syntax/#parse-stylesheet The only difference is if you log syntax errors to devtools I suppose.
Assignee | ||
Comment 28•10 years ago
|
||
Ok, that makes more sense--I didn't really look around on that document other than around where you linked me. So I guess the @charset should be removed entirely from ParseAtRule and handled elsewhere.
Reporter | ||
Comment 29•10 years ago
|
||
(In reply to kevin.m.wern from comment #28) > Ok, that makes more sense--I didn't really look around on that document > other than around where you linked me. So I guess the @charset should be > removed entirely from ParseAtRule and handled elsewhere. It already is handled elsewhere, in that the actual function of the @charset rule is handled in SheetLoadData::OnDetermineCharset in layout/style/Loader.cpp. But that's at a level prior to parsing. Does your patch cause a style sheet with a valid @charset rule to report an error to the error console?
Flags: needinfo?(kevin.m.wern)
Reporter | ||
Updated•10 years ago
|
Attachment #8595220 -
Flags: review?(dbaron) → review+
Reporter | ||
Comment 30•10 years ago
|
||
If you upload revised patches reflecting the comments, I can push them to try to check for any other test failures. (comment 29 also needs answering)
Assignee | ||
Comment 31•10 years ago
|
||
> (comment 29 also needs answering)
Just checked. There were no errors thrown. I checked using console logging in devtools, in case there is somewhere I missed.
Flags: needinfo?(kevin.m.wern)
Assignee | ||
Comment 32•10 years ago
|
||
Email is now a real email, newline removed from commit message (idk how that got there), and comment revised.
Assignee | ||
Comment 34•10 years ago
|
||
Changed email
Assignee | ||
Updated•10 years ago
|
Attachment #8595220 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8595214 -
Attachment is obsolete: true
Reporter | ||
Comment 35•10 years ago
|
||
Here's a try run to test those: https://treeherder.mozilla.org/#/jobs?repo=try&revision=391facb54086
Reporter | ||
Comment 36•10 years ago
|
||
(And I added the r=khuey to the third patch as well.)
Comment 37•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/74509f3957cf https://hg.mozilla.org/integration/mozilla-inbound/rev/8e76f9a26430 https://hg.mozilla.org/integration/mozilla-inbound/rev/a00db37abe24
Comment 38•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/74509f3957cf https://hg.mozilla.org/mozilla-central/rev/8e76f9a26430 https://hg.mozilla.org/mozilla-central/rev/a00db37abe24
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•10 years ago
|
Keywords: dev-doc-needed
Reporter | ||
Comment 39•9 years ago
|
||
And since I forgot to say so sooner: thanks for fixing this!
Comment 40•9 years ago
|
||
Doc updated: https://developer.mozilla.org/en-US/docs/Web/API/CSSRule and https://developer.mozilla.org/en-US/Firefox/Releases/40#CSSOM
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•