Code review comment for lp:~ted/url-dispatcher/url-overlay

On Thu, 2015-06-11 at 17:26 +0000, Charles Kerr wrote:

> Some questions/comments inline, no blockers though.

Great, thanks for reviewing, long MR!

> > + -lpthread
>
> -lpthread reportedly doesn't work on clang, maybe use -pthread instead here?

Oh, didn't realize that. Fixed r135.

> > +ContextThread::~ContextThread (void)
>
> not a blocker, but the 'void' isn't needed in C++ for functions taking no args

Not *needed* but enjoyed. Removed r136.

> > + void removeSession (MirPromptSession * session);
> > +
> > + static void sessionStateChangedStatic (MirPromptSession * session, MirPromptSessionState state, void * user_data);
> > + void sessionStateChanged (MirPromptSession * session, MirPromptSessionState state);
> > +
> > + static void untrustedHelperStoppedStatic (const gchar * appid, const gchar * instanceid, const gchar * helpertype, gpointer user_data);
> > + void untrustedHelperStopped(const gchar * appid, const gchar * instanceid, const gchar * helpertype);
>
> These five should be private

Fixed r137.

> > + OverlayTrackerMir * cpptracker = new OverlayTrackerMir();
> > + return reinterpret_cast<OverlayTracker *>(cpptracker);
>
> I don't think reinterpret_cast<> is needed here; you should be able to upcast implicitly; e.g.
>
> try {
> return new OverlayTrackerMir();
> }
> catch(...) {
> return nullptr;
> }

We're not actually upcasting here, we're casting to an anonymous C
pointer, so that's why we need it. Trying to merge the C and C++ worlds.

> > + } catch (...) {
> > + return nullptr;
> > + }
> > +}
> > +
> > +void
> > +overlay_tracker_delete (OverlayTracker * tracker) {
> > + g_return_if_fail(tracker != nullptr);
> > +
> > + auto cpptracker = reinterpret_cast<OverlayTrackerMir *>(tracker);
> > + delete cpptracker;
>
> no need to reinterpret_cast here either; "delete tracker" should suffice

Same, OverlayTracker isn't a C++ object so it can't be deleted.

> > + /* Allow disabling for testing, we don't want to report bugs on
> > + our tests ;-) */
> > + if (G_UNLIKELY(g_getenv("URL_DISPATCHER_DISABLE_RECOVERABLE_ERROR") != NULL)) {
> > + return;
> > + }
> > +
>
> :-)

As an aside I built this into the libwhoopsie version as well. Make sure
to use it :-)

> > GError * error = NULL;
> > gint error_stdin = 0;
> > GPid pid = 0;
> >
> > === modified file 'service/service.c'
> > --- service/service.c 2014-03-14 19:50:37 +0000
> > +++ service/service.c 2015-06-08 20:42:15 +0000
> > @@ -39,13 +39,15 @@
> >
> > guint term_source = g_unix_signal_add(SIGTERM, sig_term, mainloop);
> >
> > - dispatcher_init(mainloop);
> > + OverlayTracker * tracker = overlay_tracker_new();
> > + dispatcher_init(mainloop, tracker);
>
> Looks like dispatcher.c is the only user of this tracker; why not make it a private variable there, instantiated by dispatcher_init() and released by dispatcher_shutdown()?

Mostly to make the injection of the mock easier in the dispatcher tests.
It doesn't look as nice in C as it does in C++ when you do that. Would
be nice to change main.c to main.cpp, but this MR is big enough ☺

Also fixed a couple warning I hadn't noticed before, r138.