Bug 188601 – Add script to generate WebContent service resource files and change XPC service main SPI to have it's own header

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ben Richards]

Add script to generate WebContent service resource files

[Ben Richards]

Created attachment 347162 [details]
Patch

[Sam Weinig]

Comment on attachment 347162 [details]
Patch

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

> Tools/Scripts/generate-webcontent-resources:1
> +#!/usr/bin/env python

Please add a license to this file.

[Ben Richards]

Created attachment 347166 [details]
Patch

[Ben Richards]

Created attachment 347178 [details]
Patch

Comment on attachment 347178 [details]
Patch

Some of the things the script does try to duplicate things that the build system does when building the WebContent service—and those things would need to be kept in sync between the script and the other parts of the project whenever they change.

I imagine that the next step after this patch is to have this script run as part of building the WebContent target (rather than keep it around for people to run it manually).

If that’s the case, I think you should consider a different approach to producing these resources, which would require far less duplication and keeping-things-in-sync: leverage the existing INFOPLIST_FILE and RUNLOOP_TYPE build settings as resolved for the platform being built, and get the final entitlements as written by the process-webcontent-or-plugin-entitlements.sh script into $(TEMP_FILE_DIR)/$(FULL_PRODUCT_NAME).xcent.

What do you think?

[Ben Richards]

(In reply to mitz from comment #5)
> Comment on attachment 347178 [details]
> Patch
> 
> Some of the things the script does try to duplicate things that the build
> system does when building the WebContent service—and those things would need
> to be kept in sync between the script and the other parts of the project
> whenever they change.
> 
> I imagine that the next step after this patch is to have this script run as
> part of building the WebContent target (rather than keep it around for
> people to run it manually).
> 
> If that’s the case, I think you should consider a different approach to
> producing these resources, which would require far less duplication and
> keeping-things-in-sync: leverage the existing INFOPLIST_FILE and
> RUNLOOP_TYPE build settings as resolved for the platform being built, and
> get the final entitlements as written by the
> process-webcontent-or-plugin-entitlements.sh script into
> $(TEMP_FILE_DIR)/$(FULL_PRODUCT_NAME).xcent.
> 
> What do you think?

My current idea for next steps would be to use this script in a build phase of any client application that would like to create a custom WebContent service. I liked this idea because it would allow the client application to specify where to place these resource files at build time (such as a derived sources directory).

Regarding your suggestion,

I see what you mean about RUNLOOP_TYPE. That should be moved to a build setting in the client's custom WebContent service and be resolved for the platform being built. But overall I think this script should remain in sync as long as paths to these specific files aren't changed (and my guess is that they don't change often).

I guess I may be slightly confused as to your proposed alternative approach. Is the suggestion that we should have the WebContent service place the Info.plist, WebContentProcess.xib and final .xcent file in a well known location at build time so that they can be used by a client application?

(In reply to Ben Richards from comment #6)
> (In reply to mitz from comment #5)
> > Comment on attachment 347178 [details]
> > Patch
> > 
> > Some of the things the script does try to duplicate things that the build
> > system does when building the WebContent service—and those things would need
> > to be kept in sync between the script and the other parts of the project
> > whenever they change.
> > 
> > I imagine that the next step after this patch is to have this script run as
> > part of building the WebContent target (rather than keep it around for
> > people to run it manually).
> > 
> > If that’s the case, I think you should consider a different approach to
> > producing these resources, which would require far less duplication and
> > keeping-things-in-sync: leverage the existing INFOPLIST_FILE and
> > RUNLOOP_TYPE build settings as resolved for the platform being built, and
> > get the final entitlements as written by the
> > process-webcontent-or-plugin-entitlements.sh script into
> > $(TEMP_FILE_DIR)/$(FULL_PRODUCT_NAME).xcent.
> > 
> > What do you think?
> 
> My current idea for next steps would be to use this script in a build phase
> of any client application that would like to create a custom WebContent
> service. I liked this idea because it would allow the client application to
> specify where to place these resource files at build time (such as a derived
> sources directory).

This idea is not compatible with how software gets built in Apple’s production build system: when one of those client application gets built, it doesn’t have access to the WebKit source tree, which contains the script and the various files it references.


> 
> Regarding your suggestion,
> 
> I see what you mean about RUNLOOP_TYPE. That should be moved to a build
> setting in the client's custom WebContent service and be resolved for the
> platform being built. But overall I think this script should remain in sync
> as long as paths to these specific files aren't changed (and my guess is
> that they don't change often).
> 
> I guess I may be slightly confused as to your proposed alternative approach.
> Is the suggestion that we should have the WebContent service place the
> Info.plist, WebContentProcess.xib and final .xcent file in a well known
> location at build time so that they can be used by a client application?

Yes, exactly.

[Ben Richards]

Understood. Do you have a recommendation for a good well known location?

(In reply to Ben Richards from comment #8)
> Understood. Do you have a recommendation for a good well known location?

A new subdirectory of the existing WebKit.framework/PrivateHeaders directory is a good place for this because:

1. It is part of WebKit, and already contains content used when building other projects (Safari uses surf from the Scripts subdirectory in there)
2. Header content is included in the SDK, and is therefore available to other projects building against the SDK, but is not included in the OS, so it doesn’t take up space on non-developers’ computers
3. This content is only usable in conjunction with some private WebKit API, so it belongs in PrivateHeaders rather than the public Headers location

s/surf/stuff/ :|

[Ben Richards]

Created attachment 347228 [details]
Patch

Comment on attachment 347228 [details]
Patch

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

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10515
> +			shellScript = "DST_DIR=\"${BUILT_PRODUCTS_DIR}/WebKit.framework/PrivateHeaders/WebContentResources\"\nmkdir -p \"$DST_DIR\"\n\necho \"${DST_DIR}\" > ~/Desktop/dir.txt\n\nfor ((i = 0; i < ${SCRIPT_INPUT_FILE_COUNT}; ++i)); do\n    eval WEBCONTENT_RESOURCE=\\${SCRIPT_INPUT_FILE_${i}}\n    ditto \"${WEBCONTENT_RESOURCE}\" \"${DST_DIR}/${WEBCONTENT_RESOURCE##*/}\"\ndone\n\n";

Probably not supposed to log to a file on the Desktop :)

But this still copies, e.g., the macOS resources regardless of the build target, and still hardcodes the source path (instead of using INFOPLIST_PATH), and doesn’t make any substitutions in the Info.plist. In fact, as currently written, I think what this script does is equivalent to what a Copy Files build phase could have done. But I think we don’t want that: we want the names of the destination files to be consistent, and we want to replace at least some of the variables in the plist.

[Ben Richards]

Created attachment 347285 [details]
Patch

[Ben Richards]

This one should be a bit better!

Comment on attachment 347285 [details]
Patch

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

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10147
> +				413E67622125E6E800C0F09E /* Copy Resources to Framework Private Headers */,

Feels like some qualifier should come before “resources”.

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10506
> +				"$(TEMP_FILE_DIR)/$(FULL_PRODUCT_NAME).xcent",

Can also list $(BUILT_PRODUCTS_DIR)/$(INFOPLIST_PATH) here.

[Ben Richards]

Created attachment 347297 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 347297 [details]
Patch for landing

Clearing flags on attachment: 347297

Committed r234958: <https://trac.webkit.org/changeset/234958>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43397768>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 188683

[Ben Richards]

Created attachment 347371 [details]
Patch

[Ben Richards]

Created attachment 347405 [details]
Patch

[Ben Richards]

Created attachment 347418 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 347418 [details]
Patch

Rejecting attachment 347418 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 347418, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=347418&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=188601&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 347418 from bug 188601.
Fetching: https://bugs.webkit.org/attachment.cgi?id=347418
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 7 diffs from patch file(s).
patching file Source/WebKit/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/Scripts/copy-webcontent-resources-to-private-headers.sh
patching file Source/WebKit/Shared/API/c/WKXPCServiceMain.cpp
patching file Source/WebKit/Shared/API/c/WKXPCServiceMain.h
patching file Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm
patching file Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h
patching file Source/WebKit/WebKit.xcodeproj/project.pbxproj
Hunk #1 succeeded at 1038 with fuzz 2 (offset 82 lines).
Hunk #2 succeeded at 3576 (offset 124 lines).
Hunk #3 succeeded at 8735 (offset 274 lines).
Hunk #4 succeeded at 8954 (offset 275 lines).
Hunk #5 succeeded at 10312 (offset 275 lines).
Hunk #6 succeeded at 10447 (offset 276 lines).
Hunk #7 succeeded at 10820 (offset 295 lines).
Hunk #8 succeeded at 11000 (offset 295 lines).
Hunk #9 FAILED at 12155.
1 out of 9 hunks FAILED -- saving rejects to file Source/WebKit/WebKit.xcodeproj/project.pbxproj.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/8897700

[Alex Christensen]

Created attachment 349958 [details]
patch

[Alex Christensen]

(In reply to Sam Weinig from comment #2)
> Comment on attachment 347162 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347162&action=review
> 
> > Tools/Scripts/generate-webcontent-resources:1
> > +#!/usr/bin/env python
> 
> Please add a license to this file.

Added a license comment and committed to http://trac.webkit.org/r236092

[Ryan Haddad]

Rolled out change in https://trac.webkit.org/r236097 because it broke internal builds.

[Alex Christensen]

The internal build "failure" was a verifier not liking ${PRODUCT_BUNDLE_IDENTIFIER} in the Info.plist.  I'll copy the processed one instead.

(In reply to Alex Christensen from comment #28)
> The internal build "failure" was a verifier not liking
> ${PRODUCT_BUNDLE_IDENTIFIER} in the Info.plist.  I'll copy the processed one
> instead.

That might make it harder for clients to use their own value for that key. Could you just name the file something different than Info.plist?

[Alex Christensen]

Created attachment 350038 [details]
patch

[Alex Christensen]

Created attachment 350047 [details]
patch

[Alex Christensen]

http://trac.webkit.org/r236152