Last Comment Bug 762641 - Window matching unreliable for webapprt on Linux
: Window matching unreliable for webapprt on Linux
Status: VERIFIED FIXED
[qa!]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Webapp Runtime (show other bugs)
: Trunk
: All Linux
: P3 normal
: Firefox 16
Assigned To: Chris Coulson
: Jason Smith [:jsmith]
:
Mentors:
Depends on: 768768
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-07 13:15 PDT by Chris Coulson
Modified: 2016-03-21 12:39 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
mcastelluccio: in‑moztrap+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Make the class name part of WM_CLASS unique to each webapp (1.05 KB, patch)
2012-06-07 13:15 PDT, Chris Coulson
jst: review+
karlt: review+
Details | Diff | Splinter Review

Description Chris Coulson 2012-06-07 13:15:29 PDT
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.
Comment 1 Chris Coulson 2012-06-07 13:20:42 PDT
Not sure of the most appropriate person to review this
Comment 2 Marco Castelluccio [:marco] 2012-06-07 13:29:27 PDT
Comment on attachment 631097 [details] [diff] [review]
Make the class name part of WM_CLASS unique to each webapp

Thank you, Chris.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2012-06-07 16:26:45 PDT
Comment on attachment 631097 [details] [diff] [review]
Make the class name part of WM_CLASS unique to each webapp

Looks good to me!
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2012-06-08 12:28:14 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/05735ac0ea88
Comment 5 Jason Smith [:jsmith] 2012-06-08 12:44:15 PDT
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?
Comment 6 Chris Coulson 2012-06-08 15:57:32 PDT
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.
Comment 7 Chris Coulson 2012-06-08 16:00:11 PDT
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
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-06-09 19:15:10 PDT
https://hg.mozilla.org/mozilla-central/rev/05735ac0ea88
Comment 9 Jason Smith [:jsmith] 2012-06-09 23:38:20 PDT
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.
Comment 10 Jason Smith [:jsmith] 2012-06-26 17:24:03 PDT
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.
Comment 11 Karl Tomlinson (:karlt) 2012-06-26 17:52:57 PDT
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.
Comment 12 Jason Smith [:jsmith] 2012-06-26 18:00:54 PDT
(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.
Comment 13 Karl Tomlinson (:karlt) 2012-06-26 21:28:43 PDT
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.
Comment 14 Jason Smith [:jsmith] 2012-06-26 21:35:45 PDT
(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.
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-27 09:11:32 PDT
(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.
Comment 16 Jason Smith [:jsmith] 2012-06-27 09:18:48 PDT
(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?

Note You need to log in before you can comment on or make changes to this bug.