[webkit-reviews] review granted: [Bug 194510] Store Ad Click Attribution requests in the network process : [Attachment 361922] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Feb 13 10:30:13 PST 2019
Alex Christensen <achristensen at apple.com> has granted John Wilander
<wilander at apple.com>'s request for review:
Bug 194510: Store Ad Click Attribution requests in the network process
https://bugs.webkit.org/show_bug.cgi?id=194510
Attachment 361922: Patch
https://bugs.webkit.org/attachment.cgi?id=361922&action=review
--- Comment #21 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 361922
--> https://bugs.webkit.org/attachment.cgi?id=361922
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=361922&action=review
r=me, lots of nits
> Source/WebCore/loader/AdClickAttribution.cpp:96
> + builder.appendLiteral("Source: ");
We might find it useful in the future to have this in JSON format. We can
always change it if needed, but just a thought.
> Source/WebCore/loader/AdClickAttribution.h:100
> String registrableDomain;
> + bool isDeleted { false };
String has a constructor that takes HashTableDeletedValueType, so we don't need
a separate bool for this.
> Source/WebCore/loader/AdClickAttribution.h:167
> String registrableDomain;
> + bool isDeleted { false };
ditto
> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:67
> + builder.appendLiteral("WebCore::AdClickAttribution ");
NetworkAdClickAttribution?
> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2301
> + completionHandler(emptyString());
I'm not sure if it's important to use emptyString here or if a null string
would suffice.
> Source/WebKit/UIProcess/WebPageProxy.cpp:4106
> + if (auto adClickAttribution = navigation->adClickAttribution()) {
auto&
No need to copy the Optional.
> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2731
> + WKRetainPtr<WKStringRef> messageName(AdoptWK,
WKStringCreateWithUTF8CString("dumpAdClickAttribution"));
This is old style WKRetainPtr use. Try this instead:
auto messageName = adoptWK(WKStringCreateWithUTF8CString(...));
> Tools/WebKitTestRunner/TestInvocation.h:148
> + String m_savedAdClickAttribution;
This is never set. We can remove it.
More information about the webkit-reviews
mailing list