1418749 - Add support for window progress in Linux Cinnamon desktop using libxapp

Status: VERIFIED FIXED

(Toolkit :: Downloads API, enhancement, P5)

Description

[Óvári]

Attachment: Firefox Download Progress Windows.PNG

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171116103757

Steps to reproduce:

Download Mozilla Thunderbird with Firefox 57.0 with Windows 8.1 and Linux Mint 18.3 Cinnamon.


Actual results:

Windows 8.1 Panel shows the download progress in the panel (see attached image)
Linux Mint 18.3 did not show the download progress in the panel


Expected results:

Linux Mint 18.3 should show the download progress in the panel

It would be appreciated if the feature available for the Windows versions of Firefox be available in the Linux versions of Firefox.

Thank you

Comment 1

[Óvári]

This bug report has been written as requested by Heedermann at:
https://blog.linuxmint.com/?p=3445#comment-138189

in response to listing at:
https://blog.linuxmint.com/?p=3445#comment-138169

Thank you

Comment 2

[Gingerbread Man]

The Downloads panel is the popup that opens when you click the Downloads button on the navigation toolbar. It seems you're actually referring to the download progress shown on the taskbar. That's a Windows feature introduced in Windows 7. This OS feature will almost certainly not be duplicated in the browser.

Comment 3

[Óvári]

(In reply to Gingerbread Man from comment #2)
> The Downloads panel is the popup that opens when you click the Downloads
> button on the navigation toolbar. It seems you're actually referring to the
> download progress shown on the taskbar. That's a Windows feature introduced
> in Windows 7. This OS feature will almost certainly not be duplicated in the
> browser.

You are correct Gingerbread Man in that this enhancement bug refers to the Windows Taskbar and Linux Mint Panel showing the progress of downloads.

Are you able to advise the API (please accept our apologies if API is the incorrect terminology) that can be read by Linux Mint to gives a value for the progress of the downloads?

Thank you

Comment 4

[Óvári]

Heedermann on November 19, 2017 at 9:05 pm at https://blog.linuxmint.com/?p=3445#comment-138228 wrote:

The application has to add support for showing progress in Cinnamon, not the other way around. To add this feature, Firefox needs to use the xapps library.
Example of Timeshift adding this feature:

https://github.com/teejee2008/timeshift/commit/cdbbbba76a1b5918169052d56ea2e5c73250e661

---

Would Mozilla Firefox please implement support for the xapps library as shown in the example above?

Thank you

Comment 5

[Gingerbread Man]

(In reply to Óvári from comment #4)
> The application has to add support for showing progress in Cinnamon, not the
> other way around. To add this feature, Firefox needs to use the xapps
> library.

If the OS has the feature, then that's correct: Firefox needs to be updated to support it.

Comment 6

[Óvári]

(In reply to Gingerbread Man from comment #5)
> If the OS has the feature, then that's correct: Firefox needs to be updated
> to support it.

Are you able to change the status of this bug from UNCONFIRMED to NEW? Thank you

Comment 7

[Gingerbread Man]

I don't see any duplicates, so NEW it is then.

Comment 8

[Óvári]

(In reply to Gingerbread Man from comment #7)
> I don't see any duplicates, so NEW it is then.

Thank you Gingerbread Man for setting this bug to NEW. Do you know when this ticket could be implemented? It would be good if it could be implemented with by the release of Linux Mint 18.3 Cinnamon, or soon thereafter. Thank you

Comment 9

[:Paolo Amadini]

(In reply to Óvári from comment #8)
> Do you know when this ticket could be implemented?

This is probably a question for Linux Mint developers, because in order to make progress we'll need a good quality patch from someone experienced with Linux who would like to volunteer their time. Considering that code review will still require some time, as this issue is not a high priority, I don't think we can define a specific timeline.

Comment 10

[Michael Webster]

Hi, I can implement this - eventually.  There's no way anything might be done for Mint 18.3 (it releases in 2 days,) but hopefully something can be accomplished within our development window for 19 (along with getting more adoption from other desktop environments.)  I wrote the progress implementation in libxapp as well as the corresponding utilization of it in Cinnamon and muffin (our window manager.)

I've cloned and started looking at the source code here, though as I'm completely unfamiliar with it, I'd appreciate any pointers, no matter how broad.  I've generally been poking about widget/gtk nsWindow as where most of the changes will eventually end up, but it'll take time to understand the correct way to get there.  I briefly had a look at the corresponding platform-specific stuff for Windows, but I'm not sure how helpful that would be.  I plan to take a look at the other platforms as well.  I'll look around for documentation as well.  This honestly isn't a huge priority for myself, so I can't say what any timeline would be, but I definitely would like to have something to submit in the couple of months.

Thanks,

Michael Webster

Comment 11

[Michael Webster]

Attachment: TaskbarProgress implementation for gtk3/x11

I've got a preliminary working patch here.  I'm in the process of figuring out how to submit to the MozReview channel, but in the meantime, I'll attach it here, hopefully someone might be able to have a glance at it to be sure I'm in the ballpark.  I'm still a long way from fully understanding everything going on under the hood, but I more or less tried to ape what the Windows and Mac platforms were doing.

The actual XApps implementation is only about 50 lines of code in (widget/gtk/)nsWindow.cpp - this saved having to add a new dependency (worse, figuring out *how* to add a new dependency in the build) - the rest of it is mostly just implementation details.

I'd appreciate any comments.  I need to go thru and make sure my formatting and style is consistent, but otherwise the actual implementation is more or less complete.  One thing I'm a bit unsure on, is passing our window from JS to the cpp side.  The way I've done it works, but it feels a bit like I'm chasing my tail.

I've attempted to make it so that other implementations (for Wayland perhaps) can be added later without too much fuss.  Currently the ProgressHandler class targets gtk3, while the actual setting window property code targets X11 only.

Thanks

Comment 12

[Óvári]

(In reply to Michael Webster from comment #11)
Thank you Michael Webster!

This patch relates to the *toolkit* product. Does this require a super-reviewer? Members of the *toolkit* product super-reviewer group are:
Benjamin Smedberg
Dave Townsend
Gavin Sharp
Mike Connor
Robert Strong
Vladimir Vukicevic
https://www.mozilla.org/en-US/about/governance/policies/reviewers/

Thank you

Comment 13

[Dave Townsend [:mossop]]

Toolkit does not require super-review, just regular review: https://wiki.mozilla.org/Toolkit/Code_Review

Comment 14

[Gingerbread Man]

Marco Bonardo is on vacation until January 2nd, so I flagged the patch with review? from Neil Deakin.
https://wiki.mozilla.org/Toolkit/Submodules#Download_Manager

Comment 15

[Neil Deakin]

Comment on attachment 8937951 [details] [diff] [review]
TaskbarProgress implementation for gtk3/x11

This is mostly a gtk patch with a bit of downloads, so changing reviewer.

Comment 16

[Michael Webster]

Thanks for the quick activity (this makes me feel slightly guilty about the other projects I'm involved in!) - I'll hopefully upload a more final patch within a day, there are a couple things I need to address:

- formatting consistency with surrounding code, and a more verbose commit message.
- reducing the call frequency of the SetProgress window method - I'll track last progress value in the ProgressHandler, and only forward to the window when the value has changed (currently it updates on the same timer frequency as the download UI elements, which isn't necessary for the gtk implementation, and the less X server traffic the better.
- I also want to make sure I'm not introducing any object refcount issues between the progress handler and the window it keeps track of.  There doesn't appear to be, but I'm not familiar enough with c++ and XPCOM to be sure of it yet.

Since the review process is more or less already underway here, can I continue to just upload the patch directly here, or should I follow the recommended method from this point forward?  I don't want to cause inconvenience, but I'm sure we don't want duplicated efforts and notes in multiple locations.

btw, this all can be tested using 'xprop -spy' on the Firefox window and observing the property updates (the WM implementation isn't needed to see if/how this works.)

Thanks again!

Comment 17

[Michael Webster]

Attachment: 395821.patch

Updated patch - should (hopefully) be good to go.

Comment 18

[:Paolo Amadini]

Comment on attachment 8938593 [details] [diff] [review]
395821.patch

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

r=me for the DownloadsTaskbar.jsm changes.

::: browser/components/downloads/DownloadsTaskbar.jsm
@@ +160,5 @@
>  
> +  /**
> +   * In gtk3, the window itself implements the progress interface.
> +   */
> +  _setupGtkProgressHandler(aWindow) {

nit: _attachGtkProgressHandler for consistency.

::: widget/nsIGtkProgressHandler.idl
@@ +11,5 @@
> + * Allow the ProgressHandler instance to set a new target window.
> + */
> +
> +[scriptable, uuid(39f6fc5a-2386-4bc6-941c-d7479253bc3f)]
> +interface nsIGtkProgressHandler : nsISupports

Looks like this interface should be called nsIGtkTaskbarProgress and should extend nsITaskbarProgress.

Comment 19

[Michael Webster]

Attachment: progress-12282017.patch

Updated, thanks

Comment 20

[:Paolo Amadini]

Comment on attachment 8939048 [details] [diff] [review]
progress-12282017.patch

Remember to set the review flags again after a patch update, this is necessary when not using the workflow based on MozReview.

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 8939048 [details] [diff] [review]
progress-12282017.patch

Thank you for finding your way through Gecko's burrows to implement this.
The approach is good and I agree that changing the properties directly is
better than dealing with dlopen.  I have a number of requests for small
changes, though, and so I think another review is appropriate.

>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=4 et sw=4 tw=80: */

Please follow
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line
for new files.

>+ProgressHandler::~ProgressHandler()
>+{
>+  MOZ_LOG(gGtkProgressHandlerLog, LogLevel::Info,
>+          ("%p ~ProgressHandler()", this));
>+
>+  mPrimaryWindow = nullptr;
>+}

No need to clear mPrimaryWindow here, as it won't be used when the
ProgressHandler is destroyed, and the reference will be released
by the member's destructor.

>+  NS_ENSURE_TRUE((aCurrentValue <= aMaxValue), NS_ERROR_ILLEGAL_VALUE);

Please remove the unnecessary parentheses.

>+    progress = (gulong) (((double)aCurrentValue / aMaxValue) * 100.0);

Perhaps add a comment to indicate that rounding down is intentional so as not
to indicate 100% progress prematurely.

>+  return NS_OK;  // Is this the correct return value if not implemented?

NS_ERROR_NOT_IMPLEMENTED is possibly more useful, but the callers don't check
and so this is not important/necessary.

>+ProgressHandler::SetPrimaryWindow(mozIDOMWindowProxy *aWindow)

Although the files in this directory are not consistent about the placement of
* between the type and the name, please place beside the type in new code.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Declarations

>+  NS_ENSURE_TRUE((aWindow != nullptr), NS_ERROR_ILLEGAL_VALUE);

Please remove the unnecessary parentheses.

>+  GtkWidget *gtkwidget = GTK_WIDGET (widget->GetNativeData(NS_NATIVE_SHELLWIDGET));
>+  gpointer user_data = g_object_get_data(G_OBJECT(gtkwidget), "nsWindow");
>+
>+  mPrimaryWindow = static_cast<nsWindow *>(user_data);

mPrimaryWindow = static_cast<nsWindow*>widget;
should be sufficient here.
I'd prefer to avoid spreading
g_object_get_data(G_OBJECT(gtkwidget), "nsWindow") further than necessary.

>+if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gtk3':
>+    EXPORTS += [
>+        'ProgressHandler.h',
>+    ]

No need to export headers that are used only in the same directory.

>+if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gtk3':
>+    SOURCES += [
>+        'ProgressHandler.cpp'
>+    ]
>+

Can this be added to UNIFIED_SOURCES please?  Having files there makes compilation faster because there are fewer compiler process instantiations, and headers don't need to be parsed as often.
If not, please add a comment to explain why this can't be in UNIFIED_SOURCES.

Use one of the existing UNIFIED_SOURCES lists, please.

The GTK2 code has been removed now (bug 1278282), and so I'd like to avoid new
'gtk3' conditionals.  Would you be able to also remove the
#if (MOZ_WIDGET_GTK == 3) lines, please?

I think it could be helpful to rename ProgressHandler to TaskbarProgressHandler
or just TaskbarProgress, or something, so as to distinguish from progress HTML
elements.

>+ * See https://github.com/linuxmint/xapps/blob/master/libxapp/xapp-gtk-window.c
>+ * for further details.
>+ */
>+
>+#define PROGRESS_HINT  "_NET_WM_XAPP_PROGRESS"
>+
>+static void
>+set_window_hint_cardinal (Window       xid,
>+                          const gchar *atom_name,
>+                          gulong       cardinal)
>+{
>+  GdkDisplay *display;
>+
>+  display = gdk_display_get_default ();
>+
>+  gdk_error_trap_push ();
>+
>+  if (cardinal > 0)
>+  {
>+    XChangeProperty (GDK_DISPLAY_XDISPLAY (display),
>+                     xid,
>+                     gdk_x11_get_xatom_by_name_for_display (display, atom_name),
>+                     XA_CARDINAL, 32,
>+                     PropModeReplace,
>+                     (guchar *) &cardinal, 1);
>+  }
>+  else
>+  {
>+    XDeleteProperty (GDK_DISPLAY_XDISPLAY (display),
>+                     xid,
>+                     gdk_x11_get_xatom_by_name_for_display (display, atom_name));
>+  }
>+
>+  gdk_error_trap_pop_ignored ();
>+}

The !mGdkWindow test in SetProgress makes the gdk_error_trap_* unnecessary.
mGdkWindow is reset when the window is destroyed.
Suppressing errors is important when operating on windows outside the control
of the thread using the xid, but not necessary here where the window is owned
by this thread.

Removing these error-handling lines will also remove any need to get
permission to use code from Clement Lefebvre.  I gather you are the author of
the rest of the code.

I'm OK with the variation from Mozilla style here, given this a copy of the
reference.

>+  if (!mGdkWindow) {
>+    return;
>+  }

>+  set_window_hint_cardinal (GDK_WINDOW_XID(gdk_window_get_toplevel(mGdkWindow)),
>+                            PROGRESS_HINT,
>+                            progress);

There is a Wayland port in progress, and so we need a run-time check to detect
whether the app is under X11 or Wayland.  Testing mIsX11Display is simplest.

I expect gtk_widget_get_window(mShell) would work here and would be more
direct, but gdk_window_get_toplevel(mGdkWindow) works, if you'd like to keep
that.  mShell will be null if it does not have a window.

Gecko style is no space in "set_window_hint_cardinal(".

>+    void               SetProgress(unsigned long progress);
>+

Can you document that the parameter is a percentage, please?
Even changing the name to progressPercent, or percent would work.

> ]
>-
> XPIDL_MODULE = 'widget'

Please no unrelated whitespace changes.

>@@ -84,6 +84,12 @@ if 'gtk' in CONFIG['MOZ_WIDGET_TOOLKIT']
>     if CONFIG['MOZ_X11']:
>         DIRS += ['gtkxtbin']
> 
>+    if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gtk3':
>+        XPIDL_SOURCES += [
>+            'nsIGtkTaskbarProgress.idl',
>+            'nsITaskbarProgress.idl',
>+        ]

No need for the gtk3 test here.

Comment 22

[Michael Webster]

Hi, quick update, I apologize for the delay - Hoping to have an updated patch within a day or two.

Thanks

Comment 23

[Michael Webster]

Attachment: 0001-widget-gtk-Add-a-TaskbarProgress-implementation-for-.patch

Hi, I've rebased off the current tree and made (I believe) all requested changes, except one - 

+NS_IMETHODIMP
+TaskbarProgress::SetPrimaryWindow(mozIDOMWindowProxy* aWindow)
+{
+  NS_ENSURE_TRUE(aWindow != nullptr, NS_ERROR_ILLEGAL_VALUE);
+
+  auto* parent = nsPIDOMWindowOuter::From(aWindow);
+  RefPtr<nsIWidget> widget = mozilla::widget::WidgetUtils::DOMWindowToWidget(parent);
+
+  GtkWidget *gtkwidget = GTK_WIDGET(widget->GetNativeData(NS_NATIVE_SHELLWIDGET));
+  gpointer user_data = g_object_get_data(G_OBJECT(gtkwidget), "nsWindow");
+
+  mPrimaryWindow = static_cast<nsWindow*>(user_data);

I've hit a bit of a wall here trying to avoid the g_object_get_data route - trying to cast widget to nsWindow gives me an invalid static cast:

/home/mtwebster/bin/gecko-dev/widget/gtk/TaskbarProgress.cpp:91:50: error: invalid static_cast from type ‘RefPtr<nsBaseWidget>’ to type ‘nsWindow*’
 0:12.12    mPrimaryWindow = static_cast<nsWindow *>(widget);
 0:12.12                                                   ^

I feel I'm missing something simple here, but I don't find any similar examples elsewhere in the source to iron it out, and I'm still pretty unfamiliar with a great deal.  I'll keep working at it, but I'd welcome a pointer (no pun intended) or two.

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 8952951 [details] [diff] [review]
0001-widget-gtk-Add-a-TaskbarProgress-implementation-for-.patch

>+  mPrimaryWindow->SetProgress(progress);
>+#endif
>+
>+  return NS_ERROR_NOT_IMPLEMENTED;  // Is this the correct return value if not implemented?

Yes, but but this is also the success path.
It's probably easier to just return NS_OK, given the callers don't check.

(In reply to Michael Webster from comment #23)
> /home/mtwebster/bin/gecko-dev/widget/gtk/TaskbarProgress.cpp:91:50: error:
> invalid static_cast from type ‘RefPtr<nsBaseWidget>’ to type ‘nsWindow*’
>  0:12.12    mPrimaryWindow = static_cast<nsWindow *>(widget);
>  0:12.12                                                   ^
> 
> I feel I'm missing something simple here, but I don't find any similar
> examples elsewhere in the source to iron it out, and I'm still pretty
> unfamiliar with a great deal.

Ah, yes.  I should have seen that coming, sorry.
I think the right code is:

  mPrimaryWindow = static_cast<nsWindow*>(widget.get());

>+  if (!mGdkWindow) {
>+    return;
>+  }
>+
>+  progressPercent = CLAMP(progressPercent, 0, 100);
>+
>+  set_window_hint_cardinal(GDK_WINDOW_XID(gtk_widget_get_window(mShell)),
>+                           PROGRESS_HINT,
>+                           progressPercent);

I expect what you have here should be OK for the caller in this patch, but can
you change the !mGdkWindow test to !mShell please, just in case a new caller
makes the mistake of calling this on an nsWindow without an mShell.

The rest looks great, thanks.

Comment 25

[Michael Webster]

Attachment: widget-gtk-Add-a-TaskbarProgress-implementation-for-.patch

Thanks - updated.  All set I think.

Comment 26

[Michael Webster]

Attachment: widget-gtk-Add-a-TaskbarProgress-implementation-for-.patch

Sorry had to clean up some whitespace, updated once more.

Comment 27

[Karl Tomlinson (:karlt)]

Comment on attachment 8953333 [details] [diff] [review]
widget-gtk-Add-a-TaskbarProgress-implementation-for-.patch

Thank you.  If this try run is successful (or only has unrelated intermittent failures), then this can be checked in.

The first line of the commit message will need adjustment with bug number and r=paolo,karlt to conform to
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

If no-one adds the checkin-needed keyword (which requires elevated permissions), then request needinfo from me and I can do it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06ea771a2b455fe0f3d050bfb80ba20bf8a41706

Comment 28

[Óvári]

(In reply to Karl Tomlinson (:karlt) from comment #27)
What is meant to be the first line?
Bug 1418749 - Add support for window progress in Linux Cinnamon desktop using libxapp. r=paolo,karlt
Bug 1418749 - widget/gtk: Add a TaskbarProgress implementation for gtk3/x11. r=paolo,karlt
or something else?

The try run is complete. Can you add the checkin-needed keyword?

Thank you

Comment 29

[Karl Tomlinson (:karlt)]

(In reply to Óvári from comment #28)
> (In reply to Karl Tomlinson (:karlt) from comment #27)
> What is meant to be the first line?
> Bug 1418749 - Add support for window progress in Linux Cinnamon desktop
> using libxapp. r=paolo,karlt
> Bug 1418749 - widget/gtk: Add a TaskbarProgress implementation for gtk3/x11.
> r=paolo,karlt

The second is good.

> The try run is complete. Can you add the checkin-needed keyword?

The build failed, so not yet.

I expect mPrimaryWindow.get() is required at
https://hg.mozilla.org/try/file/ca0a5cfffd1b/widget/gtk/TaskbarProgress.cpp#l97

Comment 30

[Michael Webster]

Attachment: 0001-Bug-1418749-widget-gtk-Add-a-TaskbarProgress-impleme.patch

Hi,

I've fixed the build issue (weird - I had no warnings of any kinds regarding this, much less a build failure) and updated the commit message.

Comment 31

[Óvári]

(In reply to Karl Tomlinson (:karlt) from comment #29)
Does the title of this ticket be changed to:
widget/gtk: Add a TaskbarProgress implementation for gtk3/x11

> The build failed, so not yet.
Will you be advising another try link to treeherder or will the patch be checked-in?

Thank you

Comment 32

[Karl Tomlinson (:karlt)]

Comment on attachment 8954244 [details] [diff] [review]
0001-Bug-1418749-widget-gtk-Add-a-TaskbarProgress-impleme.patch

(In reply to Michael Webster from comment #30)
> weird - I had no warnings of any kinds regarding
> this, much less a build failure) and updated the commit message.

The build that failed would have been using clang.  Different compilers often
(and different compiler versions sometimes even) have different warnings.
Patches often seem fine until the test infrastructure has a look at them.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3792d43c10f6249ced3339a458a84494d2062f2e

Comment 33

[Karl Tomlinson (:karlt)]

(In reply to Óvári from comment #31)
> (In reply to Karl Tomlinson (:karlt) from comment #29)
> Does the title of this ticket be changed to:
> widget/gtk: Add a TaskbarProgress implementation for gtk3/x11

The title of the ticket is fine and does not need to change.
Bug summaries often describe user-visible features, while
commit messages always describe code changes.

Comment 34

[Óvári]

(In reply to Karl Tomlinson (:karlt) from comment #32)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3792d43c10f6249ced3339a458a84494d2062f2e

The try run is complete. Does all green mean success? Can you add the checkin-needed keyword?

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45ae173a1348
widget/gtk: Add a TaskbarProgress implementation for gtk3/x11. r=karlt

Comment 36

[Dorel Luca [:dluca]]

Backed out changeset 45ae173a1348 (bug 1418749) for widget leaks on Mochitests

Log of the failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=165393875&repo=mozilla-inbound&lineNumber=2720

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd9a72daf25192fca6365a616566f91a21c19daa

Push that got backout:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cf64879466c495a47732d9736ef5daa9b35cefc4

Comment 37

[Michael Webster]

Hi, sorry for not responding sooner regarding the test issue.

So, the mochitests do just fine under a normal test run (./mach mochitest -f plain) - it's only when I add '--headless' that the leak manifests.  I apologize for not checking headless mode - I only learned about it upon seeing the test log. I should read more.

Adding

    if (gfxPlatform::IsHeadless()) {
      return NS_OK;
    }  

to SetPrimaryWindow method, the issue goes away in headless mode.

To keep things cleaner-looking, I could add a custom widget factory constructor that redirects to a headless non-implementation (I don't see the point of having a visual cue implemented in a non-visual environment.)

I messed around with a couple of workarounds - having nsWindow call a TaskbarProgress method to clear its own reference during window destruction, but it doesn't seem to help (and I don't like the circularity of this anyhow) - without knowing more about the headless implementation, I can only guess what the issue is.

Any comments welcome - if the simple headless check is ok, I'll update the patch to include that.

Thanks

Comment 38

[Michael Webster]

Thought about it a bit more, checking for a native window works as well, and may be more appropriate:

    if (!widget->GetNativeData(NS_NATIVE_WINDOW)) {
      return NS_OK;
    }

It could even go in an NS_ENSURE_TRUE() wrapper, but that just adds more probably useless spam to the test builds.

Comment 39

[Óvári]

Karl Tomlinson (:karlt) are you, or do you know who is, able to advise the best way forward:
1. Comment 37
2. Comment 38
3. Something else

Thank you

Comment 40

[:Paolo Amadini]

(In reply to Michael Webster from comment #37)
> To keep things cleaner-looking, I could add a custom widget factory
> constructor that redirects to a headless non-implementation (I don't see the
> point of having a visual cue implemented in a non-visual environment.)

For the record, if a simple early return like the one in comment 38 solves the issue, there is no need for a separate implementation of the interface. This way we'll have less lines of code to reason about.

Comment 41

[Karl Tomlinson (:karlt)]

Early returns with either gfxPlatform::IsHeadless() or
!widget->GetNativeData(NS_NATIVE_WINDOW) would be fine, thanks.

Don't use NS_ENSURE_TRUE() please, as that should only be used for things that
shouldn't happen.

This means that NS_ENSURE_TRUE(mPrimaryWindow != nullptr, NS_ERROR_FAILURE) in
SetProgressState() should be changed to a plain if/return.

An nsIWidget->IsNsWindow() method or similar would be ideal, but we don't have
anything like that AFAICS, and it seems overkill to add it just for this.

I like gfxPlatform::IsHeadless() because that is the same test as that which
determines whether the nsIWidget is created as an nsWindow or a
HeadlessWidget.

I like GetNativeData(NS_NATIVE_WINDOW) because that is querying the object
rather than copying code.  It's not exactly the same as IsNsWindow(), but it
equivalent for existing code.  If you take this approach then please add a
comment to indicate that only nsWindows have native windows and
HeadlessWidgets do not.

Comment 42

[Michael Webster]

Attachment: Bug-1418749-widget-gtk-Add-a-TaskbarProgress-impleme.patch

Updated, went with GetNativeData, and replaced the macro in SetProgressState.  I placed the return there after the other sanity checks, since they could still have debugging value even if running headless.

Comment 43

[Karl Tomlinson (:karlt)]

Comment on attachment 8957411 [details] [diff] [review]
Bug-1418749-widget-gtk-Add-a-TaskbarProgress-impleme.patch

(In reply to Michael Webster from comment #42)
> I placed the return there after the other sanity checks,
> since they could still have debugging value even if running headless.

Good, thank you.

Comment 44

[Karl Tomlinson (:karlt)]

r=paolo,karlt

https://treeherder.mozilla.org/#/jobs?repo=try&revision=547ad5c65ad12f145d02cc591c6f45737b9a6a62

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45c0904bee03
Add a TaskbarProgress implementation for gtk3/x11.  r=paolo,karlt

Comment 46

[Óvári]

Thank you very much Michael Webster, paolo, karlt.

Does the status need to be changed from ASSIGNED to FIXED (or something similar)?
Thank you

Comment 47

[Óvári]

Is your patch able to be adapted to show the progress in Mozilla Thunderbird?
https://bugzilla.mozilla.org/show_bug.cgi?id=1442474

Thank you

Comment 48

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/45c0904bee03

Comment 49

[Jorg K (CEST = GMT+2)]

This has busted Linux32 and Linux64 builds on our old buildbot with gcc 4.9(?):
widget/gtk/nsWindow.cpp:7097:80: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits]

It's this line:
  progressPercent = CLAMP(progressPercent, 0, 100);
added here:
https://hg.mozilla.org/mozilla-central/rev/45c0904bee03#l6.67

Comment 50

[Michael Webster]

Sorry about that - using MIN(progressPercent, 100) there instead would be fine.  I can do a quick patch, is it ok to just attach to this existing bug?

Comment 51

[Jorg K (CEST = GMT+2)]

Best in a new bug.

Comment 52

[Jorg K (CEST = GMT+2)]

Michael, when you file a new bug, could you please make it blocking this bug since it's a regression. It's a little urgent since it's impacting the building of Thunderbird Dailies on Linux.

Also, is the code correct? The warning "comparison of unsigned expression < 0 is always false" doesn't look so good.

Comment 53

[Michael Webster]

Yes it is - I tend to overuse CLAMP as a guard I guess, in this case it's overkill since we're passing an unsigned value in the first place.

I'll prepare a bug/patch shortly.

Comment 54

[Óvári]

(In reply to Michael Webster)
Are you able to look at a LibreOffice implementation?
https://bugs.documentfoundation.org/show_bug.cgi?id=116783
Thank you

Comment 55

[Óvári]

Firefox 61.0b6 (64-bit)
Thank you