.
Created attachment 215044 [details] Replace 0 timeouts g_timeout_add() by g_idle_add() A zero timeout should be equivalent to using g_idle_add_full(G_PRIORITY_DEFAULT, ... without the nagging feeling that the wrong API was used.
Attachment 215044 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/wtf/gtk/MainThreadGtk.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp', u'Source/WebCore/platform/gtk/GtkDragAndDropHelper.cpp', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp', u'Source/WebKit/gtk/WebCoreSupport/GtkAdjustmentWatcher.cpp', u'Source/WebKit/gtk/webkit/webkitwebview.cpp', u'Tools/DumpRenderTree/gtk/DumpRenderTree.cpp', u'Tools/DumpRenderTree/gtk/EventSender.cpp']" exit_code: 1 Source/WebKit/gtk/WebCoreSupport/GtkAdjustmentWatcher.cpp:94: Use 0 instead of NULL. [readability/null] [5] Source/WebKit/gtk/WebCoreSupport/GtkAdjustmentWatcher.cpp:94: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 215047 [details] Replace 0 timeouts g_timeout_add() by g_idle_add() A zero timeout should be equivalent to using g_idle_add_full(G_PRIORITY_DEFAULT, ... without the nagging feeling that the wrong API was used.
Comment on attachment 215047 [details] Replace 0 timeouts g_timeout_add() by g_idle_add() The "weird number of spaces" review comment is due to me realigning the second line of a function (eg. trying to follow the existing style).
Attachment 215047 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/wtf/gtk/MainThreadGtk.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp', u'Source/WebCore/platform/gtk/GtkDragAndDropHelper.cpp', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp', u'Source/WebKit/gtk/WebCoreSupport/GtkAdjustmentWatcher.cpp', u'Source/WebKit/gtk/webkit/webkitwebview.cpp', u'Tools/DumpRenderTree/gtk/DumpRenderTree.cpp', u'Tools/DumpRenderTree/gtk/EventSender.cpp']" exit_code: 1 Source/WebKit/gtk/WebCoreSupport/GtkAdjustmentWatcher.cpp:94: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 215047 [details] Replace 0 timeouts g_timeout_add() by g_idle_add() View in context: https://bugs.webkit.org/attachment.cgi?id=215047&action=review Patch looks good but needs a ChangeLog with the rationale. >> Source/WebKit/gtk/WebCoreSupport/GtkAdjustmentWatcher.cpp:94 >> + const_cast<void*>(static_cast<const void*>(this)), 0); > > Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] I know you're just matching what was here before, but the entire function call should be on a single line.
I'll update the patch.
Created attachment 215146 [details] Patch
Comment on attachment 215146 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215146&action=review > Source/WebCore/ChangeLog:11 > + Patch by Bastien Nocera <hadess@hadess.net> Why not adding the author to the changelog header so that commit-queue will credit him correctly? > Source/WebKit/gtk/webkit/webkitwebview.cpp:5276 > - g_timeout_add(1, cleanupTemporarilyCachedSubresources, g_list_copy(subResources)); > + g_idle_add_full(G_PRIORITY_DEFAULT, cleanupTemporarilyCachedSubresources, g_list_copy(subResources), 0); This is not correct, I don't know why but this was not a 0 timeout.
(In reply to comment #9) > (From update of attachment 215146 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215146&action=review > > > Source/WebCore/ChangeLog:11 > > + Patch by Bastien Nocera <hadess@hadess.net> > > Why not adding the author to the changelog header so that commit-queue will credit him correctly? > > > Source/WebKit/gtk/webkit/webkitwebview.cpp:5276 > > - g_timeout_add(1, cleanupTemporarilyCachedSubresources, g_list_copy(subResources)); > > + g_idle_add_full(G_PRIORITY_DEFAULT, cleanupTemporarilyCachedSubresources, g_list_copy(subResources), 0); > > This is not correct, I don't know why but this was not a 0 timeout. I think this should be fixed in another patch.
(In reply to comment #9) > (From update of attachment 215146 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215146&action=review > > > Source/WebCore/ChangeLog:11 > > + Patch by Bastien Nocera <hadess@hadess.net> > > Why not adding the author to the changelog header so that commit-queue will credit him correctly? > AFAIK the commit-queue does that only for commiters. Using Patch by is quite common and gives him credit already :)
(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 215146 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=215146&action=review > > > > > Source/WebCore/ChangeLog:11 > > > + Patch by Bastien Nocera <hadess@hadess.net> > > > > Why not adding the author to the changelog header so that commit-queue will credit him correctly? > > > > > Source/WebKit/gtk/webkit/webkitwebview.cpp:5276 > > > - g_timeout_add(1, cleanupTemporarilyCachedSubresources, g_list_copy(subResources)); > > > + g_idle_add_full(G_PRIORITY_DEFAULT, cleanupTemporarilyCachedSubresources, g_list_copy(subResources), 0); > > > > This is not correct, I don't know why but this was not a 0 timeout. > > I think this should be fixed in another patch. Well, I'm not sure it's wrong, it seems it was added intentionally, I still don't understand it, though see: https://bugs.webkit.org/show_bug.cgi?id=108960#c24
(In reply to comment #11) > (In reply to comment #9) > > (From update of attachment 215146 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=215146&action=review > > > > > Source/WebCore/ChangeLog:11 > > > + Patch by Bastien Nocera <hadess@hadess.net> > > > > Why not adding the author to the changelog header so that commit-queue will credit him correctly? > > > > AFAIK the commit-queue does that only for commiters. I don't think so. > Using Patch by is quite common and gives him credit already :) Looks weird to me, but I'm fine.
Created attachment 215148 [details] patch for laundring
(In reply to comment #13) > > AFAIK the commit-queue does that only for commiters. > > I don't think so. Philn is right, it doesn't even credit me when I use an email address that is not the svn username for the changelog. (which is why I asked to get that modified from kov@webkit.org, which I never use in changelogs, to gns@gnome.org)
Comment on attachment 215148 [details] patch for laundring View in context: https://bugs.webkit.org/attachment.cgi?id=215148&action=review > Source/WebKit/gtk/webkit/webkitwebview.cpp:5276 > - g_timeout_add(1, cleanupTemporarilyCachedSubresources, g_list_copy(subResources)); > + g_idle_add_full(G_PRIORITY_DEFAULT, cleanupTemporarilyCachedSubresources, g_list_copy(subResources), 0); I set r- only because of this, if it's wrong, let's fix it in a follow up patch as you suggested, otherwise don't change it because this commit is about changing 0 timeouts.
Created attachment 215286 [details] patch
Comment on attachment 215286 [details] patch Thanks
Comment on attachment 215286 [details] patch Clearing flags on attachment: 215286 Committed r158103: <http://trac.webkit.org/changeset/158103>
All reviewed patches have been landed. Closing bug.
(In reply to comment #12) > (In reply to comment #10) > > (In reply to comment #9) > > > (From update of attachment 215146 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=215146&action=review > > > > > > > Source/WebCore/ChangeLog:11 > > > > + Patch by Bastien Nocera <hadess@hadess.net> > > > > > > Why not adding the author to the changelog header so that commit-queue will credit him correctly? > > > > > > > Source/WebKit/gtk/webkit/webkitwebview.cpp:5276 > > > > - g_timeout_add(1, cleanupTemporarilyCachedSubresources, g_list_copy(subResources)); > > > > + g_idle_add_full(G_PRIORITY_DEFAULT, cleanupTemporarilyCachedSubresources, g_list_copy(subResources), 0); > > > > > > This is not correct, I don't know why but this was not a 0 timeout. > > > > I think this should be fixed in another patch. > > Well, I'm not sure it's wrong, it seems it was added intentionally, I still don't understand it, though see: > > https://bugs.webkit.org/show_bug.cgi?id=108960#c24 Did a separate bug get filed for that one?
(In reply to comment #21) > (In reply to comment #12) > > (In reply to comment #10) > > > (In reply to comment #9) > > > > (From update of attachment 215146 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=215146&action=review > > > > > > > > > Source/WebCore/ChangeLog:11 > > > > > + Patch by Bastien Nocera <hadess@hadess.net> > > > > > > > > Why not adding the author to the changelog header so that commit-queue will credit him correctly? > > > > > > > > > Source/WebKit/gtk/webkit/webkitwebview.cpp:5276 > > > > > - g_timeout_add(1, cleanupTemporarilyCachedSubresources, g_list_copy(subResources)); > > > > > + g_idle_add_full(G_PRIORITY_DEFAULT, cleanupTemporarilyCachedSubresources, g_list_copy(subResources), 0); > > > > > > > > This is not correct, I don't know why but this was not a 0 timeout. > > > > > > I think this should be fixed in another patch. > > > > Well, I'm not sure it's wrong, it seems it was added intentionally, I still don't understand it, though see: > > > > https://bugs.webkit.org/show_bug.cgi?id=108960#c24 > > Did a separate bug get filed for that one? Yes, bug 123404
Thanks.