Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(369)

Issue 6759076: Add the calculated WMClass to generated .desktop files. (Closed)

Created:
9 years, 8 months ago by Elliot Glaysher
Modified:
9 years, 7 months ago
CC:
chromium-reviews, Elliot Glaysher
Visibility:
Public.

Description

Add the calculated WMClass to generated .desktop files. (Also modifies it so that we only modify the wmclass_name, and not the wmclass_class because the internal application names aren't meant for display and are very ugly.) BUG=20587 TEST=Verify with xprop that the WM_CLASS of application desktop links mmatches the StartupWMClass key in the desktop file. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80656

Patch Set 1 #

Total comments: 2

Patch Set 2 : Rebase for commit #

Patch Set 3 : Fix unit tests on linux #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+88 lines, -27 lines) Patch
M chrome/browser/shell_integration.h View 1 chunk +5 lines, -2 lines 0 comments Download
M chrome/browser/shell_integration_linux.cc View 1 2 5 chunks +23 lines, -4 lines 1 comment Download
M chrome/browser/shell_integration_unittest.cc View 1 2 8 chunks +19 lines, -7 lines 0 comments Download
M chrome/browser/ui/gtk/browser_window_gtk.cc View 1 2 chunks +7 lines, -6 lines 1 comment Download
M chrome/browser/web_applications/web_app.h View 2 chunks +11 lines, -0 lines 0 comments Download
M chrome/browser/web_applications/web_app.cc View 4 chunks +23 lines, -8 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
Elliot Glaysher
This bug caused major confusion for me. It wasn't until trevi pointed out yesterday that ...
9 years, 8 months ago (2011-04-01 20:22:14 UTC) #1
Elliot Glaysher
On 2011/04/01 20:22:14, Elliot Glaysher wrote: > This bug caused major confusion for me. It ...
9 years, 8 months ago (2011-04-04 21:31:23 UTC) #2
Evan Stade
lgtm http://codereview.chromium.org/6759076/diff/1/chrome/browser/shell_integration_linux.cc File chrome/browser/shell_integration_linux.cc (right): http://codereview.chromium.org/6759076/diff/1/chrome/browser/shell_integration_linux.cc#newcode418 chrome/browser/shell_integration_linux.cc:418: // Skip StartupWMClass; it will cerrtainly be wrong ...
9 years, 8 months ago (2011-04-05 21:12:08 UTC) #3
Evan Martin
http://codereview.chromium.org/6759076/diff/7001/chrome/browser/shell_integration_linux.cc File chrome/browser/shell_integration_linux.cc (right): http://codereview.chromium.org/6759076/diff/7001/chrome/browser/shell_integration_linux.cc#newcode430 chrome/browser/shell_integration_linux.cc:430: output_buffer += StringPrintf("StartupWMClass=%s\n", wmclass.c_str()); Do we need to worry ...
9 years, 8 months ago (2011-04-14 18:18:49 UTC) #4
Elliot Glaysher
On 2011/04/14 18:18:49, Evan Martin wrote: > http://codereview.chromium.org/6759076/diff/7001/chrome/browser/shell_integration_linux.cc > File chrome/browser/shell_integration_linux.cc (right): > > http://codereview.chromium.org/6759076/diff/7001/chrome/browser/shell_integration_linux.cc#newcode430 ...
9 years, 8 months ago (2011-04-15 22:02:18 UTC) #5
Marco Trevisan (Treviño)
9 years, 8 months ago (2011-04-21 15:53:42 UTC) #6
This is the fix needed to close the issue 20587

http://codereview.chromium.org/6759076/diff/7001/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/6759076/diff/7001/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_window_gtk.cc:303: gdk_get_program_class());
This is wrong: you must set the wmclassname as third parameter of
gtk_window_set_wmclass for it to be recognized by window managers (which are
using libraries like libwnck)!

Best implementation would be:
gtk_window_set_wmclass(window_, window_->wmclass_name, wmclassname.c_str());

And also you can add too:
gtk_window_set_role(window_, wmclassname.c_str());

Powered by Google App Engine
This is Rietveld 408576698