Summary: | ResourceLoadTiming should use reference instead of pointer | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Minor | CC: | andersca, berto, cdumez, cgarcia, commit-queue, danw, gustavo, mrobinson | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Alex Christensen
2014-05-12 16:57:57 PDT
Created attachment 231348 [details]
Patch
Created attachment 231382 [details]
Patch
Created attachment 231386 [details]
Patch
Created attachment 231403 [details]
Patch
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. 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? Created attachment 231406 [details]
Patch
> 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.
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? (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. |