WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
75707
[Windows] Many css2.1 tests fail on Apple's Windows port under NRWT
https://bugs.webkit.org/show_bug.cgi?id=75707
Summary
[Windows] Many css2.1 tests fail on Apple's Windows port under NRWT
Adam Roben (:aroben)
Reported
2012-01-06 09:10:49 PST
To reproduce (after working around
bug 64468
): 1. new-run-webkit-tests css2.1 Many tests fail. Compare this to "old-run-webkit-tests css2.1", where all the tests pass.
Attachments
Initial work in progress. UTF-8 encoding is not being handled properly.
(3.46 KB, patch)
2013-05-27 23:02 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(4.33 KB, patch)
2013-05-28 13:03 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Updated to include export definition changes needed for clean build.
(6.90 KB, patch)
2013-05-28 14:28 PDT
,
Brent Fulgham
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2012-01-06 09:11:18 PST
<
rdar://problem/10654655
>
Adam Roben (:aroben)
Comment 2
2012-01-06 09:12:23 PST
It looks like NRWT doesn't support epilogues/prologues like ORWT does, which we use to set up custom font fallback for tests in the css2.1 and svg/W3C-SVG-1.1 directories.
Adam Roben (:aroben)
Comment 3
2012-01-06 09:12:52 PST
ORWT's support was added in
http://trac.webkit.org/changeset/29822
Eric Seidel (no email)
Comment 4
2012-01-06 10:34:25 PST
Wow. I never knew these existed! I guess they're only used for Win currently?
Adam Roben (:aroben)
Comment 5
2012-01-06 11:46:38 PST
Yes, I think only Windows uses them currently.
Dirk Pranke
Comment 6
2012-01-06 13:50:50 PST
Hm. This is a problematic feature in the face of parallelism. How would this be supposed to work with multiple DRTs running at the same time? Does the persistent user style sheet affect any DRT running on the machine, or is it only in memory? I.e., how do I keep this from affecting DRTs running tests in other directories? I'm actually working on a slightly different approach to this sort of thing where NRWT will change the command line flags passed to the process on a per-directory configurable basis (as part of getting rid of the chromium GPU configuration). Could we maybe do something like that instead where every test would be told to do something different before/after/during the test but it was in-memory? Failing that, given that this seems to only be used on the Windows port at the moment, I suggest we hack this into the windows Driver.run_test() to detect when it's entering and leaving directories with these files, and fix the windows # of workers at 1 until we can figure out a better way to get this sort of functionality. WDYT?
Adam Roben (:aroben)
Comment 7
2012-01-06 14:01:50 PST
(In reply to
comment #6
)
> Hm. This is a problematic feature in the face of parallelism. > > How would this be supposed to work with multiple DRTs running at the same time? Does the persistent user style sheet affect any DRT running on the machine, or is it only in memory? I.e., how do I keep this from affecting DRTs running tests in other directories?
Right now I think it gets persisted out to disk with the rest of DRT's preferences, but I don't think that is a requirement. In fact, it's probably an accident that DRT's preferences get written to disk. So I don't think this presents a problem for concurrent testing, as long as we stop writing out preferences to disk.
> Could we maybe do something like that instead where every test would be told to do something different before/after/during the test but it was in-memory?
Perhaps. Presumably that would require DRT changes. I also wonder if it would slow things down, since we'd be loading 3 files (prologue + test + epilogue) for every css2.1 test.
Dirk Pranke
Comment 8
2012-01-06 14:07:21 PST
(In reply to
comment #7
)
> (In reply to
comment #6
) > > Hm. This is a problematic feature in the face of parallelism. > > > > How would this be supposed to work with multiple DRTs running at the same time? Does the persistent user style sheet affect any DRT running on the machine, or is it only in memory? I.e., how do I keep this from affecting DRTs running tests in other directories? > > Right now I think it gets persisted out to disk with the rest of DRT's preferences, but I don't think that is a requirement. In fact, it's probably an accident that DRT's preferences get written to disk. So I don't think this presents a problem for concurrent testing, as long as we stop writing out preferences to disk. > > > Could we maybe do something like that instead where every test would be told to do something different before/after/during the test but it was in-memory? > > Perhaps. Presumably that would require DRT changes. I also wonder if it would slow things down, since we'd be loading 3 files (prologue + test + epilogue) for every css2.1 test.
The general model I have in mind is that the command lines would potentially change per directory, and so when DRT was handed a test in a new directory, it would get restarted with the change. So, for something like this we could add a --use-user-style=$foo flag (or some equivalent) for this directory, and any test run in that directory would get handed that flag. We could add a more general --prologue and --epilogue flags to run those files on startup and shutdown (and the epilogue might not even be needed if the scripts weren't changing persistent settings). Obviously this would require some DRT changes, but it doesn't seem like they'd be too bad.
Adam Roben (:aroben)
Comment 9
2012-01-09 09:07:46 PST
(In reply to
comment #8
)
> The general model I have in mind is that the command lines would potentially change per directory, and so when DRT was handed a test in a new directory, it would get restarted with the change.
I wonder what effect this would have on performance for directories that don't contain many tests? I guess we could optimize by comparing the old and new command lines and just reusing the existing DRT if they are the same.
Dirk Pranke
Comment 10
2012-01-09 10:29:40 PST
(In reply to
comment #9
)
> (In reply to
comment #8
) > > The general model I have in mind is that the command lines would potentially change per directory, and so when DRT was handed a test in a new directory, it would get restarted with the change. > > I wonder what effect this would have on performance for directories that don't contain many tests? I guess we could optimize by comparing the old and new command lines and just reusing the existing DRT if they are the same.
Right, that was what I had in mind.
Ojan Vafai
Comment 11
2012-01-09 11:17:36 PST
Alternately, we could just hard-code this logic into Apple's WIN DRT and save ourselves a lot of work of trying to make this general.
Brent Fulgham
Comment 12
2013-05-24 10:20:22 PDT
It appears that the only cases where the prologue/epilogue feature is being used is to support the setting of a persistent user style sheet. No other ports appear to be using this feature. Given that roughly five years have passed since this mechanism was created with no additional use cases, I think it makes sense to build this into Window's DRT.
Brent Fulgham
Comment 13
2013-05-27 23:02:38 PDT
Created
attachment 203022
[details]
Initial work in progress. UTF-8 encoding is not being handled properly.
Darin Adler
Comment 14
2013-05-28 09:09:50 PDT
Comment on
attachment 203022
[details]
Initial work in progress. UTF-8 encoding is not being handled properly. View in context:
https://bugs.webkit.org/attachment.cgi?id=203022&action=review
r=me if you fix the storage leak
> Tools/DumpRenderTree/win/DumpRenderTree.cpp:962 > +String createPathIfURLHasLogueDirectory(const CFURLRef& url)
Crazy use of the word “logue” here to mean prologue or epilogue!? Type here should just be CFURLRef, not const CFURLRef&.
> Tools/DumpRenderTree/win/DumpRenderTree.cpp:967 > + return "";
empytString() is the more efficient alternative to "" when returning a String.
> Tools/DumpRenderTree/win/DumpRenderTree.cpp:971 > + UInt8* buffer = new UInt8[length];
This buffer leaks. One fix is to use an OwnArrayPtr. Another is to use Vector<UInt8>.
> Tools/DumpRenderTree/win/DumpRenderTree.cpp:973 > + if (-1 == length)
Normally we don’t do the “constant on the left” thing like this. Also, it’s impossible for CFURLGetBytes to return length if we are passing it a length we got from calling it the first time, so this check is guaranteed dead code.
> Tools/DumpRenderTree/win/DumpRenderTree.cpp:978 > + String loguePath(buffer + pathRange.location, pathRange.length); > + > + loguePath = WebCore::pathByAppendingComponent(loguePath, "resources/");
Seems like we could do this with CFURL and CFString functions rather than dropping into WebCore internal types to do it. CFURLCreateCopyAppendingPathComponent and CFURLGetFileSystemRepresentation could be used to do the job this function does without involving the CFURLGetByte functions. But I suppose this is OK as is.
Brent Fulgham
Comment 15
2013-05-28 13:03:49 PDT
Created
attachment 203080
[details]
Patch
Brent Fulgham
Comment 16
2013-05-28 13:04:36 PDT
The attached patch permits the css2.1 tests to now pass under NRWT.
Build Bot
Comment 17
2013-05-28 13:34:55 PDT
Comment on
attachment 203080
[details]
Patch
Attachment 203080
[details]
did not pass win-ews (win): Output:
http://webkit-queues.appspot.com/results/738123
Brent Fulgham
Comment 18
2013-05-28 14:28:27 PDT
Created
attachment 203088
[details]
Updated to include export definition changes needed for clean build.
Darin Adler
Comment 19
2013-05-28 14:33:16 PDT
Comment on
attachment 203088
[details]
Updated to include export definition changes needed for clean build. View in context:
https://bugs.webkit.org/attachment.cgi?id=203088&action=review
I’d like to see these patches make less use of internal WebCore data structures and functions.
> Source/WebKit/ChangeLog:9 > + * WebKit.vcxproj/WebKitExportGenerator/WebKitExports.def.in: > + Export the WebCore::directoryName method for use by DRT.
I don’t think this is a step in the right direction. Ideally DumpRenderTree is “outside WebKit” code, and we should not be exporting more functions for it to use. Especially functions that are just file system helpers that have no special WebKit magic in them.
Brent Fulgham
Comment 20
2013-05-28 16:17:55 PDT
Committed
r150851
: <
http://trac.webkit.org/changeset/150851
>
Brent Fulgham
Comment 21
2013-05-28 16:20:35 PDT
See
Bug 116898
to reduce use of private WebCore methods.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug