1631941 - LogicalSides should have a DEBUG-only mWritingMode for assertions

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

While reviewing bug 1626129 I noticed that:

• it's not particularly clear what writing mode GetLogicalSkipSides() is in -- is it the frame's writing mode, or its parent's / containing block's?
• nsIFrame::ContentSize(mozilla::WritingMode) appears to produce incorrect results if its aWritingMode argument is not the writing mode that GetLogicalSkipSides() is in, because it applies the skip sides to a LogicalMargin in aWritingMode. Most, but not all, of the callers appear to pass the frame's writing mode to this method.
• LogicalSides itself doesn't carry an mWritingMode so that methods like LogicalMargin::ApplySkipSides can have CHECK_WRITING_MODE assertions to verify that the correct conversions (if needed) have been done.

Comment 1

[Jonathan Kew [:jfkthame]]

Yes, this sounds like something we should have, to help catch any incorrect usage.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Here's a try push.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Further evidence for bug 1631947 - this has a bug that showed up in exactly one test in our test suite:
REFTEST TEST-UNEXPECTED-FAIL | layout/base/crashtests/365909-2.xhtml | assertion count 2 is more than expected 0 assertions

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Here's another try run.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Bug 1631941 - Make nsIFrame::ContentSize and nsIFrame::SynthesizeBaselineBOffsetFromContentBox apply skip sides correctly for orthogonal writing modes.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Bug 1631941 - Add DEBUG-only mWritingMode to mozilla::LogicalSides.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Bug 1631941 - Add writing mode assertions for mozilla::LogicalSides.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4c1d7dcca50
Make nsIFrame::ContentSize and nsIFrame::SynthesizeBaselineBOffsetFromContentBox apply skip sides correctly for orthogonal writing modes. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/b0b425ffb6e9
Add DEBUG-only mWritingMode to mozilla::LogicalSides. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/17391a8c2a05
Add writing mode assertions for mozilla::LogicalSides. r=jfkthame

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, I believe this patch should have made the behavior consistent with the behavior we had for almost all things -- but according to the spec that behavior is wrong, for which I filed bug 1632228.

Comment 10

[Stefan Hindli [:stefan_hindli]]

Backed out 3 changesets (Bug 1631941) for causing valgrind bustages in /builds/worker/workspace/obj-build/dist/include/mozilla/WritingModes CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=298863606&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=4283912e2318f294c400670ceb8fe0af94eab28d

Valgrind: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=298866385&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified
Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=298866385&repo=autoland&lineNumber=16300

Build bustages: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=298863621&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified
Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=298863621&repo=autoland&lineNumber=18419

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I thought I queued this for relanding this morning, but apparently it didn't work, so I did so again just now.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91ac0b7e571b
Make nsIFrame::ContentSize and nsIFrame::SynthesizeBaselineBOffsetFromContentBox apply skip sides correctly for orthogonal writing modes. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/6a53d6f1ec95
Add DEBUG-only mWritingMode to mozilla::LogicalSides. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/8a7e21f2b5f9
Add writing mode assertions for mozilla::LogicalSides. r=jfkthame

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For the record, this was the bustage fix so that it would compile correctly on non-DEBUG builds.

Comment 14

[Stefan Hindli [:stefan_hindli]]

Backed out 3 changesets (Bug 1631941) for mass build bustages CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=15f28d419ea6ee19c8e3a8f6f2cd7017ef42c4c5

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=062f5214f39368395325db095b9fa4f27c61af57&selectedJob=298934801

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=298934801&repo=autoland&lineNumber=19977

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Here's the fix for the Windows build bustage.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6120b8d2df59
Make nsIFrame::ContentSize and nsIFrame::SynthesizeBaselineBOffsetFromContentBox apply skip sides correctly for orthogonal writing modes. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/ff5ce5fd70ee
Add DEBUG-only mWritingMode to mozilla::LogicalSides. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/03e0159cd77f
Add writing mode assertions for mozilla::LogicalSides. r=jfkthame

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/6120b8d2df59
https://hg.mozilla.org/mozilla-central/rev/ff5ce5fd70ee
https://hg.mozilla.org/mozilla-central/rev/03e0159cd77f