652123 - Re-enable TestChromeMargin.cpp

Status: RESOLVED WONTFIX

(Core :: Widget: Win32, defect, P3)

Description

[Justin Lebar (not reading bugmail)]

I'm going to disable this test as part of bug 647273.  But we should fix the test and re-enable it.

Comment 1

[Ray Kraesig [:rkraesig]]

Attachment: Bug 652123 - [1/4] Convert TestChromeMargin.cpp to gtest r=emilio,smaug

Convert TestChromeMargin.cpp to use gtest, resolving its old linking
issues. (This also means that it can be moved into a directory nearer
the code it's actually testing.)

One test, which did not compile under a modern warning-flagset, has been
split into two due to unclear semantics -- revealing that this may have
concealed a bug. For now, the questionable test remains active, with
only a warning comment.

Modulo the test-splitting above, this is a minimal patch to get the
tests up and running; modernization and cleanup will follow.

Comment 2

[Ray Kraesig [:rkraesig]]

Attachment: Bug 652123 - [2/4] General cleanup and modernization r=emilio,smaug

Transform existing code to meet modern style guidelines and use a more
modern dialect of C++.

No functional changes, except possibly for some minor differences in
output formatting.

Depends on D142489

Comment 3

[Ray Kraesig [:rkraesig]]

Attachment: Bug 652123 - [3/4] Rework test structure r=emilio,smaug

The existing test structure conflated two kinds of failure:

• the test data doesn't parse
• the test data parses, and the values are different

These could be separated, but testing the latter is of marginal
(cough) utility -- the PASS tests are probably sufficient for that.
Remove them, and stop providing unused data to the FAIL tests.

Doing so shows that another test was incorrectly(?) succeeding: "4.6"
was being parsed as "4". This test has also been marked, but remains
active.

Depends on D142490

Comment 4

[Ray Kraesig [:rkraesig]]

Attachment: Bug 652123 - [4/4] Modernization (redux) r=emilio,smaug

Since should-fail tests no longer take values, replace the C-style
initializer lists with constexpr factory functions that enforce their
absence.

No functional changes.

Depends on D142491

Comment 5

[Ray Kraesig [:rkraesig]]

The chromemargin attribute itself was always marginal (*cough*), and has been removed entirely in 1930292.

Abandoning patches accordingly.

---

I'd completely forgotten about this one. On review, fortunately, it looks like nothing else has slipped through the same crack that it did. (Except possibly a minor fix associated with bug 1837008, which isn't entirely a valid bug anyway, and can be moved.)