Bug 195071 – Adopt WebCore::RegistrableDomain in WebCore::ResourceLoadStatistics and WebKit::NetworkProcessProxy

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

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.

[Radar WebKit Bug Importer]

<rdar://problem/48417690>

[John Wilander]

Created attachment 363033 [details]
Patch

[John Wilander]

I don't know what's up with the reshuffled DerivedSources files.

[Brent Fulgham]

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 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]

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 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]

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]

(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]

Created attachment 363051 [details]
Patch

[Alex Christensen]

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 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 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]

(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 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]

Created attachment 363112 [details]
Patch for landing

[John Wilander]

Forgot to revert the darn xcfilelist changes again. :/

[EWS Watchlist]

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]

Created attachment 363113 [details]
Patch for landing

[EWS Watchlist]

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]

Created attachment 363120 [details]
Patch for landing

[John Wilander]

Fixed the style error.

[WebKit Commit Bot]

Comment on attachment 363120 [details]
Patch for landing

Clearing flags on attachment: 363120

Committed r242155: <https://trac.webkit.org/changeset/242155>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.