[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 #33 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 337139
  --> https://bugs.webkit.org/attachment.cgi?id=337139
Patch

Looks almost ready.
Some comments below.

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

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

Since these are public, it would probably be more consistent to have these returning std::optional<String> or maybe just a String and String::isNull would be the error case.

> Source/WebCore/loader/ResourceTiming.cpp:106
> +        m_serverTiming = ServerTimingParser::parseServerTiming(response.httpHeaderField(HTTPHeaderName::ServerTiming));

To be noted that we are currently tightening our security model and reducing exposure of HTTP response headers in WebProcess to the minimum.
When going from NetworkProcess to WebProcess, we will in a near future, filter response headers that are not absolutely needed (see bug 184310).
We will probably need to implement something similar to m_allowTimingDetails in NetworkProcess when sending the resource response.

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

You can probably use WTF::map here so that uncheckedAppend/reserveInitialCapacity are used behind the scenes.

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

You can probably make use of CrossThreadCopy instead.

> Source/WebCore/loader/ResourceTiming.h:62
> +    ResourceTiming(const URL& url, const String& initiator, const LoadTiming& loadTiming, const NetworkLoadMetrics& networkLoadMetrics, bool allowTimingDetails, Vector<ServerTiming> serverTiming)

Should be &&

> Source/WebCore/loader/ResourceTiming.h:68
> +        , m_serverTiming(serverTiming)

WTFMove(serverTiming)

> Source/WebCore/loader/ServerTiming.cpp:51
> +    return ServerTiming(this->name.isolatedCopy(), this->duration, this->description.isolatedCopy());

Is "this->" needed?
You should also probably copy the other fields, something like:
ServerTiming { name.isolatedCopy(), duration, description.isolatedCopy(), durationSet, descriptionSet };

> Source/WebCore/loader/ServerTiming.h:48
> +    : name(name)

Use WTFMove here and below.

> Source/WebCore/loader/ServerTimingParser.cpp:56
> +                if (tokenizer.consume('=')) {

So we can have just a parameter name without value and this is valid, right?

> Source/WebCore/loader/ServerTimingParser.cpp:57
> +                    tokenizer.consumeTokenOrQuotedString(value);

We should probably check whether parsing was fine?

> Source/WebCore/loader/ServerTimingParser.h:34
> +class ServerTimingParser {

Do we need that class?
parseServerTiming as a free function seems fine.

> Source/WebCore/loader/WorkerThreadableLoader.h:33
> +#include "NetworkLoadMetrics.h"

Why is it needed?

> Source/WebCore/page/PerformanceResourceTiming.h:37
> +#include "PerformanceServerTiming.h"

If possible, we usually forward reference declarations instead of including header files here.
This can probably be done close to line 42.

> Source/WebCore/page/PerformanceServerTiming.h:37
> +    virtual ~PerformanceServerTiming();

It probably does not need to be virtual.
You might then need in PerformanceServerTiming.idl to add ImplementationLacksVTable keyword.

-- 
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/20180404/40084750/attachment-0002.html>