Bug 45428 – [chromium] Add chromium port API for accessing Web Timing information

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Gentilcore]

[chromium] Add chromium port API for accessing Web Timing information

[Tony Gentilcore]

Created attachment 66976 [details]
Patch

[chris fleizach]

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=

[Darin Fisher (:fishd, Google)]

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.

[Tony Gentilcore]

Created attachment 68451 [details]
Patch

[Tony Gentilcore]

Chris, thanks for getting this patch jump-started again :-)

Darin, I think I have all the comment addressed, could you take another pass?

[Darin Fisher (:fishd, Google)]

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()

[Tony Gentilcore]

Committed r68141: <http://trac.webkit.org/changeset/68141>