As part of review feedback for https://bugs.webkit.org/show_bug.cgi?id=163726, Dean suggested using a build phase to concatenate all JS resources into a single file rather than doing a series of stringWithContentsOfFile calls on the WebCore bundle at runtime. We should also do this for CSS.
<rdar://problem/28890058>
Created attachment 292350 [details] Patch
Created attachment 292351 [details] Patch
Comment on attachment 292351 [details] Patch This looks good to me, but I think you should get someone who knows more about the build system to check this. Two concerns: - what will happen with localization? at some point you'll need a file with all the strings that need to be localized. - does this run on every build? Previously the rsync would only change files if necessary. Is that bad?
Comment on attachment 292351 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292351&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:27979 > + shellScript = "rsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRCROOT/Modules/modern-media-controls/images\" \"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\ncd \"$SRCROOT/Modules/modern-media-controls\"\ncat controls/*.css > \"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls/modern-media-controls.css\"\n cat \"controls/scheduler.js\" \"controls/layout-node.js\" \"controls/layout-item.js\" \"controls/icon-service.js\" \"controls/time-control.js\" \"controls/time-label.js\" \"controls/slider.js\" \"controls/volume-slider.js\" \"controls/scrubber.js\" \"controls/button.js\" \"controls/start-button.js\" \"controls/icon-button.js\" \"controls/play-pause-button.js\" \"controls/skip-back-button.js\" \"controls/mute-button.js\" \"controls/airplay-button.js\" \"controls/pip-button.js\" \"controls/tracks-button.js\" \"controls/fullscreen-button.js\" \"controls/aspect-ratio-button.js\" \"controls/rewind-button.js\" \"controls/forward-button.js\" \"controls/media-controls.js\" \"controls/macos-media-controls.js\" \"controls/macos-inline-media-controls.js\" \"controls/buttons-container.js\" \"controls/placard.js\" \"controls/airplay-placard.js\" \"controls/pip-placard.js\" \"media/media-controller-support.js\" \"media/start-support.js\" \"media/mute-support.js\" \"media/media-controller.js\" \"main.js\" > \"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls/modern-media-controls.js\"\n"; There are too many quotation marks here. There is no reason to write "main.js" instead of just main.js in the command line. But can we put the list of files somewhere easier to read and inspect than inside the build rule? Maybe put the names into a file? Then it would be more like this: cat `cat JavaScript-filenames` > \"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls/modern-media-controls.js\" Or rearrange them so we can pick them up with *.js like you are picking up the CSS with *.css?
(In reply to comment #5) > Comment on attachment 292351 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=292351&action=review > > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:27979 > But can we put the list of files somewhere easier to read and inspect than > inside the build rule? Maybe put the names into a file? Then it would be > more like this: > > cat `cat JavaScript-filenames` > > \"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media- > controls/modern-media-controls.js\" I like that! > Or rearrange them so we can pick them up with *.js like you are picking up > the CSS with *.css? Alas, we can't do that with JS, the order in which the classes appear matters.
Created attachment 292603 [details] Patch for landing
Sorry, I forgot about your comments after reading Darin's. (In reply to comment #4) > Comment on attachment 292351 [details] > > - what will happen with localization? at some point you'll need a file with > all the strings that need to be localized. Yes, I will follow the same approach used for current media controls, where the strings are in a separate JS file that gets injected on top of the media controls logic. > - does this run on every build? Previously the rsync would only change files > if necessary. Is that bad? It does run on every build, I hope it's not bad.
Comment on attachment 292603 [details] Patch for landing https://trac.webkit.org/changeset/207758