Summary: | Adopt WebCore::RegistrableDomain in WebCore::ResourceLoadStatistics and WebKit::NetworkProcessProxy | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Wilander <wilander> | ||||||||||||||||
Component: | WebKit Misc. | Assignee: | John Wilander <wilander> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, commit-queue, ews-watchlist, rniwa, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=194791 | ||||||||||||||||||
Attachments: |
|
Description
John Wilander
2019-02-26 15:21:45 PST
Created attachment 363033 [details]
Patch
I don't know what's up with the reshuffled DerivedSources files. Comment on attachment 363033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363033&action=review > Source/WebCore/loader/AdClickAttribution.h:96 > + return source; We should check with Ryosuke or someone about the right way to handle this. It seems like we are going from a constant time value for the deleted value to something more costly. Maybe it doesn't matter in practice, but it seems wrong. Maybe this could be a NeverDestroyed or something? > Source/WebCore/loader/AdClickAttribution.h:140 > + : registrableDomain { domain } Is this any different than the default constructor? I.e., could this just be: explicit Destination(const RegisterableDomain&) = default; > Source/WebCore/loader/AdClickAttribution.h:163 > + return destination; Ditto perf concerns here. > Source/WebCore/loader/ResourceLoadObserver.cpp:410 > + return "Statistics for "_s + url.host().toString() + ":\n"_s + iter->value.toString(); return makeString("Statistics for ", url.host().toString(), ":\n", iter->value.toString()); > Source/WebCore/loader/ResourceLoadStatistics.cpp:-51 > -} Yay! Deleted code! > Source/WebCore/loader/ResourceLoadStatistics.cpp:129 > + Nit: Whitespace-only change. > Source/WebCore/loader/ResourceLoadStatistics.cpp:-170 > -} yay! > Source/WebCore/loader/ResourceLoadStatistics.cpp:-309 > -} Nice! > Source/WebCore/platform/RegistrableDomain.h:-64 > - } I suggest you leave this as-is, but make it private. Then call it from the static method below. That's how we handle create this in pattern in other classes. > Source/WebCore/platform/RegistrableDomain.h:82 > + static RegistrableDomain uncheckedCreateFromString(const String& domain) Suggested name: "createFromUncheckedString", and have all of this logic live in the private string constructor (like it used to). Confusion: This does seem to be "checked" since we pass it through 'topPrivatelyControlledDomain', isn't it? > Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:81 > + using OpenerDomain = WebCore::RegistrableDomain; It kind of seems like these aliases should live in 'RegisterableDomain.h", since we use these concepts in other places. Comment on attachment 363033 [details] Patch Attachment 363033 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11296198 New failing tests: http/tests/navigation/statistics.html Created attachment 363043 [details]
Archive of layout-test-results from ews100 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 363033 [details] Patch Attachment 363033 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11296233 New failing tests: http/tests/navigation/statistics.html Created attachment 363049 [details]
Archive of layout-test-results from ews117 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6
(In reply to Brent Fulgham from comment #4) > Comment on attachment 363033 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363033&action=review > > > Source/WebCore/loader/AdClickAttribution.h:96 > > + return source; > > We should check with Ryosuke or someone about the right way to handle this. > It seems like we are going from a constant time value for the deleted value > to something more costly. Maybe it doesn't matter in practice, but it seems > wrong. > > Maybe this could be a NeverDestroyed or something? I could actually revert my change. After I got the hashing stuff right in RegistrableDomain and introduced the struct constructors that accept a RegistrableDomain, it works. Thanks for poking at it. > > Source/WebCore/loader/AdClickAttribution.h:140 > > + : registrableDomain { domain } > > Is this any different than the default constructor? I.e., could this just be: Compiler complains if I do that. Says only certain functions can be defaulted. > explicit Destination(const RegisterableDomain&) = default; > > > Source/WebCore/loader/AdClickAttribution.h:163 > > + return destination; > > Ditto perf concerns here. Will revert. > > Source/WebCore/loader/ResourceLoadObserver.cpp:410 > > + return "Statistics for "_s + url.host().toString() + ":\n"_s + iter->value.toString(); > > return makeString("Statistics for ", url.host().toString(), ":\n", > iter->value.toString()); Will fix. > > Source/WebCore/loader/ResourceLoadStatistics.cpp:-51 > > -} > > Yay! Deleted code! > > > Source/WebCore/loader/ResourceLoadStatistics.cpp:129 > > + > > Nit: Whitespace-only change. Will fix. > > Source/WebCore/loader/ResourceLoadStatistics.cpp:-170 > > -} > > yay! > > > Source/WebCore/loader/ResourceLoadStatistics.cpp:-309 > > -} > > Nice! > > > Source/WebCore/platform/RegistrableDomain.h:-64 > > - } > > I suggest you leave this as-is, but make it private. Then call it from the > static method below. That's how we handle create this in pattern in other > classes. Will fix. > > Source/WebCore/platform/RegistrableDomain.h:82 > > + static RegistrableDomain uncheckedCreateFromString(const String& domain) > > Suggested name: "createFromUncheckedString", and have all of this logic live > in the private string constructor (like it used to). > > Confusion: This does seem to be "checked" since we pass it through > 'topPrivatelyControlledDomain', isn't it? I went back to what Alex and I discussed earlier which is unsafeCreateFromString(). I just got worried that the "unsafe" tag makes it sound too dangerous. But unsafe is better than unchecked because we do make a best effort. > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:81 > > + using OpenerDomain = WebCore::RegistrableDomain; > > It kind of seems like these aliases should live in 'RegisterableDomain.h", > since we use these concepts in other places. Then they'll end up in the WebCore namespace, right? Which means it won't look as clean as intended in these WebKit header files. Thanks for all the comments, Brent! New patch coming up. Created attachment 363051 [details]
Patch
Comment on attachment 363051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363051&action=review > Source/WebCore/DerivedSources-input.xcfilelist:1121 > +/Users/john_wilander/dev/WebKitSafari/OpenSource/Tools/WebKitTestRunner/../TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl You probably don't want to commit this. > Source/WebCore/loader/ResourceLoadStatistics.cpp:186 > + if (modelVersion >= 15) { > + if (!decoder.decodeString("prevalentResourceDomain", registrableDomainAsString)) I don't think this is a good patch to do little changes like this in. Just leave the strings the same. This would be hard to roll out. > Source/WebCore/loader/ResourceLoadStatistics.cpp:192 > + registrableDomain = RegistrableDomain::unsafeCreateFromString(registrableDomainAsString); I like "unchecked" better than "unsafe". This is safe. > Tools/WebKitTestRunner/DerivedSources-input.xcfilelist:13 > +$(PROJECT_DIR)/Modules/paymentrequest/PaymentDetailsInit.idl This seems out of place. PaymentDetailsInit.idl is in WebCore. Comment on attachment 363051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363051&action=review >> Tools/WebKitTestRunner/DerivedSources-input.xcfilelist:13 >> +$(PROJECT_DIR)/Modules/paymentrequest/PaymentDetailsInit.idl > > This seems out of place. PaymentDetailsInit.idl is in WebCore. I'm not sure why these are even in this patch. Check with Keith Rollin to see if this is expected (these files are generated by a script, IIRC). Comment on attachment 363051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363051&action=review >> Source/WebCore/DerivedSources-input.xcfilelist:1121 >> +/Users/john_wilander/dev/WebKitSafari/OpenSource/Tools/WebKitTestRunner/../TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl > > You probably don't want to commit this. Whoops! Check with Keith Rollin - I don't believe these file changes should be showing up in patches. (In reply to Alex Christensen from comment #11) > Comment on attachment 363051 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363051&action=review > > > Source/WebCore/DerivedSources-input.xcfilelist:1121 > > +/Users/john_wilander/dev/WebKitSafari/OpenSource/Tools/WebKitTestRunner/../TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl > > You probably don't want to commit this. Yeah, almost forgot about it. I don't know why Xcode decided to do all this shuffling. I'll revert those files and see if that works. > > Source/WebCore/loader/ResourceLoadStatistics.cpp:186 > > + if (modelVersion >= 15) { > > + if (!decoder.decodeString("prevalentResourceDomain", registrableDomainAsString)) > > I don't think this is a good patch to do little changes like this in. Just > leave the strings the same. This would be hard to roll out. OK. It has always bugged me that this one field has an uppercase 'P'. And we'll have to rev the model every time we change anything. But I'll revert it. > > Source/WebCore/loader/ResourceLoadStatistics.cpp:192 > > + registrableDomain = RegistrableDomain::unsafeCreateFromString(registrableDomainAsString); > > I like "unchecked" better than "unsafe". This is safe. I kind of too. Could "best effort" work? > > Tools/WebKitTestRunner/DerivedSources-input.xcfilelist:13 > > +$(PROJECT_DIR)/Modules/paymentrequest/PaymentDetailsInit.idl > > This seems out of place. PaymentDetailsInit.idl is in WebCore. Yeah. Something went wrong either in rebase or with Xcode. Comment on attachment 363051 [details]
Patch
static RegistrableDomain uncheckedCreateFromRegistrableDomainString(const String&); // doesn't look in Public Suffix List. This is faster, and can be used for IPC
static RegistrableDomain uncheckedCreateFromHost(const String&); // Looks in Public Suffix List same thing as URL constructor
or maybe StringView
Created attachment 363112 [details]
Patch for landing
Forgot to revert the darn xcfilelist changes again. :/ Attachment 363112 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/RegistrableDomain.h:82: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 1 in 44 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 363113 [details]
Patch for landing
Attachment 363113 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/RegistrableDomain.h:82: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 1 in 40 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 363120 [details]
Patch for landing
Fixed the style error. Comment on attachment 363120 [details] Patch for landing Clearing flags on attachment: 363120 Committed r242155: <https://trac.webkit.org/changeset/242155> All reviewed patches have been landed. Closing bug. |