Add SPI to set a list of hosts to which to send custom header fields cross-origin
Created attachment 368523 [details] Patch
Created attachment 368538 [details] Patch
Created attachment 368698 [details] Patch
Looks like mac-wk2 is crashing on every test.
Comment on attachment 368698 [details] Patch Attachment 368698 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12053841 Number of test failures exceeded the failure limit.
Created attachment 368711 [details] Archive of layout-test-results from ews107 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 368724 [details] Patch
Created attachment 368738 [details] Patch
Created attachment 368774 [details] Patch
Created attachment 368780 [details] Patch
Comment on attachment 368780 [details] Patch Attachment 368780 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12063884 New failing tests: http/tests/css/filters-on-iframes.html
Created attachment 368801 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 368780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368780&action=review Started reviewing, but sadly didn't finish. But have a couple comments already > Source/WebCore/loader/CustomHeaderFields.cpp:48 > + if (domainOrPattern.length() > 2 > + && domainOrPattern[0] == '*' > + && domainOrPattern[1] == '.' > + && domainOrPattern.length() > registrableDomainLength > + && url.host().endsWith(StringView(domainOrPattern).substring(1))) > + return true; I think the first three lines of this could be replaced with this: if (domainOrPattern.startsWith("*.") For greater clarity and almost the same efficiency. I don’t understand the registrableDomainLength check and why it’s needed; hope we have test cases that fail if it’s removed. > Source/WebCore/loader/CustomHeaderFields.h:34 > +struct WEBCORE_EXPORT CustomHeaderFields { WEBCORE_EXPORT should go before struct, not after it, as a matter of coding style. > Source/WebKit/Platform/spi/ios/UIKitSPI.h:208 > typedef enum { > - UIFontTraitPlain = 0, > - UIFontTraitItalic = 1 << 0, > - UIFontTraitBold = 1 << 1, > -} UIFontTrait; > - > -@interface UIFont () > -+ (UIFont *)fontWithFamilyName:(NSString *)familyName traits:(UIFontTrait)traits size:(CGFloat)fontSize; > -- (UIFontTrait)traits; > -@end > - > -typedef enum { > UIAllCorners = 0xFF, > } UIRectCorners; No mention of the changes to this file in the change log. I don’t understand the rationale. Please just roll this file back or comment about why it’s being changed. > Source/WebKit/UIProcess/API/APIWebsitePolicies.cpp:92 > + customHeaderFields.reserveInitialCapacity(this->customHeaderFields().size() + 1); Could compute this correctly instead of making it one two big by checking legacyCustomHeaderFields() size in a boolean.
(In reply to Darin Adler from comment #13) > Comment on attachment 368780 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=368780&action=review > > Started reviewing, but sadly didn't finish. But have a couple comments > already > > > Source/WebCore/loader/CustomHeaderFields.cpp:48 > > + if (domainOrPattern.length() > 2 > > + && domainOrPattern[0] == '*' > > + && domainOrPattern[1] == '.' > > + && domainOrPattern.length() > registrableDomainLength > > + && url.host().endsWith(StringView(domainOrPattern).substring(1))) > > + return true; > > I think the first three lines of this could be replaced with this: > > if (domainOrPattern.startsWith("*.") That's a great idea. > > For greater clarity and almost the same efficiency. I don’t understand the > registrableDomainLength check and why it’s needed; hope we have test cases > that fail if it’s removed. The registrableDomainLength check is to prevent someone from adding something like "*.com". I'll add such a test. > > Source/WebCore/loader/CustomHeaderFields.h:34 > > +struct WEBCORE_EXPORT CustomHeaderFields { > > WEBCORE_EXPORT should go before struct, not after it, as a matter of coding > style. I disagree. It is needed exactly where it is to make Windows link successfully. We have 3 existing instances of "struct WEBCORE_EXPORT" and 0 of "WEBCORE_EXPORT struct". Same with 90 "class WEBCORE_EXPORT" and 0 "WEBCORE_EXPORT class". > > Source/WebKit/Platform/spi/ios/UIKitSPI.h:208 > > typedef enum { > > - UIFontTraitPlain = 0, > > - UIFontTraitItalic = 1 << 0, > > - UIFontTraitBold = 1 << 1, > > -} UIFontTrait; > > - > > -@interface UIFont () > > -+ (UIFont *)fontWithFamilyName:(NSString *)familyName traits:(UIFontTrait)traits size:(CGFloat)fontSize; > > -- (UIFontTrait)traits; > > -@end > > - > > -typedef enum { > > UIAllCorners = 0xFF, > > } UIRectCorners; > > No mention of the changes to this file in the change log. I don’t understand > the rationale. Please just roll this file back or comment about why it’s > being changed. These changes are needed because our code hygiene has been lacking ever since we started compiling multiple cpp files together. Such strange unrelated-looking build fixes are needed almost every time we add a new file because someone has assumed a file will always be included before this one. That's what's held me back and caused me to look into https://bugs.webkit.org/show_bug.cgi?id=197534 so the iOS build doesn't have so many unrelated fixes.
Created attachment 369179 [details] Patch
Created attachment 369182 [details] Patch
Created attachment 369187 [details] Patch
Comment on attachment 369187 [details] Patch Attachment 369187 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12119745 New failing tests: svg/repaint/remove-border-property-on-root.html security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html
Created attachment 369238 [details] Archive of layout-test-results from ews214 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
ping review
> > > Source/WebCore/loader/CustomHeaderFields.h:34 > > > +struct WEBCORE_EXPORT CustomHeaderFields { > > > > WEBCORE_EXPORT should go before struct, not after it, as a matter of coding > > style. > I disagree. It is needed exactly where it is to make Windows link > successfully. We have 3 existing instances of "struct WEBCORE_EXPORT" and 0 > of "WEBCORE_EXPORT struct". Same with 90 "class WEBCORE_EXPORT" and 0 > "WEBCORE_EXPORT class". Seems accurate: scratch.cc:3:16: warning: attribute 'visibility' is ignored, place it after "struct" to apply attribute to type declaration [-Wignored-attributes] __attribute__((visibility("default"))) struct S {
Comment on attachment 369187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369187&action=review r=me > Source/WebKit/UIProcess/API/Cocoa/_WKCustomHeaderFields.mm:52 > + NSMutableDictionary<NSString *, NSString *> *dictionary = [[[NSMutableDictionary alloc] initWithCapacity:vector.size()] autorelease]; You can use +dictionaryWithCapacity here. > Source/WebKit/UIProcess/API/Cocoa/_WKCustomHeaderFields.mm:72 > + NSMutableArray *array = [[[NSMutableArray alloc] initWithCapacity:domains.size()] autorelease]; +arrayWithCapacity
Created attachment 369325 [details] Patch
Created attachment 369328 [details] Patch
Comment on attachment 369328 [details] Patch Clearing flags on attachment: 369328 Committed r245038: <https://trac.webkit.org/changeset/245038>
All reviewed patches have been landed. Closing bug.
<rdar://problem/50556833>
Reverted r245038 for reason: Breaks internal builds. Committed r245044: <https://trac.webkit.org/changeset/245044>
(In reply to Ryan Haddad from comment #28) > Reverted r245038 for reason: > > Breaks internal builds. > > Committed r245044: <https://trac.webkit.org/changeset/245044> Details in radar.
Created attachment 370005 [details] Patch
Comment on attachment 370005 [details] Patch Clearing flags on attachment: 370005 Committed r245401: <https://trac.webkit.org/changeset/245401>
Re-opened since this is blocked by bug 197990
Rolled out in https://trac.webkit.org/changeset/245470/webkit Causing internal build failures
(In reply to Shawn Roberts from comment #34) > Rolled out in https://trac.webkit.org/changeset/245470/webkit > > Causing internal build failures Could you please put a link to such failures in the radar? It worked for all internal builds I tested.
(In reply to Alex Christensen from comment #35) > (In reply to Shawn Roberts from comment #34) > Could you please put a link to such failures in the radar? It worked for > all internal builds I tested. He emailed links to you. I added them to the radar.
Created attachment 370155 [details] Patch
http://trac.webkit.org/r245481