[Webkit-unassigned] [Bug 118067] [GTK] Use the GCActivityCallback

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=118067





--- Comment #7 from Sergio Villar Senin <svillar at igalia.com>  2013-08-08 09:01:20 PST ---
(From update of attachment 207176)
View in context: https://bugs.webkit.org/attachment.cgi?id=207176&action=review

Apart from the inline comments, remember that you have to add a proper ChangeLog with the description of the changes (maybe this helps http://trac.webkit.org/wiki/CodeReview)

> runtime/GCActivityCallback.h:72
> +    }

You don't need this if you aren't doing anything different from the #else following this code.

> runtime/GCActivityCallback.h:117
> +#endif

At least cancelTimer(), scheduleTimer() and m_delay are shared with the other ports. There should be a better way to write this.

> runtime/GCActivityCallback.cpp:44
> +#include <gdk/gdk.h>

Why gdk?

> runtime/GCActivityCallback.cpp:87
> +}

Isn't this exactly the same as Qt? Couldn't we do "#elif PLATFORM(QT) || PLATFORM(GTK)" ?

> runtime/GCActivityCallback.cpp:168
> +    

Check the indentation and remove the extra lines at the end of the function.

> runtime/GCActivityCallback.cpp:174
> +	stop();

Check indentation. Also the implementation is exactly the same as EFL, we might think about sharing it.

> runtime/GCActivityCallback.cpp:179
> +

Extra line.

> runtime/GCActivityCallback.cpp:181
> +		return;

Indentation.

> runtime/GCActivityCallback.cpp:202
> +		   return;

Indentation. Also any reason why we could not share EFL's code?. Actually, I don't know exactly about the details but looks like the EFL code might be shared among ports (not sure about the ASSERT though)

> heap/HeapTimer.cpp:44
> +#include <gdk/gdk.h>

Again why gdk?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.