Bug 132846 – ResourceLoadTiming should use reference instead of pointer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alex Christensen]

As promised in https://bugs.webkit.org/show_bug.cgi?id=132574 I'm using references for ResourceLoadTiming, which is instanced in PerformanceResourceTiming and ResourceResponseBase.  This avoids unnecessary null checks and reference counting, and it makes the code nicer to read.

[Alex Christensen]

Created attachment 231348 [details]
Patch

[Alex Christensen]

Created attachment 231382 [details]
Patch

[Alex Christensen]

Created attachment 231386 [details]
Patch

[Alex Christensen]

Created attachment 231403 [details]
Patch

[Alex Christensen]

The fourth patch tried to get rid of my use of mutable in ResourceResponseBase.h, but it doesn't work nicely with the way Soup uses ResourceLoadTiming.  I think the third patch is the way to go.  It uses mutable, but there are lots of mutable things in ResourceResponseBase.

[Anders Carlsson]

Comment on attachment 231386 [details]
Patch

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

Is there a reason why you can't just have an std::unique_ptr reference to the ResourceLoadTiming object? Maybe it doesn't matter.
r- because of the assignment operator.

> Source/WebCore/platform/network/ResourceLoadTiming.h:65
> +    void operator=(const ResourceLoadTiming& other)
> +    {
> +        domainLookupStart = other.domainLookupStart;
> +        domainLookupEnd = other.domainLookupEnd;
> +        connectStart = other.connectStart;
> +        connectEnd = other.connectEnd;
> +        requestStart = other.requestStart;
> +        responseStart = other.responseStart;
> +        secureConnectionStart = other.secureConnectionStart;
> +    }

This is not a correct assignment operator, it needs to return *this.

> Source/WebCore/platform/network/ResourceResponseBase.cpp:555
>      lazyInit(CommonAndUncommonFields);

Do we need to call this now?

[Alex Christensen]

Created attachment 231406 [details]
Patch

[Alexey Proskuryakov]

> Is there a reason why you can't just have an std::unique_ptr reference to the ResourceLoadTiming object? 

It seems nice to avoid a separate allocation, given that nearly(?) all of the responses will have the timing data.

[Alexey Proskuryakov]

Comment on attachment 231406 [details]
Patch

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

And the downside is of course that touching ResourceTiming header will make half of WebCore rebuild, because half of WebCore indirectly includes ResourceResponse.h.

> Source/WebCore/platform/network/mac/WebCoreResourceHandleAsDelegate.mm:197
> +        timing.domainLookupStart = domainLookupStart <= 0.0 ? -1 : (domainLookupStart - referenceStart) * 1000;

I don't think that we ever need to compare to 0.0, can it be simply 0?

[Alex Christensen]

(In reply to comment #9)
> I don't think that we ever need to compare to 0.0, can it be simply 0?
I'll do this when I refactor the duplicate code.

[Alex Christensen]

http://trac.webkit.org/changeset/168845