Closed Bug 787210 Opened 7 years ago Closed 7 years ago

Modify import-tests.py script to include addition of ahem font, if necessary

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: jwir3, Assigned: jwir3)

References

Details

Attachments

(1 file, 2 obsolete files)

For importing multicolumn tests from the w3c, there is a need for the ahem font. While we have this font installed in our tree, we don't currently support accessing it from the tests in layout/reftests/w3c-tests/submitted. We should modify the import script to do the following:

If the CSS rule:
   font-family: ahem
is detected for a given test, we need to add the following to the reftests.list file:
   HTTP(..)
in front of the reftest line for that test. (We might be able to do this manually, since we will somehow have to specify which tests are failing in our tree on a new import, anyway).

We will also need to add the following to the <style> element for the given test:
   @font-face {
     font-family: ahem;
     src: url('../../../fonts/Ahem.ttf');
   }
these are so that the font can be located by the test requiring them.
dbaron,

I said in the description that:

> (We might be able to do this manually, since we will somehow have to specify which
> tests are failing in our tree on a new import, anyway).

Is this actually true? How should we go about indicating a test should fail (i.e. so that it doesn't create a perma-orange on tbpl)? We could just put it into the reftest.list file, but then, if my understanding is correct, will just get blown away the next time import-tests.py is run. Perhaps we should also add some capability to specify in import-tests.py that a test fails?
Attached patch b787210 (obsolete) — Splinter Review
Attachment #657461 - Flags: review?(dbaron)
Comment on attachment 657461 [details] [diff] [review]
b787210

Two things:
 * instead of a regex for font-family: ahem, I think you should use the requirement flags, since there's an ahem flag documented at http://wiki.csswg.org/test/format#requirement-flags .  Since we already parse the document and build a xml.dom.minidom structure, you should be able to find the meta and parse the flags in a somewhat saner way (using the dom to do everything except parse the contents of the attribute).

 * instead of adding to the end of the style element, I'd suggest adding an entirely separate style element.  (Ideally after the last style element, but if not, then maybe before the first.)

Other than that, I think this looks good.
Attachment #657461 - Flags: review?(dbaron) → review-
Attached patch b787210 (v2) (obsolete) — Splinter Review
Detected flags as per review comments - we now have access to ALL flags, not just whether or not the ahem font is needed.

Also restructured how the <style> element is added. It's now added before the first <style> element. I was going to put it after the last one, as suggested, but this would have required two trips through the document (not really a big deal since we're not too concerned with efficiency, but it also cluttered up the code a bit since we don't know where the last <style> element can occur).
Attachment #657461 - Attachment is obsolete: true
Attachment #662717 - Flags: review?(dbaron)
David:

Quick review ping, if you have time. :)
Blocks: 774358
OS: Linux → All
Hardware: x86_64 → All
Attached patch b787210 (v3)Splinter Review
Found a couple of issues with the previous version of this patch when trying to import multicolumn tests.
Attachment #662717 - Attachment is obsolete: true
Attachment #662717 - Flags: review?(dbaron)
Attachment #665644 - Flags: review?(dbaron)
Comment on attachment 665644 [details] [diff] [review]
b787210 (v3)

>+        if 'ahem' in gTestFlags[testKey]:
>+            test[0:] = ["HTTP(../../..)"] + test
>+        if testKey in gFailList:
>             test[0:] = ["fails"] + test

|test[0:] =| could just be |test =|.


> def main():
>-    global gDestPath, gLog, gTestfiles
>+    global gDestPath, gLog, gTestfiles, gTestFlags

Seems like you should either mention both gTestFlags and gFailList or
neither.


>+def add_test_items(fn, spec):
>+    global gTestFlags, gStyleElements

This global line is not needed.  (gStyleElements doesn't
exist; this function doesn't use gTestFlags.)



copy_and_prefix should only add the ahem style block to the test
itself and not to its support files.

It should also only insert a single style element, not one
for every style element.


Also, it's not clear to me why gTestFlags needs to be a global map -- it
seems like it might have been preferable to just pass around the flags 
for the current test (instead of or in addition to passing around the
test name).  It's ok as is, but I think changing it wouldn't be a bad
idea.


r=dbaron with that
Attachment #665644 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] from comment #7)

> copy_and_prefix should only add the ahem style block to the test
> itself and not to its support files.

Well, there are some instances where the reference file actually needs the ahem font added, as well. Is the reference file considered a support file?
(In reply to Scott Johnson (:jwir3) from comment #8)
> Well, there are some instances where the reference file actually needs the
> ahem font added, as well. Is the reference file considered a support file?

No.  The test and reference each have their own call to map_file.
(In reply to David Baron [:dbaron] from comment #9)
> (In reply to Scott Johnson (:jwir3) from comment #8)
> > Well, there are some instances where the reference file actually needs the
> > ahem font added, as well. Is the reference file considered a support file?
> 
> No.  The test and reference each have their own call to map_file.

Ok, no problem them. I'll make that change.
(In reply to David Baron [:dbaron] from comment #7)
> Also, it's not clear to me why gTestFlags needs to be a global map -- it
> seems like it might have been preferable to just pass around the flags 
> for the current test (instead of or in addition to passing around the
> test name).  It's ok as is, but I think changing it wouldn't be a bad
> idea.

The reason I made it a global variable was so that we didn't have to pass it back up to main(). The problem is that this would complicate the code a bit, since main() calls add_test_items(), which calls map_file() inside of a list constructor. map_file() then performs the resolution of test flags and copying (where the actual logic is located to add the styles necessary for the ahem font). So, it seemed like we'd be bubbling up a variable that's somewhat unrelated to the functions in order to get this behavior. Additionally, we'd have to remove the map_file statements from the list construction, and make them separate variables so that the test flags could be returned from map_file() in order to propagate upwards. 

There's a tradeoff between when the global variable makes things easier and when it makes the code more obfuscated (since it can be changed at unexpected times). I thought, in this case, that it was probably better to utilize the global (although I should mention I made that call before the code became as complicated as it is now, so perhaps that decision should be rethought...)

What do you think?
Why would you need to do more than change map_file to pass the result of load_flags_for to copy_file and copy_support_files?
(In reply to David Baron [:dbaron] from comment #12)
> Why would you need to do more than change map_file to pass the result of
> load_flags_for to copy_file and copy_support_files?

Because main() uses the results of the testFlags map to add the necessary HTTP(../../..) to the reftest.list file. Perhaps this should go in map_file instead...
Ah, ok.  Probably best to leave it in the global, then.

One other thought, though:  if the reference requires Ahem, the flag for that is probably in the test rather than in the reference.  So you might need to add the Ahem block to the reference too when the test requires Ahem.
(In reply to David Baron [:dbaron] from comment #14)
> Ah, ok.  Probably best to leave it in the global, then.
> 
> One other thought, though:  if the reference requires Ahem, the flag for
> that is probably in the test rather than in the reference.  So you might
> need to add the Ahem block to the reference too when the test requires Ahem.

Yes, actually you're totally right. I asked about this in #css on the w3c irc channel, and I was told that the flags _should_ be in the reference file as well, if the reference file needs the ahem font. Since these weren't added to the reference files in the case of the multicol tests, fantasai gave me access to the w3c test hg repository so that I could check in versions of the tests in a personal directory.

So, in other words, I took the approach of modifying the reference files to include the flags if they aren't there, but needed, since this seems to be the agreed-upon approach (rather than adding the <style> block to the reference if the test has the ahem font).
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/ca741411bd17
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.