Bug 163785 – [Modern Media Controls] Concatenate JS and CSS files into a single JS and CSS resources

• Component: Media
• Status: RESOLVED FIXED

[Antoine Quint]

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.

[Radar WebKit Bug Importer]

<rdar://problem/28890058>

[Antoine Quint]

Created attachment 292350 [details]
Patch

[Antoine Quint]

Created attachment 292351 [details]
Patch

[Dean Jackson]

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?

[Darin Adler]

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?

[Antoine Quint]

(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.

[Antoine Quint]

Created attachment 292603 [details]
Patch for landing

[Antoine Quint]

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.

[Antoine Quint]

Comment on attachment 292603 [details]
Patch for landing

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