Bug 119636 – dataLog dumpers for WebCore's basic geometry types

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Tim Horton]

This way you can dataLog(anIntRect, someFloatPoint, "\n") and all will be well. Will save (Simon and I at least) lots of typing.

[Tim Horton]

Created attachment 208452 [details]
take one

I'm somewhat tempted to put them all in one big implementation file (GeometryFormatters? I have no idea) instead of spreading datalog stuff throughout all these classes (especially because of IntPoint and IntSize lacking implementation files causing PrintStream.h inclusion in their headers -- but maybe PrintStream.h doesn't change often and that won't be a problem?).

[Darin Adler]

Comment on attachment 208452 [details]
take one

What’s the rationale for using member functions for these? Easier to invoke from the debugger?

[Darin Adler]

(In reply to comment #1)
> especially because of IntPoint and IntSize lacking implementation files causing PrintStream.h inclusion in their headers

Make IntPoint.cpp and IntSize.cpp instead; what’s the argument against that?

[Tim Horton]

(In reply to comment #3)
> (In reply to comment #1)
> > especially because of IntPoint and IntSize lacking implementation files causing PrintStream.h inclusion in their headers
> 
> Make IntPoint.cpp and IntSize.cpp instead; what’s the argument against that?

Valid point :)

(In reply to comment #2)
> (From update of attachment 208452 [details])
> What’s the rationale for using member functions for these? Easier to invoke from the debugger?

Is there a better way? It looked to me like DataLog falls back on calling T::dump() when it doesn't know about the type. Maybe pizlo can comment.

[Tim Horton]

(In reply to comment #2)
> (From update of attachment 208452 [details])
> What’s the rationale for using member functions for these? Easier to invoke from the debugger?

Also, you don't invoke these manually; they get invoked via dataLog()

[Darin Adler]

(In reply to comment #4)
> (In reply to comment #2)
> > What’s the rationale for using member functions for these? Easier to invoke from the debugger?
> 
> Is there a better way? It looked to me like DataLog falls back on calling T::dump() when it doesn't know about the type. Maybe pizlo can comment.

We should fix DataLog so it calls dump(const X&), which in turn can be a template that calls X::dump. That way you can make something dump-able without changing the class itself.

[EFL EWS Bot]

Comment on attachment 208452 [details]
take one

Attachment 208452 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1359579

[Filip Pizlo]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > What’s the rationale for using member functions for these? Easier to invoke from the debugger?
> > 
> > Is there a better way? It looked to me like DataLog falls back on calling T::dump() when it doesn't know about the type. Maybe pizlo can comment.
> 
> We should fix DataLog so it calls dump(const X&), which in turn can be a template that calls X::dump. That way you can make something dump-able without changing the class itself.

This already works. The canonical way to make something dump able is to add a dump(PrintStream&) method. But you can also add a WTF::printInternal(PrintStream&, const T&) function and get the same effect. 

In JSC the idiom I ultimately converged to was preferring a dump method over a printInternal function, except in the case of enums (obviously). I don't have a good reason for that preference other than it usually leads to less code, since a function in WTF means opening and then closing that namespace, and having to use whatever namespace T is in (or have a lot of JSC::T like code).

[Filip Pizlo]

Comment on attachment 208452 [details]
take one

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

> Source/WebCore/platform/graphics/IntRect.h:211
> +    void dump(WTF::PrintStream& out) const;

Don't need to say "WTF::"

[Filip Pizlo]

(In reply to comment #1)
> Created an attachment (id=208452) [details]
> take one
> 
> I'm somewhat tempted to put them all in one big implementation file (GeometryFormatters? I have no idea) instead of spreading datalog stuff throughout all these classes (especially because of IntPoint and IntSize lacking implementation files causing PrintStream.h inclusion in their headers -- but maybe PrintStream.h doesn't change often and that won't be a problem?).

It's already the case that a bunch of JSC and WTF headers include PrintStream.h - and no, it doesn't change often.

[Tim Horton]

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

[Antonio Gomes]

I remember seeing objections for such kind of print methods addition to these classes.

[Antonio Gomes]

(In reply to comment #12)
> I remember seeing objections for such kind of print methods addition to these classes.

https://bugs.webkit.org/show_bug.cgi?id=79176 - [Bug 79176] Add dump method for WebCore::Int{Point, Rect, Size}

[Tim Horton]

(In reply to comment #13)
> (In reply to comment #12)
> > I remember seeing objections for such kind of print methods addition to these classes.
> 
> https://bugs.webkit.org/show_bug.cgi?id=79176 - [Bug 79176] Add dump method for WebCore::Int{Point, Rect, Size}

Yeah, they've come back up again on #webkit too.