Summary: | Add SPI to set a list of hosts to which to send custom header fields cross-origin | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, darin, dbates, ews-watchlist, ggaren, japhet, rniwa, ryanhaddad, sroberts, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 197990 | ||||||||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Alex Christensen
2019-04-29 17:48:43 PDT
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. 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> All reviewed patches have been landed. Closing bug. 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
|