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

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8817991 - Flags: review?(jmaher)
(Assignee)

Comment 4

2 years ago
Posted patch Add a stylo reftest (obsolete) — Splinter Review
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).
Attachment #8817995 - Flags: review?(bobbyholley)
: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.
(Assignee)

Comment 6

2 years ago
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.
Attachment #8817991 - Attachment is obsolete: true
Attachment #8817991 - Flags: review?(jmaher)
(Assignee)

Comment 7

2 years ago
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.
(Assignee)

Comment 8

2 years ago
Daniel, could you review the userContent/XBL part?
Attachment #8818014 - Flags: review?(jmaher)
Attachment #8818014 - Flags: review?(dholbert)
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!
Attachment #8818014 - Flags: review?(jmaher) → review+
(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?
Flags: needinfo?(slyu)
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 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+.)
Attachment #8817992 - Flags: review?(dholbert) → review+
(Assignee)

Comment 13

2 years ago
Hmm, all XBL owners/peers are either not accepting review, on PTO, or not reading bugmail anymore :(
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.
Flags: needinfo?(slyu)
(Assignee)

Comment 15

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

Canceling the review per the last comment.
Attachment #8817995 - Attachment is obsolete: true
Attachment #8817995 - Flags: review?(bobbyholley)
(Assignee)

Comment 16

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

Boris, you are the XBL owner. Could you review this?
Attachment #8818014 - Flags: review?(dholbert) → review?(bzbarsky)
Comment on attachment 8818014 [details] [diff] [review]
Add a user stylesheet for reftest, v2

r=me.
Attachment #8818014 - Flags: review?(bzbarsky) → review+
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

2 years ago
Pushed by VYV03354@nifty.ne.jp:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/10c60fcb8aaf
https://hg.mozilla.org/mozilla-central/rev/d70bb3fba851
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Comment 21

2 years ago
Stylo reftests do not have usercss/ yet. Do I have to add it manually?
Flags: needinfo?(slyu)
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.
Flags: needinfo?(slyu)
(Assignee)

Comment 23

2 years ago
(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
Flags: needinfo?(slyu)
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.
Flags: needinfo?(slyu)
You need to log in before you can comment on or make changes to this bug.