Summary: | [LayoutTests] Decouple http/tests/media/resources/create-id3-db.php from webserver | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Gambrell <cgambrell> | ||||||||||||||
Component: | Tools / Tests | Assignee: | Chris Gambrell <cgambrell> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | ap, cgambrell, darin, eric.carlson, ews-watchlist, glenn, jbedard, jer.noble, philipj, pnormand, ryanhaddad, sergio, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=224528 | ||||||||||||||||
Attachments: |
|
Description
Chris Gambrell
2021-04-14 10:34:49 PDT
This script was added as part of fixing bug 125926. Eric Carlson's worry, IIUC, is that adding a binary file without anyway to regenerate it, was an issue. Since the db is now in json format, I think this worry is less concerning perhaps? The create-id3-db script could be ported to Python still, but I'd say it's not high priority. I've checked http/tests/media/media-play-stream-chunked-icy.html still pass'es as expected on GTK with the patch from bug 224528 applied. Created attachment 427576 [details]
Patch
Comment on attachment 427576 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427576&action=review > LayoutTests/ChangeLog:3 > + [LayoutTests] Replace http/tests/media/resources/create-id3-db.php with supported language Worth changing the title of this bug to "Remove" > LayoutTests/ChangeLog:8 > + Should probably include the rationale for removing, we can generally expect folks to be able to understand and modify JSON Created attachment 427577 [details]
Patch
> > LayoutTests/ChangeLog:8 > > + > > Should probably include the rationale for removing, we can generally expect > folks to be able to understand and modify JSON Addressed in comment 6 The replacement for the the PHP database is LayoutTests/media/content/metadata.json, I'd like someone more familiar with our media tests to verify that this is the right approach, the problem I see is that although the metadata.json is certainly easier to edit than it's PHP counterpart, the contents of the file still aren't trivial to regenerate (In reply to Jonathan Bedard from comment #8) > The replacement for the the PHP database is > LayoutTests/media/content/metadata.json, I'd like someone more familiar with > our media tests to verify that this is the right approach, the problem I see > is that although the metadata.json is certainly easier to edit than it's PHP > counterpart, the contents of the file still aren't trivial to regenerate The old database was generated by the php. If metadata.json is generated by the python we should be OK. If it isn't, why not? Comment on attachment 427577 [details]
Patch
Oops, clearing the flags.
I agree with Eric. We should replace these scripts with the new scripts to generate the checked-in JSON file. Not remove them with no replacement. Sorry, I didn’t mean to clear the flags, but once I had set review+ and commit-queue+ that was the only way I knew to undo it. Talking with Alexey and Chris about this, the plan is to change this script to a stand-alone PHP script so we can pull PHP out of our web servers. Created attachment 428175 [details]
Patch
Created attachment 428176 [details]
Patch
Comment on attachment 428176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428176&action=review > LayoutTests/media/content/create-id3-db.php:1 > +#!/usr/bin/env php We don't need the ".php" extension any more. It's actually undesirable since it risks counting this file as a test if things get moved around. Just drop the .php and make the file executable. > LayoutTests/media/content/create-id3-db.php:17 > // $ Tools/Scripts/run-webkit-httpd Change the running instructions, this should no longer involve the httpd server. > LayoutTests/media/content/create-id3-db.php:31 > + if (!function_exists('sys_get_temp_dir')) { I don't think we need this check, since we aren't importing the file any more. > LayoutTests/media/content/create-id3-db.php:51 > + if (!function_exists('file_put_contents')) { I don't think we need this check, since we aren't importing the file any more. > LayoutTests/media/content/create-id3-db.php:65 > + if (!function_exists('stream_copy_to_stream')) { I don't think we need this check, since we aren't importing the file any more. > LayoutTests/media/content/create-id3-db.php:80 > + if (!function_exists('scandir')) { I don't think we need this check, since we aren't importing the file any more. > LayoutTests/media/content/create-id3-db.php:168 > + } We only ported Created attachment 428183 [details]
Patch
(In reply to Jonathan Bedard from comment #15) > Comment on attachment 428176 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428176&action=review > > > LayoutTests/media/content/create-id3-db.php:1 > > +#!/usr/bin/env php > > We don't need the ".php" extension any more. It's actually undesirable since > it risks counting this file as a test if things get moved around. Just drop > the .php and make the file executable. > > > LayoutTests/media/content/create-id3-db.php:17 > > // $ Tools/Scripts/run-webkit-httpd > > Change the running instructions, this should no longer involve the httpd > server. > > > LayoutTests/media/content/create-id3-db.php:31 > > + if (!function_exists('sys_get_temp_dir')) { > > I don't think we need this check, since we aren't importing the file any > more. > > > LayoutTests/media/content/create-id3-db.php:51 > > + if (!function_exists('file_put_contents')) { > > I don't think we need this check, since we aren't importing the file any > more. > > > LayoutTests/media/content/create-id3-db.php:65 > > + if (!function_exists('stream_copy_to_stream')) { > > I don't think we need this check, since we aren't importing the file any > more. > > > LayoutTests/media/content/create-id3-db.php:80 > > + if (!function_exists('scandir')) { > > I don't think we need this check, since we aren't importing the file any > more. > > > LayoutTests/media/content/create-id3-db.php:168 > > + } > > We only ported Fixed in comment 16 Comment on attachment 428183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428183&action=review > LayoutTests/ChangeLog:3 > + [LayoutTests] Replace http/tests/media/resources/create-id3-db.php result with JSON We should change the title of this bug, something to the effect of "decouple create-id3-db from webserver" > LayoutTests/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). Reviewed by line needs to go above the change explanation Created attachment 428189 [details]
Patch
(In reply to Jonathan Bedard from comment #18) > Comment on attachment 428183 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428183&action=review > > > LayoutTests/ChangeLog:3 > > + [LayoutTests] Replace http/tests/media/resources/create-id3-db.php result with JSON > > We should change the title of this bug, something to the effect of "decouple > create-id3-db from webserver" > > > LayoutTests/ChangeLog:9 > > + Reviewed by NOBODY (OOPS!). > > Reviewed by line needs to go above the change explanation Fixed in comment 19 Committed r277311 (237571@main): <https://commits.webkit.org/237571@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428189 [details]. |