[webkit-reviews] review denied: [Bug 61152] [Resource Timing] Implement Resource Timing interface : [Attachment 139128] Patch

[bugzilla-daemon at webkit.org]

Tony Gentilcore <tonyg at chromium.org> has denied James Simonsen
<simonjam at chromium.org>'s request for review:
Bug 61152: [Resource Timing] Implement Resource Timing interface
https://bugs.webkit.org/show_bug.cgi?id=61152

Attachment 139128: Patch
https://bugs.webkit.org/attachment.cgi?id=139128&action=review

------- Additional Comments from Tony Gentilcore <tonyg at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=139128&action=review


Setting r- for now until we figure out the comment about initiator type/time.

Everything else is nit-picky.

> Source/WebCore/ChangeLog:6
> +	   This patch implements the Resource Timing interface. It doesn't do
anything

I recommend a link to the spec in the ChangeLog.

> Source/WebCore/page/Performance.cpp:63
> +    return frame()->document()->scriptExecutionContext();

Are we guaranteed a Frame here?

Also, Document is a ScriptExecutionContext, so is the scriptExecutionContext()
call doing anything useful?

> Source/WebCore/page/Performance.cpp:94
> +    for (Vector<RefPtr<PerformanceEntry> >::const_iterator resource =
m_resourceTimingBuffer.begin(); resource != m_resourceTimingBuffer.end();
++resource)

Vector does have an append() that takes a Vector and appends all. Would it be
cleaner to avoid the iterator here and modify PerformanceEntryList to take
advantage of that? We seem to append all more than once.

> Source/WebCore/page/Performance.cpp:138
> +{

Add FIXME

> Source/WebCore/page/Performance.h:43
> +#include "ResourceResponse.h"

Can these both be forward declared?

> Source/WebCore/page/PerformanceResourceTiming.cpp:50
> +    case ResourceRequest::InitiatorIsCSS:

There was a recent thread about these. Are they being reworked in the spec?

> Source/WebCore/page/PerformanceResourceTiming.cpp:68
> +    case ResourceRequest::InitiatorIsUnknown:

Are there known unknowns? If so, this shouldn't hit the ASSERT_NOT_REACHED().

> Source/WebCore/page/PerformanceResourceTiming.h:36
> +#include "Performance.h"

Unused?

> Source/WebCore/page/PerformanceResourceTiming.h:38
> +#include "ResourceLoadTiming.h"

I always forget whether it is okay to forward declare something used as a
RefPtr. You have it one way for Document and another way for
ResourceLoadTiming. So either this can be forward declared or else we are
pulling in Document.h transitively and it should be explicit.

> Source/WebCore/page/PerformanceResourceTiming.h:39
> +#include "ResourceRequest.h"

Forward declare like ResourceResponse?

> Source/WebCore/page/PerformanceResourceTiming.h:41
> +#include <wtf/RefCounted.h>

Should be wtf/RefPtr.h, right? And also add wtf/text/WTFString.h.

> Source/WebCore/platform/network/ResourceRequestBase.h:153
> +	   double initiatedTime() const { return m_initiatedTime; }

The devtools timeline seems to already know initiatedTime and initiatorType.
DevTools and ResourceTiming should probably share the same data store for this
information.

Have you looked into how that works and whether any of these ResourceRequest
modifications are absolutely necessary?

> Source/WebCore/platform/network/ResourceRequestBase.h:214
> +	   double m_initiatedTime;

Worth enforcing const here?