Bug 123229 - Name all the GLib timeout sources
Summary: Name all the GLib timeout sources
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-23 15:58 PDT by Bastien Nocera
Modified: 2013-10-28 08:20 PDT (History)
12 users (show)

See Also:


Attachments
Replace 0 timeouts g_timeout_add() by g_idle_add() (9.40 KB, patch)
2013-10-23 16:00 PDT, Bastien Nocera
no flags Details | Formatted Diff | Diff
Name all the GLib timeout sources (13.75 KB, patch)
2013-10-23 16:01 PDT, Bastien Nocera
no flags Details | Formatted Diff | Diff
Name all the GLib timeout sources (13.25 KB, patch)
2013-10-24 03:12 PDT, Bastien Nocera
no flags Details | Formatted Diff | Diff
patch (15.83 KB, patch)
2013-10-28 01:26 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
patch (16.45 KB, patch)
2013-10-28 02:41 PDT, Philippe Normand
andersca: review+
andersca: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (16.38 KB, patch)
2013-10-28 07:52 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bastien Nocera 2013-10-23 15:58:58 PDT
.
Comment 1 Bastien Nocera 2013-10-23 16:00:06 PDT
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.
Comment 2 Bastien Nocera 2013-10-23 16:01:47 PDT
Created attachment 215007 [details]
Name all the GLib timeout sources

See http://www.hadess.net/2013/10/reducing-wake-ups-2013-edition.html
Comment 3 Carlos Garcia Campos 2013-10-24 00:52:16 PDT
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.
Comment 4 Carlos Garcia Campos 2013-10-24 00:57:56 PDT
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.
Comment 5 Carlos Garcia Campos 2013-10-24 00:59:22 PDT
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.
Comment 6 Bastien Nocera 2013-10-24 03:12:51 PDT
Created attachment 215042 [details]
Name all the GLib timeout sources

See http://www.hadess.net/2013/10/reducing-wake-ups-2013-edition.html
Comment 7 Bastien Nocera 2013-10-24 03:13:38 PDT
(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.
Comment 8 Carlos Garcia Campos 2013-10-24 03:31:54 PDT
(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
Comment 9 Bastien Nocera 2013-10-24 03:34:09 PDT
(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.
Comment 10 Philippe Normand 2013-10-25 00:10:29 PDT
Carlos, I can take care of this if you haven't already?
Comment 11 Carlos Garcia Campos 2013-10-25 00:58:46 PDT
(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.
Comment 12 Philippe Normand 2013-10-28 01:26:09 PDT
Created attachment 215288 [details]
patch
Comment 13 Carlos Garcia Campos 2013-10-28 02:37:26 PDT
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.
Comment 14 Philippe Normand 2013-10-28 02:41:49 PDT
Created attachment 215292 [details]
patch
Comment 15 Anders Carlsson 2013-10-28 06:45:48 PDT
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.
Comment 16 Philippe Normand 2013-10-28 07:52:02 PDT
Created attachment 215307 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2013-10-28 08:20:45 PDT
Comment on attachment 215307 [details]
Patch for landing

Clearing flags on attachment: 215307

Committed r158110: <http://trac.webkit.org/changeset/158110>
Comment 18 WebKit Commit Bot 2013-10-28 08:20:48 PDT
All reviewed patches have been landed.  Closing bug.