Bug 180562 - Turn on ENABLE_APPLICATION_MANIFEST on iOS
Summary: Turn on ENABLE_APPLICATION_MANIFEST on iOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-07 17:40 PST by David Quesada
Modified: 2017-12-11 10:09 PST (History)
14 users (show)

See Also:


Attachments
Patch (8.35 KB, patch)
2017-12-07 19:23 PST, David Quesada
no flags Details | Formatted Diff | Diff
Patch for EWS, v2 (11.22 KB, patch)
2017-12-07 23:05 PST, David Quesada
no flags Details | Formatted Diff | Diff
Patch v3 (12.04 KB, patch)
2017-12-08 08:40 PST, David Quesada
no flags Details | Formatted Diff | Diff
Patch v4 (12.72 KB, patch)
2017-12-08 13:16 PST, David Quesada
no flags Details | Formatted Diff | Diff
Patch v5 (13.03 KB, patch)
2017-12-08 14:21 PST, David Quesada
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Quesada 2017-12-07 17:40:01 PST
.
Comment 1 Radar WebKit Bug Importer 2017-12-07 17:40:43 PST
<rdar://problem/35924737>
Comment 2 David Quesada 2017-12-07 19:23:18 PST
Created attachment 328777 [details]
Patch

Patch for EWS
Comment 3 David Quesada 2017-12-07 23:05:39 PST
Created attachment 328793 [details]
Patch for EWS, v2

Fixed a compile error, enabled relevant LayoutTests on ios.
Comment 4 David Quesada 2017-12-08 08:40:48 PST
Created attachment 328817 [details]
Patch v3

Fix the other build errors in CachedResourceRequest caused by files being unified-source-bundled differently and removing an implicit #include when the feature is enabled.
Comment 5 Sam Weinig 2017-12-08 11:13:24 PST
What makes this feature iOS only? In general, we would like everything to be enable on all supported platforms.
Comment 6 Brady Eidson 2017-12-08 11:14:51 PST
Comment on attachment 328817 [details]
Patch v3

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

> LayoutTests/platform/ios/TestExpectations:24
> +http/tests/security/contentSecurityPolicy/manifest-src-allowed.html [ Pass ]
> +http/tests/security/contentSecurityPolicy/manifest-src-blocked.html [ Pass ]
> +applicationmanifest/ [ Pass ]

Why explicit PASSes? Where are they otherwise skipped?

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:39
> +#include "ServiceWorkerRegistrationData.h"

This can't be a necessary part of your patch
Comment 7 David Quesada 2017-12-08 11:28:36 PST
(In reply to Sam Weinig from comment #5)
> What makes this feature iOS only? In general, we would like everything to be
> enable on all supported platforms.

(In reply to Brady Eidson from comment #6)
> Comment on attachment 328817 [details]
> Patch v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328817&action=review
> 
> > LayoutTests/platform/ios/TestExpectations:24
> > +http/tests/security/contentSecurityPolicy/manifest-src-allowed.html [ Pass ]
> > +http/tests/security/contentSecurityPolicy/manifest-src-blocked.html [ Pass ]
> > +applicationmanifest/ [ Pass ]
> 
> Why explicit PASSes? Where are they otherwise skipped?

They're skipped in LayoutTests/TestExpectations, since the feature was disabled while I was landing all the pieces.

> 
> > Source/WebCore/loader/cache/CachedResourceRequest.cpp:39
> > +#include "ServiceWorkerRegistrationData.h"
> 
> This can't be a necessary part of your patch

Do you mean I can't include this line in this patch, or you don't see why this line is needed? Before enabling the flag, CachedResourceRequest.cpp implicitly depended on having this header included by another file in the unified sources. When the feature is enabled and additional files are included in the unified sources, CachedResourceRequest.cpp is moved to a different combined source and is the first file included, so it doesn't get this header. I should have included this in an earlier patch, but I mistakenly thought it was just a build error in the tree at the time, so I worked around it.
Comment 8 David Quesada 2017-12-08 11:41:08 PST
(In reply to Sam Weinig from comment #5)
> What makes this feature iOS only? In general, we would like everything to be
> enable on all supported platforms.

It really isn't iOS-specific, I'll enable it on Mac too. The support for testing it is only implemented for Cocoa, so it's not quite ready to be enabled on other platforms yet.
Comment 9 David Quesada 2017-12-08 13:16:58 PST
Created attachment 328859 [details]
Patch v4

Not iOS-only.
Comment 10 David Quesada 2017-12-08 14:21:33 PST
Created attachment 328869 [details]
Patch v5

Only enable the layout tests for mac-wk2 and ios-wk2 platforms.
Comment 11 Geoffrey Garen 2017-12-11 09:49:27 PST
Comment on attachment 328869 [details]
Patch v5

R=me
Comment 12 WebKit Commit Bot 2017-12-11 10:09:36 PST
Comment on attachment 328869 [details]
Patch v5

Clearing flags on attachment: 328869

Committed r225747: <https://trac.webkit.org/changeset/225747>
Comment 13 WebKit Commit Bot 2017-12-11 10:09:38 PST
All reviewed patches have been landed.  Closing bug.