Closed
Bug 1156227
Opened 10 years ago
Closed 10 years ago
<br> trips assert about suppressing line breaks in ruby
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox40 | --- | affected |
People
(Reporter: jruderman, Assigned: xidorn)
References
Details
(Keywords: assertion, testcase)
Attachments
(3 files)
138 bytes,
text/html
|
Details | |
5.28 KB,
text/plain
|
Details | |
4.37 KB,
patch
|
Details | Diff | Splinter Review |
Assertion failure: !(0 != ((reflowStatus) & 0x0100)) && !pushedFrame (Any line break inside ruby box should has been suppressed), at layout/generic/nsRubyBaseContainerFrame.cpp:777
(Btw, there's a typo in the assertion: 'has' should be 'have')
Reporter | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee: nobody → quanxunzhen
Attachment #8595633 -
Flags: review?(dholbert)
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8595633 [details] [diff] [review]
patch
I'd like to fix this with bug 1157011. The testcase in that bug should certainly trigger this assertion if it is not crashed first.
Attachment #8595633 -
Flags: review?(dholbert)
Comment 4•10 years ago
|
||
For future reference, RE this bug & bug 1156222:
It's helpful, for review purposes, if you could briefly explain what's going wrong & causing the assertion-failure (or whatever the bug is) in a bug comment, before (or around when) you request review.
It's not always exactly clear to a reviewer (or to someone looking at this bug in the future) how the patch maps to the actual bug symptoms, and it takes a bit of extra time to infer that when reviewing. In my case, I'm unfamiliar with the assertion in comment 0, and I haven't looked at BRFrame recently, and I wasn't aware that ruby had this line-break-suppressing behavior [though it makes sense]. I'm not suggesting that you cover all of that content preemptively, but at least a quick summary of what's causing the problem would be helpful in getting the reviewer on the right track to understanding the patch quickly.
e.g. something like "Ah, so: We have a check to see if the BR frame is inside of ruby content (via ShouldSuppressLineBreak), and we suppress the line-break if so. But with this testcase, the BR frame's custom display value is making that check fail, so we break when we shouldn't."
Comment 5•10 years ago
|
||
Er, sorry, meant to post that on bug 1157011. Anyway, just wanted to give you a heads-up, for faster/easier/happier reviews. :)
Assignee | ||
Comment 6•10 years ago
|
||
Sorry about that. I should have noticed that you are probably not familiar with the code here.
Comment 7•10 years ago
|
||
No worries -- though, even when the reviewer's more familiar with the code than I was in this case, I'd still suggest including a brief summary of the issue that you've identified & are fixing, even if it's just a paraphrased English version of the code in the patch. It helps jog the memory, and it clarifies the point of the patch -- and it's also nice for the benefit of other people looking at the bug to have a chance at figuring out what's going on. :)
Assignee | ||
Comment 8•10 years ago
|
||
Should have been fixed by bug 1157011.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•