1322634 - Add an automated test to ensure userContent.css will not break again

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Add a user stylesheet for reftest

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Add a reftest to make sure userContent.css will not break again

Comment 3

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f871d17103c6c3909cb2b9404c4046fb005f42f3

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Add a stylo reftest

I don't know whether this already works with stylo. If not, feel free to mark fail (and file a bug to track the fix).

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

:emk, I am not sure of the history and reason for this patch.  Why I want to know more is the binding.xml appears to be loaded for all reftests and has more than a few lines of code.  Does this need to be a reftest?  could it be part of the reftests run inside of mochitest or is there a better reason to include this in reftest proper?

from a perspective of setting up the code properly in reftest, I have no problem with the patch, but I would want to understand it a lot more or get someone who is familiar with the oddities of xbl to look over that part of the code.

Comment 6

[Masatoshi Kimura [:emk]]

Comment on attachment 8817991 [details] [diff] [review]
Add a user stylesheet for reftest

Sorry, wrong file attached. bindings.xml should be much smaller. I'll attach a new patch.

Comment 7

[Masatoshi Kimura [:emk]]

I included a binding test to prevent a regression like bug 1232903.

(In reply to Joel Maher ( :jmaher) from comment #5)
> could it be
> part of the reftests run inside of mochitest or is there a better reason to
> include this in reftest proper?

Is it possible? userContent.css will be loaded only once at process startup.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Add a user stylesheet for reftest, v2

Daniel, could you review the userContent/XBL part?

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8818014 [details] [diff] [review]
Add a user stylesheet for reftest, v2

Review of attachment 8818014 [details] [diff] [review]:
-----------------------------------------------------------------

thanks, this looks better!

Comment 10

[Bobby Holley (:bholley)]

(In reply to Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953) from comment #4)
> Created attachment 8817995 [details] [diff] [review]
> Add a stylo reftest
> 
> I don't know whether this already works with stylo. If not, feel free to
> mark fail (and file a bug to track the fix).

My guess is that it's probably better just to skip this and pull it in next time we update our test lists. Shing, can you confirm that the script will notice new directories when we run it again?

Comment 11

[Daniel Holbert [:dholbert]]

This looks pretty simple, but I also don't really know XBL, so this isn't a one-click r+ for me -- I need to do a bit of research to effectively review that part.

I'm on PTO and mostly-AFK until next week (and it looks like this isn't super-time-critical), so I'll take a look then. (Alternately, feel free to find another reviewer to perform this function. :))

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8817992 [details] [diff] [review]
Add a reftest to make sure userContent.css will not break again

Review of attachment 8817992 [details] [diff] [review]:
-----------------------------------------------------------------

(Well, this part I can easily r+.)

Comment 13

[Masatoshi Kimura [:emk]]

Hmm, all XBL owners/peers are either not accepting review, on PTO, or not reading bugmail anymore :(

Comment 14

[Shing Lyu [:shinglyu]]

No, you don't need to manually update the stylo reftest. I'll pull the change next time I update the reftest list. I'll keep that on my radar.

Comment 15

[Masatoshi Kimura [:emk]]

Comment on attachment 8817995 [details] [diff] [review]
Add a stylo reftest

Canceling the review per the last comment.

Comment 16

[Masatoshi Kimura [:emk]]

Comment on attachment 8818014 [details] [diff] [review]
Add a user stylesheet for reftest, v2

Boris, you are the XBL owner. Could you review this?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8818014 [details] [diff] [review]
Add a user stylesheet for reftest, v2

r=me.

Comment 18

[Boris Zbarsky [:bzbarsky]]

That said, for the XBL test it might be a good idea to use reftest-wait and remove that in the XBL constructor, because I'm not sure we really guarantee that the XBL load will block the load event.  We might, but I wouldn't stake too much on it.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10c60fcb8aaf
Add a user stylesheet for reftest. r=jmaher,bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/d70bb3fba851
Add a reftest to make sure userContent.css will not break again. r=dholbert

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/10c60fcb8aaf
https://hg.mozilla.org/mozilla-central/rev/d70bb3fba851

Comment 21

[Masatoshi Kimura [:emk]]

Stylo reftests do not have usercss/ yet. Do I have to add it manually?

Comment 22

[Shing Lyu [:shinglyu]]

Thanks for asking, we'll pull new tests from non-stylo reftest into stylo reftest periodically, so you don't need to do that yourself.

Comment 23

[Masatoshi Kimura [:emk]]

(In reply to Shing Lyu [:shinglyu] from comment #22)
> Thanks for asking, we'll pull new tests from non-stylo reftest into stylo
> reftest periodically, so you don't need to do that yourself.

But is the automated pick working correctly? layout/reftests/reftest-stylo.list has no update since Jan 18[1] while other files got more recent update (for example [2]). Should I file a bug?

[1] https://hg.mozilla.org/mozilla-central/filelog/0eef1d5a39366059677c6d7944cfe8a97265a011/layout/reftests/reftest-stylo.list
[2] https://hg.mozilla.org/mozilla-central/filelog/0eef1d5a39366059677c6d7944cfe8a97265a011/layout/reftests/w3c-css/received/reftest-stylo.list

Comment 24

[Cameron McCormack (:heycam)]

I think probably we won't re-run the script that generates the reftest-stylo.list files, since we've been doing a bunch of manual editing of them since the last run of the script.  But hopefully soon (within the next month or two?) we will be removing them and running stylo reftest jobs against the standard manifest files.