Bug 45428 - [chromium] Add chromium port API for accessing Web Timing information
Summary: [chromium] Add chromium port API for accessing Web Timing information
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-08 18:00 PDT by Tony Gentilcore
Modified: 2010-09-23 08:30 PDT (History)
2 users (show)

See Also:


Attachments
Patch (13.83 KB, patch)
2010-09-08 18:02 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (13.92 KB, patch)
2010-09-22 15:08 PDT, Tony Gentilcore
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2010-09-08 18:00:02 PDT
[chromium] Add chromium port API for accessing Web Timing information
Comment 1 Tony Gentilcore 2010-09-08 18:02:42 PDT
Created attachment 66976 [details]
Patch
Comment 2 chris fleizach 2010-09-22 00:53:35 PDT
Comment on attachment 66976 [details]
Patch

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

comments are mainly minor

> WebKit/chromium/src/WebFrameImpl.h:94
> +    virtual WebPerformance performance() const;

any reason why this is not
virtual WebPerformance&

> WebKit/chromium/src/WebPerformance.cpp:44
> +

shouldn't this be
WebPerformance::WebPerformance(const WebPerformance& p)
     : m_private(p)

> WebKit/chromium/src/WebPerformance.cpp:55
> +

i think this happens automatically in C++ unless you say explicit in the constructor

> WebKit/chromium/src/WebPerformance.cpp:154
> +

this should be at the top with the other constructor

> WebKit/chromium/src/WebPerformance.cpp:160
> +

ditto with the other operator=
Comment 3 Darin Fisher (:fishd, Google) 2010-09-22 09:04:25 PDT
Comment on attachment 66976 [details]
Patch

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

> WebKit/chromium/public/WebPerformance.h:46
> +    virtual ~WebPerformance();

this destructor does not need to be virtual.  this class is not designed
to be subclassed, right?  no need to bloat this object with a vtable pointer.

> WebKit/chromium/public/WebPerformance.h:47
> +    WebPerformance& operator=(const WebPerformance& p);

any non-inline methods outside of WEBKIT_IMPLEMENTATION, need to be marked WEBKIT_API.

in general, we do not export constructors or destructors.  in this case, you
should define a void assign(const WebPerformance&) function and call that to
implement both the copy-ctor as well as the assignment operator.

>> WebKit/chromium/src/WebFrameImpl.h:94
>> +    virtual WebPerformance performance() const;
> 
> any reason why this is not
> virtual WebPerformance&

yeah, it should not be a reference.  WebPreference is intended to
be allocated on the stack as a temporary value.  it works like a
RefPtr<T>.  see the definition of WebPrivatePtr<T> if you are curious.

>> WebKit/chromium/src/WebPerformance.cpp:154
>> +
> 
> this should be at the top with the other constructor

no, it is better to list the functions in the .cpp file in the same order in which they appear in the header file.
Comment 4 Tony Gentilcore 2010-09-22 15:08:05 PDT
Created attachment 68451 [details]
Patch
Comment 5 Tony Gentilcore 2010-09-22 15:09:09 PDT
Chris, thanks for getting this patch jump-started again :-)

Darin, I think I have all the comment addressed, could you take another pass?
Comment 6 Darin Fisher (:fishd, Google) 2010-09-22 16:54:48 PDT
Comment on attachment 68451 [details]
Patch

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

> WebKit/chromium/src/WebFrameImpl.cpp:700
> +        return WebPerformance(0);

nit: no (0) here.  just return WebPerformance()
Comment 7 Tony Gentilcore 2010-09-23 08:30:31 PDT
Committed r68141: <http://trac.webkit.org/changeset/68141>