[webkit-reviews] review denied: [Bug 175569] Add the PerformanceServerTiming Interface which makes Server-Timing header timing values available to JavaScript running in the browser. : [Attachment 336619] Patch

Thu Mar 29 11:00:41 PDT 2018

Alex Christensen <achristensen at apple.com> has denied cvazac at gmail.com's request
for review:
Bug 175569: Add the PerformanceServerTiming Interface which makes Server-Timing
header timing values available to JavaScript running in the browser.
https://bugs.webkit.org/show_bug.cgi?id=175569

Attachment 336619: Patch

https://bugs.webkit.org/attachment.cgi?id=336619&action=review

--- Comment #22 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 336619
  --> https://bugs.webkit.org/attachment.cgi?id=336619
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336619&action=review

Cool!

> Source/WebCore/loader/ParsedServerTiming.h:14
> + *	  * Neither the name of Google Inc. nor the names of its

I think you have the wrong copyright header.  Google isn't mentioned in the
copyright of this file.

> Source/WebCore/loader/ParsedServerTiming.h:39
> +class ParsedServerTiming {

ServerTimingParser?

> Source/WebCore/loader/ParsedServerTiming.h:41
> +    static std::unique_ptr<Vector<std::unique_ptr<ServerTiming>>>
parseServerTiming(const String&);

This should be Vector<ServerTiming> or std::optional<Vector<ServerTiming>> if
it can fail.

> Source/WebCore/loader/ResourceTiming.cpp:113
> +	       serverTiming.append(adoptRef(new
PerformanceServerTiming(entry->name(), entry->duration(),
entry->description())));

adoptRef(*new ...)
Replace Vector<RefPtr<...>> with Vector<Ref<...>> when possible so we know it
can't be null.

> Source/WebCore/loader/ServerTiming.cpp:49
> +std::unique_ptr<ServerTiming> ServerTiming::isolatedCopy() const
> +{
> +    return std::make_unique<ServerTiming>(*this);
> +}

This should call .isolatedCopy on each string.

> Source/WebCore/loader/ServerTiming.h:36
> +    : m_name(name) { }

indent

> Source/WebCore/loader/ServerTiming.h:38
> +    : m_name(other.m_name), m_duration(other.m_duration),
m_description(other.m_description) { }

ditto

> Source/WebCore/loader/ServerTiming.h:44
> +    std::unique_ptr<ServerTiming> isolatedCopy() const;

ServerTiming.  unique_ptr unnecessary here.

> Source/WebCore/page/PerformanceResourceTiming.cpp:84
> +    resourceTiming.populateServerTiming(m_serverTiming);

m_serverTiming(resourceTiming.serverTiming());
or something like that.  Return-by-reference is confusing.  It leaves it
unclear if m_serverTiming was expected to be empty before or not.

> LayoutTests/imported/w3c/web-platform-tests/server-timing/cross_origin.html:1
> +<!DOCTYPE html>

These tests need a ChangeLog and a -expected.txt file so we can see what
passes.