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

[bugzilla-daemon at webkit.org]

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

--- Comment #24 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 336619
  --> https://bugs.webkit.org/attachment.cgi?id=336619
Patch

Some additional points.

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

> Source/WebCore/loader/HTTPHeaderField.h:88
> +    static bool isCommentText(UChar);

I would keep the namespace RTFC7230 and define functions we want to expose there.
The other functions would remain declared only in the cpp file.

> Source/WebCore/loader/HeaderFieldTokenizer.cpp:2
> + * Copyright 2017 The Chromium Authors. All rights reserved.

Is this class coming from Chromium?

> Source/WebCore/loader/HeaderFieldTokenizer.h:35
> +class HeaderFieldTokenizer final {

No need for final here.

> Source/WebCore/loader/HeaderFieldTokenizer.h:38
> +    HeaderFieldTokenizer(const String& header_field);

explicit.

> Source/WebCore/loader/HeaderFieldTokenizer.h:45
> +    bool consumeToken(String& output);

Why not returning directly a String or an optional<String> if we need to disambiguate error cases?
I wonder whether it should not be returning something like optional<StringView> to reduce the memory allocation.

> Source/WebCore/loader/ResourceTiming.cpp:109
> +void ResourceTiming::populateServerTiming(Vector<RefPtr<PerformanceServerTiming>>& serverTiming)

Instead of passing an out parameter, can we directly return a Vector<Ref<>>?
For improved efficiency, you could then write it as wtf::map(*m_serverTiming) [] (auto&& item) { return adoptRef(*new ...)l };

If serverTiming is already full of entries, that might be fine. Or we could use the reserveCapacity/uncheckedAppend pattern.

> Source/WebCore/loader/ResourceTiming.cpp:122
> +            serverTiming->append(entry->isolatedCopy());

We probably have a CrossThreadCopier that will do that for you.

> Source/WebCore/loader/ResourceTiming.h:78
> +    std::unique_ptr<Vector<std::unique_ptr<ServerTiming>>> m_serverTiming;

Can we just have a Vector<ServerTiming>?

> Source/WebCore/loader/ServerTiming.cpp:37
> +        }

We could early return here.

> Source/WebCore/loader/ServerTiming.h:32
> +class ServerTiming {

How about making it a struct instead?

> Source/WebCore/page/PerformanceServerTiming.h:36
> +    PerformanceServerTiming(const String& name, double duration, const String& description);

We usually make the constructor private and add a static create method.
We also usually pass String&& as in parameters.

> Source/WebCore/page/PerformanceServerTiming.h:39
> +    String name() const { return m_name; }

can be a const String&, ditto below.

-- 
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/382283b8/attachment-0001.html>