|
Description
cvazac
2017-08-15 07:31:59 PDT
Created attachment 333605 [details]
Patch
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.
Created attachment 333619 [details]
Patch
Created attachment 333624 [details]
Patch
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.
Created attachment 333627 [details]
Patch
Created attachment 333657 [details]
Patch
Created attachment 333660 [details]
Patch
Created attachment 333668 [details]
Patch
Created attachment 333686 [details]
Patch
Created attachment 333687 [details]
Patch
Created attachment 333805 [details]
Patch
Created attachment 333817 [details]
Patch
Created attachment 333902 [details]
Patch
Created attachment 333943 [details]
Patch
Created attachment 333945 [details]
Patch
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 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
Created attachment 336586 [details]
Patch
Created attachment 336619 [details]
Patch
Bots are finally green! Can I get a review for this, please? :) 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 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 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. 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? Created attachment 337036 [details]
Patch
I'm not quite ready for another review yet, a few more things to address. (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` Created attachment 337052 [details]
Patch
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 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
Created attachment 337139 [details]
Patch
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. Created attachment 337383 [details]
Patch
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? Created attachment 337385 [details]
Patch
Created attachment 337498 [details]
Patch
Created attachment 337502 [details]
Patch
Created attachment 337505 [details]
Patch
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 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 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 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
Created attachment 337545 [details]
Patch
Created attachment 337602 [details]
Patch
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 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
Created attachment 337607 [details]
Patch
Created attachment 337620 [details]
Patch
Created attachment 337624 [details]
Patch
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). Created attachment 337670 [details]
Patch
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 on attachment 337670 [details]
Patch
r-. This feature needs to be guarded behind a runtime flag.
Created attachment 338227 [details]
Patch
This starts to become a big patch. I would recommend introducing the runtime flag in a separate patch. (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 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 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 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 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 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 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 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 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
(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. Created attachment 340213 [details]
Patch
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 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
Created attachment 340311 [details]
Patch
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? (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 Created attachment 340339 [details]
Patch
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 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 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 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
Created attachment 340410 [details]
Patch
Comment on attachment 340410 [details] Patch Clearing flags on attachment: 340410 Committed r231813: <https://trac.webkit.org/changeset/231813> All reviewed patches have been landed. Closing bug. This added ServerTimingParser.h/cpp at the top level of the WebCore Xcode project. Please fix. (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. |