1263398 - Implement nsIFileWatcherService for GTK3

Status: ASSIGNED

(Core :: DOM: Core & HTML, defect)

Description

[Andrea Marchesini [:baku]]

Attachment: gtk.patch

Comment 1

[Andrea Marchesini [:baku]]

Andrew, do you know who can review this patch?

Comment 2

[Andrew Overholt [:overholt]]

Karl Tomlinson can probably do it.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8739697 [details] [diff] [review]
gtk.patch

Andrew, I think baku was looking for someone to review the GFileMonitor usage.
If you are happy reviewing the rest, then that would be great too, but someone more familiar with nsINativeFileWatcherService and the threads involved could do that.

Comment 4

[Andrew Comminos [:acomminos]]

Comment on attachment 8739697 [details] [diff] [review]
gtk.patch

Review of attachment 8739697 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! I'm not too familiar with the file watcher code, so I focused on the GLib usage.

I'm a bit concerned about the usage of G_FILE_MONITOR_WATCH_MOVES- if you could clarify whether we need this or not, that would be great.

::: toolkit/components/build/nsToolkitCompsModule.cpp
@@ +41,5 @@
>  #include "mozilla/AddonPathService.h"
>  
>  #if defined(XP_WIN)
>  #include "NativeFileWatcherWin.h"
> +#elif MOZ_WIDGET_GTK == 3

I'm assuming you're checking for this instead of MOZ_ENABLE_GIO since you use G_FILE_MONITOR_WATCH_MOVES, which is available since gio 2.44. I don't believe GTK3 requires GLib 2.44 however, which is quite new (we probably don't even have it on the build slaves).

::: toolkit/components/filewatcher/NativeFileWatcherGtk.cpp
@@ +352,5 @@
> +      return;
> +    }
> +
> +    GFileMonitor* monitor = g_file_monitor(file,
> +                                           G_FILE_MONITOR_WATCH_MOVES,

As stated previously, G_FILE_MONITOR_WATCH_MOVES is only available on gio 2.44+.

Also, is this even needed? The additional events sent to the "changed" signal with G_FILE_MONITOR_WATCH_MOVES set (G_FILE_MONITOR_EVENT_RENAMED, G_FILE_MONITOR_EVENT_MOVED_IN, G_FILE_MONITOR_EVENT_MOVED_OUT) don't seem to be used. Legacy events (CHANGED, DELETED) will still get sent for moves without the flag.

Unless I'm missing something, I would suggest just scrapping the flag.

@@ +353,5 @@
> +    }
> +
> +    GFileMonitor* monitor = g_file_monitor(file,
> +                                           G_FILE_MONITOR_WATCH_MOVES,
> +                                           nullptr, nullptr);

Might be helpful to log the GError returned here in the 4th param.