Bug 60236 - full-screen-stacking-context-expected.txt should be under platform/mac
Summary: full-screen-stacking-context-expected.txt should be under platform/mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks: 60237
  Show dependency treegraph
 
Reported: 2011-05-04 18:33 PDT by Hajime Morrita
Modified: 2011-05-05 17:23 PDT (History)
7 users (show)

See Also:


Attachments
Patch (728 bytes, patch)
2011-05-04 18:38 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (5.38 KB, patch)
2011-05-05 11:32 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (11.10 KB, patch)
2011-05-05 13:18 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-05-04 18:33:22 PDT
A test result commited at r85811 contains the layout dump, which is platform dependent.
We should move it to platform directly.
Comment 1 Hajime Morrita 2011-05-04 18:38:04 PDT
Created attachment 92356 [details]
Patch
Comment 2 Kent Tamura 2011-05-04 20:42:03 PDT
Comment on attachment 92356 [details]
Patch

The patch has only ChangeLog.
Comment 3 Hajime Morrita 2011-05-04 21:04:21 PDT
Hi Kent-san, thank you for taking a look!

(In reply to comment #2)
> (From update of attachment 92356 [details])
> The patch has only ChangeLog.
Hmm. It seems webkit-patch post doesn't support simple file move...
I didn't know that.
Comment 4 Jer Noble 2011-05-04 23:11:57 PDT
(In reply to comment #0)
> A test result commited at r85811 contains the layout dump, which is platform dependent.
> We should move it to platform directly.

Why is the layout dump platform dependant?  The pixel test, perhaps, but the layout itself should be platform agnostic.
Comment 5 Jer Noble 2011-05-04 23:15:26 PDT
Note that other tests in the LayoutTests/fullscreen/ directory have pixel test results in LayoutTests/platform/mac/fullscreen/ and layout dumps within LayoutTests/fullscreen/, notably the full-screen-zIndex.html test.
Comment 6 Tony Chang 2011-05-05 10:41:46 PDT
(In reply to comment #4)
> Why is the layout dump platform dependant?  The pixel test, perhaps, but the layout itself should be platform agnostic.

Any test that has text output will likely differ by platforms since the font metrics are different.

For example, full-screen-zIndex.html has separate -expected.txt results for chromium-win, chromium-linux and gtk.  The test is skipped on win and qt.
Comment 7 Jer Noble 2011-05-05 11:04:24 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > Why is the layout dump platform dependant?  The pixel test, perhaps, but the layout itself should be platform agnostic.
> 
> Any test that has text output will likely differ by platforms since the font metrics are different.

Ah, I see, so the bounds of each text line dumped will be different.  Should these tests be re-written to remove the text portions?
Comment 8 Tony Chang 2011-05-05 11:19:35 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > Why is the layout dump platform dependant?  The pixel test, perhaps, but the layout itself should be platform agnostic.
> > 
> > Any test that has text output will likely differ by platforms since the font metrics are different.
> 
> Ah, I see, so the bounds of each text line dumped will be different.  Should these tests be re-written to remove the text portions?

Another option is to use layoutTestController.dumpAsText(true).  This tells DRT to use the dumpAsText output + include a pixel result.
Comment 9 Alexey Proskuryakov 2011-05-05 11:21:49 PDT
> Should these tests be re-written to remove the text portions?

If that can be done without making the tests harder to run manually, and the tests can't be text-only, then that's definitely the best solution.

My dream is to have a font that's rendered by all platforms identically :)
Comment 10 Jer Noble 2011-05-05 11:26:12 PDT
(In reply to comment #8)
> Another option is to use layoutTestController.dumpAsText(true).  This tells DRT to use the dumpAsText output + include a pixel result.

This seems like the optimal scenario.  I'll look into doing this.
Comment 11 Jer Noble 2011-05-05 11:32:53 PDT
Created attachment 92435 [details]
Patch
Comment 12 Tony Chang 2011-05-05 11:49:47 PDT
Comment on attachment 92435 [details]
Patch

Can you delete the other platform/*/fullscreen/full-screen-zIndex-expected.txt files in this patch?  Otherwise, this looks good.
Comment 13 Jer Noble 2011-05-05 13:18:44 PDT
Created attachment 92458 [details]
Patch
Comment 14 Tony Chang 2011-05-05 14:20:33 PDT
Comment on attachment 92458 [details]
Patch

Thanks!
Comment 15 Jer Noble 2011-05-05 14:30:44 PDT
Comment on attachment 92458 [details]
Patch

Adding cq+.
Comment 16 WebKit Commit Bot 2011-05-05 16:06:41 PDT
The commit-queue encountered the following flaky tests while processing attachment 92458 [details]:

java/lc3/JavaObject/JavaObjectToLong-005.html bug 60309 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 17 WebKit Commit Bot 2011-05-05 16:08:08 PDT
Comment on attachment 92458 [details]
Patch

Clearing flags on attachment: 92458

Committed r85897: <http://trac.webkit.org/changeset/85897>
Comment 18 WebKit Commit Bot 2011-05-05 16:08:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 WebKit Review Bot 2011-05-05 17:23:52 PDT
http://trac.webkit.org/changeset/85897 might have broken GTK Linux 32-bit Release
The following tests are not passing:
fullscreen/full-screen-stacking-context.html