Ease printf debugging in WebKit.
Created attachment 151321 [details] Patch
(In reply to comment #1) > Created an attachment (id=151321) [details] > Patch Some work in progress/idea. Obviously need to be finished.
see discussion here: https://bugs.webkit.org/show_bug.cgi?id=79176
Comment on attachment 151321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151321&action=review > Source/WTF/wtf/DebugHelpers.h:32 > +#include "windows.h" <windows.h>? > Source/WTF/wtf/DebugHelpers.h:61 > + OutputDebugString((wchar_t*)output.utf8().data()); I think it is better to use OutputDebugStringW(wchar_t* utf16ButNoSurrogate) or converting code page CP_ACP with OutputDebugStringA(char* mbcsStringButNotUTF8)
Comment on attachment 151321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151321&action=review >> Source/WTF/wtf/DebugHelpers.h:32 >> +#include "windows.h" > > <windows.h>? Well for OutputDebugString. I don't have a Windows machine to test so I would rely on someone else to guide me.
(In reply to comment #5) > (From update of attachment 151321 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151321&action=review > > >> Source/WTF/wtf/DebugHelpers.h:32 > >> +#include "windows.h" > > > > <windows.h>? > > Well for OutputDebugString. I don't have a Windows machine to test so I would rely on someone else to guide me. Ah I just saw, yes of course with <> it's better :)
Created attachment 151461 [details] Patch
Comment on attachment 151461 [details] Patch Attachment 151461 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13183080
Created attachment 151465 [details] Patch
Comment on attachment 151465 [details] Patch Attachment 151465 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13053003
Created attachment 151470 [details] Patch
Attachment 151470 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmak..." exit_code: 1 Source/WTF/wtf/DebugHelpers.h:52: Missing spaces around < [whitespace/operators] [3] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 151489 [details] Patch
Comment on attachment 151489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151489&action=review My informal comments on the patch. In general I think this is useful stuff. > Source/WTF/wtf/DebugHelpers.h:76 > + inline WTFDebug& space() > + { > + m_stream->m_space = true; > + m_stream->buffer.append(' '); > + return *this; > + } > + inline WTFDebug& nospace() > + { > + m_stream->m_space = false; > + return *this; > + } > + inline WTFDebug& maybeSpace() > + { > + if (m_stream->m_space) > + m_stream->buffer.append(' '); > + return *this; > + } It's not clear when is maybeSpace() supposed to be used. Could you give an example? > Source/WTF/wtf/DebugHelpers.h:145 > + class Stream : public RefCounted<Stream> { Why is it refcounted?
Comment on attachment 151489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151489&action=review >> Source/WTF/wtf/DebugHelpers.h:76 >> + } > > It's not clear when is maybeSpace() supposed to be used. Could you give an example? All the overload of << use it. And you have the concept with inline WTFDebug& operator<<(WTFDebug debug, const Vector<T>& vector). I set the debug.nospace() in that function, all overloads (depending the template value) which will be used after will have a no space. But the regular case they have a space, that's why the maybeSpace(). >> Source/WTF/wtf/DebugHelpers.h:145 >> + class Stream : public RefCounted<Stream> { > > Why is it refcounted? When using the default copy constructor m_stream will not be deep copied.
Huge discussion on mailing list... on the bug report... so... Any conclusions? Btw, I'm pro-debug-header-for-printf-style-debug-and-happiness.
(In reply to comment #16) > Huge discussion on mailing list... on the bug report... so... Any conclusions? > > Btw, I'm pro-debug-header-for-printf-style-debug-and-happiness. Based on the feedback of webkit-dev I will make another proposal.
Created attachment 173272 [details] Patch
Created attachment 173274 [details] WIP that I have locally.
Comment on attachment 173272 [details] Patch I don't think the flag is needed. It's one file to build so it's fine and you just don't include it if you don't need it. If I recall correctly on the ML it was wtfPrint that people preferred. Also it seems to be a better place on WTF than WebCore. It will not work on Mac also, it seems you need to output in stderr.
When I have tried to use the << operator over WTF and isolate the world of debuggers like I did in this patch on it (rather than adding in IntRect the way we use debug with it), I've noticed that it would create a cyclic dependency over WTF and WebCore. WTF should not need to use types like WebCore's IntRect on it. Obviously this just happened because I've went into this isolated approach, but this brought my attention to whether we should put this debug after all. I've decided to move to WebCore instead. Is this a bad move? It's also by purpose that I didn't put space between debugs. I've went into a simplistic approach, the user should be free to print the way he wants to, like: debug(__PRETTY_FUNCTION__, ": ", rect, "\n"); I've decided to put a flag just to avoid breakage or including unnecessary functions over platforms that don't really think it is useful. But for the others that think it can be helpful, then you should pass --debug-helpers on build to enable it.
(In reply to comment #21) > When I have tried to use the << operator over WTF and isolate the world of debuggers like I did in this patch on it (rather than adding in IntRect the way we use debug with it), I've noticed that it would create a cyclic dependency over WTF and WebCore. WTF should not need to use types like WebCore's IntRect on it. Obviously this just happened because I've went into this isolated approach, but this brought my attention to whether we should put this debug after all. I've decided to move to WebCore instead. Is this a bad move? > > It's also by purpose that I didn't put space between debugs. I've went into a simplistic approach, the user should be free to print the way he wants to, like: debug(__PRETTY_FUNCTION__, ": ", rect, "\n"); > > I've decided to put a flag just to avoid breakage or including unnecessary functions over platforms that don't really think it is useful. But for the others that think it can be helpful, then you should pass --debug-helpers on build to enable it. s/to whether we should/to where we should
(In reply to comment #21) > When I have tried to use the << operator over WTF and isolate the world of debuggers like I did in this patch on it (rather than adding in IntRect the way we use debug with it), I've noticed that it would create a cyclic dependency over WTF and WebCore. WTF should not need to use types like WebCore's IntRect on it. Obviously this just happened because I've went into this isolated approach, but this brought my attention to whether we should put this debug after all. I've decided to move to WebCore instead. Is this a bad move? > People didn't want << so you should not worry about those. > It's also by purpose that I didn't put space between debugs. I've went into a simplistic approach, the user should be free to print the way he wants to, like: debug(__PRETTY_FUNCTION__, ": ", rect, "\n"); > ok. > I've decided to put a flag just to avoid breakage or including unnecessary functions over platforms that don't really think it is useful. But for the others that think it can be helpful, then you should pass --debug-helpers on build to enable it.
*** Bug 110452 has been marked as a duplicate of this bug. ***
Comment on attachment 173272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173272&action=review > configure.ac:543 > +# check whether to enable DebugHelpers.h support > +AC_MSG_CHECKING([whether to enable DebugHelpers.h support]) > +AC_ARG_ENABLE(debug_helpers, > + AC_HELP_STRING([--enable-debug-helpers], > + [enable support for DebugHelpers.h [default=no]]), > + [],[enable_debug_helpers="no"]) > +AC_MSG_RESULT([$enable_debug_helpers]) This change and the change to the makefile shouldn't be necessary. Please see: http://trac.webkit.org/wiki/AddingFeatures
A feature flag for easier printf support? That seems a bit overkill, but I don't know the whole story.
The original idea was to have only a header in WebKit source used by nobody, so when someone want to debug something he just include this header and enjoy the happiness of an ease print-style-debug :-)
Comment on attachment 173272 [details] Patch Have you seen wtf/DataLog and wtf/PrintStream? Would that do what you want? In particular you can make any object printable by just implementing WTF::printInternal(PrintStream&, const YourType&)