Neerja Pancholi[:neerja] Assignee
	Description • 8 years ago

This bug was created as a sub part of the fix for Bug 698783 and represents a fix for part 2(b) from David Baron's comment on the original bug.

Copying David Baron's comment here for Bug 698783 ->

So as we discussed earlier today, I think there are a few things to do here that should be separated into a few different patches:

 (1) we should get rid of the _moz_ in the internal constants for these properties, anytime.  That's just code cleanup.

 (2) we should import the multicol reftests from the CSSWG test repository.  This has a few steps:
     (a) rerun layout/reftests/w3c-css/import-tests.py , and if there are any changes, make a patch with them and push it to try.
     (b) then, add importing of the multicol tests, and do the same.  This depends on (2a).
     (c) evaluate the failures of the multicol tests added in (b) and figure out if we're doing worse than other browsers that have unprefixed.  This depends on (2b).

 (3) write a patch to layout/style/nsCSSPropList.h to remove the -moz- prefixes there, but add them back as aliases in layout/style/nsCSSPropAliasList.h, and then make the same changes to layout/style/test/property_database.js and any other tests that show failures (probably a small number).  This depends on (2c) to decide whether we're ready to do it.

 (4) Substitute all of the rest of the tree to remove the -moz- prefixes for these properties.  This depends on (3).

 (5) Remove the aliases introduced in (3).  This depends on (3) and (4), and shouldn't be done until after we've successfully shipped (3).

All of these (1, 2a, 2b, 2c, 3, 4, and 5) could be different bugs, although it's ok if 2a and 2b are in a single bug, and it's also ok if 3 and 4 are in a single bug, or even 1, 3, and 4.  But things that happen at different times should be in separate bugs even if they could be in the same bug if done at the same time.

	David Baron :dbaron:
	Comment 2 • 8 years ago
mozreview-review

Comment on attachment 8781291 [details]
Bug 1295261 - Added importing of multicol tests from CSSWG with py script and checking in these new tests + script

https://reviewboard.mozilla.org/r/71686/#review69242

r=dbaron with these comments

::: layout/reftests/w3c-css/import-tests.py:182
(Diff revision 1)
> +def is_xml(fn):
> +    return fn.endswith(".xht") or fn.endswith(".xml")

So the overall distribution of file extensions in the CSSWG test repository is (for any occurring >=5 times):

  15277 xht
   8151 html
   2512 png
   1816 htm
   1438 glif
   1224 xml
    727 css
    302 woff2
    251 xhtml
    233 svg
     62 ttf
     57 js
     49 txt
     44 gif
     29 list
     29 jpg
     27 py
     20 htaccess
     14 tmpl
     13 xml-removed
     13 TTF
     12 woff
     12 pm
     12 otf
     12 md
      9 sh
      5 pl
      5 json
      5 ico
      5 cur


Given that, you also need to make this allow
  or fn.endswith(".xhtml") or fn.endswith(".svg")
, but otherwise I think it's fine.

::: layout/reftests/w3c-css/import-tests.py:220
(Diff revision 1)
> +        if str(link.getAttribute("href")) != "":
> -        arr.append(os.path.join(os.path.dirname(fn), str(link.getAttribute("href"))))
> +            arr.append(os.path.join(os.path.dirname(fn), str(link.getAttribute("href"))))

This should at least have an "else" that prints a warning to the import log.

	David Baron :dbaron:
	Comment 3 • 8 years ago

And you'll also need to update the failures.list to reflect the failures and rerun the import.

	Neerja Pancholi[:neerja] Assignee
	Comment 5 • 8 years ago

Created a non-blocking bug for this bug -> Bug 1299006

	David Baron :dbaron:
	Comment 7 • 8 years ago

The revision 3 patch in this bug doesn't look at all like revisions 1 and 2.

	David Baron :dbaron:
	Comment 8 • 8 years ago

It looks like mozreview overwrote the old patch when you pushed a second one associated with this bug without also pushing the first one at the same time.

	David Baron :dbaron:
	Comment 9 • 8 years ago

To fix that, you probably want to push the first patch alone (to get mozreview to overwrite back the other way, to keep the review state in the right place), and then push both.

	David Baron :dbaron:
	Comment 12 • 8 years ago
mozreview-review

Comment on attachment 8786454 [details]
Bug 1295261 - Changed failures.list and reran import script for multicol tests.

https://reviewboard.mozilla.org/r/75374/#review73304

	David Baron :dbaron:
	Comment 19 • 8 years ago
mozreview-review

Comment on attachment 8787372 [details]
Bug 1295261 - Reimport multicol tests after applying fix for import-tests.py (Bug 1299012)

https://reviewboard.mozilla.org/r/76158/#review76046

::: layout/reftests/w3c-css/received/css-multicol-1/multicol-columns-003.xht:7
(Diff revision 1)
> -  <link rel="help" href="http://www.w3.org/TR/css3-multicol/#the-number-and-width-of--moz-columns" title="3. The number and width of -moz-columns" />
> -  <link rel="match" href="multicol--moz-columns-001-ref.xht" />
> +  <link rel="help" href="http://www.w3.org/TR/css3-multicol/#the-number-and-width-of-moz-columns" title="3. The number and width of -moz-columns" />
> +  <link rel="match" href="multicol-moz-columns-001-ref.xht" />

So things like this still shouldn't have the "moz-" in them at all, which I think my proposed fix in bug 1299012 should help with.

	David Baron :dbaron:
	Comment 20 • 8 years ago
mozreview-review

Comment on attachment 8788633 [details]
Bug 1295261 - Changed failures.list and reran import script after applying fix for import-tests.py (Bug 1299012)

https://reviewboard.mozilla.org/r/77046/#review76048

::: layout/reftests/w3c-css/failures.list:114
(Diff revision 1)
> -fails css-multicol-1/multicol-rule-fraction-002.xht
> +fails-if(OSX||winWidget) css-multicol-1/multicol-rule-fraction-002.xht
>  fails css-multicol-1/multicol-rule-fraction-003.xht
>  fuzzy(106,354) css-multicol-1/multicol-rule-groove-000.xht
>  fuzzy(94,256) css-multicol-1/multicol-rule-hidden-000.xht
>  fuzzy(106,354) css-multicol-1/multicol-rule-inset-000.xht
> -fails css-multicol-1/multicol-rule-large-001.xht
> +random css-multicol-1/multicol-rule-large-001.xht

Why is this marked "random"?  That generally requires more justification, since it makes the test useless since it will never again report either an unexpected pass or an unexpected fail.


(fuzzy has some of the same problems, which I hope I'll talk somebody into fixing in bug 1252361)

::: layout/reftests/w3c-css/failures.list
(Diff revision 1)
>  fuzzy(225,1060) css-multicol-1/multicol-width-invalid-001.xht
>  fuzzy(225,1060) css-multicol-1/multicol-width-large-002.xht
>  fails css-multicol-1/multicol-zero-height-001.xht
>  fails css-multicol-1/multicol-nested-column-rule-001.xht
>  fuzzy(94,256) css-multicol-1/multicol-rule-none-000.xht
> -fails css-multicol-1/multicol-rule-samelength-001.xht

Why the removal?

::: layout/reftests/w3c-css/received/reftest.list:137
(Diff revision 1)
> -fails HTTP(../../..) == css-multicol-1/multicol-rule-fraction-002.xht css-multicol-1/multicol-rule-fraction-002-ref.xht
> +fails-if(OSX||winWidget) HTTP(../../..) == css-multicol-1/multicol-rule-fraction-002.xht css-multicol-1/multicol-rule-fraction-002-ref.xht
>  fails HTTP(../../..) == css-multicol-1/multicol-rule-fraction-003.xht css-multicol-1/multicol-rule-fraction-3-ref.xht
>  fuzzy(106,354) HTTP(../../..) == css-multicol-1/multicol-rule-groove-000.xht css-multicol-1/multicol-rule-groove-000-ref.xht
>  fuzzy(94,256) HTTP(../../..) == css-multicol-1/multicol-rule-hidden-000.xht css-multicol-1/multicol-rule-hidden-000-ref.xht
>  fuzzy(106,354) HTTP(../../..) == css-multicol-1/multicol-rule-inset-000.xht css-multicol-1/multicol-rule-ridge-000-ref.xht
> -fails HTTP(../../..) == css-multicol-1/multicol-rule-large-001.xht css-multicol-1/multicol-rule-large-001-ref.xht
> +random HTTP(../../..) == css-multicol-1/multicol-rule-large-001.xht css-multicol-1/multicol-rule-large-001-ref.xht

Again, I wouldn't expect to see "random" here.

	Neerja Pancholi[:neerja] Assignee
	Comment 21 • 8 years ago
mozreview-review-reply

Comment on attachment 8788633 [details]
Bug 1295261 - Changed failures.list and reran import script after applying fix for import-tests.py (Bug 1299012)

https://reviewboard.mozilla.org/r/77046/#review76048

> Why is this marked "random"?  That generally requires more justification, since it makes the test useless since it will never again report either an unexpected pass or an unexpected fail.
> 
> 
> (fuzzy has some of the same problems, which I hope I'll talk somebody into fixing in bug 1252361)

This test seemed to pass/fail randomly on different try runs that's why I marked it as random. Although, on the latest try run everything seems to be fine so I removed the random annotation.

> Why the removal?

Was removed because this test started passing after fix for Bug 1299012 was applied.

> Again, I wouldn't expect to see "random" here.

Changed the random annotation back to "fails".

	David Baron :dbaron:
	Comment 28 • 8 years ago
mozreview-review

Comment on attachment 8787372 [details]
Bug 1295261 - Reimport multicol tests after applying fix for import-tests.py (Bug 1299012)

https://reviewboard.mozilla.org/r/76158/#review80112

	David Baron :dbaron:
	Comment 29 • 8 years ago
mozreview-review

Comment on attachment 8788633 [details]
Bug 1295261 - Changed failures.list and reran import script after applying fix for import-tests.py (Bug 1299012)

https://reviewboard.mozilla.org/r/77046/#review80116

r=dbaron, although I wonder if some of the fails-if() should really be fuzzy-if()?  Do they represent failures of an essential characteristic of the test or just slight pixel differences?  That said, that could be cleaned up in a later patch in a different bug.

I think this bug has gotten pretty confusing, though.  Bug 1299012 should really have been a new revision of an existing patch on this existing bug, rather than a separate bug.  Then the effects of the changes in the importing could have been visible through the changes in the patch that actually has the import (or at least would be if I could believe interdiffs in MozReview).  It would be best to get this landed as soon as possible (although preferably addressing the review comments there, although honestly I'd be ok landing it and addressing the review comment in a followup bug just to make the review process less confusing) and fix up further issues in followup bugs.

	Neerja Pancholi[:neerja] Assignee
	Comment 34 • 8 years ago

I pushed some minor changes to failures.list after dbaron's r+. I re-based these patches and then proceeded with a try run. 

The latest run looks good: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c98453efad5&selectedJob=28354254

Ready for checkin.
(Daniel was kind enough to volunteer to autoland it for me so not setting the check-in flag)

	Pulsebot
	Comment 35 • 8 years ago

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a4c6bd0c3953
Added importing of multicol tests from CSSWG with py script and checking in these new tests + script r=dbaron
https://hg.mozilla.org/integration/autoland/rev/3471ca76ffb5
Changed failures.list and reran import script for multicol tests. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/90fffe47e10a
Reimport multicol tests after applying fix for import-tests.py (Bug 1299012) r=dbaron
https://hg.mozilla.org/integration/autoland/rev/e4dee063ef64
Changed failures.list and reran import script after applying fix for import-tests.py (Bug 1299012) r=dbaron

	Phil Ringnalda (:philor)
	Comment 36 • 8 years ago

Backed out in https://hg.mozilla.org/integration/autoland/rev/b7b705333876 for Android reftest failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=4331216&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=4331260&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=4331265&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=4333088&repo=autoland

	Neerja Pancholi[:neerja] Assignee
	Comment 41 • 8 years ago

Checked the Android tests and fixed with latest push.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f92d588fd520&selectedJob=28428494

	Ryan VanderMeulen [:RyanVM]
	Comment 42 • 8 years ago

Looks like you still need some Android reftest fuzz. You've got the some of the same failures that you were previously backed out for.

	Daniel Holbert [:dholbert]
	Comment 43 • 8 years ago

The reftest failures in comment 41 look like they *technically* have nothing to do with this bug.  The failures are subtle fuzziness on some corner pixels of widgets, in these two tests:
Android opt R5 / Debug R6:
  https://dxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/363858-1.html
  max difference: 4, number of differing pixels: 2
Android opt R7/ Debug 13:
 https://dxr.mozilla.org/mozilla-central/source/layout/reftests/css-disabled/select/select-fieldset-1.html
 max difference: 12, number of differing pixels: 1

Nothing in this bug should affect the rendering of those tests.  However, the failures do seem to be reliably "caused" by the patches here. (Comment 41's try run, as well as the earlier landing, https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e4dee063ef64&selectedJob=4332975 )

The only way I can imagine this bug's patches triggering these failures would be by virtue of shifting the reftest "bucketing", which occasionally does expose subtle rendering failures.

Neerja, let's add one final patch here to mark these two tests as fuzzy-if(Android,4,2) and fuzzy-if(Android,12,1), with a commit message saying something like "Annotate some minor android reftest fuzziness triggered by test rebucketing".  That'll merit one final (hopefully!) try run, and then we can hopefully get this closed out for good!

	Daniel Holbert [:dholbert]
	Comment 44 • 8 years ago

Sorry, I meant "Debug R13" and "Debug R19" above.  (Not Debug R6 and Debug R13)

ALSO: I'm just noticing that there's one third test that'll also need the same fuzzy treatment:
Android Debug R26
https://dxr.mozilla.org/mozilla-central/source/layout/reftests/font-inflation/threshold-select-combobox-contents-under-1.html
  max difference: 4, number of differing pixels: 2

That one will want a fuzzy-if(Android,4,2) annotation as well.

	Neerja Pancholi[:neerja] Assignee
	Comment 46 • 8 years ago

Added the annotations you mentioned and submitted a patch. 
New test triggered:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d71ebd084dee

Clearing needinfo.
Thanks!

	Daniel Holbert [:dholbert]
	Comment 47 • 8 years ago
mozreview-review

Comment on attachment 8797362 [details]
Bug 1295261 - Annotate some minor android reftest fuzziness triggered by test rebucketing

https://reviewboard.mozilla.org/r/82950/#review81616

r=me, assuming the Try run looks good. Thanks!

	Neerja Pancholi[:neerja] Assignee
	Comment 48 • 8 years ago

Thanks Daniel! :)

	Daniel Holbert [:dholbert]
	Comment 49 • 8 years ago

Sure! Try looks good -- autolanding!

	Pulsebot
	Comment 50 • 8 years ago

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f61e87d6ab4
Added importing of multicol tests from CSSWG with py script and checking in these new tests + script r=dbaron
https://hg.mozilla.org/integration/autoland/rev/c1dc69b2f178
Changed failures.list and reran import script for multicol tests. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/af04c77a8654
Reimport multicol tests after applying fix for import-tests.py (Bug 1299012) r=dbaron
https://hg.mozilla.org/integration/autoland/rev/851aae110458
Changed failures.list and reran import script after applying fix for import-tests.py (Bug 1299012) r=dbaron
https://hg.mozilla.org/integration/autoland/rev/97a9b83bc394
Annotate some minor android reftest fuzziness triggered by test rebucketing r=dholbert

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 51 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1f61e87d6ab4
https://hg.mozilla.org/mozilla-central/rev/c1dc69b2f178
https://hg.mozilla.org/mozilla-central/rev/af04c77a8654
https://hg.mozilla.org/mozilla-central/rev/851aae110458
https://hg.mozilla.org/mozilla-central/rev/97a9b83bc394

	Gérard Talbot
	Comment 52 • 8 years ago

Is bug 774358 a DUPLICATE of this bug? Just asking.

	Daniel Holbert [:dholbert]
	Comment 54 • 8 years ago

Yes -- thanks. Marked as such.