Closed Bug 1439055 Opened 2 years ago Closed 2 years ago

Upstream ib-split remove-* reftests to WPT.

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Just doing this in pieces so it's easier to review.
Boris, just FYI, I've cleaned up the tests and such so that they match the WPT template better. I don't know if you'd prefer me to just do the minimal changes so the WPT lint passes instead, just let me know.

Also, these tests are slightly weird from WPT since there are refs that are also a test. I think WPT should handle it fine, but let me know if you want me to rename them for clarity. Though I couldn't think of a good name...

Anyway, just let me know if you prefer me to do the quick'n dirty thing instead of upstreaming it in pieces while cleaning up.
Flags: needinfo?(bzbarsky)
Blocks: 1425167
Also not sure I'll have time to clean up _all_ the ib tests, but I think it's preferable to do so if possible :)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> Also, these tests are slightly weird from WPT since there are refs that are
> also a test. I think WPT should handle it fine, but let me know if you want
> me to rename them for clarity. Though I couldn't think of a good name...

You did the <link rel="match">es as described in https://wiki.csswg.org/test/reftest#the-reftest-comparison-links , right?
Flags: needinfo?(emilio)
Yes, just for an example, we have these two lines in the manifest:

  == remove-from-split-inline-1.html remove-from-split-inline-1-ref.html
  == remove-from-split-inline-1-ref.html remove-from-split-inline-1-noib-ref.html

Unless I've messed up, the result of these patches should be:

remove-from-split-inline-1.html:

  <link rel="match" href="remove-from-split-inline-1-ref.html">

remove-from-split-inline-1-ref.html:

  <link rel="match" href="remove-from-split-inline-1-noib-ref.html">

remove-from-split-inline-1-noib-ref.html:

  <!-- nothing -->

Does that sound right? Per the above multiple "match" links means "any reference is valid", so I can't just put two links on remove-from-split-inline-1.html, since otherwise I could hide bugs where remove-from-split-inline-1-ref.html renders differently to remove-from-split-inline-1-noib-ref.html.
Flags: needinfo?(emilio) → needinfo?(dbaron)
Yes, that sounds right.
Flags: needinfo?(dbaron)
[ Triage 2017/02/20: P3 ]
Priority: -- → P3
Comment on attachment 8951820 [details]
Bug 1439055: Tidy the ib-split remove-* tests.

https://reviewboard.mozilla.org/r/221114/#review235502

r=me

::: commit-message-82780:4
(Diff revision 1)
> +Bug 1439055: Tidy the ib-split remove-* tests. r?bz
> +
> +Mostly removing some unneeded tags (<meta name=flags> is no longer needed, and
> +tagging it "dom" didn't seem appropriate to me). Also adding some tags that

https://wiki.csswg.org/test/format#requirement-flags defines this field at least for the CSSWG tests.  "dom" means the test requires support for a script-manipulated DOM to make sense.  Which these tests totally do.

Now I have no problem with just dropping this `<meta>` completely...
Attachment #8951820 - Flags: review?(bzbarsky) → review+
Comment on attachment 8951821 [details]
But 1439055: Use window.onload instead of <body onload>, so that tests map more easily visually to the ref.

https://reviewboard.mozilla.org/r/221116/#review235506

::: layout/reftests/ib-split/remove-from-split-inline-1.html
(Diff revision 1)
>  }
>  </script>
>  <style>
>   body > span { border: 3px solid blue }
>  </style>
> -<body onload='doit()'>

I actually find this test harder to read after the `<body>` tag is removed.  You have to mentally reconstruct the DOM to see what the selectors are really going to match.

It would be better to just consistently include the `<body>` in test and reference, imo, for all of these.
Attachment #8951821 - Flags: review?(bzbarsky) → review+
Comment on attachment 8951822 [details]
Bug 1439055: Export the ib-split remove-* reftests.

https://reviewboard.mozilla.org/r/221118/#review235514

Thank you for doing this, and doing it in easily-digestible pieces!
Attachment #8951822 - Flags: review?(bzbarsky) → review+
Flags: needinfo?(bzbarsky)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/909aa82e265e
Tidy the ib-split remove-* tests. r=bz
https://hg.mozilla.org/integration/autoland/rev/e8fd06bdea01
Add a few <body> tags to match better test and references. r=bz
https://hg.mozilla.org/integration/autoland/rev/7ba4b70ba892
Export the ib-split remove-* reftests. r=bz
You need to log in before you can comment on or make changes to this bug.