Created attachment 38922 [details] Implement --without-metrics switch for the Qt port Using run-webkit-tests --strict on the Qt port should compare the mac expected files with the actual files of the Qt port with the font size metrics removed from the files. In this way only the render tree is compared and so the obvious differences can be tracked, and the related bugs can be corrected. The attached patch completes and corrects the implementation of --strict and renames the switch to --without-metrics.
Created attachment 38924 [details] Add Changelog entry to my without-metrics patch
Comment on attachment 38924 [details] Add Changelog entry to my without-metrics patch Indentation issues in this patch.
(In reply to comment #2) > (From update of attachment 38924 [details]) > Indentation issues in this patch. Seems that there are indentation issues in the whole run-webkit-tests file. Should there be the same policy applied to perl scripts as to the cpp files? The check-webkit-style script only checks cpp and h files so I tried to apply the indentation of the context, which seemed to me to be the most consequent in the obviously messy script.
Comment on attachment 38924 [details] Add Changelog entry to my without-metrics patch As discussed over IRC, this patch should not remove --strict and it should just compare between test result and expected of the port (i.e. not mixing it with Qt vs Mac). Also, a name like exclude-metrics or skip-metrics sounds like a better name than without-metrics. r- until the issue is addressed.
Created attachment 39140 [details] Change my patch as discussed on IRC
Comment on attachment 39140 [details] Change my patch as discussed on IRC Good stuff, but can you please split up the --strict fix and the new option into two separate patches? A few comments: > print "\tThis allows the pixel tests to have consistent color values across all machines.\n"; > - > + > if (isPerianInstalled()) { Don't add whitespace-hunks > + if ( !isAppleMacWebKit() && $strictTesting && !$isText ) { No space in "if (something)" > + $excludeMetrics = 1; > + my $expectedFile = "$testDirectory/platform/mac/$expectedFileName"; > + if ( !-s $expectedFile ) { > + $expectedFile = "$testDirectory/$expectedFileName"; > + } There can be several layers of results, see expectedDirectoryForTest() and @platformResultHierarchy > + $modBase = catfile( $simplifiedDir, basename($base)); Space after (
Created attachment 39149 [details] Refactor --strict switch to --ignore-metrics Refactor --strict switch to --ignore-metrics and correct the implementation to make the feature usable on all platforms. This is much simpler and straightforward then my previous patches and it can be easily extended to support advanced comparisons (eg. to other platform's expected files) later.
Comment on attachment 39149 [details] Refactor --strict switch to --ignore-metrics Nice. A few finishing touches: > +my $simpleTag = "simple"; This is no longer needed. > + 'ignore-metrics' => \$ignoreMetrics, Should be postixed with a ! since it's a boolean flag, this will make GetOptions support --no-ignore-metrics, etc > 'strip-editing-callbacks!' => \$stripEditingCallbacks, Like this one > +sub simplify($$) Suggest renaming to stripMetrics() > + my $simpleExpected = $expected; > + my $simpleActual = $actual; No need to assign these, you already have it in $actual and $expected. > + $simpleActual = $actual; And def. no need to re-assign a second time ;) > + $simpleActual =~ s/at \(-?[0-9]+,-?[0-9]+\) *//g; Just use $actual here, it's passed by value > + $simpleExpected =~ s/at \(-?[0-9]+,-?[0-9]+\) *//g; Same, just use $expected directly
Created attachment 39155 [details] Corrected copy-paste issues, and applied suggestions to previous patch
Created attachment 39157 [details] Fixed my previous patch which had an error.
I'll land this tomorrow. Also, I just realized the stripMetrics() function can be simplified to: my ($actual, $expected) = @_; foreach my $result ($actual, $expected) { $result =~ s/at \(-?[0-9]+,-?[0-9]+\) *//g; ... } But I'll do that change before landing.
Thank you very much for the extensive review, suggestions and help. I also noticed this possibility of simplification and I noticed a typo in the Changelog text too (succesfull -> succesful), so I created a fixed patch: http://gist.github.com/182506 ----------- (In reply to comment #11) > I'll land this tomorrow. > > Also, I just realized the stripMetrics() function can be simplified to: > > my ($actual, $expected) = @_; > > foreach my $result ($actual, $expected) { > $result =~ s/at \(-?[0-9]+,-?[0-9]+\) *//g; > ... > } > > But I'll do that change before landing.
Landed in r48148