Bug 123260 – Replace 0 timeouts g_timeout_add() by g_idle_add()

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Bastien Nocera]

.

[Bastien Nocera]

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.

[WebKit Commit Bot]

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.

[Bastien Nocera]

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.

[Bastien Nocera]

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).

[WebKit Commit Bot]

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.

[Anders Carlsson]

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.

[Philippe Normand]

I'll update the patch.

[Philippe Normand]

Created attachment 215146 [details]
Patch

[Carlos Garcia Campos]

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 <[e-mail]>

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.

[Philippe Normand]

(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 <[e-mail]>
> 
> 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.

[Philippe Normand]

(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 <[e-mail]>
> 
> 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 :)

[Carlos Garcia Campos]

(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 <[e-mail]>
> > 
> > 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

[Carlos Garcia Campos]

(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 <[e-mail]>
> > 
> > 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.

[Philippe Normand]

Created attachment 215148 [details]
patch for laundring

[Gustavo Noronha (kov)]

(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 [e-mail], which I never use in changelogs, to [e-mail])

[Carlos Garcia Campos]

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.

[Philippe Normand]

Created attachment 215286 [details]
patch

[Carlos Garcia Campos]

Comment on attachment 215286 [details]
patch

Thanks

[WebKit Commit Bot]

Comment on attachment 215286 [details]
patch

Clearing flags on attachment: 215286

Committed r158103: <http://trac.webkit.org/changeset/158103>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Bastien Nocera]

(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 <[e-mail]>
> > > 
> > > 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?

[Philippe Normand]

(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 <[e-mail]>
> > > > 
> > > > 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

[Bastien Nocera]

Thanks.