Bug 223309 - modern-media-controls script should not be allocated in heap
Summary: modern-media-controls script should not be allocated in heap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 223421 223961
  Show dependency treegraph
 
Reported: 2021-03-16 22:54 PDT by Yusuke Suzuki
Modified: 2021-03-30 14:58 PDT (History)
25 users (show)

See Also:


Attachments
Patch (21.28 KB, patch)
2021-03-17 02:59 PDT, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (21.38 KB, patch)
2021-03-17 03:10 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (16.38 KB, patch)
2021-03-17 11:52 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (16.71 KB, patch)
2021-03-17 12:15 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (36.72 KB, patch)
2021-03-17 13:22 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (38.37 KB, patch)
2021-03-17 13:26 PDT, Yusuke Suzuki
drousso: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (39.24 KB, patch)
2021-03-17 14:28 PDT, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (37.12 KB, patch)
2021-03-17 16:36 PDT, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (37.12 KB, patch)
2021-03-17 16:45 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2021-03-16 22:54:49 PDT
We should not allocate heap memory.
And we should not concatenate localized string data.
Comment 1 Yusuke Suzuki 2021-03-16 22:56:43 PDT
(In reply to Yusuke Suzuki from comment #0)
> We should not allocate heap memory.
> And we should not concatenate localized string data.

Currently, I have no idea how to change localized string part. We should not concatenate them. We should

1. Have JSON data for localized strings
2. Pass it to media control script
3. And parse it inside media control script

This localized string part makes script UTF-16, this is bad for memory.
In this bug, let's just first minify the script.
Comment 2 Yusuke Suzuki 2021-03-17 02:59:46 PDT
Created attachment 423460 [details]
Patch
Comment 3 Yusuke Suzuki 2021-03-17 03:10:47 PDT
Created attachment 423463 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2021-03-17 04:02:56 PDT
<rdar://problem/75520193>
Comment 5 Eric Carlson 2021-03-17 09:10:25 PDT
Comment on attachment 423463 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36078
> +			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' | python \"$SRCROOT/../JavaScriptCore/Scripts/js-minify.py\" > \"$DST_DIR/modern-media-controls.js\"\n";

I can't tell if this is already the case, but it would be helpful to not minify the sources in development builds.
Comment 6 Alexey Proskuryakov 2021-03-17 11:11:44 PDT
Comment on attachment 423463 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36071
> +				"$(SRCROOT)/../JavaScriptCore/Scripts/js-minify.py",

Is this really the only input path? And why are there no output paths?

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36078
>> +			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' | python \"$SRCROOT/../JavaScriptCore/Scripts/js-minify.py\" > \"$DST_DIR/modern-media-controls.js\"\n";
> 
> I can't tell if this is already the case, but it would be helpful to not minify the sources in development builds.

A big problem with this is that there isn't a great definition of what a "development build" is. Just debug vs. release, or something else?
Comment 7 Yusuke Suzuki 2021-03-17 11:18:13 PDT
Comment on attachment 423463 [details]
Patch

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

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36071
>> +				"$(SRCROOT)/../JavaScriptCore/Scripts/js-minify.py",
> 
> Is this really the only input path? And why are there no output paths?

Maybe we should add these files to output files? They are not included before this patch.

>>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36078
>>> +			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' | python \"$SRCROOT/../JavaScriptCore/Scripts/js-minify.py\" > \"$DST_DIR/modern-media-controls.js\"\n";
>> 
>> I can't tell if this is already the case, but it would be helpful to not minify the sources in development builds.
> 
> A big problem with this is that there isn't a great definition of what a "development build" is. Just debug vs. release, or something else?

I think we should check Debug build or not. This is what 

#if !defined(NDEBUG)
        // Setting a scriptURL allows the source to be debuggable in the inspector.
        URL scriptURL = URL({ }, "mediaControlsScript"_s);
            // Setting a scriptURL allows the source to be debuggable in the inspector.
            URL scriptURL = URL({ }, makeString("mediaControlsScript"_s, index));
#else
        URL scriptURL;
            URL scriptURL;
#endif

part is doing.
Comment 8 Devin Rousso 2021-03-17 11:22:51 PDT
Comment on attachment 423463 [details]
Patch

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

>>>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36078
>>>> +			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' | python \"$SRCROOT/../JavaScriptCore/Scripts/js-minify.py\" > \"$DST_DIR/modern-media-controls.js\"\n";
>>> 
>>> I can't tell if this is already the case, but it would be helpful to not minify the sources in development builds.
>> 
>> A big problem with this is that there isn't a great definition of what a "development build" is. Just debug vs. release, or something else?
> 
> I think we should check Debug build or not. This is what 
> 
> #if !defined(NDEBUG)
>         // Setting a scriptURL allows the source to be debuggable in the inspector.
>         URL scriptURL = URL({ }, "mediaControlsScript"_s);
>             // Setting a scriptURL allows the source to be debuggable in the inspector.
>             URL scriptURL = URL({ }, makeString("mediaControlsScript"_s, index));
> #else
>         URL scriptURL;
>             URL scriptURL;
> #endif
> 
> part is doing.

I think @Eric Carlson meant "development build" more as "engineering build" which is any time an engineer does a local build.

Having it be debug-only is really not ideal since 99% of features involving media controls do not need assertions or anything like that.

> Source/WebCore/rendering/RenderThemeIOS.mm:1296
> +            m_mediaControlsAdditionalScript = String(RenderThemeIOSAdditions_mediaControlsScript);

you'll need to change `RenderThemeIOSAdditions_mediaControlsScript` in WebKitAdditions

> Source/WebCore/rendering/RenderThemeMac.mm:338
> +            m_mediaControlsAdditionalScript = String(RenderThemeMacAdditions_mediaControlsScript);

you'll need to change `RenderThemeMacAdditions_mediaControlsScript` in WebKitAdditions
Comment 9 Devin Rousso 2021-03-17 11:38:42 PDT
Comment on attachment 423463 [details]
Patch

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

>>>>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36078
>>>>> +			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' | python \"$SRCROOT/../JavaScriptCore/Scripts/js-minify.py\" > \"$DST_DIR/modern-media-controls.js\"\n";
>>>> 
>>>> I can't tell if this is already the case, but it would be helpful to not minify the sources in development builds.
>>> 
>>> A big problem with this is that there isn't a great definition of what a "development build" is. Just debug vs. release, or something else?
>> 
>> I think we should check Debug build or not. This is what 
>> 
>> #if !defined(NDEBUG)
>>         // Setting a scriptURL allows the source to be debuggable in the inspector.
>>         URL scriptURL = URL({ }, "mediaControlsScript"_s);
>>             // Setting a scriptURL allows the source to be debuggable in the inspector.
>>             URL scriptURL = URL({ }, makeString("mediaControlsScript"_s, index));
>> #else
>>         URL scriptURL;
>>             URL scriptURL;
>> #endif
>> 
>> part is doing.
> 
> I think @Eric Carlson meant "development build" more as "engineering build" which is any time an engineer does a local build.
> 
> Having it be debug-only is really not ideal since 99% of features involving media controls do not need assertions or anything like that.

As an alternative to making the visibility of the media controls script based on a compile flag, I think you could name it something like `__InjectedScript_mediaControls.js` which would be ignored by Web Inspector unless the "Show WebKit-internal scripts" engineering setting (which are only shown for local engineering builds) is enabled.
Comment 10 Yusuke Suzuki 2021-03-17 11:48:52 PDT
Comment on attachment 423463 [details]
Patch

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

>>>>>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36078
>>>>>> +			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' | python \"$SRCROOT/../JavaScriptCore/Scripts/js-minify.py\" > \"$DST_DIR/modern-media-controls.js\"\n";
>>>>> 
>>>>> I can't tell if this is already the case, but it would be helpful to not minify the sources in development builds.
>>>> 
>>>> A big problem with this is that there isn't a great definition of what a "development build" is. Just debug vs. release, or something else?
>>> 
>>> I think we should check Debug build or not. This is what 
>>> 
>>> #if !defined(NDEBUG)
>>>         // Setting a scriptURL allows the source to be debuggable in the inspector.
>>>         URL scriptURL = URL({ }, "mediaControlsScript"_s);
>>>             // Setting a scriptURL allows the source to be debuggable in the inspector.
>>>             URL scriptURL = URL({ }, makeString("mediaControlsScript"_s, index));
>>> #else
>>>         URL scriptURL;
>>>             URL scriptURL;
>>> #endif
>>> 
>>> part is doing.
>> 
>> I think @Eric Carlson meant "development build" more as "engineering build" which is any time an engineer does a local build.
>> 
>> Having it be debug-only is really not ideal since 99% of features involving media controls do not need assertions or anything like that.
> 
> As an alternative to making the visibility of the media controls script based on a compile flag, I think you could name it something like `__InjectedScript_mediaControls.js` which would be ignored by Web Inspector unless the "Show WebKit-internal scripts" engineering setting (which are only shown for local engineering builds) is enabled.

Do we have a Xcode environment variable which can be switched based on "development build"?
Comment 11 Yusuke Suzuki 2021-03-17 11:50:17 PDT
Comment on attachment 423463 [details]
Patch

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

>>>>>>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36078
>>>>>>> +			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' | python \"$SRCROOT/../JavaScriptCore/Scripts/js-minify.py\" > \"$DST_DIR/modern-media-controls.js\"\n";
>>>>>> 
>>>>>> I can't tell if this is already the case, but it would be helpful to not minify the sources in development builds.
>>>>> 
>>>>> A big problem with this is that there isn't a great definition of what a "development build" is. Just debug vs. release, or something else?
>>>> 
>>>> I think we should check Debug build or not. This is what 
>>>> 
>>>> #if !defined(NDEBUG)
>>>>         // Setting a scriptURL allows the source to be debuggable in the inspector.
>>>>         URL scriptURL = URL({ }, "mediaControlsScript"_s);
>>>>             // Setting a scriptURL allows the source to be debuggable in the inspector.
>>>>             URL scriptURL = URL({ }, makeString("mediaControlsScript"_s, index));
>>>> #else
>>>>         URL scriptURL;
>>>>             URL scriptURL;
>>>> #endif
>>>> 
>>>> part is doing.
>>> 
>>> I think @Eric Carlson meant "development build" more as "engineering build" which is any time an engineer does a local build.
>>> 
>>> Having it be debug-only is really not ideal since 99% of features involving media controls do not need assertions or anything like that.
>> 
>> As an alternative to making the visibility of the media controls script based on a compile flag, I think you could name it something like `__InjectedScript_mediaControls.js` which would be ignored by Web Inspector unless the "Show WebKit-internal scripts" engineering setting (which are only shown for local engineering builds) is enabled.
> 
> Do we have a Xcode environment variable which can be switched based on "development build"?

Or, I'll just drop minification thing from this patch since this does not offer memory reduction (this reduces size of framework, but this is not a main goal of this patch).
Comment 12 Yusuke Suzuki 2021-03-17 11:52:15 PDT
Created attachment 423512 [details]
Patch
Comment 13 Yusuke Suzuki 2021-03-17 12:15:59 PDT
Created attachment 423513 [details]
Patch
Comment 14 Yusuke Suzuki 2021-03-17 12:27:54 PDT
Comment on attachment 423513 [details]
Patch

Ah, let me wait. It seems that we are allocating memory for String(CFString). We should serialize script into C characters as we do in builtin code and place it into __DATA_CONST.
Comment 15 Alexey Proskuryakov 2021-03-17 12:38:12 PDT
> Maybe we should add these files to output files? They are not included before this patch.

1. Input files. Seems certain that we need more here, as otherwise incremental builds will not be triggered when inputs change.

2. Output files. This script seems to put outputs directly in their final location, so nothing else probably depends on it. I don't know what old and new Xcode build systems will do when output is missing. Will they run this script phase unconditionally, wasting incremental build time even when nothing changes?

As for engineering builds, strategically there should be as little difference between local builds and what ships to customers as possible. Things like minification in particular tend to have bugs, and we've seen a few before. I acknowledge the need to debug, yet I would encourage thinking of ways to make these scripts debuggable in some other way, that's not automagically triggered by "local builds".
Comment 16 Devin Rousso 2021-03-17 12:41:27 PDT
I think my suggestion of using `__InjectedScript_*.js` might actually be a valid alternative that satisfies both cases because that will not only be hidden from Web Inspector in non-local builds but will also be ignored by the debugger when pausing (local engineering builds can enable the "Pause in WebKit-internal scripts" setting in Web Inspector).
Comment 17 Yusuke Suzuki 2021-03-17 13:01:12 PDT
(In reply to Alexey Proskuryakov from comment #15)
> > Maybe we should add these files to output files? They are not included before this patch.
> 
> 1. Input files. Seems certain that we need more here, as otherwise
> incremental builds will not be triggered when inputs change.
> 
> 2. Output files. This script seems to put outputs directly in their final
> location, so nothing else probably depends on it. I don't know what old and
> new Xcode build systems will do when output is missing. Will they run this
> script phase unconditionally, wasting incremental build time even when
> nothing changes?
> 
> As for engineering builds, strategically there should be as little
> difference between local builds and what ships to customers as possible.
> Things like minification in particular tend to have bugs, and we've seen a
> few before. I acknowledge the need to debug, yet I would encourage thinking
> of ways to make these scripts debuggable in some other way, that's not
> automagically triggered by "local builds".

Since WTFString(CFString) will allocate memory anyway, I'll go to the different course, basically just use UserAgentScripts as we already do for QuickTimePluginReplacement.js since UserAgentScripts is designed for this `user-agent specific script source` purpose.
Comment 18 Yusuke Suzuki 2021-03-17 13:22:32 PDT
Created attachment 423520 [details]
Patch
Comment 19 Yusuke Suzuki 2021-03-17 13:26:02 PDT
Created attachment 423522 [details]
Patch
Comment 20 Devin Rousso 2021-03-17 14:03:32 PDT
Comment on attachment 423522 [details]
Patch

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

r=me once EWS is happy, nice fix! :)

> Source/WebCore/Modules/modern-media-controls/js-files:-1
> -main.js

This file is also used by `LayoutTests/media/modern-media-controls/resources/media-controls-loader.js`.  IMO it's fine to copy this list into that file though (which would also avoid a sync XHR).

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-36084
> -			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.js\"\n";

NIT: Does removing the `perl -0777 -pe 's{/\\*.*?\\*/}{}gs'` mean that comments are not stripped anymore?  I guess this won't be an issue though if we minify the JS files later anyways.

> Source/WebCore/html/HTMLMediaElement.cpp:7151
> +    Vector<String, 3> mediaControlsScripts = RenderTheme::singleton().mediaControlsScripts();

`auto`?

> Source/WebCore/html/HTMLMediaElement.cpp:7171
> +                scope.clearException();

Can we log the reason that the evaluate failed?  This shouldn't ever happen (i.e. the media controls script shouldn't have any errors), so it'd be nice to have in a sysdiagnose.
Comment 21 Yusuke Suzuki 2021-03-17 14:28:43 PDT
Created attachment 423529 [details]
Patch
Comment 22 Yusuke Suzuki 2021-03-17 16:36:14 PDT
Created attachment 423538 [details]
Patch
Comment 23 Yusuke Suzuki 2021-03-17 16:45:14 PDT
Created attachment 423539 [details]
Patch
Comment 24 Yusuke Suzuki 2021-03-17 19:00:33 PDT
Committed r274607 (235444@main): <https://commits.webkit.org/235444@main>