Implement MessageLoopForUI using GLib. This gets some exercise from...

Implement MessageLoopForUI using GLib.  This gets some exercise from
base_unittest.

BUG=1319
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=2834

[dsh, 2008-09-24 22:08:34]

Sorry this took a while.  Now I know way more about GLib than I ever thought
possible (OK, no, it still confuses the hell out of me).

I haven't built/tested on Mac or Win, but I will do that now.

Thanks,
Dan

[Evan Martin, 2008-09-24 22:23:35]

This looks pretty good to me.

I defer to Darin for the gnarly expected MessagePump behaviors.

http://codereview.chromium.org/4261/diff/1/6
File base/message_pump_glib.cc (right):

http://codereview.chromium.org/4261/diff/1/6#newcode25
Line 25: RunState s;
Would you mind using a full name like "state"?

http://codereview.chromium.org/4261/diff/1/6#newcode67
Line 67: // A brief refresher on GLib:
This is an awesome comment.

http://codereview.chromium.org/4261/diff/1/6#newcode192
Line 192: DCHECK(*idle_source_attached_address) << "Finalizing a rouge idle
source!";
Typo: "rogue"

http://codereview.chromium.org/4261/diff/1/6#newcode205
Line 205: // to resort and internal list.
s/and/an/

http://codereview.chromium.org/4261/diff/1/6#newcode207
Line 207: g_source_set_priority (source, priority);
no space before paren here

http://codereview.chromium.org/4261/diff/1/6#newcode212
Line 212: if (state_)
curlies around multiline "if" bodies, so open curly here

http://codereview.chromium.org/4261/diff/1/6#newcode229
Line 229: // 6?  It should be 6 to avoid executing delayed work too early.
I recall Darin having some complicated comments on this subject, so I'll defer
to him.

http://codereview.chromium.org/4261/diff/1/3
File base/message_pump_glib.h (right):

http://codereview.chromium.org/4261/diff/1/3#newcode41
Line 41: // recursive calls to Run, because it is never removed, the idle source
I'm sorta confused by this comment.  Does "it" here refer to the work source? 
Maybe just removing the first comma would have helped.

[darin (slow to review), 2008-09-24 22:54:37]

looks pretty good overall.  here's some comments:

http://codereview.chromium.org/4261/diff/1/5
File base/message_loop.cc (right):

http://codereview.chromium.org/4261/diff/1/5#newcode101
Line 101: pump_ = new base::MessagePumpGLib();
as discussed, let's change this to base::MessagePumpForUI.

http://codereview.chromium.org/4261/diff/1/6
File base/message_pump_glib.cc (right):

http://codereview.chromium.org/4261/diff/1/6#newcode28
Line 28: s.should_do_work = true;
nit: please add a comment explaining why should_do_work is initialized to true.

http://codereview.chromium.org/4261/diff/1/6#newcode47
Line 47: if (state_) {
ScheduleWork can be called from any thread, and this doesn't appear to be very
thread safe.

http://codereview.chromium.org/4261/diff/1/6#newcode67
Line 67: // A brief refresher on GLib:
agreed, but how about some paragraph breaks? :-)

http://codereview.chromium.org/4261/diff/1/6#newcode74
Line 74: // system.  File descriptors can be attached the sources.  The poll may
block
"File descriptors can be attached [to] the sources."

http://codereview.chromium.org/4261/diff/1/6#newcode87
Line 87: gint* timeout_ms) {
nit: looks like there is room to put timeout_ms on the previous line.

http://codereview.chromium.org/4261/diff/1/6#newcode95
Line 95: } else {
nit / pet-peeve: kill the "else" after a return statement so that you can reduce
indentation of the following block.

http://codereview.chromium.org/4261/diff/1/6#newcode229
Line 229: // 6?  It should be 6 to avoid executing delayed work too early.
this looks just like the windows code that also needs to convert a TimeDelta to
an integer milliseconds value.  rounding up as this code does is good so that we
prefer waiting a little too much instead of not waiting at all or not waiting
enough.

http://codereview.chromium.org/4261/diff/1/3
File base/message_pump_glib.h (right):

http://codereview.chromium.org/4261/diff/1/3#newcode48
Line 48: GSource source;
nit: perhaps you could just make WorkSource extend from GSource?  that gives the
same result, except that you can then avoid the ugly reinterpret_cast used to
map between the two.  you can use a safe static_cast, which will be more future
proof.

[Mark Mentovai, 2008-09-25 17:43:04]

LGTM, with questions and comments.

http://codereview.chromium.org/4261/diff/1/5
File base/message_loop.cc (right):

http://codereview.chromium.org/4261/diff/1/5#newcode14
Line 14: #include "base/message_pump_glib.h"
Move down to live with other platform-specific #ifdefs?

http://codereview.chromium.org/4261/diff/1/5#newcode101
Line 101: pump_ = new base::MessagePumpGLib();
Eh, this should probably be next to the Mac TYPE_UI, but since it's probably all
going to change anyway...

http://codereview.chromium.org/4261/diff/1/5#newcode102
Line 102: #endif // OS_LINUX
Style: two spaces between #endif and //.

http://codereview.chromium.org/4261/diff/1/6
File base/message_pump_glib.cc (right):

http://codereview.chromium.org/4261/diff/1/6#newcode7
Line 7: #include "base/message_pump_glib.h"
Style: should be the first #include, even before <math.h>.  Including
message_pump_glib.h first in at least one .cc file ensures that the header
includes everything it needs.

http://codereview.chromium.org/4261/diff/1/6#newcode89
Line 89: RunState* state = self->state_;
It looks like this has potential to blow up.  If you instantiate a
MessagePumpGLib and then someone else runs a loop with g_main_context_iteration
or g_main_loop_run or whatever, the work sources might be called while state_ is
NULL.

(I have a similar bug in the CFRunLoop Mac code.)

This isn't the only place that this could happen: it could also happen in Quit
if someone calls it from outside Run, although you do have a DCHECK there.

http://codereview.chromium.org/4261/diff/1/6#newcode96
Line 96: *timeout_ms = GetTimeIntervalMilliseconds(self->delayed_work_time_);
What happens if GetTimeIntervalMilliseconds returns -1?

http://codereview.chromium.org/4261/diff/1/6#newcode213
Line 213: reinterpret_cast<WorkSource*>(source)->idle_source_attached_address =
You reinterpret_cast<WorkSource*>(source) twice in this function, it would be
cleaner to just do the cast once and save the result in a local WorkSource*.

http://codereview.chromium.org/4261/diff/1/6#newcode218
Line 218: // be deleted when Dispatch returns FALSE or the loop is destroyed.
If someone calls Run and then at some point WorkSourceDispatch returns FALSE,
will the work source be deleted right away?  What happens if we break out of the
outermost call to Run but then call it again later?  (It seems like an unlikely
scenario, but the Mac version and I think the Windows version of the UI pump at
least can tolerate this restart behavior.)  Maybe I just don't have a complete
understanding of glib's memory management...

http://codereview.chromium.org/4261/diff/1/3
File base/message_pump_glib.h (right):

http://codereview.chromium.org/4261/diff/1/3#newcode57
Line 57: static GSourceFuncs WorkSourceFuncs;
This can be const, right?  IdleSourceFuncs too?

Actually, you can take them out of the class entirely, these GSourceFuncs
variables can just be static const variables over in the .cc file, there's no
reason for them to be part of the private interface.

http://codereview.chromium.org/4261/diff/1/3#newcode77
Line 77: void AddIdleSource(void);
You only have one site where you add WorkSourceFuncs and one site where you add
IdleSourceFuncs.  How come one gets a dedicated Add*Source, and the other one is
done by calling AddSource directly?

http://codereview.chromium.org/4261/diff/1/3#newcode79
Line 79: static int GetTimeIntervalMilliseconds(Time from);
This, too, doesn't need to be part of the private interface.

[dsh, 2008-09-25 22:30:35]

Pretty much anything I didn't comment on here I fixed according to how people
wanted them.  I'll send the patch right after this.

http://codereview.chromium.org/4261/diff/1/6
File base/message_pump_glib.cc (right):

http://codereview.chromium.org/4261/diff/1/6#newcode47
Line 47: if (state_) {
On 2008/09/24 22:54:37, Darin Fisher (Google) wrote:
> ScheduleWork can be called from any thread, and this doesn't appear to be very
> thread safe.

Yes, it's not.  Thanks for catching this.  There is a lot of info to absorb
here.  The approach I'm taking to fix this seems a tad hacky, but actually I
think it probably works fairly well and at least avoids locks and race
conditions.  (I'm sending the revised patch right after this).

http://codereview.chromium.org/4261/diff/1/6#newcode89
Line 89: RunState* state = self->state_;
On 2008/09/25 17:43:04, Mark Mentovai wrote:
> It looks like this has potential to blow up.  If you instantiate a
> MessagePumpGLib and then someone else runs a loop with
g_main_context_iteration
> or g_main_loop_run or whatever, the work sources might be called while state_
is
> NULL.
> 
> (I have a similar bug in the CFRunLoop Mac code.)
> 
> This isn't the only place that this could happen: it could also happen in Quit
> if someone calls it from outside Run, although you do have a DCHECK there.

This is also a good point that I didn't think of, but since the context is
private, I don't think the sources can get fired outside of Run.

http://codereview.chromium.org/4261/diff/1/6#newcode96
Line 96: *timeout_ms = GetTimeIntervalMilliseconds(self->delayed_work_time_);
On 2008/09/25 17:43:04, Mark Mentovai wrote:
> What happens if GetTimeIntervalMilliseconds returns -1?

Then poll won't timeout (at least not by our timeout argument).  This is working
as intended.

http://codereview.chromium.org/4261/diff/1/6#newcode218
Line 218: // be deleted when Dispatch returns FALSE or the loop is destroyed.
On 2008/09/25 17:43:04, Mark Mentovai wrote:
> If someone calls Run and then at some point WorkSourceDispatch returns FALSE,
> will the work source be deleted right away?  What happens if we break out of
the
> outermost call to Run but then call it again later?  (It seems like an
unlikely
> scenario, but the Mac version and I think the Windows version of the UI pump
at
> least can tolerate this restart behavior.)  Maybe I just don't have a complete
> understanding of glib's memory management...

Aiyeee!  This is a bug.  WorkSourceDispatch should never return FALSE (simple
fix).  It should never be removed in that way, only when the loop is destroyed. 
Thanks!

http://codereview.chromium.org/4261/diff/1/6#newcode229
Line 229: // 6?  It should be 6 to avoid executing delayed work too early.
On 2008/09/24 22:54:37, Darin Fisher (Google) wrote:
> this looks just like the windows code that also needs to convert a TimeDelta
to
> an integer milliseconds value.  rounding up as this code does is good so that
we
> prefer waiting a little too much instead of not waiting at all or not waiting
> enough.
> 
> 

Well I basically just copy-pasted it, although the arguments are different.

http://codereview.chromium.org/4261/diff/1/3
File base/message_pump_glib.h (right):

http://codereview.chromium.org/4261/diff/1/3#newcode48
Line 48: GSource source;
On 2008/09/24 22:54:37, Darin Fisher (Google) wrote:
> nit: perhaps you could just make WorkSource extend from GSource?  that gives
the
> same result, except that you can then avoid the ugly reinterpret_cast used to
> map between the two.  you can use a safe static_cast, which will be more
future
> proof.

Good point!

http://codereview.chromium.org/4261/diff/1/3#newcode57
Line 57: static GSourceFuncs WorkSourceFuncs;
On 2008/09/25 17:43:04, Mark Mentovai wrote:
> This can be const, right?  IdleSourceFuncs too?
> 
> Actually, you can take them out of the class entirely, these GSourceFuncs
> variables can just be static const variables over in the .cc file, there's no
> reason for them to be part of the private interface.

Unfortunately, no.  I can't make them const because g_source_new expects a
non-const GSourceFuncs* (booo!).  The static functions are private, so I need
the GSourceFuncs members to be part of the class, or I need to make the
callbacks public.  I would rather have everything be private.

[dsh, 2008-09-25 22:39:45]

The only real thing I changed here was to address thread-safety in ScheduleWork.
 I use a pipe that we can write to to wake up poll (if it's asleep) and check to
see if we've got work to do.  I'm reasonably happy with this, but it still feels
a bit hacky.  Maybe I can commit this chunk and then figure something better out
later (unless it's so vile people can't stand it in the tree).

Everything else people mentioned were straight-forward, and have been done
unless I missed something.

Thanks!

[Mark Mentovai, 2008-09-25 23:38:35]

http://codereview.chromium.org/4261/diff/10/215
File base/message_pump_glib.cc (right):

http://codereview.chromium.org/4261/diff/10/215#newcode14
Line 14: static const char kWorkScheduled = 0;
I'd use char constants ('\0', '\1', or really anything you like) here.  My first
comment was "cute, but you should use an int," but then I saw how you're using
these below.

http://codereview.chromium.org/4261/diff/10/215#newcode62
Line 62: write_fd_work_scheduled_ = pipe_fd[1];
I might have done this as a union, but this is fine.

http://codereview.chromium.org/4261/diff/10/215#newcode110
Line 110: CHECK(write(write_fd_work_scheduled_, &kWorkScheduled, 1)) <<
You should check that the return value is 1.  write can return -1 on failure,
but the check will succeed.  Same on the other write.

http://codereview.chromium.org/4261/diff/10/215#newcode116
Line 116: // This is an optimization.  Delayed work may not be immanent, we may
just
Spelling nit: imminent.

http://codereview.chromium.org/4261/diff/10/215#newcode183
Line 183: while (0 < read(self->read_fd_work_scheduled_, &byte, 1))
This is itching for curlies, since it encloses some multi-line stuff.

http://codereview.chromium.org/4261/diff/10/215#newcode187
Line 187: state->should_do_work = true;
Cute!

[dsh, 2008-09-30 23:16:42]

So I'm just using the default context here.  Everything else is the same except
to address the fact we need to manually remove sources now.  There's a DCHECK
that asserts we can only run on one thread.  If we need a UI thread that doesn't
touch GTK stuff, we could add it using a second context without much work.

[Mark Mentovai, 2008-10-01 00:55:33]

LGTM