Window matching unreliable for webapprt on Linux

VERIFIED FIXED in Firefox 16

Status

Firefox Graveyard
Webapp Runtime
P3
normal
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: Chris Coulson, Assigned: Chris Coulson)

Tracking

Trunk
Firefox 16
All
Linux
Bug Flags:
in-testsuite -
in-moztrap +

Details

(Whiteboard: [qa!])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 631097 [details] [diff] [review]
Make the class name part of WM_CLASS unique to each webapp

Window matching doesn't work reliably for the webapprt in Linux. What I mean by this is that the shell doesn't always associate the webapp window with the correct launcher, so it displays the wrong icon in the launcher bar or dock.

This happens if you don't use the shell to launch an app.

It might be useful for me to give a bit of background info on how this works in Unity and gnome-shell.

In Unity, the desktop environment tries each of the following:
- Match the class name part of WM_CLASS to the name of a desktop file (eg, "Firefox" matches to the desktop file called "firefox.desktop")
- Match either the instance name or class name part of WM_CLASS to the StartupWMClass field of a desktop file (preferring the instance name).
- Match the PID of the process owning the window to a PID that the shell just launched (only works for apps launched via the shell)
- Some fallback using the startup notification mechanism

In gnome shell, the shell tries each of the following:
- Match the class name part of WM_CLASS to the name of a desktop file
- Match the PID of the process owning the window to a PID that the shell just launched (only works for apps launched via the shell)
- Some fallback using the startup notification mechanism

Currently in Firefox, the class name part of WM_CLASS is not overridden, and comes directly from argv[0]. The instance name part is derived from the "windowtype" attribute on the XUL window. So, for Firefox, it looks like this:

WM_CLASS(STRING) = "Navigator", "Firefox"

For all webapps, it looks like this:

WM_CLASS(STRING) = "Webapprt", "Webapprt-stub"

I think the only way to make this work reliably in both gnome shell and unity is to make the class name part of WM_CLASS unique to each app (ie, the bit that is currently "Webapprt-stub"), and it would need to match the basename of the desktop file created by the webapp installer.

The attached patch implements this.
(Assignee)

Comment 1

5 years ago
Not sure of the most appropriate person to review this
Comment on attachment 631097 [details] [diff] [review]
Make the class name part of WM_CLASS unique to each webapp

Thank you, Chris.
Attachment #631097 - Flags: review?(jst)
Attachment #631097 - Flags: review?(karlt)
Comment on attachment 631097 [details] [diff] [review]
Make the class name part of WM_CLASS unique to each webapp

Looks good to me!
Attachment #631097 - Flags: review?(jst) → review+
Attachment #631097 - Flags: review?(karlt) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/05735ac0ea88
Trying to understand what needs to be done to verify this fix. Could you clarify what test cases would need to be used to confirm this is fixed?
(Assignee)

Comment 6

5 years ago
A simple test case which you should be able to perform in either unity or gnome shell:

1) Launch a webapp from the shell (ie, the dash or launcher in unity or the activities overlay in gnome shell)
2) Close the webapp and then launch it from a terminal window (ie, ~/.http\;www.lordofultima.com/webapprt-stub)

Without this change, the second case at least should result in a generic icon appearing in the launcher of the shell (and also the top bar in gnome shell), rather than the correct application icon.

With the fix, the correct icon for the application will be displayed in both cases.
(Assignee)

Comment 7

5 years ago
You can also use xprop to look at the WM_CLASS property on the top-level window to make sure it matches the filename of the applications desktop file. Here, I get:

Before:
WM_CLASS(STRING) = "Webapprt", "Webapprt-stub"

After:
WM_CLASS(STRING) = "Webapprt", "owa-http;www.lordofultima.com"

chr1s@farnsworth:~$ cat ~/.local/share/applications/owa-http\;www.lordofultima.com.desktop 
[Desktop Entry]
Name=Lord of Ultima
Comment=Lord of Ultima is EA's popular browser based strategy game that simulates an immersive medieval civilization.
Exec="/home/chr1s/.http;www.lordofultima.com/webapprt-stub"
Icon=/home/chr1s/.http;www.lordofultima.com/http;www.lordofultima.com.png
Type=Application
Terminal=false
Assignee: nobody → chrisccoulson
Priority: -- → P3
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/05735ac0ea88
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Marco - Could you verify this fix by reproducing the bug on an old build and verifying it's fixed on the latest inbound or nightly build? See Chris's comments on how to verify this.
Whiteboard: [qa+]

Updated

5 years ago
Flags: in-moztrap?(mar.castelluccio)
Flags: in-moztrap?(mar.castelluccio) → in-moztrap+
Busted. Ran the test case in comment 6, and I end up getting an icon that's a ? mark, not the app icon in step 2 on launch. Tested with Springpad from apps.mozillalabs.com/appdir. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

5 years ago
Whiteboard: [qa+]
Usually when a bug fix leaves the same symptoms remaining on some systems, the most productive thing to do is to file a new bug blocking the original fix.  Those on the bug with the partial fix will know that it is not a complete fix.

It is easier to track changes to code when one bug report maps to one landing, and diagnosing the cause of the remaining bug would be done more tidily in its own bug.

The bug can be reopened if the patch is backed out of all active trains.
(In reply to Karl Tomlinson (:karlt) from comment #11)
> Usually when a bug fix leaves the same symptoms remaining on some systems,
> the most productive thing to do is to file a new bug blocking the original
> fix.  Those on the bug with the partial fix will know that it is not a
> complete fix.

I don't agree in this case. The fix in this case was targeted for Ubuntu with Unity and the terminal, so the case you describe does not apply to this situation. Generally, if the bug fix does not do what it's intending to do (unless its a feature implementation bug or some other special case), it's a reopen, unless when testing this I find something completely new, which in that case opens a new bug. 

> 
> It is easier to track changes to code when one bug report maps to one
> landing, and diagnosing the cause of the remaining bug would be done more
> tidily in its own bug.
> 
> The bug can be reopened if the patch is backed out of all active trains.

Umm...no? Backout does not necessarily apply at all to reopening, that applies to "disabling" in the firefox tracking flags. 

In this case, if the bug fix does not do what's it's intending to do, then it's a reopen. And I definitely believe that's the case here.
Looks like comment 10 is likely a regression from bug 763358.
I wonder whether g_set_prgname might be more appropriate than gdk_set_program_class anyway.
(In reply to Karl Tomlinson (:karlt) from comment #13)
> Looks like comment 10 is likely a regression from bug 763358.
> I wonder whether g_set_prgname might be more appropriate than
> gdk_set_program_class anyway.

Okay, fair enough. I'll file a new bug for this. I'm blocked on verifying this fix though.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(In reply to Jason Smith [:jsmith] from comment #12)
> Generally, if the bug fix does not do what it's intending to do
> (unless its a feature implementation bug or some other special case), it's a
> reopen, unless when testing this I find something completely new, which in
> that case opens a new bug. 

I disagree. As Karl says, unless a FIXED bug's patch is backed out, you shouldn't REOPEN it. The alternative is tracking multiple landings in a single bug, and that gets awfully confusing rather quickly.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #15)
> (In reply to Jason Smith [:jsmith] from comment #12)
> > Generally, if the bug fix does not do what it's intending to do
> > (unless its a feature implementation bug or some other special case), it's a
> > reopen, unless when testing this I find something completely new, which in
> > that case opens a new bug. 
> 
> I disagree. As Karl says, unless a FIXED bug's patch is backed out, you
> shouldn't REOPEN it. The alternative is tracking multiple landings in a
> single bug, and that gets awfully confusing rather quickly.

This sounds like a conversation that probably should be discussed elsewhere (important though). Is there a discussion thread appropriate discussion such as this?

Updated

5 years ago
Depends on: 768768
Whiteboard: [qa verification blocked]

Updated

5 years ago
QA Contact: jsmith

Updated

5 years ago
Whiteboard: [qa verification blocked] → [qa+]

Updated

5 years ago
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.