WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
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
Details
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
Details
Patch
(209.78 KB, patch)
2019-02-26 18:41 PST
,
John Wilander
no flags
Details
Formatted Diff
Diff
Patch for landing
(210.17 KB, patch)
2019-02-27 12:18 PST
,
John Wilander
no flags
Details
Formatted Diff
Diff
Patch for landing
(170.58 KB, patch)
2019-02-27 12:25 PST
,
John Wilander
no flags
Details
Formatted Diff
Diff
Patch for landing
(170.59 KB, patch)
2019-02-27 13:06 PST
,
John Wilander
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2019-02-26 15:23:17 PST
<
rdar://problem/48417690
>
John Wilander
Comment 2
2019-02-26 16:10:28 PST
Created
attachment 363033
[details]
Patch
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
Created
attachment 363051
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug