Closed Bug 1159729 Opened 5 years ago Closed 5 years ago

Use of the wrong variable (aoffset instead of offset) in nsCSSRendering::GetTextDecorationRectInternal ?

Categories

(Core :: Layout, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: Sylvestre, Assigned: jfkthame)

References

Details

(Keywords: clang-analyzer, regression)

Attachments

(2 files)

scan-build (based on clang-analyzer) found that the offset variable is useless.
https://people.mozilla.org/~sledru/reports/fx-scan-build/report-nsCSSRendering.cpp-GetTextDecorationRectInternal-96-1.html#EndPath

This shows that something is wrong.
Either the whole switch case is useless

Or this code should use offset instead of aOffset
r.y = baseline + floor(aOffset + 0.5);

https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp?from=layout/base/nsCSSRendering.cpp#4522
Jonathan, I see that you touched at the end of this method. Does it ring a bell? thanks
Flags: needinfo?(jfkthame)
> r.y = baseline + floor(aOffset + 0.5);

This line used to be:

  r.y = baseline - floor(offset + 0.5);

until https://hg.mozilla.org/mozilla-central/rev/9c8f4a869327

Sounds like we need regression tests here....
Blocks: 1074735
Keywords: regression
Yes, looks like this should still be |offset| rather than |aOffset| (and likewise a few lines earlier). Which does indicate a gap in test coverage, given that nothing went orange.

I'll run a fix through tryserver to check for anything unexpected, and see if I can come up with a test that breaks appropriately.
Flags: needinfo?(jfkthame)
This is clearly what was intended here. I'm experimenting with possible tests that could have detected this, though it's a bit of a pain because the effect (if any) of this code is dependent on font metrics and platform-specific details of how we interpret them. Still, we should be able to have a test on at least some platforms.
Attachment #8599438 - Flags: review?(smontagu)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Summary: Useless variable in nsCSSRendering::GetTextDecorationRectInternal ? → Use of the wrong variable (aoffset instead of offset) in nsCSSRendering::GetTextDecorationRectInternal ?
Here's a test that fails on current trunk due to this bug (try run in comment 5) because it fails to lift the overline clear of the text, and passes once it's fixed (comment 6).
Attachment #8599706 - Flags: review?(smontagu)
Attachment #8599438 - Flags: review?(smontagu) → review+
Attachment #8599706 - Flags: review?(smontagu) → review+
https://hg.mozilla.org/mozilla-central/rev/e03d014c257f
https://hg.mozilla.org/mozilla-central/rev/f4be5f25b35a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.