Bug 175569 - Add the PerformanceServerTiming Interface which makes Server-Timing header timing values available to JavaScript running in the browser.
Summary: Add the PerformanceServerTiming Interface which makes Server-Timing header ti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 184758
Blocks:
  Show dependency treegraph
 
Reported: 2017-08-15 07:31 PDT by cvazac
Modified: 2018-11-29 13:57 PST (History)
14 users (show)

See Also:


Attachments
Patch (50.77 KB, patch)
2018-02-12 10:23 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (52.36 KB, patch)
2018-02-12 12:46 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (158.66 KB, patch)
2018-02-12 13:28 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (158.67 KB, patch)
2018-02-12 13:43 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (158.66 KB, patch)
2018-02-12 18:51 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (158.94 KB, patch)
2018-02-12 19:02 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (158.66 KB, patch)
2018-02-12 20:20 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (159.23 KB, patch)
2018-02-13 07:07 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (159.26 KB, patch)
2018-02-13 07:18 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (158.39 KB, patch)
2018-02-14 09:00 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (158.64 KB, patch)
2018-02-14 10:52 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (158.31 KB, patch)
2018-02-15 08:27 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (157.89 KB, patch)
2018-02-15 14:17 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (157.97 KB, patch)
2018-02-15 14:24 PST, cvazac
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews201 for win-future (11.46 MB, application/zip)
2018-02-15 16:29 PST, Build Bot
no flags Details
Patch (158.41 KB, patch)
2018-03-27 08:18 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (158.88 KB, patch)
2018-03-27 14:17 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (154.56 KB, patch)
2018-04-02 15:29 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (169.48 KB, patch)
2018-04-02 18:57 PDT, cvazac
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-sierra (2.96 MB, application/zip)
2018-04-02 20:49 PDT, Build Bot
no flags Details
Patch (183.27 KB, patch)
2018-04-03 19:50 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (183.49 KB, patch)
2018-04-06 12:33 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (182.83 KB, patch)
2018-04-06 12:58 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (184.17 KB, patch)
2018-04-09 08:43 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (184.71 KB, patch)
2018-04-09 09:30 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (184.70 KB, patch)
2018-04-09 09:50 PDT, cvazac
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.72 MB, application/zip)
2018-04-09 11:20 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.21 MB, application/zip)
2018-04-09 11:27 PDT, Build Bot
no flags Details
Patch (185.53 KB, patch)
2018-04-09 14:52 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (185.53 KB, patch)
2018-04-10 06:48 PDT, cvazac
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-sierra (2.95 MB, application/zip)
2018-04-10 08:32 PDT, Build Bot
no flags Details
Patch (185.53 KB, patch)
2018-04-10 08:41 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (185.53 KB, patch)
2018-04-10 10:59 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (185.53 KB, patch)
2018-04-10 11:47 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (185.82 KB, patch)
2018-04-10 20:04 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (228.94 KB, patch)
2018-04-18 10:59 PDT, cvazac
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.23 MB, application/zip)
2018-04-18 12:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.95 MB, application/zip)
2018-04-18 12:29 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.43 MB, application/zip)
2018-04-18 12:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.14 MB, application/zip)
2018-04-18 12:58 PDT, Build Bot
no flags Details
Patch (183.43 KB, patch)
2018-05-11 12:52 PDT, cvazac
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-sierra (3.13 MB, application/zip)
2018-05-11 15:05 PDT, Build Bot
no flags Details
Patch (196.07 KB, patch)
2018-05-14 07:15 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (198.86 KB, patch)
2018-05-14 12:19 PDT, cvazac
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.03 MB, application/zip)
2018-05-14 13:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.20 MB, application/zip)
2018-05-14 14:14 PDT, Build Bot
no flags Details
Patch (199.16 KB, patch)
2018-05-15 08:00 PDT, cvazac
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description cvazac 2017-08-15 07:31:59 PDT
Parse Server-Timing response headers and make the metric name(s), value(s), and description(s) available on the `serverTiming` property of `PerformanceNavigationTiming` and `PerformanceResourceTiming` entries on the global performance timeline and to PerformanceObservers. 

Spec found here:
https://w3c.github.io/server-timing/

Landed as "experimental feature" in Chrome:
https://www.chromestatus.com/features/5695708376072192
Comment 1 cvazac 2018-02-12 10:23:33 PST
Created attachment 333605 [details]
Patch
Comment 2 Build Bot 2018-02-12 10:26:32 PST
Attachment 333605 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 cvazac 2018-02-12 12:46:33 PST
Created attachment 333619 [details]
Patch
Comment 4 cvazac 2018-02-12 13:28:51 PST
Created attachment 333624 [details]
Patch
Comment 5 Build Bot 2018-02-12 13:33:28 PST
Attachment 333624 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 202 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 cvazac 2018-02-12 13:43:29 PST
Created attachment 333627 [details]
Patch
Comment 7 cvazac 2018-02-12 18:51:59 PST
Created attachment 333657 [details]
Patch
Comment 8 cvazac 2018-02-12 19:02:28 PST
Created attachment 333660 [details]
Patch
Comment 9 cvazac 2018-02-12 20:20:55 PST
Created attachment 333668 [details]
Patch
Comment 10 cvazac 2018-02-13 07:07:22 PST
Created attachment 333686 [details]
Patch
Comment 11 cvazac 2018-02-13 07:18:19 PST
Created attachment 333687 [details]
Patch
Comment 12 cvazac 2018-02-14 09:00:42 PST
Created attachment 333805 [details]
Patch
Comment 13 cvazac 2018-02-14 10:52:57 PST
Created attachment 333817 [details]
Patch
Comment 14 cvazac 2018-02-15 08:27:20 PST
Created attachment 333902 [details]
Patch
Comment 15 cvazac 2018-02-15 14:17:19 PST
Created attachment 333943 [details]
Patch
Comment 16 cvazac 2018-02-15 14:24:07 PST
Created attachment 333945 [details]
Patch
Comment 17 Build Bot 2018-02-15 16:29:43 PST
Comment on attachment 333945 [details]
Patch

Attachment 333945 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6524626

New failing tests:
http/tests/preload/onerror_event.html
Comment 18 Build Bot 2018-02-15 16:29:53 PST
Created attachment 333966 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 19 cvazac 2018-03-27 08:18:18 PDT
Created attachment 336586 [details]
Patch
Comment 20 cvazac 2018-03-27 14:17:30 PDT
Created attachment 336619 [details]
Patch
Comment 21 cvazac 2018-03-29 08:05:45 PDT
Bots are finally green! Can I get a review for this, please? :)
Comment 22 Alex Christensen 2018-03-29 11:00:41 PDT
Comment on attachment 336619 [details]
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.
Comment 23 Alex Christensen 2018-03-29 11:16:50 PDT
Comment on attachment 336619 [details]
Patch

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

> Source/WebCore/loader/ParsedServerTiming.cpp:68
> +            entries->append(std::make_unique<ServerTiming>(entry));

This should become entries.append(WTFMove(entry));
Comment 24 youenn fablet 2018-03-29 11:33:30 PDT
Comment on attachment 336619 [details]
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.
Comment 25 youenn fablet 2018-03-29 12:49:22 PDT
By the way, is there any implication in leaking such header value information to third party origins? Is the plan to rely on Timing-Allow-Origin?
Comment 26 cvazac 2018-04-02 15:29:07 PDT
Created attachment 337036 [details]
Patch
Comment 27 cvazac 2018-04-02 15:29:49 PDT
I'm not quite ready for another review yet, a few more things to address.
Comment 28 cvazac 2018-04-02 15:30:46 PDT
(In reply to youenn fablet from comment #25)
> By the way, is there any implication in leaking such header value
> information to third party origins? Is the plan to rely on
> Timing-Allow-Origin?

Yes, exactly. Parsing server timing headers is gated by `m_allowTimingDetails`
Comment 29 cvazac 2018-04-02 18:57:34 PDT
Created attachment 337052 [details]
Patch
Comment 30 Build Bot 2018-04-02 20:49:06 PDT
Comment on attachment 337052 [details]
Patch

Attachment 337052 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7187299

New failing tests:
imported/w3c/web-platform-tests/server-timing/test_server_timing.html
imported/w3c/web-platform-tests/server-timing/cross_origin.html
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html
Comment 31 Build Bot 2018-04-02 20:49:07 PDT
Created attachment 337057 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 32 cvazac 2018-04-03 19:50:31 PDT
Created attachment 337139 [details]
Patch
Comment 33 youenn fablet 2018-04-04 16:56:27 PDT
Comment on attachment 337139 [details]
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.
Comment 34 cvazac 2018-04-06 12:33:42 PDT
Created attachment 337383 [details]
Patch
Comment 35 youenn fablet 2018-04-06 12:44:16 PDT
Comment on attachment 337383 [details]
Patch

I was going to say r+, patch is not applying though.
Some comments below.

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

> Source/WebCore/loader/ServerTimingParser.cpp:41
> +    if (!headerValue.isNull()) {

We should early return here.

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

If value is null here, is it fine skipping it/adding the entry or should we raise some kind of error?

> Source/WebCore/loader/ServerTimingParser.h:36
> +Vector<ServerTiming> parseServerTiming(const String&);

It would probably be good to add some unit tests for this method.
See Tools/TestWebKitAPI/Tests/ for examples.

> Source/WebCore/page/mac/WheelEventDeltaFilterMac.h:31
> +#include <wtf/MonotonicTime.h>

Probably not needed/unrelated?
Comment 36 cvazac 2018-04-06 12:58:26 PDT
Created attachment 337385 [details]
Patch
Comment 37 cvazac 2018-04-09 08:43:22 PDT
Created attachment 337498 [details]
Patch
Comment 38 cvazac 2018-04-09 09:30:20 PDT
Created attachment 337502 [details]
Patch
Comment 39 cvazac 2018-04-09 09:50:55 PDT
Created attachment 337505 [details]
Patch
Comment 40 Build Bot 2018-04-09 11:20:00 PDT
Comment on attachment 337505 [details]
Patch

Attachment 337505 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7254958

New failing tests:
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html
imported/w3c/web-platform-tests/server-timing/cross_origin.html
Comment 41 Build Bot 2018-04-09 11:20:02 PDT
Created attachment 337512 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 42 Build Bot 2018-04-09 11:27:42 PDT
Comment on attachment 337505 [details]
Patch

Attachment 337505 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7254923

New failing tests:
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html
imported/w3c/web-platform-tests/server-timing/cross_origin.html
Comment 43 Build Bot 2018-04-09 11:27:43 PDT
Created attachment 337513 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 44 cvazac 2018-04-09 14:52:02 PDT
Created attachment 337545 [details]
Patch
Comment 45 cvazac 2018-04-10 06:48:42 PDT
Created attachment 337602 [details]
Patch
Comment 46 Build Bot 2018-04-10 08:32:51 PDT
Comment on attachment 337602 [details]
Patch

Attachment 337602 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7268089

New failing tests:
imported/w3c/web-platform-tests/workers/name-property.html
Comment 47 Build Bot 2018-04-10 08:32:53 PDT
Created attachment 337606 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 48 cvazac 2018-04-10 08:41:55 PDT
Created attachment 337607 [details]
Patch
Comment 49 cvazac 2018-04-10 10:59:43 PDT
Created attachment 337620 [details]
Patch
Comment 50 cvazac 2018-04-10 11:47:16 PDT
Created attachment 337624 [details]
Patch
Comment 51 youenn fablet 2018-04-10 13:58:39 PDT
Comment on attachment 337624 [details]
Patch

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

> Source/WebCore/loader/ResourceTiming.h:30
> +#include "PerformanceServerTiming.h"

Can you try forward-declare PerformanceServerTiming?

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

If you can forward declare PerformanceServerTiming, could you also check whether we can easily change this constructor to take r-values for each meaningful field (provided it is used only/mainly for  ResourceTiming::isolatedCopy).
Comment 52 cvazac 2018-04-10 20:04:49 PDT
Created attachment 337670 [details]
Patch
Comment 53 youenn fablet 2018-04-10 20:43:38 PDT
Comment on attachment 337670 [details]
Patch

r=me.

As a side note, LoadTiming has an isolatedCopy method but it seems it does not have anything to isolate.
The isolatedCopy seems equivalent to a copy constructor.
Comment 54 Ryosuke Niwa 2018-04-10 22:44:09 PDT
Comment on attachment 337670 [details]
Patch

r-. This feature needs to be guarded behind a runtime flag.
Comment 55 cvazac 2018-04-18 10:59:19 PDT
Created attachment 338227 [details]
Patch
Comment 56 youenn fablet 2018-04-18 11:10:58 PDT
This starts to become a big patch.
I would recommend introducing the runtime flag in a separate patch.
Comment 57 cvazac 2018-04-18 11:48:54 PDT
(In reply to youenn fablet from comment #56)
> This starts to become a big patch.
> I would recommend introducing the runtime flag in a separate patch.

Ok, can/should I just delete the latest patch? Or shall I submit a new one with the changes revered?
Comment 58 Build Bot 2018-04-18 12:28:05 PDT
Comment on attachment 338227 [details]
Patch

Attachment 338227 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7360357

New failing tests:
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html
imported/w3c/web-platform-tests/server-timing/cross_origin.html
Comment 59 Build Bot 2018-04-18 12:28:07 PDT
Created attachment 338241 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 60 Build Bot 2018-04-18 12:29:20 PDT
Comment on attachment 338227 [details]
Patch

Attachment 338227 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7360324

New failing tests:
imported/w3c/web-platform-tests/server-timing/cross_origin.html
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html
Comment 61 Build Bot 2018-04-18 12:29:21 PDT
Created attachment 338242 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 62 Build Bot 2018-04-18 12:49:50 PDT
Comment on attachment 338227 [details]
Patch

Attachment 338227 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7360435

New failing tests:
imported/w3c/web-platform-tests/server-timing/cross_origin.html
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html
Comment 63 Build Bot 2018-04-18 12:49:52 PDT
Created attachment 338246 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 64 Build Bot 2018-04-18 12:58:30 PDT
Comment on attachment 338227 [details]
Patch

Attachment 338227 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7360520

New failing tests:
imported/w3c/web-platform-tests/server-timing/cross_origin.html
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html
Comment 65 Build Bot 2018-04-18 12:58:32 PDT
Created attachment 338248 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 66 youenn fablet 2018-04-18 13:00:29 PDT
(In reply to cvazac from comment #57)
> (In reply to youenn fablet from comment #56)
> > This starts to become a big patch.
> > I would recommend introducing the runtime flag in a separate patch.
> 
> Ok, can/should I just delete the latest patch? Or shall I submit a new one
> with the changes revered?

I would make the runtime flag patch blocking this particular bug.
When the runtime flag patch lands, it should be a small refresh to your previous patch here.
Comment 67 cvazac 2018-04-23 09:20:12 PDT
added: https://bugs.webkit.org/show_bug.cgi?id=184758
Comment 68 cvazac 2018-05-11 12:52:09 PDT
Created attachment 340213 [details]
Patch
Comment 69 Build Bot 2018-05-11 15:05:09 PDT
Comment on attachment 340213 [details]
Patch

Attachment 340213 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7654639

New failing tests:
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.https.html
imported/w3c/web-platform-tests/server-timing/resource_timing_idl.https.html
imported/w3c/web-platform-tests/server-timing/navigation_timing_idl.html
imported/w3c/web-platform-tests/server-timing/test_server_timing.https.html
imported/w3c/web-platform-tests/server-timing/navigation_timing_idl.https.html
Comment 70 Build Bot 2018-05-11 15:05:10 PDT
Created attachment 340226 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 71 cvazac 2018-05-14 07:15:44 PDT
Created attachment 340311 [details]
Patch
Comment 72 youenn fablet 2018-05-14 09:00:50 PDT
Comment on attachment 340311 [details]
Patch

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

> Source/WebCore/loader/ResourceTiming.cpp:79
>      , m_allowTimingDetails(passesTimingAllowCheck(response, securityOrigin))

Do we have some tests that combine ServerTiming and Timing-Allow-Origin?
Comment 73 cvazac 2018-05-14 10:58:17 PDT
(In reply to youenn fablet from comment #72)
> Comment on attachment 340311 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340311&action=review
> 
> > Source/WebCore/loader/ResourceTiming.cpp:79
> >      , m_allowTimingDetails(passesTimingAllowCheck(response, securityOrigin))
> 
> Do we have some tests that combine ServerTiming and Timing-Allow-Origin?

Adding one here: 
https://github.com/w3c/web-platform-tests/pull/10991
Comment 74 cvazac 2018-05-14 12:19:39 PDT
Created attachment 340339 [details]
Patch
Comment 75 Build Bot 2018-05-14 13:45:24 PDT
Comment on attachment 340339 [details]
Patch

Attachment 340339 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7680672

New failing tests:
imported/w3c/web-platform-tests/server-timing/service_worker_idl.html
Comment 76 Build Bot 2018-05-14 13:45:26 PDT
Created attachment 340351 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 77 Build Bot 2018-05-14 14:14:25 PDT
Comment on attachment 340339 [details]
Patch

Attachment 340339 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7680705

New failing tests:
imported/w3c/web-platform-tests/server-timing/service_worker_idl.html
Comment 78 Build Bot 2018-05-14 14:14:27 PDT
Created attachment 340356 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 79 cvazac 2018-05-15 08:00:11 PDT
Created attachment 340410 [details]
Patch
Comment 80 WebKit Commit Bot 2018-05-15 12:24:36 PDT
Comment on attachment 340410 [details]
Patch

Clearing flags on attachment: 340410

Committed r231813: <https://trac.webkit.org/changeset/231813>
Comment 81 WebKit Commit Bot 2018-05-15 12:24:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 82 Radar WebKit Bug Importer 2018-05-15 12:26:34 PDT
<rdar://problem/40266028>
Comment 83 Simon Fraser (smfr) 2018-11-29 13:43:47 PST
This added ServerTimingParser.h/cpp at the top level of the WebCore Xcode project. Please fix.
Comment 84 Joseph Pecoraro 2018-11-29 13:57:29 PST
(In reply to Simon Fraser (smfr) from comment #83)
> This added ServerTimingParser.h/cpp at the top level of the WebCore Xcode
> project. Please fix.

I'll take care of this.