Bug 79176 – Add dump method for WebCore::Int{Point, Rect, Size}

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[Leo Yang]

Conversion a IntPoint to string is useful for debug purpose.

[Leo Yang]

Created attachment 128117 [details]
Patch

[Antonio Gomes]

Comment on attachment 128117 [details]
Patch

feel like making it #ifndef NDEBUG?

[Leo Yang]

(In reply to comment #2)
> (From update of attachment 128117 [details])
> feel like making it #ifndef NDEBUG?

In debug mode, people can use debugger. It seems that toString is not needed in debug mode then.

[Leo Yang]

Comment on attachment 128117 [details]
Patch

Sending to cq...

[WebKit Review Bot]

Comment on attachment 128117 [details]
Patch

Rejecting attachment 128117 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t.h:30,
                 from Source/Platform/chromium/public/WebFloatPoint.h:37,
                 from Source/Platform/chromium/public/WebFloatQuad.h:35,
                 from Source/Platform/chromium/src/WebFloatQuad.cpp:32:
out/Debug/obj/gen/webcore_headers/IntPoint.h:30: fatal error: PlatformString.h: No such file or directory
compilation terminated.
make: *** [out/Debug/obj.target/webkit_platform/Source/Platform/chromium/src/WebFloatQuad.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/11520354

[Leo Yang]

(In reply to comment #5)
> (From update of attachment 128117 [details])
> Rejecting attachment 128117 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> t.h:30,
>                  from Source/Platform/chromium/public/WebFloatPoint.h:37,
>                  from Source/Platform/chromium/public/WebFloatQuad.h:35,
>                  from Source/Platform/chromium/src/WebFloatQuad.cpp:32:
> out/Debug/obj/gen/webcore_headers/IntPoint.h:30: fatal error: PlatformString.h: No such file or directory
> compilation terminated.
> make: *** [out/Debug/obj.target/webkit_platform/Source/Platform/chromium/src/WebFloatQuad.o] Error 1
> make: *** Waiting for unfinished jobs....
> 
> Full output: http://queues.webkit.org/results/11520354

It seems that Chromium is doing something special. See Source/Platform/Platform.gyp/Platform.gyp:

54             'variables': {
 55                 # List of headers that are #included in Platform API headers that exist inside
 56                 # the WebCore directory. These are only included when WEBKIT_IMPLEMENTATION=1.
 57                 # Since Platform/ can't add WebCore/* to the include path, this build step
 58                 # copies these headers into the shared intermediate directory and adds that to the include path.
 59                 # This is temporary, the better solution is to move these headers into the Platform
 60                 # directory for all ports and just use them as normal.
 61                 'webcore_headers': [
 62                     '../../WebCore/platform/graphics/FloatPoint.h',
 63                     '../../WebCore/platform/graphics/FloatQuad.h',
 64                     '../../WebCore/platform/graphics/FloatRect.h',
 65                     '../../WebCore/platform/graphics/FloatSize.h',
 66                     '../../WebCore/platform/graphics/IntPoint.h',
 67                     '../../WebCore/platform/graphics/IntRect.h',
 68                     '../../WebCore/platform/graphics/IntSize.h',
 69                 ],
 70                 'output_dir': '<(SHARED_INTERMEDIATE_DIR)/webcore_headers'

[WebKit Review Bot]

Comment on attachment 128117 [details]
Patch

Attachment 128117 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11556412

[Alexey Proskuryakov]

Comment on attachment 128117 [details]
Patch

In WebCore, functions with this purpose have "dump" in the name. Also, it's much easier to use a free-standing function than a method in a debugger:

void dumpIntPoint(const IntPoint*);

[Antonio Gomes]

ap, some context from talking to leo yesterday on IRC, not sure you saw it:

<agomes> leoyang, ping
<agomes> leoyang, maybe see this as a reference:
<agomes> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/Region.cpp#L175
<agomes> we could do Int{Rect,Point,Size}::dump()
<agomes> guarded by debug builds
<agomes> same for Float{Rect,Point,Size}
<agomes> what do you think?
<leoyang> agomes: it's a good idea
<leoyang> agomes: Do you think String ClassName::dumpAsText() is better?
<leoyang> hard coded printf is not flexiable
<agomes> it coudl it as well. I was just checking what is already there
<agomes> what would you use instead of printf?
<leoyang> maybe on some platform, someone want to log into their own log system.
<leoyang> I mean we just returns String and then let the user decide what to do.
<leoyang> agomes ^

[Alexey Proskuryakov]

I think that we should design to make specific use cases easy, not for a general case that may never be used in practice.

We have multiple approaches to debug dumping in the code base, sometimes sending to stdio, other times to a stream. The latter is most useful when a compound type is printed, making it possible to format output nicely.

It's not exactly sure to me what the use case is here. Bug description says "debug purposes", but function name doesn't make it clear that it's a free-form string for debugging only. And you don't need any special code to print in gdb, as "print" command would do just fine.

[Antonio Gomes]

(In reply to comment #10)
> I think that we should design to make specific use cases easy, not for a general case that may never be used in practice.
> 
> We have multiple approaches to debug dumping in the code base, sometimes sending to stdio, other times to a stream. The latter is most useful when a compound type is printed, making it possible to format output nicely.
> 
> It's not exactly sure to me what the use case is here. Bug description says "debug purposes", but function name doesn't make it clear that it's a free-form string for debugging only. And you don't need any special code to print in gdb, as "print" command would do just fine.

Fair enough.

I like the way Region::dump works, and that is why I proposed it originally. But if you are feeling strong about showDebug(IntRect), that is fine with me too.

[Antonio Gomes]

so what is the plan herE? so we move on...

::dump method? or a namespace free show(IntRect*)

[Leo Yang]

I would add IntPoint::dump() and guard it with #ifndef NDEBUG. I would also add this method on Int{Rect, Point} and Float{Point, Point3D, Quad, Size, Rect}. If all of you agree with this. I'll change the bug title and upload a patch.

[Leo Yang]

Changing bug title.

[Leo Yang]

Created attachment 130268 [details]
Patch v2

[Antonio Gomes]

Comment on attachment 130268 [details]
Patch v2

LGTM, but I will let @ap to review. Particularly, I still prefer a toString, where you can output it your own way.

[Alexey Proskuryakov]

So what's the use case for these functions? In debugger, you can achieve the same result with print command. How do you intend to use them?

[Antonio Gomes]

In my common debugging case (not necessarily in a debugger) I would for example do:

...
_code_

prinft("%s blah:%s bleh:%.2f", __PRETTY_FUNCTION__, rect.toString().utf8().data(), layoutUnit);

_more code_
...

That is why i do not think the way it is now it equally useful, since it does not allow formatted output.

You opposed to the this (the first patch), and suggested a namespace free dumpIntRect(rect). IMO both IntRect::dump (current patch) and dumpIntRect(IntRect*) (your suggestion) are not the best, *but* I go with what can be useful for others as well. Maybe I can adapt...

[Leo Yang]

This kind of basic classes are usually in many hot code path. Debugger is kind of annoying sometimes because the break points will be hit many times. If someone wants to debug the hot code path, toString or printf is useful. IMO, toString is more flexible for formatting, but just as other classes, like Region, dump() is also ok to me.

[Eric Seidel (no email)]

I don't think these are helpful enough to warrent adding.  In the past there have been concerns about gdb being dumb and not understanding foo.bar(), which is why we have dumpBar(foo) functions.  In any case, I'm not sure this added debugging code is actually that useful.  Folks who need this will just add it when they are debugging something.

I believe there is also a gdb helper script which does some of this for you.
http://trac.webkit.org/browser/trunk/Tools/gdb

[Eric Seidel (no email)]

Comment on attachment 130268 [details]
Patch v2

Cleared review? from attachment 130268 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).