RESOLVED FIXED 195071
Adopt WebCore::RegistrableDomain in WebCore::ResourceLoadStatistics and WebKit::NetworkProcessProxy
https://bugs.webkit.org/show_bug.cgi?id=195071
Summary Adopt WebCore::RegistrableDomain in WebCore::ResourceLoadStatistics and WebKi...
John Wilander
Reported 2019-02-26 15:21:45 PST
This is a follow-up task from https://bugs.webkit.org/show_bug.cgi?id=194791 where we further adopt the new WebCore::RegistrableDomain class in WebCore::ResourceLoadStatistics and WebKit::NetworkProcessProxy.
Attachments
Patch (208.83 KB, patch)
2019-02-26 16:10 PST, John Wilander
no flags
Archive of layout-test-results from ews100 for mac-highsierra (2.43 MB, application/zip)
2019-02-26 17:22 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews117 for mac-highsierra (2.50 MB, application/zip)
2019-02-26 17:56 PST, EWS Watchlist
no flags
Patch (209.78 KB, patch)
2019-02-26 18:41 PST, John Wilander
no flags
Patch for landing (210.17 KB, patch)
2019-02-27 12:18 PST, John Wilander
no flags
Patch for landing (170.58 KB, patch)
2019-02-27 12:25 PST, John Wilander
no flags
Patch for landing (170.59 KB, patch)
2019-02-27 13:06 PST, John Wilander
no flags
Radar WebKit Bug Importer
Comment 1 2019-02-26 15:23:17 PST
John Wilander
Comment 2 2019-02-26 16:10:28 PST
John Wilander
Comment 3 2019-02-26 16:11:09 PST
I don't know what's up with the reshuffled DerivedSources files.
Brent Fulgham
Comment 4 2019-02-26 17:14:31 PST
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.
EWS Watchlist
Comment 5 2019-02-26 17:22:19 PST
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
EWS Watchlist
Comment 6 2019-02-26 17:22:20 PST
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
EWS Watchlist
Comment 7 2019-02-26 17:56:33 PST
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
EWS Watchlist
Comment 8 2019-02-26 17:56:34 PST
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
John Wilander
Comment 9 2019-02-26 18:33:28 PST
(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.
John Wilander
Comment 10 2019-02-26 18:41:24 PST
Alex Christensen
Comment 11 2019-02-27 11:08:18 PST
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.
Brent Fulgham
Comment 12 2019-02-27 11:10:05 PST
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).
Brent Fulgham
Comment 13 2019-02-27 11:10:44 PST
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.
John Wilander
Comment 14 2019-02-27 11:12:38 PST
(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.
Alex Christensen
Comment 15 2019-02-27 11:41:46 PST
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
John Wilander
Comment 16 2019-02-27 12:18:14 PST
Created attachment 363112 [details] Patch for landing
John Wilander
Comment 17 2019-02-27 12:20:29 PST
Forgot to revert the darn xcfilelist changes again. :/
EWS Watchlist
Comment 18 2019-02-27 12:21:48 PST
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.
John Wilander
Comment 19 2019-02-27 12:25:35 PST
Created attachment 363113 [details] Patch for landing
EWS Watchlist
Comment 20 2019-02-27 12:29:23 PST
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.
John Wilander
Comment 21 2019-02-27 13:06:20 PST
Created attachment 363120 [details] Patch for landing
John Wilander
Comment 22 2019-02-27 13:07:08 PST
Fixed the style error.
WebKit Commit Bot
Comment 23 2019-02-27 13:34:32 PST
Comment on attachment 363120 [details] Patch for landing Clearing flags on attachment: 363120 Committed r242155: <https://trac.webkit.org/changeset/242155>
WebKit Commit Bot
Comment 24 2019-02-27 13:34:34 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.