Closed
Bug 1195978
Opened 9 years ago
Closed 9 years ago
getRelativeRuleLine yields wrong result after DOMUtils.parseStyleSheet
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: tromey, Assigned: tromey)
References
Details
Attachments
(1 file, 4 obsolete files)
3.87 KB,
patch
|
tromey
:
review+
|
Details | Diff | Splinter Review |
I noticed today that my fix for bug 1013814 causes a regression. After the patch, if I call DOMUtils.parseStyleSheet on style sheet coming from a <style> element, then future calls to getRelativeRuleLine will yield wrong results (in particular they underflow and are turned into very large numbers due to the use of unsigned math). I tracked this down to this line: https://dxr.mozilla.org/mozilla-central/source/layout/style/CSSStyleSheet.cpp#2316 which sets the line number to 1 -- but I think it should be the line number of the <style> element. I have a patch for this that I'll attach. It works for my case but I haven't run the full test suite.
Assignee | ||
Comment 1•9 years ago
|
||
Here's the patch.
Assignee | ||
Comment 2•9 years ago
|
||
Comment on attachment 8649469 [details] [diff] [review] set line number when re-parsing sheet I am wondering if this seems like a reasonable approach or if there is some other way I should try. If it looks ok I will write the test and try a full test run.
Attachment #8649469 -
Flags: feedback?(cam)
Comment 3•9 years ago
|
||
Comment on attachment 8649469 [details] [diff] [review] set line number when re-parsing sheet Review of attachment 8649469 [details] [diff] [review]: ----------------------------------------------------------------- Yeah I think this is OK. We do preserve the line number when the sheet's text is modified through the DOM.
Attachment #8649469 -
Flags: feedback?(cam) → feedback+
Assignee | ||
Comment 4•9 years ago
|
||
Patch with test.
Attachment #8649469 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8655680 -
Flags: review?(cam)
Comment 5•9 years ago
|
||
Comment on attachment 8655680 [details] [diff] [review] set line number when re-parsing sheet Review of attachment 8655680 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/inspector/tests/test_parseStyleSheet.html @@ +8,5 @@ > + <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> > + <style> > + body { > + color: red; Indentation looks off here.
Attachment #8655680 -
Flags: review?(cam) → review+
Assignee | ||
Comment 6•9 years ago
|
||
Updated per review.
Attachment #8655680 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8656025 -
Flags: review+
Assignee | ||
Comment 7•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=784259ef3991
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 8•9 years ago
|
||
Removing checkin-needed for now because the patch had a conflict during a rebase.
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Attachment #8656556 -
Flags: review+
Comment 10•9 years ago
|
||
Hi Tom, this patch failed to apply: adding 1195978 to series file renamed 1195978 -> Bug-1195978---set-line-number-when-re-parsing-shee.patch applying Bug-1195978---set-line-number-when-re-parsing-shee.patch patching file layout/style/CSSStyleSheet.cpp Hunk #2 FAILED at 2309 1 out of 2 hunks FAILED -- saving rejects to file layout/style/CSSStyleSheet.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh Bug-1195978---set-line-number-when-re-parsing-shee.patch could you take a look, thanks!
Flags: needinfo?(ttromey)
Keywords: checkin-needed
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #10) > Hi Tom, this patch failed to apply: > > adding 1195978 to series file > renamed 1195978 -> Bug-1195978---set-line-number-when-re-parsing-shee.patch > applying Bug-1195978---set-line-number-when-re-parsing-shee.patch > patching file layout/style/CSSStyleSheet.cpp > Hunk #2 FAILED at 2309 > 1 out of 2 hunks FAILED -- saving rejects to file > layout/style/CSSStyleSheet.cpp.rej > patch failed, unable to continue (try -v) > patch failed, rejects left in working directory > errors during apply, please fix and refresh > Bug-1195978---set-line-number-when-re-parsing-shee.patch > > could you take a look, thanks! Hah, the patch that caused me to do a rebase (comment #9) was since reverted. I'll attach the replacement patch shortly.
Flags: needinfo?(ttromey)
Assignee | ||
Comment 12•9 years ago
|
||
Rebased for the revert.
Attachment #8656556 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8657128 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e33c66fc2ca
Keywords: checkin-needed
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8e33c66fc2ca
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•