Bug 214955 - Remove unused JS and CSS files of media controls
Summary: Remove unused JS and CSS files of media controls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-29 18:28 PDT by Peng Liu
Modified: 2021-03-24 22:44 PDT (History)
38 users (show)

See Also:


Attachments
Patch (346.55 KB, patch)
2020-07-29 18:30 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (211.65 KB, patch)
2020-07-29 18:37 PDT, Peng Liu
dbates: review+
Details | Formatted Diff | Diff
[Patch] WIP (1.21 MB, patch)
2021-03-17 19:46 PDT, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (1.21 MB, patch)
2021-03-17 20:01 PDT, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (1.21 MB, patch)
2021-03-18 00:28 PDT, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
Patch (1.25 MB, patch)
2021-03-18 13:59 PDT, Devin Rousso
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (1.22 MB, patch)
2021-03-19 09:58 PDT, Devin Rousso
eric.carlson: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (1.23 MB, patch)
2021-03-19 13:37 PDT, Devin Rousso
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (1.23 MB, patch)
2021-03-22 10:13 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2020-07-29 18:28:33 PDT
Remove unused JS and CSS files of media controls
Comment 1 Peng Liu 2020-07-29 18:30:54 PDT
Created attachment 405538 [details]
Patch
Comment 2 Peng Liu 2020-07-29 18:37:49 PDT
Created attachment 405539 [details]
Patch
Comment 3 Daniel Bates 2020-08-03 17:33:31 PDT
Comment on attachment 405539 [details]
Patch

Patch looks good. Looks like something is still using the files from the EWS failure.
Comment 4 Daniel Bates 2020-08-03 17:35:49 PDT
Probably just need updated expectation...don't look too closely
Comment 5 Daniel Bates 2020-08-03 17:36:12 PDT
I didn't look closely is what I meant ^^^
Comment 6 Radar WebKit Bug Importer 2020-08-05 18:29:22 PDT
<rdar://problem/66604040>
Comment 7 Devin Rousso 2021-03-17 19:46:00 PDT
Created attachment 423550 [details]
[Patch] WIP
Comment 8 Devin Rousso 2021-03-17 20:01:23 PDT
Created attachment 423552 [details]
[Patch] WIP

rebase
Comment 9 Darin Adler 2021-03-17 21:40:15 PDT
No ChangeLog yet.
Comment 10 Darin Adler 2021-03-17 21:40:40 PDT
Should not set review? if not ready to review. The webkit-patch tool can trigger EWS without setting review?.
Comment 11 Devin Rousso 2021-03-18 00:28:49 PDT
Created attachment 423563 [details]
[Patch] WIP

rebase (again)

also
 - combine the `RenderThemeMac` and `RenderThemeIOS` media controls functions into `RenderThemeCocoa` since they're now the same
 - fix the CSP issue by making sure that the `<img>` created by `iconService` is attached to the UA `shadowRoot` for `isInUserAgentShadowTree` before setting the `src`
 - fix the DerivedSources makefile to use all prerequisites for the modern media controls JS/CSS, not just ones edited since the last `make`
Comment 12 Devin Rousso 2021-03-18 13:59:42 PDT
Created attachment 423649 [details]
Patch
Comment 13 Devin Rousso 2021-03-19 09:58:23 PDT
Created attachment 423740 [details]
Patch
Comment 14 Eric Carlson 2021-03-19 10:44:13 PDT
Comment on attachment 423740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423740&action=review

> LayoutTests/ChangeLog:9
> +        Remove tests (and their expectations) that used `ModernMediaControlsEnabled`.

do you mean `ModernMediaControlsEnabled=false`?
Comment 15 Devin Rousso 2021-03-19 11:30:20 PDT
Comment on attachment 423740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423740&action=review

>> LayoutTests/ChangeLog:9
>> +        Remove tests (and their expectations) that used `ModernMediaControlsEnabled`.
> 
> do you mean `ModernMediaControlsEnabled=false`?

Yes, but there was only one test that had `ModernMediaControlsEnabled=true` :P

I'll be more explicit/clear.
Comment 16 Peng Liu 2021-03-19 13:31:32 PDT
Comment on attachment 423740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423740&action=review

> Source/WebKit/ChangeLog:13
> +        Replace `ModernMediaControlsEnabled` setting with `ENABLE_MODERN_MEDIA_CONTROLS` build flag.

Nit. I guess you mean `MODERN_MEDIA_CONTROLS` flag here but there is the macro `ENABLE_MODERN_MEDIA_CONTROLS` in this patch. :-)
Comment 17 Devin Rousso 2021-03-19 13:37:36 PDT
Created attachment 423768 [details]
Patch
Comment 18 Devin Rousso 2021-03-22 10:13:10 PDT
Created attachment 423904 [details]
Patch
Comment 19 EWS 2021-03-22 15:17:58 PDT
Committed r274810: <https://commits.webkit.org/r274810>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423904 [details].
Comment 20 Aakash Jain 2021-03-24 11:09:18 PDT
(In reply to EWS from comment #19)
> Committed r274810: <https://commits.webkit.org/r274810>
This caused fast/parser/xml-colon-entity.html to start failing. EWS indicated this issue on previous version of patch on this bug in https://ews-build.webkit.org/#/builders/51/builds/9405

History: https://results.webkit.org/?suite=layout-tests&test=fast%2Fparser%2Fxml-colon-entity.html
Comment 21 Robert Jenner 2021-03-24 11:20:12 PDT
Re-added the test expectations to Failure for "fast/parser/xml-colon-entity.html" that was removed with a patch from this bug. Test expectation re-added here:

https://trac.webkit.org/changeset/274951/webkit