Bug 195071

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 Flags
Patch
none
Archive of layout-test-results from ews100 for mac-highsierra
none
Archive of layout-test-results from ews117 for mac-highsierra
none
Patch
none
Patch for landing
none
Patch for landing
none
Patch for landing none

Description John Wilander 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.
Comment 1 Radar WebKit Bug Importer 2019-02-26 15:23:17 PST
<rdar://problem/48417690>
Comment 2 John Wilander 2019-02-26 16:10:28 PST
Created attachment 363033 [details]
Patch
Comment 3 John Wilander 2019-02-26 16:11:09 PST
I don't know what's up with the reshuffled DerivedSources files.
Comment 4 Brent Fulgham 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.
Comment 5 EWS Watchlist 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
Comment 6 EWS Watchlist 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
Comment 7 EWS Watchlist 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
Comment 8 EWS Watchlist 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
Comment 9 John Wilander 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.
Comment 10 John Wilander 2019-02-26 18:41:24 PST
Created attachment 363051 [details]
Patch
Comment 11 Alex Christensen 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.
Comment 12 Brent Fulgham 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).
Comment 13 Brent Fulgham 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.
Comment 14 John Wilander 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.
Comment 15 Alex Christensen 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
Comment 16 John Wilander 2019-02-27 12:18:14 PST
Created attachment 363112 [details]
Patch for landing
Comment 17 John Wilander 2019-02-27 12:20:29 PST
Forgot to revert the darn xcfilelist changes again. :/
Comment 18 EWS Watchlist 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.
Comment 19 John Wilander 2019-02-27 12:25:35 PST
Created attachment 363113 [details]
Patch for landing
Comment 20 EWS Watchlist 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.
Comment 21 John Wilander 2019-02-27 13:06:20 PST
Created attachment 363120 [details]
Patch for landing
Comment 22 John Wilander 2019-02-27 13:07:08 PST
Fixed the style error.
Comment 23 WebKit Commit Bot 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>
Comment 24 WebKit Commit Bot 2019-02-27 13:34:34 PST
All reviewed patches have been landed.  Closing bug.