Summary: | dataLog dumpers for WebCore's basic geometry types | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||
Component: | Layout and Rendering | Assignee: | Tim Horton <thorton> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ap, darin, dbates, eflews.bot, fpizlo, gyuyoung.kim, sam, simon.fraser, tonikitoo | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Tim Horton
2013-08-09 12:56:05 PDT
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?).
Comment on attachment 208452 [details]
take one
What’s the rationale for using member functions for these? Easier to invoke from the debugger?
(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? (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. (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() (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. 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 (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). 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::" (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. I remember seeing objections for such kind of print methods addition to these classes. (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} (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. |