[Webkit-unassigned] [Bug 175569] Add the PerformanceServerTiming Interface which makes Server-Timing header timing values available to JavaScript running in the browser.

Thu Mar 29 11:00:41 PDT 2018

https://bugs.webkit.org/show_bug.cgi?id=175569

Alex Christensen <achristensen at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #336619|review?                     |review-
              Flags|                            |

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180329/3364b294/attachment.html>