Bug 132917 - remove duplicate code in web timing
Summary: remove duplicate code in web timing
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Trivial
Assignee: Nobody
Depends on:
Reported: 2014-05-14 11:12 PDT by Alex Christensen
Modified: 2014-05-14 11:30 PDT (History)
1 user (show)

See Also:

Patch (10.16 KB, patch)
2014-05-14 11:15 PDT, Alex Christensen
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2014-05-14 11:12:01 PDT
I added some duplicate code copying timing data from the NSURLConnection to the ResourceLoadTiming in https://bugs.webkit.org/show_bug.cgi?id=132574 and I'm going to move that code to ResourceHandle.
Comment 1 Alex Christensen 2014-05-14 11:15:48 PDT
Created attachment 231459 [details]
Comment 2 Alexey Proskuryakov 2014-05-14 11:22:36 PDT
Comment on attachment 231459 [details]

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

> Source/WebCore/platform/network/ResourceHandle.h:33
> +#include "ResourceLoadTiming.h"

Please don't include it, just forward declare.

> Source/WebCore/platform/network/ResourceHandle.h:134
> +    static void transferTimingData(NSURLConnection*, ResourceLoadTiming&);

I'd say "get", not "transfer".

> Source/WebCore/platform/network/mac/WebCoreResourceHandleAsDelegate.mm:175
> +    ResourceHandle::transferTimingData(connection, resourceResponse.resourceLoadTiming());

This is not what I was thinking of - I think that a nicer way to implement this would be to call into ResourceHandle here, which would then read the timing data, and call the client. Duplicating even the ResourceHandle::transferTimingData still leaves the potential for us to forget about it in future reimplementations.

It's OK to land as is though, the patch is an improvement.
Comment 3 Alex Christensen 2014-05-14 11:30:35 PDT
(In reply to comment #2)
> Please don't include it, just forward declare.
> I'd say "get", not "transfer".

Committed with these changes to http://trac.webkit.org/changeset/168849