Bug 123229 – Name all the GLib timeout sources

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Bastien Nocera]

.

[Bastien Nocera]

Created attachment 215006 [details]
Replace 0 timeouts g_timeout_add() by g_idle_add()

A zero timeout should be equivalent to using g_idle_add()
without the nagging feeling that the wrong API was used.

[Bastien Nocera]

Created attachment 215007 [details]
Name all the GLib timeout sources

See http://www.hadess.net/2013/10/reducing-wake-ups-2013-edition.html

[Carlos Garcia Campos]

Comment on attachment 215006 [details]
Replace 0 timeouts g_timeout_add() by g_idle_add()

Not exactly true, because g_idle and g_timeout use different priorities by default. g_timeout_add(0 is equivalent to g_idle_add_full(G_PRIORITY_DEFAULT. But I agree with you and I have changed some of those in the past.

[Carlos Garcia Campos]

Comment on attachment 215007 [details]
Name all the GLib timeout sources

View in context: https://bugs.webkit.org/attachment.cgi?id=215007&action=review

We don't use usually attach more than one patch to the same bug report, could you please file a new bug report and upload an updated patch with the style issues fixed please? You can run Tools/Scripts/check-webkit-style script to make sure your patch doesn't have style issues before uploading them. Also, if you ask for review, setting the review flag to ?, the style bot will also run the style checker for you.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:415
> +        g_source_set_name_by_id (m_readyTimerHandler, "[WebKit] mediaPlayerPrivateReadyStateTimeoutCallback");

WebKit coding style doesn't allow space between function name and parentheses.

> Tools/DumpRenderTree/gtk/TestRunnerGtk.cpp:319
> +        guint id;
> +        id = g_timeout_add_seconds(timeoutSeconds, waitToDumpWatchdogFired, 0);

Use a single line here.

> Tools/MiniBrowser/gtk/BrowserWindow.c:164
> +        guint id;
> +        id = g_timeout_add(500, (GSourceFunc)resetEntryProgress, window->uriEntry);

Ditto.

[Carlos Garcia Campos]

WebKit patches should include a ChangeLog entry, you can use Tools/Scripts/prepare-ChangeLog script to generate the changelog entry for your patch before submitting it. See also:

http://www.webkit.org/coding/contributing.html

Thanks the patches.

[Bastien Nocera]

Created attachment 215042 [details]
Name all the GLib timeout sources

See http://www.hadess.net/2013/10/reducing-wake-ups-2013-edition.html

[Bastien Nocera]

(In reply to comment #5)
> WebKit patches should include a ChangeLog entry, you can use Tools/Scripts/prepare-ChangeLog script to generate the changelog entry for your patch before submitting it.

I can't do that, I'm working off a tarball.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #5)
> > WebKit patches should include a ChangeLog entry, you can use Tools/Scripts/prepare-ChangeLog script to generate the changelog entry for your patch before submitting it.
> 
> I can't do that, I'm working off a tarball.

Ok, I'll add the changelog entry for you then

[Bastien Nocera]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > WebKit patches should include a ChangeLog entry, you can use Tools/Scripts/prepare-ChangeLog script to generate the changelog entry for your patch before submitting it.
> > 
> > I can't do that, I'm working off a tarball.
> 
> Ok, I'll add the changelog entry for you then

Thanks. Bug 123260 will have the same problem.

[Philippe Normand]

Carlos, I can take care of this if you haven't already?

[Carlos Garcia Campos]

(In reply to comment #10)
> Carlos, I can take care of this if you haven't already?

Of course, I didn't have time yesterday in the end.

[Philippe Normand]

Created attachment 215288 [details]
patch

[Carlos Garcia Campos]

Comment on attachment 215288 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215288&action=review

> Source/WebCore/platform/gtk/SharedTimerGtk.cpp:61
> +    g_source_set_name_by_id(sharedTimer, "[WebKit] timeout_cb");

I wonder how useful the name is in cases like this one where a generic name is used. Maybe we could use the class name SharedTimer::timeout_cb, or simply rename these callbacks to something more descriptive (in this case, it doesn't even follow the webkit style), like sharedTimerTimeoutCallback.

[Philippe Normand]

Created attachment 215292 [details]
patch

[Anders Carlsson]

Comment on attachment 215292 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215292&action=review

> Source/WebCore/ChangeLog:8
> +        Give a name to GLiib timeout sources, this is helpful when

Typo, GLiib.

[Philippe Normand]

Created attachment 215307 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 215307 [details]
Patch for landing

Clearing flags on attachment: 215307

Committed r158110: <http://trac.webkit.org/changeset/158110>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.