WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
119636
dataLog dumpers for WebCore's basic geometry types
https://bugs.webkit.org/show_bug.cgi?id=119636
Summary
dataLog dumpers for WebCore's basic geometry types
Tim Horton
Reported
2013-08-09 12:56:05 PDT
This way you can dataLog(anIntRect, someFloatPoint, "\n") and all will be well. Will save (Simon and I at least) lots of typing.
Attachments
take one
(6.79 KB, patch)
2013-08-09 14:10 PDT
,
Tim Horton
fpizlo
: review+
eflews.bot
: commit-queue-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Tim Horton
Comment 1
2013-08-09 14:10:15 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?).
Darin Adler
Comment 2
2013-08-09 14:27:06 PDT
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
Comment 3
2013-08-09 14:28:02 PDT
(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
Comment 4
2013-08-09 14:32:05 PDT
(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
Comment 5
2013-08-09 14:34:07 PDT
(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
Comment 6
2013-08-09 14:35:08 PDT
(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 7
2013-08-09 14:49:31 PDT
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
Comment 8
2013-08-09 15:29:38 PDT
(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 9
2013-08-09 15:31:34 PDT
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
Comment 10
2013-08-09 15:33:17 PDT
(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
Comment 11
2013-08-12 12:12:31 PDT
http://trac.webkit.org/changeset/153952
Antonio Gomes
Comment 12
2013-08-12 12:41:50 PDT
I remember seeing objections for such kind of print methods addition to these classes.
Antonio Gomes
Comment 13
2013-08-12 12:43:22 PDT
(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
Comment 14
2013-08-12 12:55:43 PDT
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug