[webkit-reviews] review granted: [Bug 223309] modern-media-controls script should not be allocated in heap : [Attachment 423522] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted  review:
Bug 223309: modern-media-controls script should not be allocated in heap
https://bugs.webkit.org/show_bug.cgi?id=223309

Attachment 423522: Patch

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




--- Comment #20 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 423522
  --> https://bugs.webkit.org/attachment.cgi?id=423522
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.