diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt index 6a8bd1858..14b62e124 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt @@ -113,7 +113,7 @@ class GutenbergView : FrameLayout { var requestInterceptor: GutenbergRequestInterceptor = DefaultGutenbergRequestInterceptor() /** - * Optional delegate for transforming media before upload (resize, transcode, + * Optional processor that transforms media before upload (resize, transcode, * strip EXIF). * * To perform the upload yourself, set [mediaUploader] instead. @@ -123,9 +123,9 @@ class GutenbergView : FrameLayout { * the page begins loading, and advertised to the page then; setting it * afterward has no effect, so the setter throws to surface the mistake. */ - var mediaUploadDelegate: MediaUploadDelegate? = null + var mediaProcessor: MediaProcessor? = null set(value) { - check(!hasStartedLoading) { lateMediaAssignmentMessage("mediaUploadDelegate") } + check(!hasStartedLoading) { lateMediaAssignmentMessage("mediaProcessor") } field = value } @@ -134,11 +134,11 @@ class GutenbergView : FrameLayout { * queue, resumable transport). Setting it makes the host own every upload and its * whole lifecycle; GutenbergKit stays out of the network entirely for media. * - * Same lifecycle rules as [mediaUploadDelegate]: set it before the editor loads, + * Same lifecycle rules as [mediaProcessor]: set it before the editor loads, * and this view owns it for its lifetime — so you needn't retain it yourself, just * don't strongly retain this [GutenbergView] from your uploader. * - * A [mediaUploadDelegate] can still transform the file first; only delivery moves + * A [mediaProcessor] can still transform the file first; only delivery moves * to the uploader. */ var mediaUploader: MediaUploader? = null @@ -155,7 +155,7 @@ class GutenbergView : FrameLayout { /** * True once the editor page has begun loading and the upload server's - * configuration has been captured. After this the [mediaUploadDelegate] can no + * configuration has been captured. After this the [mediaProcessor] can no * longer take effect, so its setter throws. */ @Volatile private var hasStartedLoading = false @@ -663,13 +663,13 @@ class GutenbergView : FrameLayout { /** * Invoked when the editor page begins loading. Starts the upload server once — - * capturing the [mediaUploadDelegate] provided before load — then advertises + * capturing the [mediaProcessor] provided before load — then advertises * the editor globals (including the server's port and token) to the page. * * Starting the server here, on the UI thread, rather than from the - * [mediaUploadDelegate] setter keeps its whole lifecycle — start here, stop in + * [mediaProcessor] setter keeps its whole lifecycle — start here, stop in * [onDetachedFromWindow] — on the UI thread, so it can't race a - * background-thread delegate assignment. + * background-thread processor assignment. */ private fun onEditorPageStarted() { if (!hasStartedLoading) { @@ -697,9 +697,9 @@ class GutenbergView : FrameLayout { private fun startUploadServer() { // Nothing to route through the native server unless the host provided a - // delegate or an uploader — leave it down and let uploads fall to the default + // processor or an uploader — leave it down and let uploads fall to the default // WebView path. (Matches iOS.) - if (mediaUploadDelegate == null && mediaUploader == null) return + if (mediaProcessor == null && mediaUploader == null) return // The native upload server relays through InternalMediaClient, which needs a // site root and an auth header (every host provides one — the editor injects @@ -733,7 +733,7 @@ class GutenbergView : FrameLayout { siteApiNamespace = configuration.siteApiNamespace.toList() ) uploadServer = MediaUploadServer( - uploadDelegate = mediaUploadDelegate, + processor = mediaProcessor, internalClient = internalClient, uploader = mediaUploader, cacheDir = context.cacheDir, diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt index abe448e51..685db033c 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -57,14 +57,14 @@ internal class MediaUploadResponse( ) /** - * The result of a delegate's [MediaUploadDelegate.processFile]. + * The result of a processor's [MediaProcessor.processFile]. */ sealed class ProcessedProxyFile { - /** The delegate did not modify the file; the original upload is forwarded unchanged. */ + /** The processor did not modify the file; the original upload is forwarded unchanged. */ data object Original : ProcessedProxyFile() /** - * The delegate produced a file to upload, along with its MIME type and + * The processor produced a file to upload, along with its MIME type and * filename. Both are used verbatim, so a format change (e.g. transcoding MOV * to MP4, or an in-place EXIF strip) must report the resulting type and * filename for WordPress to store the file correctly. @@ -75,23 +75,23 @@ sealed class ProcessedProxyFile { /** * Transforms media before GutenbergKit delivers it. * - * A delegate only changes *bytes* — GutenbergKit still uploads the result to the + * A processor only changes *bytes* — GutenbergKit still uploads the result to the * configured site and owns the whole lifecycle (retries, cleanup). Because it never * performs the upload itself, it cannot deliver media to the wrong place. Set - * [GutenbergView.mediaUploadDelegate] to resize images, transcode video, strip EXIF, + * [GutenbergView.mediaProcessor] to resize images, transcode video, strip EXIF, * etc. * * This is the safe, common extension point: most hosts want only this. To perform the * upload yourself, implement [MediaUploader] instead. */ -interface MediaUploadDelegate { +interface MediaProcessor { /** - * Whether this delegate might transform a file with the given metadata. + * Whether this processor might transform a file with the given metadata. * * A cheap, metadata-only gate the server consults *before* materializing the * upload to a temp file. Return false to decline a file by type — e.g. an - * image-only delegate returning false for a video — so the server forwards - * the original upload to WordPress without first copying a file the delegate + * image-only processor returning false for a video — so the server forwards + * the original upload to WordPress without first copying a file the processor * won't touch. * * With a [MediaUploader] set this can't decline the upload itself — an uploader @@ -130,7 +130,7 @@ data class MediaUploadField(val name: String, val value: String) * Everything a [MediaUploader] needs to reproduce a native upload: the file to send, * its metadata, the editor's non-file form fields, and the request's query. * - * @property file The file to upload — already processed, if a [MediaUploadDelegate] ran. + * @property file The file to upload — already processed, if a [MediaProcessor] ran. * @property mimeType The file's MIME type. * @property filename The file's name. * @property fields The editor's non-file form fields, in order, each decoded as UTF-8 — @@ -203,7 +203,7 @@ interface MediaUploader { * stop on detach. */ internal class MediaUploadServer( - private val uploadDelegate: MediaUploadDelegate?, + private val processor: MediaProcessor?, private val internalClient: InternalMediaClient?, private val uploader: MediaUploader? = null, cacheDir: File? = null, @@ -364,25 +364,25 @@ internal class MediaUploadServer( val mimeType = filePart.contentType val filename = filePart.filename ?: "upload" - // Ask the delegate — from metadata alone — whether it will touch a file + // Ask the processor — from metadata alone — whether it will touch a file // like this. If not, forward the original upload to WordPress directly, - // skipping a full temp-file copy of a file the delegate won't process - // (e.g. a video handed to an image-only delegate). + // skipping a full temp-file copy of a file the processor won't process + // (e.g. a video handed to an image-only processor). // An uploader takes over delivery for *every* file, so with one set there is no // passthrough to fall to and the gate can't decline the upload outright. It // still decides whether processFile runs, though — a declined file is handed to - // the uploader unprocessed rather than to a delegate that said it won't touch it + // the uploader unprocessed rather than to a processor that said it won't touch it // — so the answer is carried into processAndUpload rather than short-circuited // away here. Asked exactly once per upload, matching iOS. - val delegateWantsFile = uploadDelegate?.handlesFile(mimeType, filename) == true - if (uploader == null && !delegateWantsFile) { + val processorWantsFile = processor?.handlesFile(mimeType, filename) == true + if (uploader == null && !processorWantsFile) { return passthroughResponse(request, query) } val tempFile = writePartToTempFile(filePart) ?: return errorResponse(500, "Failed to save file") - return processAndRespond(request, tempFile, filePart, extraParts, query, delegateWantsFile) + return processAndRespond(request, tempFile, filePart, extraParts, query, processorWantsFile) } @Suppress("TooGenericExceptionCaught") @@ -410,7 +410,7 @@ internal class MediaUploadServer( * The response's own `Content-Type` wins over the JSON default, matched * case-insensitively — HTTP header names are case-insensitive, and * [HttpResponse] serializes every entry it is given, so a plain map merge - * would emit the name twice for a delegate that spells it `content-type`. + * would emit the name twice for a processor that spells it `content-type`. */ private fun relayResponse(response: MediaUploadResponse): HttpResponse { val hasContentType = response.headers.keys.any { it.lowercase() == "content-type" } @@ -466,12 +466,12 @@ internal class MediaUploadServer( @Suppress("TooGenericExceptionCaught") private suspend fun processAndRespond( request: HttpRequest, tempFile: File, filePart: MultipartPart, - extraParts: List, query: String, delegateWantsFile: Boolean + extraParts: List, query: String, processorWantsFile: Boolean ): HttpResponse { try { val uploadResult = processAndUpload( tempFile, filePart.contentType, filePart.filename ?: "upload", - extraParts, query, delegateWantsFile + extraParts, query, processorWantsFile ) val response = when (uploadResult) { is UploadResult.Uploaded -> { @@ -479,7 +479,7 @@ internal class MediaUploadServer( uploadResult.response } is UploadResult.Passthrough -> { - // Delegate didn't modify the file — forward the original + // The processor didn't modify the file — forward the original // request body to WordPress without re-encoding. Log.d(TAG, "Passthrough: forwarding original request body to WordPress") performPassthroughUpload(request, query) @@ -493,7 +493,7 @@ internal class MediaUploadServer( throw e // Never swallow coroutine cancellation. } catch (e: Exception) { // Any other failure — IOException from the upload call, JSON parse - // errors, a throwing host delegate, or "no internal media client + // errors, a throwing host processor, or "no internal media client // configured" — must still be answered WITH CORS headers. Otherwise // it escapes to HttpServer's header-less 500 fallback and the browser // rejects the preflighted cross-origin fetch with an opaque "Failed to @@ -506,7 +506,7 @@ internal class MediaUploadServer( } } - // MARK: - Delegate Pipeline + // MARK: - Processor Pipeline private sealed class UploadResult { data class Uploaded(val response: MediaUploadResponse) : UploadResult() @@ -527,21 +527,21 @@ internal class MediaUploadServer( private suspend fun processAndUpload( file: File, mimeType: String, filename: String, - extraParts: List, query: String, delegateWantsFile: Boolean + extraParts: List, query: String, processorWantsFile: Boolean ): UploadResult { - // Process (resize, transcode, etc.) — but only for a file the delegate's - // metadata gate accepted. handlesFile returning false is the delegate saying it + // Process (resize, transcode, etc.) — but only for a file the processor's + // metadata gate accepted. handlesFile returning false is the processor saying it // won't touch a file like this, so handing it one anyway would break the // contract the gate documents. With an uploader set the file still gets // delivered; it just skips processing on its way there. - val processed = if (delegateWantsFile) { - uploadDelegate?.processFile(file, mimeType, filename) ?: ProcessedProxyFile.Original + val processed = if (processorWantsFile) { + processor?.processFile(file, mimeType, filename) ?: ProcessedProxyFile.Original } else { ProcessedProxyFile.Original } // Resolve the file to upload and its metadata. Processed uses the - // delegate's values verbatim, so a format change is reported to WordPress. + // processor's values verbatim, so a format change is reported to WordPress. val targetFile: File val targetMimeType: String val targetFilename: String @@ -590,7 +590,7 @@ internal class MediaUploadServer( ?: error("No media uploader or internal media client configured") return UploadResult.Uploaded(result) } finally { - // The processed file (if the delegate produced a new one) is ours to + // The processed file (if the processor produced a new one) is ours to // clean up — covers the success and throw paths alike. if (targetFile != file) { targetFile.delete() @@ -718,7 +718,7 @@ internal open class InternalMediaClient( /** * Forwards the original request body to WordPress without re-encoding. * - * Used when the delegate's `processFile` returned the file unchanged — + * Used when the processor's `processFile` returned the file unchanged — * the incoming multipart body is already valid for WordPress. */ open suspend fun passthroughUpload( diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt index 5db7e718a..bc3a1df0d 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt @@ -44,7 +44,7 @@ class GutenbergViewUploadServerTest { /** * Invokes the private `onEditorPageStarted` hook (fired from the WebViewClient's * `onPageStarted`) to simulate the editor page beginning to load — the point at - * which the delegate is captured and the upload server starts. + * which the processor is captured and the upload server starts. */ private fun startLoading(view: GutenbergView) { val method = GutenbergView::class.java.getDeclaredMethod("onEditorPageStarted") @@ -62,15 +62,15 @@ class GutenbergViewUploadServerTest { private fun idle() = shadowOf(Looper.getMainLooper()).idle() @Test - fun `the upload server starts when the page begins loading, capturing the delegate`() { + fun `the upload server starts when the page begins loading, capturing the processor`() { val view = makeView() try { - // A delegate provided before load is captured when the page starts. - view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + // A processor provided before load is captured when the page starts. + view.mediaProcessor = mock(MediaProcessor::class.java) startLoading(view) idle() assertNotNull( - "a delegate provided before load should bring up the upload server", + "a processor provided before load should bring up the upload server", uploadServerOf(view) ) } finally { @@ -79,14 +79,14 @@ class GutenbergViewUploadServerTest { } @Test - fun `no delegate means no upload server`() { + fun `no processor means no upload server`() { val view = makeView() try { - // No delegate provided — uploads should use the default WebView path. + // No processor provided — uploads should use the default WebView path. startLoading(view) idle() assertNull( - "with no delegate, no upload server should be started", + "with no processor, no upload server should be started", uploadServerOf(view) ) } finally { @@ -95,7 +95,7 @@ class GutenbergViewUploadServerTest { } @Test - fun `the upload server starts for an uploader with no delegate`() { + fun `the upload server starts for an uploader with no processor`() { val view = makeView() try { // An uploader alone must bring the server up: it is the only route the @@ -128,15 +128,15 @@ class GutenbergViewUploadServerTest { } @Test - fun `setting the delegate after the page has started loading throws`() { + fun `setting the processor after the page has started loading throws`() { val view = makeView() try { startLoading(view) idle() - // The delegate is captured at load; a later assignment is a programmer + // The processor is captured at load; a later assignment is a programmer // error and must surface loudly rather than silently do nothing. assertThrows(IllegalStateException::class.java) { - view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + view.mediaProcessor = mock(MediaProcessor::class.java) } } finally { detach(view) @@ -146,7 +146,7 @@ class GutenbergViewUploadServerTest { @Test fun `detaching the view stops and clears the upload server`() { val view = makeView() - view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + view.mediaProcessor = mock(MediaProcessor::class.java) startLoading(view) idle() assertNotNull(uploadServerOf(view)) diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt index a842c2bf4..60af5c723 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -33,7 +33,7 @@ class MediaUploadServerTest { @Before fun setUp() { - server = MediaUploadServer(uploadDelegate = null, internalClient = null, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = null, internalClient = null, cacheDir = tempFolder.root) } @After @@ -53,7 +53,7 @@ class MediaUploadServerTest { fun `stop cancels an internally-created scope but leaves a caller-supplied one alone`() { // No scope supplied → the server owns one, which stop() must cancel. val owningServer = - MediaUploadServer(uploadDelegate = null, internalClient = null, cacheDir = tempFolder.root) + MediaUploadServer(processor = null, internalClient = null, cacheDir = tempFolder.root) val ownedScope = ownedScopeOf(owningServer) assertNotNull("server should own a scope when none is supplied", ownedScope) assertTrue(ownedScope!!.isActive) @@ -63,7 +63,7 @@ class MediaUploadServerTest { // A caller-supplied scope belongs to the caller — stop() must not cancel it. val callerScope = CoroutineScope(Dispatchers.IO) val borrowingServer = MediaUploadServer( - uploadDelegate = null, + processor = null, internalClient = null, cacheDir = tempFolder.root, scope = callerScope @@ -152,7 +152,7 @@ class MediaUploadServerTest { // the ordinary path rather than an edge case. val uploader = ContentTypeDeleteClient() server.stop() - server = MediaUploadServer(uploadDelegate = null, internalClient = uploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = null, internalClient = uploader, cacheDir = tempFolder.root) val response = sendRawRequest( method = "DELETE", @@ -174,7 +174,7 @@ class MediaUploadServerTest { val client = MockInternalMediaClient() server.stop() server = MediaUploadServer( - uploadDelegate = null, internalClient = client, uploader = uploader, cacheDir = tempFolder.root + processor = null, internalClient = client, uploader = uploader, cacheDir = tempFolder.root ) val boundary = "test-boundary-uploader" @@ -205,7 +205,7 @@ class MediaUploadServerTest { val uploader = RecordingUploader() server.stop() server = MediaUploadServer( - uploadDelegate = null, internalClient = MockInternalMediaClient(), uploader = uploader, + processor = null, internalClient = MockInternalMediaClient(), uploader = uploader, cacheDir = tempFolder.root ) @@ -245,15 +245,15 @@ class MediaUploadServerTest { } @Test - fun `a delegate still processes the file an uploader delivers`() { - // With both set, the delegate still processes — only delivery moves to + fun `a processor still processes the file an uploader delivers`() { + // With both set, the processor still processes — only delivery moves to // the uploader. val uploader = RecordingUploader() - val delegate = ProcessOnlyDelegate() + val processor = ProcessOnlyProcessor() val client = MockInternalMediaClient() server.stop() server = MediaUploadServer( - uploadDelegate = delegate, internalClient = client, uploader = uploader, + processor = processor, internalClient = client, uploader = uploader, cacheDir = tempFolder.root ) @@ -270,21 +270,21 @@ class MediaUploadServerTest { ) assertNotNull(uploader.received) - assertTrue(delegate.processFileCalled) + assertTrue(processor.processFileCalled) assertFalse(client.uploadCalled) } @Test - fun `an uploader sees a file the delegate's metadata gate would have declined`() { - // The gate exists to skip a temp copy for a file the delegate won't touch. An + fun `an uploader sees a file the processor's metadata gate would have declined`() { + // The gate exists to skip a temp copy for a file the processor won't touch. An // uploader takes over delivery for every file, so passing through here would // silently bypass it. val uploader = RecordingUploader() val client = MockInternalMediaClient() - val delegate = DeclineByMetadataDelegate() + val processor = DeclineByMetadataProcessor() server.stop() server = MediaUploadServer( - uploadDelegate = delegate, internalClient = client, uploader = uploader, + processor = processor, internalClient = client, uploader = uploader, cacheDir = tempFolder.root ) @@ -303,16 +303,16 @@ class MediaUploadServerTest { assertEquals("clip.mov", uploader.received?.filename) assertFalse(client.passthroughUploadCalled) // ...but a declined file must still not reach processFile: handlesFile - // returning false is the delegate saying it won't touch a file like this. - assertFalse(delegate.processFileCalled) + // returning false is the processor saying it won't touch a file like this. + assertFalse(processor.processFileCalled) } @Test fun `routes upload with a query string and relays the query`() { - val delegate = ProcessOnlyDelegate() + val processor = ProcessOnlyProcessor() val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, internalClient = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = mockUploader, cacheDir = tempFolder.root) // `@wordpress/media-utils` uploads to `/wp/v2/media?_embed=wp:featuredmedia`, // so the middleware forwards that query on to the native server. Routing must @@ -331,7 +331,7 @@ class MediaUploadServerTest { ) assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) - // The delegate returns Original, so this is the passthrough branch. + // The processor returns Original, so this is the passthrough branch. // Pin which branch ran — `lastQuery` is recorded by both, so without this // the query assertion would pass even if routing collapsed onto one path. assertTrue(mockUploader.passthroughUploadCalled) @@ -339,14 +339,14 @@ class MediaUploadServerTest { assertEquals("?_embed=wp:featuredmedia", mockUploader.lastQuery) } - // MARK: - Upload with delegate + // MARK: - Upload with processor @Test - fun `processes with the delegate, then delivers through the internal client`() { - val delegate = TranscodingDelegate() + fun `processes with the processor, then delivers through the internal client`() { + val processor = TranscodingProcessor() val client = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, internalClient = client, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = client, cacheDir = tempFolder.root) val boundary = "test-boundary-123" val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray()) @@ -362,7 +362,7 @@ class MediaUploadServerTest { ) assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) - // The delegate only transforms; GutenbergKit performs the upload. + // The processor only transforms; GutenbergKit performs the upload. assertTrue(client.uploadCalled) // The server relays WordPress's raw response body verbatim. @@ -373,11 +373,11 @@ class MediaUploadServerTest { } @Test - fun `forwards the delegate's processed metadata to the uploader`() { - val delegate = TranscodingDelegate() + fun `forwards the processor's processed metadata to the uploader`() { + val processor = TranscodingProcessor() val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, internalClient = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-meta" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) @@ -392,7 +392,7 @@ class MediaUploadServerTest { body = body ) - // The delegate changed the format, so the uploader must receive the new + // The processor changed the format, so the uploader must receive the new // metadata — not the original video/quicktime + clip.mov. assertTrue(mockUploader.uploadCalled) assertEquals("video/mp4", mockUploader.lastUploadMimeType) @@ -400,11 +400,11 @@ class MediaUploadServerTest { } @Test - fun `deletes the delegate's processed file after upload`() { - val delegate = TranscodingDelegate() + fun `deletes the processor's processed file after upload`() { + val processor = TranscodingProcessor() val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, internalClient = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-cleanup" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) @@ -419,10 +419,10 @@ class MediaUploadServerTest { body = body ) - // The server owns the file the delegate produced and must delete it once the + // The server owns the file the processor produced and must delete it once the // upload finishes — the finally in processAndUpload covers success and throw // paths alike. A leaked processed file is a full-size temp per upload. - val processed = requireNotNull(delegate.producedFile) { "processFile was not called" } + val processed = requireNotNull(processor.producedFile) { "processFile was not called" } assertFalse("Processed temp file should be deleted after upload", processed.exists()) } @@ -442,7 +442,7 @@ class MediaUploadServerTest { // one — a flipped comparison would do the opposite and wipe an in-flight upload. server.stop() server = MediaUploadServer( - uploadDelegate = null, + processor = null, internalClient = null, cacheDir = tempFolder.root, ioDispatcher = Dispatchers.Unconfined @@ -455,12 +455,12 @@ class MediaUploadServerTest { // MARK: - Fallback to the internal media client @Test - fun `uses passthrough when delegate does not modify file`() { - val delegate = ProcessOnlyDelegate() + fun `uses passthrough when processor does not modify file`() { + val processor = ProcessOnlyProcessor() val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, internalClient = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-456" val body = buildMultipartBody(boundary, "doc.pdf", "application/pdf", "fake pdf data".toByteArray()) @@ -476,7 +476,7 @@ class MediaUploadServerTest { ) assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) - assertTrue(delegate.processFileCalled) + assertTrue(processor.processFileCalled) // Passthrough: original body forwarded directly, not re-encoded. assertTrue(mockUploader.passthroughUploadCalled) assertFalse(mockUploader.uploadCalled) @@ -486,12 +486,12 @@ class MediaUploadServerTest { } @Test - fun `skips processing and the temp copy when the delegate declines by metadata`() { - val delegate = DeclineByMetadataDelegate() + fun `skips processing and the temp copy when the processor declines by metadata`() { + val processor = DeclineByMetadataProcessor() val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, internalClient = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-decline" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "fake movie".toByteArray()) @@ -507,9 +507,9 @@ class MediaUploadServerTest { ) assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) - // Declined by metadata → the delegate is never asked to process (so the + // Declined by metadata → the processor is never asked to process (so the // file was never materialized), and the upload is passed through directly. - assertFalse(delegate.processFileCalled) + assertFalse(processor.processFileCalled) assertTrue(mockUploader.passthroughUploadCalled) assertFalse(mockUploader.uploadCalled) } @@ -894,7 +894,7 @@ class MediaUploadServerTest { // MARK: - Mocks - private class ProcessOnlyDelegate : MediaUploadDelegate { + private class ProcessOnlyProcessor : MediaProcessor { @Volatile var processFileCalled = false override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { @@ -908,7 +908,7 @@ class MediaUploadServerTest { * must pass through without materializing the file; with one, delivery still * happens but [processFile] must not be called. [processFileCalled] pins both. */ - private class DeclineByMetadataDelegate : MediaUploadDelegate { + private class DeclineByMetadataProcessor : MediaProcessor { @Volatile var processFileCalled = false override fun handlesFile(mimeType: String, filename: String): Boolean = false @@ -919,9 +919,9 @@ class MediaUploadServerTest { } } - /** A delegate that produces a new file with changed metadata (e.g. a transcode). */ - private class TranscodingDelegate : MediaUploadDelegate { - /** The processed file this delegate wrote, for cleanup assertions. */ + /** A processor that produces a new file with changed metadata (e.g. a transcode). */ + private class TranscodingProcessor : MediaProcessor { + /** The processed file this processor wrote, for cleanup assertions. */ @Volatile var producedFile: File? = null override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { diff --git a/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt b/android/app/src/main/java/com/example/gutenbergkit/DemoMediaProcessor.kt similarity index 94% rename from android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt rename to android/app/src/main/java/com/example/gutenbergkit/DemoMediaProcessor.kt index dcad5644e..b50477dd2 100644 --- a/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt +++ b/android/app/src/main/java/com/example/gutenbergkit/DemoMediaProcessor.kt @@ -5,24 +5,24 @@ import android.graphics.BitmapFactory import android.graphics.Matrix import android.media.ExifInterface import android.util.Log -import org.wordpress.gutenberg.MediaUploadDelegate +import org.wordpress.gutenberg.MediaProcessor import org.wordpress.gutenberg.ProcessedProxyFile import java.io.File import java.io.IOException /** - * Demo media upload delegate that resizes images to a maximum dimension of 2000px. + * Demo media processor that resizes images to a maximum dimension of 2000px. * * Only transforms the file; GutenbergKit performs the upload. */ -class DemoMediaUploadDelegate : MediaUploadDelegate { +class DemoMediaProcessor : MediaProcessor { companion object { - private const val TAG = "DemoMediaUploadDelegate" + private const val TAG = "DemoMediaProcessor" } // Only non-GIF images are ever resized (see processFile), so decline // everything else by metadata — the server then skips copying a file this - // delegate would only pass through. + // processor would only pass through. override fun handlesFile(mimeType: String, filename: String): Boolean { return mimeType.startsWith("image/") && mimeType != "image/gif" } diff --git a/android/app/src/main/java/com/example/gutenbergkit/EditorActivity.kt b/android/app/src/main/java/com/example/gutenbergkit/EditorActivity.kt index 20f3e84b2..c2a48ac0f 100644 --- a/android/app/src/main/java/com/example/gutenbergkit/EditorActivity.kt +++ b/android/app/src/main/java/com/example/gutenbergkit/EditorActivity.kt @@ -338,7 +338,7 @@ fun EditorScreen( } }) if (enableNativeMediaUpload) { - mediaUploadDelegate = DemoMediaUploadDelegate() + mediaProcessor = DemoMediaProcessor() } onGutenbergViewCreated(this) } diff --git a/docs/integration.md b/docs/integration.md index 4ac4e2ea5..b11cf8ee8 100644 --- a/docs/integration.md +++ b/docs/integration.md @@ -246,33 +246,34 @@ val configuration = EditorConfiguration.builder() ## Media Handling -The host can customize how media is processed and uploaded by supplying a -`MediaUploadDelegate` at init: +The host can transform media before upload by supplying a `MediaProcessor` at init. +To take over the upload itself, supply a `MediaUploader` instead — a processor only +changes bytes; GutenbergKit still delivers them. ```swift let editor = EditorViewController( configuration: configuration, - mediaUploadDelegate: ResizingDelegate(maxDimension: 2000) + mediaProcessor: ResizingProcessor(maxDimension: 2000) ) ``` ### Don't conform the object that owns the editor -GutenbergKit never hands your delegate the editor: every value crossing that boundary is a -value type — a file URL, a MIME type, a filename. So a delegate can only reach the editor +GutenbergKit never hands your processor the editor: every value crossing that boundary is a +value type — a file URL, a MIME type, a filename. So a processor can only reach the editor if you put it there. That happens when you conform the object that already holds the editor in order to drive -it. The editor holds the delegate strongly in return — deliberately, so an in-flight upload +it. The editor holds the processor strongly in return — deliberately, so an in-flight upload can't lose it mid-request — which closes a retain cycle ARC cannot break. The editor is never deallocated, and each one strands a bound loopback listener. ```swift -// Leaks: coordinator -> editor -> mediaUploadDelegate -> coordinator -final class PostEditorCoordinator: MediaUploadDelegate { +// Leaks: coordinator -> editor -> mediaProcessor -> coordinator +final class PostEditorCoordinator: MediaProcessor { var editor: EditorViewController! init(blog: Blog, configuration: EditorConfiguration) { - editor = EditorViewController(configuration: configuration, mediaUploadDelegate: self) + editor = EditorViewController(configuration: configuration, mediaProcessor: self) } } ``` @@ -287,7 +288,7 @@ final class PostEditorCoordinator { init(blog: Blog, configuration: EditorConfiguration) { editor = EditorViewController( configuration: configuration, - mediaUploadDelegate: BlogMediaDelegate(siteID: blog.dotComID, maxDimension: 2000) + mediaProcessor: BlogMediaProcessor(siteID: blog.dotComID, maxDimension: 2000) ) } } @@ -298,12 +299,12 @@ are finished with the editor. It is terminal — the editor cannot upload or del afterwards — so call it when the editor is going away, not when it is merely covered or backgrounded. -### Reusing a delegate across editor sessions +### Reusing a processor across editor sessions -The editor holds the delegate for its lifetime and releases it when it goes, so a delegate +The editor holds the processor for its lifetime and releases it when it goes, so a processor built for a single editor needs no reference of its own. To use the same instance for several editors, keep your own reference — the editor drops only its own. Sharing is also -the safer shape: a delegate owned by something longer-lived than any editor is a leaf, so +the safer shape: a processor owned by something longer-lived than any editor is a leaf, so it cannot form the cycle above and there is nothing to tear down. It may be called concurrently if more than one editor is live, and it must not hold on to any editor it has served. diff --git a/ios/Demo-iOS/Sources/Views/EditorView.swift b/ios/Demo-iOS/Sources/Views/EditorView.swift index f2104dacd..8d358c48a 100644 --- a/ios/Demo-iOS/Sources/Views/EditorView.swift +++ b/ios/Demo-iOS/Sources/Views/EditorView.swift @@ -136,7 +136,7 @@ private struct _EditorView: UIViewControllerRepresentable { let viewController = EditorViewController( configuration: configuration, dependencies: dependencies, - mediaUploadDelegate: enableNativeMediaUpload ? context.coordinator : nil + mediaProcessor: enableNativeMediaUpload ? context.coordinator : nil ) viewController.delegate = context.coordinator viewController.webView.isInspectable = true @@ -190,7 +190,7 @@ private struct _EditorView: UIViewControllerRepresentable { } @MainActor - class Coordinator: NSObject, EditorViewControllerDelegate, MediaUploadDelegate { + class Coordinator: NSObject, EditorViewControllerDelegate, MediaProcessor { let viewModel: EditorViewModel init(viewModel: EditorViewModel) { @@ -296,11 +296,11 @@ private struct _EditorView: UIViewControllerRepresentable { return nil } - // MARK: - MediaUploadDelegate + // MARK: - MediaProcessor /// Only non-GIF images are ever resized (see `processFile`), so decline /// everything else by metadata — the server then skips copying a file - /// this delegate would only pass through. + /// this processor would only pass through. nonisolated func handlesFile(ofType mimeType: String, named _: String) -> Bool { mimeType.hasPrefix("image/") && mimeType != "image/gif" } diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index 8e27697d6..1597e0a8f 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -104,60 +104,64 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// Used by `EditorViewController.warmup()` to reduce first-render latency. private let isWarmupMode: Bool - /// Delegate for transforming media before upload — resize, transcode, strip EXIF. + /// Transforms media before upload — resize, transcode, strip EXIF. /// /// To perform the upload yourself, pass a ``mediaUploader`` instead. /// /// Supplied at `init`, with the rest of the editor's configuration, because that is - /// when it takes effect: the delegate is captured into the page's initial + /// when it takes effect: the processor is captured into the page's initial /// configuration as the editor begins loading. Taking it there rather than through a /// settable property leaves no window in which a host can hand one over too late for /// it to ever run. (Android keeps a settable property and a fail-fast for exactly /// that case — a `View` is inflated, not constructed by the host, so there is no /// initializer to put this in.) /// - /// The editor holds this strongly for its lifetime, so a delegate built for a single + /// The rest of this describes a **reference-type** conformer, which is what a host + /// that needs to observe or reuse its processor will write. ``MediaProcessor`` is not + /// class-bound, and a value-type conformer is copied at `init` — see the protocol's + /// documentation for what that changes. + /// + /// The editor holds this strongly for its lifetime, so a processor built for a single /// editor needs no reference of its own. **To reuse one across editor sessions, keep /// your own reference to it.** The editor's release — on `deinit`, or on - /// ``stopMediaHandling()`` — drops only *its* reference: a delegate the host still + /// ``stopMediaHandling()`` — drops only *its* reference: a processor the host still /// holds survives to be passed to the next editor, and one nobody else holds does not. /// /// That release is not always prompt, and not always on the main thread. A request in - /// flight holds its own reference until it unwinds, so if this editor is the delegate's - /// last owner, the delegate is freed when the host's `processFile` returns — on the + /// flight holds its own reference until it unwinds, so if this editor is the processor's + /// last owner, the processor is freed when the host's `processFile` returns — on the /// task's executor, not the caller's thread. Keep a reference of your own if that /// matters to the conformer. /// - /// Sharing an instance is the safer shape rather than a compromise. A delegate owned + /// Sharing an instance is the safer shape rather than a compromise. A processor owned /// by something longer-lived than any editor is a leaf, so the cycle below cannot form /// and there is nothing to call. Two caveats when you do: it may be called /// concurrently if more than one editor is live, and it must not hold on to any editor /// it has served. /// /// The one rule: **don't conform the object that owns this editor.** Nothing here - /// hands a delegate the editor — every value crossing this boundary is a value type — + /// hands a processor the editor — every value crossing this boundary is a value type — /// so the only way one reaches the editor is if you store it there, which is what /// happens when the coordinator that drives the editor also conforms. Holding this - /// strongly is deliberate — losing the delegate mid-request was the failure actually + /// strongly is deliberate — losing the processor mid-request was the failure actually /// being hit — but it means that shape closes a cycle ARC cannot break, and the editor /// cannot detect its own teardown to break it for you. If you must write it, call /// ``stopMediaHandling()`` when you are done with the editor. - // swiftlint:disable:next weak_delegate - public private(set) var mediaUploadDelegate: (any MediaUploadDelegate)? + public private(set) var mediaProcessor: (any MediaProcessor)? /// Takes over media upload on the host's own stack (background session, offline /// queue, resumable transport). Passing one makes the host own every upload and its /// whole lifecycle; GutenbergKit stays out of the network entirely for media. /// - /// Same ownership rules as ``mediaUploadDelegate``: supplied at `init`, held for the + /// Same ownership rules as ``mediaProcessor``: supplied at `init`, held for the /// editor's lifetime, and not conformed by the object that owns the editor. /// - /// Reuse is the expected shape here, more so than for a delegate: the transports this + /// Reuse is the expected shape here, more so than for a processor: the transports this /// exists for outlive any one editor by definition — a background `URLSession` has a /// fixed identifier and must survive app relaunch, an offline queue spans sessions. /// Build the uploader once, hold it, and pass the same instance to each editor. /// - /// A ``mediaUploadDelegate`` can still transform the file first; only delivery + /// A ``mediaProcessor`` can still transform the file first; only delivery /// moves to the uploader. public private(set) var mediaUploader: (any MediaUploader)? @@ -168,7 +172,18 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro private let controller: GutenbergEditorController private let bundleProvider: EditorAssetBundleProvider private let lockdownModeMonitor: LockdownModeMonitor - private var uploadServer: MediaUploadServer? + /// Whether the host supplied anything for the native upload server to route. + /// + /// Read twice by `startUploadServer()` — once before starting, once after the bind + /// returns — and the two reads have to agree. They did not: the first gained + /// `mediaUploader` and the second was left checking the processor alone, so an + /// uploader-only host bound a listener, immediately stopped it, and fell back to the + /// WebView path with nothing logged. One property, so they cannot disagree again. + private var hasMediaHandling: Bool { + mediaProcessor != nil || mediaUploader != nil + } + + private(set) var uploadServer: MediaUploadServer? // MARK: - Private Properties (UI) @@ -217,22 +232,22 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// - dependencies: Pre-fetched editor dependencies. Pass them when you have them — /// the editor fetches its own otherwise, behind a progress bar. /// - mediaPicker: Supplies media from the host's own picker. - /// - mediaUploadDelegate: Customizes media processing and upload. **Don't conform - /// the object that owns this editor.** Nothing here hands the delegate the editor, - /// so the only way one reaches it is if you store it there — and the editor holds - /// the delegate strongly in return, closing a cycle ARC cannot break. Use a leaf + /// - mediaProcessor: Transforms media before upload. **Don't conform the object + /// that owns this editor.** Nothing here hands the processor the editor, so the + /// only way one reaches it is if you store it there — and the editor holds the + /// processor strongly in return, closing a cycle ARC cannot break. Use a leaf /// object carrying the settings it needs. If you must write the retaining shape, - /// call ``stopMediaHandling()`` when you are done. To reuse one delegate across - /// editors, keep your own reference — the editor drops only its own when it goes. + /// call ``stopMediaHandling()`` when you are done. See ``mediaProcessor`` for the + /// lifetime rules, including what a value-type conformer does differently. /// - mediaUploader: Takes over media upload on the host's own stack. Same ownership - /// rules as `mediaUploadDelegate`. + /// rules as `mediaProcessor`. /// - httpClient: Replaces the client used for editor and media requests. /// - isWarmupMode: Loads the editor shell without dependencies, to warm WebKit. public init( configuration: EditorConfiguration, dependencies: EditorDependencies? = nil, mediaPicker: MediaPickerController? = nil, - mediaUploadDelegate: (any MediaUploadDelegate)? = nil, + mediaProcessor: (any MediaProcessor)? = nil, mediaUploader: (any MediaUploader)? = nil, httpClient: EditorHTTPClient? = nil, isWarmupMode: Bool = false @@ -251,7 +266,7 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro ) self.bundleProvider = EditorAssetBundleProvider(httpClient: httpClient) self.mediaPicker = mediaPicker - self.mediaUploadDelegate = mediaUploadDelegate + self.mediaProcessor = mediaProcessor self.mediaUploader = mediaUploader self.lockdownModeMonitor = LockdownModeMonitor() self.controller = GutenbergEditorController(configuration: configuration, lockdownModeMonitor: self.lockdownModeMonitor) @@ -366,23 +381,23 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro } /// Releases the editor's media handling: stops the local upload server, drops the - /// host's ``mediaUploadDelegate`` and ``mediaUploader``, and withdraws the upload + /// host's ``mediaProcessor`` and ``mediaUploader``, and withdraws the upload /// endpoint from the page. /// /// Most hosts never need this. Releasing the editor runs `deinit`, which does the - /// same work. It is only required when the delegate holds the editor back — which - /// happens if you conformed the object that owns it, the one shape the delegate - /// documentation asks you to avoid — because that cycle keeps `deinit` from ever + /// same work. It is only required when a handler holds the editor back — which + /// happens if you conformed the object that owns it, the one shape ``mediaProcessor`` + /// asks you to avoid — because that cycle keeps `deinit` from ever /// running, stranding a bound loopback `NWListener` for every editor opened. /// /// Terminal, not a pause: this editor cannot upload or delete media afterwards, and /// any upload in flight is cancelled — though cancellation is cooperative, so a - /// `processFile` that ignores it runs to completion and holds the delegate until it + /// `processFile` that ignores it runs to completion and holds the processor until it /// returns. Call it when the editor is going away — not /// when it is covered, backgrounded, or otherwise coming back. Calling it more than /// once is safe. /// - /// Scoped to this editor. It drops this editor's reference, so a delegate you share + /// Scoped to this editor. It drops this editor's reference, so a handler you share /// across editors keeps working for the others. public func stopMediaHandling() { // Host-driven, and the reason is narrower than "UIKit can't tell us". It can. @@ -410,7 +425,7 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro // it in at document start) and the trade reverses. uploadServer?.stop() uploadServer = nil - mediaUploadDelegate = nil + mediaProcessor = nil mediaUploader = nil revokeNativeUploadEndpoint() } @@ -471,9 +486,9 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro } deinit { - // The ordinary path: with no cycle, ARC releases the delegate when the editor + // The ordinary path: with no cycle, ARC releases the handlers when the editor // goes and this stops the server. A host that retains the editor from its own - // delegate never reaches here — `stopMediaHandling()` is its way out. + // handler never reaches here — `stopMediaHandling()` is its way out. uploadServer?.stop() } @@ -579,12 +594,12 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// The server binds to localhost on a random port. If it fails to start, the editor /// falls back to Gutenberg's default upload behavior (the JS override won't activate /// because `nativeUploadPort` will be nil in GBKit). - private func startUploadServer() async { + func startUploadServer() async { // Nothing to route through the native server unless the host provided a - // delegate or an uploader. The editor owns whichever it was given — both + // processor or an uploader. The editor owns whichever it was given — both // properties are strong — so there's no released-before-load case to guard // against; they live as long as it does. - guard mediaUploadDelegate != nil || mediaUploader != nil else { + guard hasMediaHandling else { return } @@ -611,18 +626,18 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro do { let server = try await MediaUploadServer.start( - uploadDelegate: mediaUploadDelegate, + processor: mediaProcessor, uploader: mediaUploader, internalClient: internalClient ) // `stopMediaHandling()` can land while the bind is in flight: it is a - // main-actor call and this is suspended. It clears the delegate, so a nil one - // here means media handling was stopped after this started, and storing the - // server would undo a terminal call — the page would be handed a port that was - // just withdrawn, and in the cycle the call exists for, `deinit` never runs to - // stop it. - guard mediaUploadDelegate != nil else { + // main-actor call and this is suspended. It clears both handlers, so the + // entry guard's condition failing here means media handling was stopped after + // this started, and storing the server would undo a terminal call — the page + // would be handed a port that was just withdrawn, and in the cycle the call + // exists for, `deinit` never runs to stop it. + guard hasMediaHandling else { server.stop() return } diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift similarity index 68% rename from ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift rename to ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift index f9aa87ec0..8acb65673 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift @@ -32,13 +32,13 @@ struct MediaUploadResponse: Sendable { } } -/// The result of a delegate's ``MediaUploadDelegate/processFile(at:mimeType:filename:)``. +/// The result of a processor's ``MediaProcessor/processFile(at:mimeType:filename:)``. public enum ProcessedProxyFile: Sendable { - /// The delegate did not modify the file; the original upload is forwarded + /// The processor did not modify the file; the original upload is forwarded /// to WordPress unchanged. case original - /// The delegate produced a file to upload, along with its MIME type and + /// The processor produced a file to upload, along with its MIME type and /// filename. Both are used verbatim, so a format change (e.g. transcoding /// MOV to MP4, or an in-place EXIF strip) must report the resulting type and /// filename for WordPress to store the file correctly. @@ -47,21 +47,55 @@ public enum ProcessedProxyFile: Sendable { /// Transforms media before GutenbergKit delivers it. /// -/// A delegate only changes *bytes* — GutenbergKit still uploads the result to the +/// A processor only changes *bytes* — GutenbergKit still uploads the result to the /// configured site and owns the whole lifecycle (retries, cleanup). Because it never -/// performs the upload itself, it cannot deliver media to the wrong place. Set -/// ``EditorViewController/mediaUploadDelegate`` to resize images, transcode video, +/// performs the upload itself, it cannot deliver media to the wrong place. Pass one +/// as ``EditorViewController/mediaProcessor`` to resize images, transcode video, /// strip EXIF, etc. /// /// This is the safe, common extension point: most hosts want only this. To perform /// the upload yourself, conform to ``MediaUploader`` instead. -public protocol MediaUploadDelegate: AnyObject, Sendable { - /// Whether this delegate might transform a file with the given metadata. +/// +/// Deliberately **not** class-bound. ``EditorViewController`` holds its processor +/// strongly for its own lifetime, so a conformer that holds the view controller back +/// closes a retain cycle ARC cannot break — neither object is freed, and the editor +/// stops tearing down its upload server. Dropping the class requirement lets you +/// conform with a `struct` capturing only what the transform needs, which is the +/// shape that avoids this; a class bound invited the opposite. Note a +/// value type is not automatic protection — a `struct` that stores the view +/// controller cycles just the same. The rule is simply: do not hold it back. +/// +/// A value-type conformer is **copied** when you hand it to the editor's initializer, +/// and the editor holds that copy for its lifetime. Mutating your own instance +/// afterwards changes nothing the editor will run, and there is no way to swap in a +/// new value — the property is `private(set)`, so a different processor means a +/// different editor. If you need settings the host can change while an editor is open, +/// read them inside `processFile` through a reference the conformer captures. That +/// reference must itself be `Sendable` — an actor, or a class made safe with a lock — +/// because this protocol is `Sendable` and a `struct` conformer's stored properties +/// inherit that requirement. +/// +/// Two requirements a `struct` makes easy to miss, both of which compile silently: +/// `processFile` cannot be `mutating` (a `mutating` witness does not satisfy a +/// non-mutating requirement), and its argument labels must match exactly. Either +/// mistake resolves to the no-op default below instead of failing to build, leaving a +/// processor that is never called. That +/// reference must itself be `Sendable` — an actor, or a class made safe with a lock — +/// because this protocol is `Sendable` and a `struct` conformer's stored properties +/// inherit that requirement. +/// +/// Two requirements a `struct` makes easy to miss, both of which compile silently: +/// `processFile` cannot be `mutating` (a `mutating` witness does not satisfy a +/// non-mutating requirement), and its argument labels must match exactly. Either +/// mistake resolves to the no-op default below instead of failing to build, leaving a +/// processor that is never called. +public protocol MediaProcessor: Sendable { + /// Whether this processor might transform a file with the given metadata. /// /// A cheap, metadata-only gate the server consults *before* materializing the /// upload to a temp file. Return `false` to decline a file by type — e.g. an - /// image-only delegate returning `false` for a video — so the server forwards - /// the original upload to WordPress without first copying a file the delegate + /// image-only processor returning `false` for a video — so the server forwards + /// the original upload to WordPress without first copying a file the processor /// won't touch. /// /// With a ``MediaUploader`` set this can't decline the upload itself — an @@ -83,7 +117,7 @@ public protocol MediaUploadDelegate: AnyObject, Sendable { } /// Default implementations. -extension MediaUploadDelegate { +extension MediaProcessor { public func handlesFile(ofType mimeType: String, named filename: String) -> Bool { true } @@ -115,7 +149,7 @@ public struct MediaUploadField: Sendable, Hashable, Codable { /// Everything a ``MediaUploader`` needs to reproduce a native upload: the file to /// send, its metadata, the editor's non-file form fields, and the request's query. public struct MediaUpload: Sendable { - /// The file to upload — already processed, if a ``MediaUploadDelegate`` ran. + /// The file to upload — already processed, if a ``MediaProcessor`` ran. public let fileURL: URL /// The file's MIME type. @@ -150,13 +184,22 @@ public struct MediaUpload: Sendable { /// /// This is a choice of *who executes the requests*, not where they go: an uploader /// and GutenbergKit's internal media client both target the same configured site. -/// Setting ``EditorViewController/mediaUploader`` makes the host own that upload +/// Supplying ``EditorViewController/mediaUploader`` makes the host own that upload /// end-to-end — the request, its own retries, and its recovery and cleanup — with /// GutenbergKit out of the network entirely. Because the host does the retries /// itself, there's no raw response left for the editor to retry behind it. The /// attachment you return lives on that same configured site, where the editor reads /// and updates it by ID. -public protocol MediaUploader: AnyObject, Sendable { +/// +/// Deliberately **not** class-bound, for the same reason as ``MediaProcessor``: the +/// editor holds its uploader strongly, so a conformer that holds the view controller +/// back forms a retain cycle neither object escapes. The operative rule is that one: +/// do not store the ``EditorViewController``. A value type does not enforce it — a +/// `struct` holding the view controller cycles the same way — and it carries the same +/// copy-at-`init` caveat described on ``MediaProcessor``. An uploader that owns a +/// queue, a background session, or a retry counter wants a class; capture it behind a +/// reference either way. +public protocol MediaUploader: Sendable { /// Upload a (possibly processed) file and return the finished WordPress /// attachment JSON the editor inserts — the same object a direct /// `POST /wp/v2/media` returns. Return only once the upload is genuinely done, diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 76b2fa0dc..54768e6db 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -29,14 +29,14 @@ final class MediaUploadServer: Sendable { /// Creates and starts a new upload server. /// /// - Parameters: - /// - uploadDelegate: Optional delegate for transforming files before upload. + /// - processor: Optional processor that transforms the file before delivery. /// - uploader: Optional host uploader that performs the upload on its own stack. /// - internalClient: GutenbergKit's own client for the configured site. Delivers /// uploads when no host uploader does, and every media delete. /// - maxRequestBodySize: The maximum allowed request body size in bytes. /// Requests exceeding this limit receive a 413 response. Defaults to 4 GB. static func start( - uploadDelegate: (any MediaUploadDelegate)? = nil, + processor: (any MediaProcessor)? = nil, uploader: (any MediaUploader)? = nil, internalClient: InternalMediaClient? = nil, maxRequestBodySize: Int64 = HTTPRequestParser.defaultMaxBodySize @@ -48,7 +48,7 @@ final class MediaUploadServer: Sendable { cleanOrphanedUploads() } - let context = UploadContext(uploadDelegate: uploadDelegate, uploader: uploader, internalClient: internalClient) + let context = UploadContext(processor: processor, uploader: uploader, internalClient: internalClient) // A generous ceiling for receiving the upload body. The body read is // primarily bounded by the per-read idle timeout (which reaps a stalled @@ -72,7 +72,7 @@ final class MediaUploadServer: Sendable { let uploadServer = MediaUploadServer(server: server, cleanupTask: cleanupTask) #if DEBUG - countServerStarted(delegate: uploadDelegate) + countServerStarted(processor: processor, uploader: uploader) #endif return uploadServer } @@ -86,7 +86,7 @@ final class MediaUploadServer: Sendable { /// editor stops it on `deinit`, so returning to zero is the normal outcome — monotone /// growth is the ownership cycle described on /// ``EditorViewController/stopMediaHandling()``. Nothing else produces it: - /// `EditorViewController.warmup()` passes no delegate, so it never starts a server. + /// `EditorViewController.warmup()` passes neither handler, so it never starts a server. /// /// This population is the only detectable symptom of that cycle. A `deinit` assertion /// on the editor cannot work — a cycle is precisely what stops `deinit` from running — @@ -102,7 +102,10 @@ final class MediaUploadServer: Sendable { /// overlap across a push or a modal transition; four is not a shape hosts produce. private static let liveServerLeakThreshold = 4 - private static func countServerStarted(delegate: (any MediaUploadDelegate)?) { + private static func countServerStarted( + processor: (any MediaProcessor)?, + uploader: (any MediaUploader)? + ) { let count = censusLock.withLock { liveServerCount += 1 return liveServerCount @@ -110,15 +113,20 @@ final class MediaUploadServer: Sendable { guard count >= liveServerLeakThreshold else { return } - let name = delegate.map { String(describing: type(of: $0)) } ?? "the host's delegate" + // Name every handler that was supplied, not just the first. With both set the + // retainer is as likely to be the uploader, and naming only the processor sends + // the reader to audit an object that may be a value type holding nothing at all. + let names = [processor.map { String(describing: type(of: $0)) }, + uploader.map { String(describing: type(of: $0)) }].compactMap { $0 } + let name = names.isEmpty ? "the host's media handler" : names.joined(separator: ", ") Logger.uploadServer.fault( """ \(count, privacy: .public) media upload servers are live, one bound loopback \ listener each. Editors are leaking: a host that both owns EditorViewController \ - and is its own media upload delegate (\(name, privacy: .public)) forms a retain \ + and is one of its own media handlers (\(name, privacy: .public)) forms a retain \ cycle ARC cannot break, so the editor's deinit never runs. Call \ EditorViewController.stopMediaHandling() when you are done with the editor, or \ - keep the delegate a leaf object that doesn't reference the editor. + keep the handler a leaf object that doesn't reference the editor. """ ) } @@ -184,19 +192,19 @@ final class MediaUploadServer: Sendable { let filename = filePart.filename ?? "upload" let mimeType = filePart.contentType - // Ask the delegate — from metadata alone — whether it will touch a file like + // Ask the processor — from metadata alone — whether it will touch a file like // this. If not, forward the original upload to WordPress directly, skipping a - // full temp-file copy of a file the delegate won't process (e.g. a video handed - // to an image-only delegate). + // full temp-file copy of a file the processor won't process (e.g. a video handed + // to an image-only processor). // // An uploader takes over delivery for *every* file, so with one set there is // no passthrough to fall to and the gate can't decline the upload outright. // It still decides whether `processFile` runs, though — a declined file is - // handed to the uploader unprocessed rather than to a delegate that said it + // handed to the uploader unprocessed rather than to a processor that said it // won't touch it — so the answer is carried into `processAndUpload` rather // than discarded here. Asked exactly once per upload, matching Android. - let delegateWantsFile = context.uploadDelegate?.handlesFile(ofType: mimeType, named: filename) ?? false - guard context.uploader != nil || delegateWantsFile else { + let processorWantsFile = context.processor?.handlesFile(ofType: mimeType, named: filename) ?? false + guard context.uploader != nil || processorWantsFile else { do { return try await passthroughResponse(request, query: query, internalClient: context.internalClient) } catch { @@ -204,7 +212,7 @@ final class MediaUploadServer: Sendable { } } - // Someone wants the file — the delegate, the uploader, or both. Stream the + // Someone wants the file — the processor, the uploader, or both. Stream the // part body to a dedicated temp file for them: the library's RequestBody may // be a byte-range slice of a larger temp file whose lifecycle is tied to ARC, // so they need a standalone file that outlives the handler return. @@ -222,7 +230,7 @@ final class MediaUploadServer: Sendable { } // From here on always clean up the original temp file. The processed - // file (if the delegate produced a new one) is cleaned up inside + // file (if the processor produced a new one) is cleaned up inside // processAndUpload so its throw paths are covered too. defer { try? FileManager.default.removeItem(at: fileURL) } @@ -230,14 +238,14 @@ final class MediaUploadServer: Sendable { let uploadResult = try await processAndUpload( fileURL: fileURL, mimeType: mimeType, filename: filename, extraParts: extraParts, query: query, - delegateWantsFile: delegateWantsFile, context: context + processorWantsFile: processorWantsFile, context: context ) switch uploadResult { case .uploaded(let uploaded): Logger.uploadServer.debug("Uploaded file to WordPress") return relayResponse(uploaded) case .passthrough: - // Delegate didn't modify the file — forward the original request + // The processor didn't modify the file — forward the original request // body to WordPress without re-encoding. return try await passthroughResponse(request, query: query, internalClient: context.internalClient) } @@ -247,7 +255,7 @@ final class MediaUploadServer: Sendable { } /// Forwards the original request body to WordPress unchanged (no multipart - /// re-encoding) and relays the response. Used when the delegate won't touch + /// re-encoding) and relays the response. Used when the processor won't touch /// the file — it declined by metadata (`handlesFile` returned false) or /// `processFile` returned `.original`. private static func passthroughResponse( @@ -310,7 +318,7 @@ final class MediaUploadServer: Sendable { /// /// The response's own `Content-Type` wins over the JSON default. `HTTPResponse` /// serializes every header it is given, so appending the default unconditionally - /// would emit the name twice for a delegate that sets it. + /// would emit the name twice for a processor that sets it. private static func relayResponse(_ response: MediaUploadResponse) -> HTTPResponse { let hasContentType = response.headers.keys.contains { $0.lowercased() == "content-type" } return HTTPResponse( @@ -335,14 +343,14 @@ final class MediaUploadServer: Sendable { return errorResponse(status: 500, message: error.localizedDescription) } - // MARK: - Delegate Pipeline + // MARK: - Processor Pipeline - /// Result of the delegate processing + upload pipeline. + /// Result of the processing + upload pipeline. private enum UploadResult { /// The uploader or internal media client completed the upload; /// carries the raw WordPress response to relay. case uploaded(MediaUploadResponse) - /// The delegate didn't modify the file, so the original body is forwarded. + /// The processor didn't modify the file, so the original body is forwarded. /// The caller should forward the original request body to WordPress. case passthrough } @@ -350,22 +358,22 @@ final class MediaUploadServer: Sendable { private static func processAndUpload( fileURL: URL, mimeType: String, filename: String, extraParts: [MultipartPart], query: String, - delegateWantsFile: Bool, context: UploadContext + processorWantsFile: Bool, context: UploadContext ) async throws -> UploadResult { // Step 1: Process (resize, transcode, etc.) — but only for a file the - // delegate's metadata gate accepted. `handlesFile` returning false is the - // delegate saying it won't touch a file like this, so handing it one anyway + // processor's metadata gate accepted. `handlesFile` returning false is the + // processor saying it won't touch a file like this, so handing it one anyway // would break the contract the gate documents. With an uploader set the file // still gets delivered; it just skips processing on its way there. let processed: ProcessedProxyFile - if let delegate = context.uploadDelegate, delegateWantsFile { - processed = try await delegate.processFile(at: fileURL, mimeType: mimeType, filename: filename) + if let processor = context.processor, processorWantsFile { + processed = try await processor.processFile(at: fileURL, mimeType: mimeType, filename: filename) } else { processed = .original } // Resolve the file to upload and its metadata. `.processed` uses the - // delegate's values verbatim, so a format change is reported to WordPress. + // processor's values verbatim, so a format change is reported to WordPress. let uploadURL: URL let uploadMimeType: String let uploadFilename: String @@ -380,7 +388,7 @@ final class MediaUploadServer: Sendable { uploadFilename = processedFilename } - // The processed file (if the delegate produced a new one) is ours to + // The processed file (if the processor produced a new one) is ours to // clean up — on success it has been uploaded, on failure it is abandoned. // Cleaning up here rather than in the caller covers the throw paths too. defer { @@ -558,30 +566,30 @@ enum UploadError: Error, LocalizedError { // MARK: - Upload Context -/// Container for the upload delegate, host uploader, and internal media client, +/// Container for the media processor, host uploader, and internal media client, /// captured by the HTTPServer handler closure and read on each request. /// -/// All are held **strongly**, so a delegate that admitted a file for processing +/// All are held **strongly**, so a processor that admitted a file for processing /// will process it — the three reads within a request can't disagree, and an -/// in-flight upload keeps the host's delegate alive until it unwinds. That lifetime +/// in-flight upload keeps the host's processor alive until it unwinds. That lifetime /// comes from the handler closure, which the listener retains for the server's /// lifetime; it therefore holds just as well on the paths that take the client /// alone rather than the whole context. This matches Android, which holds its -/// `uploadDelegate` as a plain `val` for the same reason. +/// `processor` as a plain `val` for the same reason. /// -/// Strong is safe *given* `EditorViewController` now owns `mediaUploadDelegate` +/// Strong is safe *given* `EditorViewController` now owns `mediaProcessor` /// strongly too — but be exact about what that trades away. Weak here did break one /// ring: every other edge in `EditorViewController → uploadServer → HTTPServer → -/// listener → newConnectionHandler → handler → UploadContext → delegate` is strong, +/// listener → newConnectionHandler → handler → UploadContext → processor` is strong, /// so this was its only weak link. What it could not break is the shorter ring /// straight through the property. A host that retains the view controller back now -/// leaks either way, so weak here buys a partial guard in exchange for the delegate +/// leaks either way, so weak here buys a partial guard in exchange for the processor /// vanishing mid-request — which is the failure that was actually being hit. /// -/// A `struct`, so it is implicitly `Sendable`: `MediaUploadDelegate` is a `Sendable` -/// protocol and `InternalMediaClient` is `@unchecked Sendable`. +/// A `struct`, so it is implicitly `Sendable`: `MediaProcessor` and `MediaUploader` +/// are `Sendable` protocols and `InternalMediaClient` is `@unchecked Sendable`. private struct UploadContext: Sendable { - let uploadDelegate: (any MediaUploadDelegate)? + let processor: (any MediaProcessor)? let uploader: (any MediaUploader)? let internalClient: InternalMediaClient? } @@ -646,7 +654,7 @@ class InternalMediaClient: @unchecked Sendable { /// Forwards the original request body to WordPress without re-encoding. /// - /// Used when the delegate's `processFile` returned the file unchanged — + /// Used when the processor's `processFile` returned the file unchanged — /// the incoming multipart body is already valid for WordPress. func passthroughUpload(body: RequestBody, contentType: String, query: String) async throws -> MediaUploadResponse { var request = URLRequest(url: mediaEndpointURL(query: query)) diff --git a/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaTeardownTests.swift b/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaTeardownTests.swift index 726b44746..98d66e49b 100644 --- a/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaTeardownTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaTeardownTests.swift @@ -8,7 +8,7 @@ import Testing /// Pins that ``EditorViewController/stopMediaHandling()`` opens the ownership cycle a host /// can form, and that a host which doesn't form one needs nothing. /// -/// The editor holds `mediaUploadDelegate` strongly so an in-flight upload can't lose it +/// The editor holds `mediaProcessor` strongly so an in-flight upload can't lose it /// mid-request. The cost is that a host which holds the editor back closes a cycle ARC /// cannot break — and `deinit`, which does this work on every other path, is exactly what /// a cycle prevents. `stopMediaHandling()` is the way out, and it has to be the host's @@ -21,13 +21,13 @@ struct EditorViewControllerMediaTeardownTests: MakesTestFixtures { static let testApiRoot = URL(string: "https://test.example.com/wp-json/wp/v2")! @MainActor - @Test("stopMediaHandling frees the editor and the host delegate that owns it") + @Test("stopMediaHandling frees the editor and the host processor that owns it") func stopMediaHandlingBreaksTheOwnershipCycle() async { weak var weakEditor: EditorViewController? - weak var weakHost: EditorOwningDelegate? + weak var weakHost: EditorOwningProcessor? do { - let host = EditorOwningDelegate(configuration: makeConfiguration()) + let host = EditorOwningProcessor(configuration: makeConfiguration()) weakEditor = host.editor weakHost = host host.editor.stopMediaHandling() @@ -35,19 +35,19 @@ struct EditorViewControllerMediaTeardownTests: MakesTestFixtures { await waitForRelease { weakHost == nil && weakEditor == nil } - #expect(weakHost == nil, "host delegate leaked — stopMediaHandling did not release it") - #expect(weakEditor == nil, "EditorViewController leaked — cycle through mediaUploadDelegate") + #expect(weakHost == nil, "host processor leaked — stopMediaHandling did not release it") + #expect(weakEditor == nil, "EditorViewController leaked — cycle through mediaProcessor") } @MainActor @Test("a host that does not retain the editor is freed without stopMediaHandling") - func standaloneDelegateIsFreed() async { + func standaloneProcessorIsFreed() async { weak var weakEditor: EditorViewController? do { let editor = EditorViewController( configuration: makeConfiguration(), - mediaUploadDelegate: StandaloneDelegate() + mediaProcessor: StandaloneProcessor() ) weakEditor = editor } @@ -57,6 +57,52 @@ struct EditorViewControllerMediaTeardownTests: MakesTestFixtures { #expect(weakEditor == nil, "EditorViewController leaked — nothing here retains it") } + // MARK: - Which handlers bring the server up + + /// The regression this pins: `startUploadServer()` reads "did the host supply a + /// handler" twice — once before starting, once after the bind returns — and the two + /// reads drifted. The first gained `mediaUploader`, the second kept checking the + /// processor alone, so an uploader-only host bound a listener and then immediately + /// stopped it. `uploadServer` stayed nil, the page was advertised `nativeUploadPort: + /// nil`, and `api-fetch.js` fell through to the plain WebView path — so the host's + /// `upload(_:)` was never called for any file, with nothing logged. + /// + /// Android pins the same gate (`GutenbergViewUploadServerTest`, "the upload server + /// starts for an uploader with no processor"); iOS had no equivalent, which is why the + /// drift survived three commits with a green suite. + @MainActor + @Test( + "the upload server starts for whichever handler the host supplied", + .enabled(if: canBindUploadServer), + arguments: [ + ("uploader only", false, true), + ("processor only", true, false), + ("both", true, true) + ] + ) + func uploadServerStartsForAnyHandler(_ label: String, processor: Bool, uploader: Bool) async { + let editor = EditorViewController( + configuration: makeConfiguration(), + mediaProcessor: processor ? StandaloneProcessor() : nil, + mediaUploader: uploader ? InertUploader() : nil + ) + defer { editor.stopMediaHandling() } + + await editor.startUploadServer() + + #expect(editor.uploadServer != nil, "\(label): no upload server, so the host's media handling never runs") + } + + @MainActor + @Test("no handler leaves the upload server down", .enabled(if: canBindUploadServer)) + func noHandlerLeavesServerDown() async { + let editor = EditorViewController(configuration: makeConfiguration()) + + await editor.startUploadServer() + + #expect(editor.uploadServer == nil, "started a server with nothing to route through it") + } + /// Polls instead of asserting outright, because a `UIViewController` can sit in an /// autorelease pool past the end of the scope that held it. Asserting synchronously /// passes in isolation and fails in a full suite, where other tests keep the main @@ -69,18 +115,18 @@ struct EditorViewControllerMediaTeardownTests: MakesTestFixtures { } } -/// The shape that cycles: owns the editor *and* is its delegate. Hosts reach for this +/// The shape that cycles: owns the editor *and* is its processor. Hosts reach for this /// because the coordinator driving the editor already has the site context. @MainActor -private final class EditorOwningDelegate: MediaUploadDelegate { - /// Implicitly unwrapped so `self` can be passed as the editor's delegate: every stored +private final class EditorOwningProcessor: MediaProcessor { + /// Implicitly unwrapped so `self` can be passed as the editor's processor: every stored /// property then has a value (nil) on entry to `init`, which is what makes `self` - /// available there. Taking the delegate at `init` doesn't prevent this shape — it just + /// available there. Taking the processor at `init` doesn't prevent this shape — it just /// moves where the host writes it. private(set) var editor: EditorViewController! init(configuration: EditorConfiguration) { - editor = EditorViewController(configuration: configuration, mediaUploadDelegate: self) + editor = EditorViewController(configuration: configuration, mediaProcessor: self) } nonisolated func handlesFile(ofType mimeType: String, named filename: String) -> Bool { false } @@ -90,7 +136,7 @@ private final class EditorOwningDelegate: MediaUploadDelegate { } } -private final class StandaloneDelegate: MediaUploadDelegate { +private final class StandaloneProcessor: MediaProcessor { func handlesFile(ofType mimeType: String, named filename: String) -> Bool { false } func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { @@ -99,3 +145,29 @@ private final class StandaloneDelegate: MediaUploadDelegate { } #endif + +/// Supplied only to bring the upload server up; never invoked by these tests. +private struct InertUploader: MediaUploader { + func upload(_ upload: MediaUpload) async throws -> Data { Data() } +} + +/// Whether `HTTPServer` can bind here — it cannot in some sandboxes, and these tests +/// assert on a real listener. +private let canBindUploadServer: Bool = { + let result = UnsafeSendableBox(false) + let semaphore = DispatchSemaphore(value: 0) + Task { + if let server = try? await MediaUploadServer.start() { + server.stop() + result.value = true + } + semaphore.signal() + } + semaphore.wait() + return result.value +}() + +private final class UnsafeSendableBox: @unchecked Sendable { + var value: T + init(_ value: T) { self.value = value } +} diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 26d8a0982..50d0a9f1d 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -102,9 +102,9 @@ struct MediaUploadServerTests { @Test("routes /upload with a query string and relays the query") func uploadWithQueryString() async throws { - let delegate = ProcessOnlyDelegate() + let processor = ProcessOnlyProcessor() let mockUploader = MockInternalMediaClient() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, internalClient: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } // `@wordpress/media-utils` uploads to `/wp/v2/media?_embed=wp:featuredmedia`, @@ -123,7 +123,7 @@ struct MediaUploadServerTests { let (_, response) = try await URLSession.shared.data(for: request) let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 201) - // The delegate returns `.original`, so this is the passthrough branch. + // The processor returns `.original`, so this is the passthrough branch. // Pin which branch ran — `lastQuery` is recorded by both, so without this // the query assertion would pass even if routing collapsed onto one path. #expect(mockUploader.passthroughUploadCalled) @@ -157,11 +157,11 @@ struct MediaUploadServerTests { #expect(httpResponse.value(forHTTPHeaderField: "Content-Type") == "text/plain") } - @Test("processes with the delegate, then delivers and relays verbatim") - func delegateProcessThenDeliver() async throws { - let delegate = ResizingDelegate() + @Test("processes with the processor, then delivers and relays verbatim") + func processesThenDelivers() async throws { + let processor = ResizingProcessor() let internalClient = MockInternalMediaClient() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, internalClient: internalClient) + let server = try await MediaUploadServer.start(processor: processor, internalClient: internalClient) defer { server.stop() } let boundary = UUID().uuidString @@ -179,7 +179,7 @@ struct MediaUploadServerTests { let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 201) - // The delegate only transforms; GutenbergKit performs the upload. + // The processor only transforms; GutenbergKit performs the upload. #expect(internalClient.uploadCalled) // The server relays WordPress's raw response body verbatim. @@ -190,11 +190,51 @@ struct MediaUploadServerTests { #expect(json["media_type"] as? String == "file") } - @Test("uses passthrough when delegate does not modify file") - func delegatePassthrough() async throws { - let delegate = ProcessOnlyDelegate() + /// Pins the one capability dropping `: AnyObject` exists to deliver: a value type can + /// conform, and the server actually calls it. + /// + /// Every other conformer in the tree is a class, so without this nothing exercises the + /// boxed-existential path — copied into `UploadContext`, captured by the `@Sendable` + /// handler closure, read again at `processFile`. Re-imposing a class requirement, or + /// breaking that path, would otherwise compile and pass green and surface only in a + /// host's build. + /// + /// Asserts through the client's recorded metadata rather than state on the processor, + /// because a `struct` witnessing a non-mutating requirement cannot record anything — + /// which is the point. + @Test("a value-type processor is admitted, called, and its result delivered") + func valueTypeProcessorRuns() async throws { + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start( + processor: ValueTypeProcessor(), internalClient: internalClient + ) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody( + boundary: boundary, filename: "clip.mov", mimeType: "video/quicktime", + data: Data("movie".utf8) + ) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + // The transcoded metadata could only come from `processFile` having run. + #expect(internalClient.uploadCalled) + #expect(internalClient.lastUploadMimeType == "video/mp4") + #expect(internalClient.lastUploadFilename == "clip.mp4") + } + + @Test("uses passthrough when processor does not modify file") + func processorPassthrough() async throws { + let processor = ProcessOnlyProcessor() let mockUploader = MockInternalMediaClient() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, internalClient: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -212,7 +252,7 @@ struct MediaUploadServerTests { let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 201) - #expect(delegate.processFileCalled) + #expect(processor.processFileCalled) // Passthrough: original body forwarded directly, not re-encoded. #expect(mockUploader.passthroughUploadCalled) #expect(!mockUploader.uploadCalled) @@ -223,11 +263,11 @@ struct MediaUploadServerTests { #expect(json["id"] as? Int == 99) } - @Test("skips processing and the temp copy when the delegate declines by metadata") - func delegateDeclinesByMetadata() async throws { - let delegate = DeclineByMetadataDelegate() + @Test("skips processing and the temp copy when the processor declines by metadata") + func processorDeclinesByMetadata() async throws { + let processor = DeclineByMetadataProcessor() let mockUploader = MockInternalMediaClient() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, internalClient: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -244,18 +284,18 @@ struct MediaUploadServerTests { let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 201) - // Declined by metadata → the delegate is never asked to process (so the file + // Declined by metadata → the processor is never asked to process (so the file // was never materialized), and the upload is passed through directly. - #expect(!delegate.processFileCalled) + #expect(!processor.processFileCalled) #expect(mockUploader.passthroughUploadCalled) #expect(!mockUploader.uploadCalled) } - @Test("forwards the delegate's processed metadata to the uploader") + @Test("forwards the processor's processed metadata to the uploader") func processedMetadataForwarded() async throws { - let delegate = ResizingDelegate() + let processor = ResizingProcessor() let mockUploader = MockInternalMediaClient() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, internalClient: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -270,18 +310,18 @@ struct MediaUploadServerTests { _ = try await URLSession.shared.data(for: request) - // The delegate changed the format, so the uploader must receive the new + // The processor changed the format, so the uploader must receive the new // metadata — not the original video/quicktime + clip.mov. #expect(mockUploader.uploadCalled) #expect(mockUploader.lastUploadMimeType == "video/mp4") #expect(mockUploader.lastUploadFilename == "clip.mp4") } - @Test("deletes the delegate's processed file after upload") + @Test("deletes the processor's processed file after upload") func deletesProcessedFile() async throws { - let delegate = ResizingDelegate() + let processor = ResizingProcessor() let mockUploader = MockInternalMediaClient() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, internalClient: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -296,10 +336,10 @@ struct MediaUploadServerTests { _ = try await URLSession.shared.data(for: request) - // The server owns the file the delegate produced and must delete it once the + // The server owns the file the processor produced and must delete it once the // upload finishes — the defer in processAndUpload covers the success and throw // paths alike. A leaked processed file is a full-size temp per upload. - let processedURL = try #require(delegate.producedURL) + let processedURL = try #require(processor.producedURL) #expect(!FileManager.default.fileExists(atPath: processedURL.path(percentEncoded: false))) } @@ -451,11 +491,11 @@ struct MediaUploadServerTests { #expect(received.query == "?_embed=wp:featuredmedia") } - @Test("a delegate still processes the file an uploader delivers") - func delegateProcessesForUploader() async throws { - let delegate = ProcessOnlyDelegate() + @Test("a processor still processes the file an uploader delivers") + func processorRunsForUploader() async throws { + let processor = ProcessOnlyProcessor() let uploader = RecordingUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, uploader: uploader, internalClient: MockInternalMediaClient()) + let server = try await MediaUploadServer.start(processor: processor, uploader: uploader, internalClient: MockInternalMediaClient()) defer { server.stop() } let boundary = UUID().uuidString @@ -469,20 +509,20 @@ struct MediaUploadServerTests { _ = try await URLSession.shared.data(for: request) - // The delegate still processes; only delivery moves to the uploader. - #expect(delegate.processFileCalled) + // The processor still processes; only delivery moves to the uploader. + #expect(processor.processFileCalled) #expect(uploader.received != nil) } - @Test("an uploader sees a file the delegate's metadata gate would have declined") + @Test("an uploader sees a file the processor's metadata gate would have declined") func uploaderSeesDeclinedFile() async throws { - // The gate exists to skip a temp copy for a file the delegate won't touch. An + // The gate exists to skip a temp copy for a file the processor won't touch. An // uploader takes over delivery for every file, so passing through here would // silently bypass it. - let delegate = DeclineByMetadataDelegate() + let processor = DeclineByMetadataProcessor() let uploader = RecordingUploader() let internalClient = MockInternalMediaClient() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, uploader: uploader, internalClient: internalClient) + let server = try await MediaUploadServer.start(processor: processor, uploader: uploader, internalClient: internalClient) defer { server.stop() } let boundary = UUID().uuidString @@ -499,8 +539,8 @@ struct MediaUploadServerTests { #expect(uploader.received?.filename == "clip.mov") #expect(!internalClient.passthroughUploadCalled) // ...but a declined file must still not reach `processFile`: `handlesFile` - // returning false is the delegate saying it won't touch a file like this. - #expect(!delegate.processFileCalled) + // returning false is the processor saying it won't touch a file like this. + #expect(!processor.processFileCalled) } @Test("an uploader that throws surfaces as a failure, with no GutenbergKit retry") @@ -527,85 +567,85 @@ struct MediaUploadServerTests { #expect(!internalClient.passthroughUploadCalled) } - @Test("retains the delegate for the server's lifetime, and releases it after") - func retainsDelegateForServerLifetime() async throws { - weak var weakDelegate: ProcessOnlyDelegate? + @Test("retains the processor for the server's lifetime, and releases it after") + func retainsProcessorForServerLifetime() async throws { + weak var weakProcessor: ProcessOnlyProcessor? do { - var delegate: ProcessOnlyDelegate? = ProcessOnlyDelegate() - weakDelegate = delegate - let server = try await MediaUploadServer.start(uploadDelegate: delegate) + var processor: ProcessOnlyProcessor? = ProcessOnlyProcessor() + weakProcessor = processor + let server = try await MediaUploadServer.start(processor: processor) defer { server.stop() } - // The server owns the delegate while it runs: the host can assign one and drop + // The server owns the processor while it runs: the host can assign one and drop // its own reference, and every request still sees it. The host reference has to // go *before* the assert, or the local satisfies it and the server's ownership // is never what is under test — held weakly, this is already nil here. - delegate = nil - #expect(weakDelegate != nil) + processor = nil + #expect(weakProcessor != nil) } - // …and lets go when it stops, so the delegate isn't leaked for the process's + // …and lets go when it stops, so the processor isn't leaked for the process's // lifetime. Asserted outright rather than polled: `HTTPServer.stop()` clears the // listener's `newConnectionHandler`, which is what holds the handler closure and - // through it this delegate, so the release lands synchronously on this thread + // through it this processor, so the release lands synchronously on this thread // instead of trailing an asynchronous `NWListener` cancellation onto its queue. - #expect(weakDelegate == nil) + #expect(weakProcessor == nil) } - @Test("stopping frees a delegate that holds the server back") - func stopReleasesDelegateThatRetainsTheServer() async throws { + @Test("stopping frees a processor that holds the server back") + func stopReleasesProcessorThatRetainsTheServer() async throws { // The server-side half of the ownership story, and the one nothing else covers. // `EditorViewController.stopMediaHandling()` clears its own properties *and* stops // the server, because releasing only one leaves the loop routed through the other: - // `listener -> newConnectionHandler -> handler -> UploadContext -> delegate -> server`. + // `listener -> newConnectionHandler -> handler -> UploadContext -> processor -> server`. // - // Polled rather than asserted outright, unlike `retainsDelegateForServerLifetime`: + // Polled rather than asserted outright, unlike `retainsProcessorForServerLifetime`: // `releaseConnectionHandler()` opens the loop on the caller's thread, but it is not // the only thing that does. Cancelling an `NWListener` also releases the blocks it // captured, for a deployment target of iOS 16 or later (this package requires 17) — // rdar://89677097, documented in the macOS 13 release notes — and that release lands // on the listener's own queue. Confirmed by no-op'ing `releaseConnectionHandler()`: - // the delegate is still freed, a poll tick later. Before that OS change the blocks + // the processor is still freed, a poll tick later. Before that OS change the blocks // were held for the listener's lifetime, so a lowered deployment target hangs here // instead of quietly stranding listeners. - weak var weakDelegate: ServerRetainingDelegate? + weak var weakProcessor: ServerRetainingProcessor? var server: MediaUploadServer? do { - let delegate = ServerRetainingDelegate() - weakDelegate = delegate - let started = try await MediaUploadServer.start(uploadDelegate: delegate) - delegate.server = started // closes the loop: server -> handler -> delegate -> server + let processor = ServerRetainingProcessor() + weakProcessor = processor + let started = try await MediaUploadServer.start(processor: processor) + processor.server = started // closes the loop: server -> handler -> processor -> server server = started } - #expect(weakDelegate != nil, "the server should own the delegate while it runs") + #expect(weakProcessor != nil, "the server should own the processor while it runs") server?.stop() server = nil - for _ in 0..<100 where weakDelegate != nil { + for _ in 0..<100 where weakProcessor != nil { try await Task.sleep(for: .milliseconds(10)) } - #expect(weakDelegate == nil, "delegate leaked — stopping did not release the handler's references") + #expect(weakProcessor == nil, "processor leaked — stopping did not release the handler's references") } - @Test("still processes for a delegate the host has dropped its reference to") - func processesForHostReleasedDelegate() async throws { - // The delegate is read at the admission gate and again at processFile, separated + @Test("still processes for a processor the host has dropped its reference to") + func processesForHostReleasedProcessor() async throws { + // The processor is read at the admission gate and again at processFile, separated // by a synchronous disk copy and an unbounded processFile. // Held weakly, a host that dropped its reference changed the answer between // those reads: a file admitted for processing was forwarded unprocessed. The // host dropping it before the request is the same condition, deterministically. let mockUploader = MockInternalMediaClient() - var delegate: TranscodingDelegate? = TranscodingDelegate() - weak let weakDelegate = delegate - let server = try await MediaUploadServer.start(uploadDelegate: delegate, internalClient: mockUploader) + var processor: TranscodingProcessor? = TranscodingProcessor() + weak let weakProcessor = processor + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } // Drop the host's only strong reference. Under the documented contract the - // server owns the delegate from here, so the upload must still be processed. - delegate = nil + // server owns the processor from here, so the upload must still be processed. + processor = nil let boundary = UUID().uuidString let body = buildMultipartBody(boundary: boundary, filename: "clip.mov", mimeType: "video/quicktime", data: Data("movie".utf8)) @@ -621,7 +661,7 @@ struct MediaUploadServerTests { // The server kept it alive, so the processed metadata reached the uploader. // Against a weak container this fails with the real symptom: the passthrough // branch runs and the original video/quicktime is forwarded unprocessed. - #expect(weakDelegate != nil) + #expect(weakProcessor != nil) #expect(mockUploader.uploadCalled) #expect(mockUploader.lastUploadMimeType == "video/mp4") #expect(!mockUploader.passthroughUploadCalled) @@ -1041,9 +1081,9 @@ private final class ThrowingUploader: MediaUploader, @unchecked Sendable { } } -/// A delegate that transcodes, used to check the server holds it across the whole +/// A processor that transcodes, used to check the server holds it across the whole /// request rather than re-reading a reference the host may have dropped. -private final class TranscodingDelegate: MediaUploadDelegate, @unchecked Sendable { +private final class TranscodingProcessor: MediaProcessor, @unchecked Sendable { func handlesFile(ofType mimeType: String, named filename: String) -> Bool { true } @@ -1055,7 +1095,7 @@ private final class TranscodingDelegate: MediaUploadDelegate, @unchecked Sendabl } } -private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendable { +private final class ProcessOnlyProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false @@ -1067,11 +1107,11 @@ private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendabl } } -/// A delegate that declines every file by metadata via `handlesFile`. With no +/// A processor that declines every file by metadata via `handlesFile`. With no /// uploader the server must pass through without ever materializing the file; with /// one, delivery still happens but `processFile` must not be called. /// `processFileCalled` pins both. -private final class DeclineByMetadataDelegate: MediaUploadDelegate, @unchecked Sendable { +private final class DeclineByMetadataProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false @@ -1085,12 +1125,23 @@ private final class DeclineByMetadataDelegate: MediaUploadDelegate, @unchecked S } } -/// A delegate that produces a new file with changed metadata (e.g. a transcode). -private final class ResizingDelegate: MediaUploadDelegate, @unchecked Sendable { +/// A processor that produces a new file with changed metadata (e.g. a transcode). +/// A value-type processor. `struct`, and `Sendable` without `@unchecked` — both are the +/// point: this is the shape ``MediaProcessor``'s documentation now recommends. +private struct ValueTypeProcessor: MediaProcessor { + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + let processed = url.deletingLastPathComponent() + .appending(component: "value-\(UUID().uuidString).mp4") + try Data("transcoded".utf8).write(to: processed) + return .processed(processed, mimeType: "video/mp4", filename: "clip.mp4") + } +} + +private final class ResizingProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _producedURL: URL? - /// The URL of the processed file this delegate wrote, for cleanup assertions. + /// The URL of the processed file this processor wrote, for cleanup assertions. var producedURL: URL? { lock.withLock { _producedURL } } func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { @@ -1177,9 +1228,9 @@ private extension Data { } } -/// Holds the server that owns it, closing `server -> handler -> delegate -> server`. +/// Holds the server that owns it, closing `server -> handler -> processor -> server`. /// Only `stop()` — which drops the listener's captured blocks — opens it. -private final class ServerRetainingDelegate: MediaUploadDelegate, @unchecked Sendable { +private final class ServerRetainingProcessor: MediaProcessor, @unchecked Sendable { var server: MediaUploadServer? func handlesFile(ofType mimeType: String, named filename: String) -> Bool { false }