Add support for window progress in Linux Cinnamon desktop using libxapp

VERIFIED FIXED in Firefox 61

Status

()

enhancement
P5
normal
VERIFIED FIXED
2 years ago
11 hours ago

People

(Reporter: ovari123, Assigned: miketwebster)

Tracking

57 Branch
mozilla61
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments, 7 obsolete attachments)

Reporter

Description

2 years ago
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
Reporter

Comment 1

2 years ago
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

2 years ago
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.
Has Regression Range: --- → irrelevant
Component: Untriaged → Downloads Panel
OS: Unspecified → Linux
Hardware: Unspecified → All
Whiteboard: [WONTFIX?]
Reporter

Comment 3

2 years ago
(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
Reporter

Comment 4

2 years ago
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
Reporter

Updated

2 years ago
Summary: Show download progress in panel → Add support for window progress in Cinnamon desktop using libxapp

Comment 5

2 years ago
(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.
Component: Downloads Panel → Downloads API
Product: Firefox → Toolkit
Summary: Add support for window progress in Cinnamon desktop using libxapp → Add support for window progress in Linux Cinnamon desktop using libxapp
Whiteboard: [WONTFIX?]
Reporter

Comment 6

2 years ago
(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

2 years ago
I don't see any duplicates, so NEW it is then.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter

Comment 8

2 years ago
(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

2 years ago
(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.
Priority: -- → P5
Assignee

Comment 10

2 years ago
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
Assignee

Comment 11

2 years ago
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
Attachment #8937951 - Flags: feedback+
Reporter

Comment 12

2 years ago
(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
Toolkit does not require super-review, just regular review: https://wiki.mozilla.org/Toolkit/Code_Review

Updated

2 years ago
Attachment #8937951 - Flags: review?(enndeakin)

Comment 14

2 years ago
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
Assignee: nobody → miketwebster
Status: NEW → ASSIGNED

Comment 15

2 years ago
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.
Attachment #8937951 - Flags: review?(paolo.mozmail)
Attachment #8937951 - Flags: review?(karlt)
Attachment #8937951 - Flags: review?(enndeakin)
Assignee

Comment 16

2 years ago
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!
Assignee

Comment 17

2 years ago
Posted patch 395821.patch (obsolete) — Splinter Review
Updated patch - should (hopefully) be good to go.
Attachment #8937951 - Attachment is obsolete: true
Attachment #8937951 - Flags: review?(paolo.mozmail)
Attachment #8937951 - Flags: review?(karlt)

Comment 18

2 years ago
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.
Attachment #8938593 - Flags: feedback+

Updated

2 years ago
Attachment #8938593 - Flags: review?(karlt)
Assignee

Comment 19

2 years ago
Posted patch progress-12282017.patch (obsolete) — Splinter Review
Updated, thanks
Attachment #8938593 - Attachment is obsolete: true
Attachment #8938593 - Flags: review?(karlt)

Comment 20

2 years ago
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.
Attachment #8939048 - Flags: review?(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.
Attachment #8939048 - Flags: review?(karlt)
Attachment #8939048 - Flags: review-
Attachment #8939048 - Flags: feedback+
Assignee

Comment 22

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

Thanks
Assignee

Comment 23

Last year
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.
Attachment #8939048 - Attachment is obsolete: true
Attachment #8952951 - Flags: feedback+
Assignee

Updated

Last year
Attachment #8952951 - Flags: feedback+ → feedback?(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.
Attachment #8952951 - Flags: feedback?(karlt) → feedback+
Assignee

Comment 25

Last year
Thanks - updated.  All set I think.
Attachment #8952951 - Attachment is obsolete: true
Attachment #8953331 - Flags: review?(karlt)
Assignee

Comment 26

Last year
Sorry had to clean up some whitespace, updated once more.
Attachment #8953331 - Attachment is obsolete: true
Attachment #8953331 - Flags: review?(karlt)
Attachment #8953333 - Flags: review?(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
Attachment #8953333 - Flags: review?(karlt) → review+
Reporter

Comment 28

Last year
(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
(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
Assignee

Comment 30

Last year
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.
Attachment #8953333 - Attachment is obsolete: true
Attachment #8954244 - Flags: review?(karlt)
Reporter

Comment 31

Last year
(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 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
Attachment #8954244 - Flags: review?(karlt) → review+
(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.
Reporter

Comment 34

Last year
(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

Last year
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45ae173a1348
widget/gtk: Add a TaskbarProgress implementation for gtk3/x11. r=karlt
Keywords: checkin-needed
See Also: → 1442474
Assignee

Comment 37

Last year
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
Assignee

Comment 38

Last year
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.
Reporter

Comment 39

Last year
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
Flags: needinfo?(karlt)
(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.
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.
Flags: needinfo?(karlt)
Assignee

Comment 42

Last year
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.
Attachment #8954244 - Attachment is obsolete: true
Flags: needinfo?(miketwebster)
Attachment #8957411 - Flags: review?(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.
Attachment #8957411 - Flags: review?(karlt) → review+

Comment 45

Last year
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45c0904bee03
Add a TaskbarProgress implementation for gtk3/x11.  r=paolo,karlt
Keywords: checkin-needed
Reporter

Comment 46

Last year
Thank you very much Michael Webster, paolo, karlt.

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

Comment 47

Last year
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

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/45c0904bee03
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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
Flags: needinfo?(miketwebster)
Flags: needinfo?(karlt)
Assignee

Comment 50

Last year
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?
Flags: needinfo?(miketwebster)
Best in a new bug.
Flags: needinfo?(karlt)
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.
Assignee

Comment 53

Last year
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.
Assignee

Updated

Last year
Depends on: 1445503
Reporter

Comment 54

Last year
(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
Reporter

Comment 55

Last year
Firefox 61.0b6 (64-bit)
Thank you
Status: RESOLVED → VERIFIED
Comment hidden (spam)
You need to log in before you can comment on or make changes to this bug.