Linux status icon for Ubuntu Unity

App Indicator implementation

App Indicator has been implemented as part of libgtk2ui (since app indicators
use gtk). A StatusIconLinux interface has been created alongside linux_ui
interface since we don't want gtk2ui library to depend on the "browser" target
(where status_icon.h is). This interface will be used by implementions of
linux_ui (e.g. libgtk2ui) to support status icons. AppIndicatorIcon is such an
implementation (part of libgtk2ui) which enables app indicator support.
App indicators require us to pass in a GtkMenu (they will not be shown without
a menu) and so some "tools" have been added to create a GtkMenu from a menu
model (similar to GtkMenu implementation).
App indicators do not support setting the icon from a pixbuf and so we have to
create a temporary icon file and pass in the icon directory to libappindicator.

Note: This does not support updating the app indicator menus automatically
when the menu-model is changed externally (i.e. if the change is not due to a
menu item click).

BUG=258651
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212476

[Elliot Glaysher, 2013-07-10 22:55:03]

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
File chrome/browser/ui/libgtk2ui/app_indicator_icon.cc (right):

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
chrome/browser/ui/libgtk2ui/app_indicator_icon.cc:22: typedef enum {/*<
prefix=APP_INDICATOR_CATEGORY >*/
Please remove /* style comments.

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
chrome/browser/ui/libgtk2ui/app_indicator_icon.cc:162: BrowserThread::FILE,
I thought that we weren't supposed to do things directly on the FILE thread, but
the content::BrowserThread::GetBlockingPool() SequenceWorkerPool. I may be wrong
about this.

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
File chrome/browser/ui/libgtk2ui/gtk2_util.cc (right):

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
chrome/browser/ui/libgtk2ui/gtk2_util.cc:151: GdkPixbuf*
GdkPixbufFromSkBitmap(const SkBitmap& bitmap) {
For organizational purposes, please put this in skia_utils_gtk2.{cc,h}.

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
File chrome/browser/ui/libgtk2ui/menu_util.cc (right):

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
chrome/browser/ui/libgtk2ui/menu_util.cc:94: GtkWidget*
GetDefaultImageForCommandId(int command_id) {
As I don't see any of these commands in an appindicator menu (and both
Unity/Gnome have been moving away from icons in menus), could you remove this
method and modify BuildMenuItemWithLabel() (or just pull it out if its only used
in one or two places?)

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.h (right):

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.h:30: static
StatusIconLinuxWrapper* CreateWrappedStatusIcon();
If this static constructor method is the prefered way of building one of these
objects, should the actual constructor be private?

[sidharthms, 2013-07-11 05:15:12]

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
File chrome/browser/ui/libgtk2ui/app_indicator_icon.cc (right):

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
chrome/browser/ui/libgtk2ui/app_indicator_icon.cc:22: typedef enum {/*<
prefix=APP_INDICATOR_CATEGORY >*/
On 2013/07/10 22:55:03, Elliot Glaysher wrote:
> Please remove /* style comments.

Done.

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
chrome/browser/ui/libgtk2ui/app_indicator_icon.cc:162: BrowserThread::FILE,
On 2013/07/10 22:55:03, Elliot Glaysher wrote:
> I thought that we weren't supposed to do things directly on the FILE thread,
but
> the content::BrowserThread::GetBlockingPool() SequenceWorkerPool. I may be
wrong
> about this.

Done.

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
File chrome/browser/ui/libgtk2ui/gtk2_util.cc (right):

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
chrome/browser/ui/libgtk2ui/gtk2_util.cc:95: const SkBitmap
GdkPixbufToImageSkia(GdkPixbuf* pixbuf) {
Should I move this one to skia_utils_gtk2 too?

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
chrome/browser/ui/libgtk2ui/gtk2_util.cc:151: GdkPixbuf*
GdkPixbufFromSkBitmap(const SkBitmap& bitmap) {
On 2013/07/10 22:55:03, Elliot Glaysher wrote:
> For organizational purposes, please put this in skia_utils_gtk2.{cc,h}.

Done.

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
File chrome/browser/ui/libgtk2ui/menu_util.cc (right):

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
chrome/browser/ui/libgtk2ui/menu_util.cc:94: GtkWidget*
GetDefaultImageForCommandId(int command_id) {
On 2013/07/10 22:55:03, Elliot Glaysher wrote:
> As I don't see any of these commands in an appindicator menu (and both
> Unity/Gnome have been moving away from icons in menus), could you remove this
> method and modify BuildMenuItemWithLabel() (or just pull it out if its only
used
> in one or two places?)

Done.

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.h (right):

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.h:30: static
StatusIconLinuxWrapper* CreateWrappedStatusIcon();
On 2013/07/10 22:55:03, Elliot Glaysher wrote:
> If this static constructor method is the prefered way of building one of these
> objects, should the actual constructor be private?

Done.

[Elliot Glaysher, 2013-07-11 21:35:08]

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
File chrome/browser/ui/libgtk2ui/gtk2_util.cc (right):

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
chrome/browser/ui/libgtk2ui/gtk2_util.cc:95: const SkBitmap
GdkPixbufToImageSkia(GdkPixbuf* pixbuf) {
On 2013/07/11 05:15:12, sidharthms wrote:
> Should I move this one to skia_utils_gtk2 too?

I totally missed this. Yes.

[sidharthms, 2013-07-12 01:12:14]

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
File chrome/browser/ui/libgtk2ui/gtk2_util.cc (right):

https://codereview.chromium.org/18334003/diff/21001/chrome/browser/ui/libgtk2...
chrome/browser/ui/libgtk2ui/gtk2_util.cc:95: const SkBitmap
GdkPixbufToImageSkia(GdkPixbuf* pixbuf) {
On 2013/07/11 21:35:08, Elliot Glaysher wrote:
> On 2013/07/11 05:15:12, sidharthms wrote:
> > Should I move this one to skia_utils_gtk2 too?
> 
> I totally missed this. Yes.

Done.

[Elliot Glaysher, 2013-07-12 22:37:36]

lgtm; please wait for sky and atwilson's lgtms too.

[Pete Williamson, 2013-07-13 00:32:01]

A quick drive by comment...

https://codereview.chromium.org/18334003/diff/48001/chrome/browser/ui/libgtk2...
File chrome/browser/ui/libgtk2ui/gtk2_util.cc (right):

https://codereview.chromium.org/18334003/diff/48001/chrome/browser/ui/libgtk2...
chrome/browser/ui/libgtk2ui/gtk2_util.cc:43: const std::string& target) {
This function looks like something that we might have a string utility for. 
Have you checked the string utilities to see if we can re-use something that
already exists instead of re-implementing this?

[Elliot Glaysher, 2013-07-15 17:35:41]

https://codereview.chromium.org/18334003/diff/48001/chrome/browser/ui/libgtk2...
File chrome/browser/ui/libgtk2ui/gtk2_util.cc (right):

https://codereview.chromium.org/18334003/diff/48001/chrome/browser/ui/libgtk2...
chrome/browser/ui/libgtk2ui/gtk2_util.cc:43: const std::string& target) {
On 2013/07/13 00:32:01, Pete Williamson wrote:
> This function looks like something that we might have a string utility for. 
> Have you checked the string utilities to see if we can re-use something that
> already exists instead of re-implementing this?

Like every function in this file, this one was copied from some gtk specific
file, since the aura port can't otherwise touch gtk code. Copying it is the
correct thing to do.

[sky, 2013-07-15 20:41:09]

https://codereview.chromium.org/18334003/diff/48001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.h (right):

https://codereview.chromium.org/18334003/diff/48001/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.h:13: class
StatusIconLinuxWrapper : public StatusIcon,
Description?

https://codereview.chromium.org/18334003/diff/48001/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.h:45: };
DISALLOW_...

https://codereview.chromium.org/18334003/diff/48001/ui/linux_ui/linux_ui.h
File ui/linux_ui/linux_ui.h (right):

https://codereview.chromium.org/18334003/diff/48001/ui/linux_ui/linux_ui.h#ne...
ui/linux_ui/linux_ui.h:70: virtual StatusIconLinux* CreateLinuxStatusIcon()
const = 0;
scoped_ptr?

https://codereview.chromium.org/18334003/diff/48001/ui/linux_ui/status_icon_l...
File ui/linux_ui/status_icon_linux.h (right):

https://codereview.chromium.org/18334003/diff/48001/ui/linux_ui/status_icon_l...
ui/linux_ui/status_icon_linux.h:27: };
Add a protected virtual destructor.

https://codereview.chromium.org/18334003/diff/48001/ui/linux_ui/status_icon_l...
ui/linux_ui/status_icon_linux.h:38: // passed, subclass should destroy the
native context menu.
This comment isn't particularly clear. Doesn't the platform need to destroy the
menu regardless of what is passed in? Otherwise it could be referencing a
deleted MenuModel, right?

https://codereview.chromium.org/18334003/diff/48001/ui/linux_ui/status_icon_l...
ui/linux_ui/status_icon_linux.h:45: Delegate* delegate_;
Constructor needs to initialize this to NULL.

[sidharthms, 2013-07-16 01:12:05]

https://codereview.chromium.org/18334003/diff/48001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.h (right):

https://codereview.chromium.org/18334003/diff/48001/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.h:13: class
StatusIconLinuxWrapper : public StatusIcon,
On 2013/07/15 20:41:10, sky wrote:
> Description?

Done.

https://codereview.chromium.org/18334003/diff/48001/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.h:45: };
On 2013/07/15 20:41:10, sky wrote:
> DISALLOW_...

Done.

https://codereview.chromium.org/18334003/diff/48001/ui/linux_ui/linux_ui.h
File ui/linux_ui/linux_ui.h (right):

https://codereview.chromium.org/18334003/diff/48001/ui/linux_ui/linux_ui.h#ne...
ui/linux_ui/linux_ui.h:70: virtual StatusIconLinux* CreateLinuxStatusIcon()
const = 0;
On 2013/07/15 20:41:10, sky wrote:
> scoped_ptr?

Done.

https://codereview.chromium.org/18334003/diff/48001/ui/linux_ui/status_icon_l...
File ui/linux_ui/status_icon_linux.h (right):

https://codereview.chromium.org/18334003/diff/48001/ui/linux_ui/status_icon_l...
ui/linux_ui/status_icon_linux.h:27: };
On 2013/07/15 20:41:10, sky wrote:
> Add a protected virtual destructor.

Done.

https://codereview.chromium.org/18334003/diff/48001/ui/linux_ui/status_icon_l...
ui/linux_ui/status_icon_linux.h:38: // passed, subclass should destroy the
native context menu.
On 2013/07/15 20:41:10, sky wrote:
> This comment isn't particularly clear. Doesn't the platform need to destroy
the
> menu regardless of what is passed in? Otherwise it could be referencing a
> deleted MenuModel, right?

Done.

https://codereview.chromium.org/18334003/diff/48001/ui/linux_ui/status_icon_l...
ui/linux_ui/status_icon_linux.h:45: Delegate* delegate_;
On 2013/07/15 20:41:10, sky wrote:
> Constructor needs to initialize this to NULL.

Done.

[sky, 2013-07-16 13:15:35]

LGTM

[Andrew T Wilson (Slow), 2013-07-17 14:55:13]

*/status_icons/* lgtm

[Andrew T Wilson (Slow), 2013-07-17 14:55:21]

https://codereview.chromium.org/18334003/diff/61001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.h (right):

https://codereview.chromium.org/18334003/diff/61001/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.h:13: // Wraper
class for StatusIconLinux that implements the standard StatusIcon
nit: wrapper

[sidharthms, 2013-07-17 23:03:01]

https://codereview.chromium.org/18334003/diff/61001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.h (right):

https://codereview.chromium.org/18334003/diff/61001/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.h:13: // Wraper
class for StatusIconLinux that implements the standard StatusIcon
On 2013/07/17 14:55:21, Andrew T Wilson wrote:
> nit: wrapper

Done. Sorry!

[commit-bot: I haz the power, 2013-07-17 23:09:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sidharthms@chromium.org/18334003/84001

[commit-bot: I haz the power, 2013-07-18 00:57:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[dewittj, 2013-07-18 16:47:37]

On 2013/07/18 00:57:17, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build on linux_aura.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
> Your code is likely broken or HEAD is junk. Please ensure your
> code is not broken then alert the build sheriffs.
> Look at the try server FAQ for more details.

FYI crrev.com/17611003 landed which will likely require a rebase of this patch.

[commit-bot: I haz the power, 2013-07-18 18:36:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sidharthms@chromium.org/18334003/116001

[commit-bot: I haz the power, 2013-07-18 23:58:34]

Change committed as 212476

[Mark Seaborn, 2013-07-27 04:38:20]

https://chromiumcodereview.appspot.com/18334003/diff/116001/chrome/browser/ui...
File chrome/browser/ui/libgtk2ui/gtk2_util.cc (right):

https://chromiumcodereview.appspot.com/18334003/diff/116001/chrome/browser/ui...
chrome/browser/ui/libgtk2ui/gtk2_util.cc:14: #include
"ui/base/keycodes/keyboard_code_conversion_x.cc"
This should be ".h", not ".cc"!

I just got the following link error on a linux_aura bot as a result:

../../third_party/gold/gold64: error:
obj/chrome/browser/ui/libgtk2ui/libgtk2ui.a(obj/chrome/browser/ui/libgtk2ui/gtk2ui.gtk2_util.o):
multiple definition of 'ui::KeyboardCodeFromXKeysym(unsigned int)'
../../third_party/gold/gold64:
obj/ui/libui.a(obj/ui/base/keycodes/ui.keyboard_code_conversion_x.o): previous
definition here
../../third_party/gold/gold64: error:
obj/chrome/browser/ui/libgtk2ui/libgtk2ui.a(obj/chrome/browser/ui/libgtk2ui/gtk2ui.gtk2_util.o):
multiple definition of 'ui::KeyboardCodeFromXKeyEvent(_XEvent*)'
../../third_party/gold/gold64:
obj/ui/libui.a(obj/ui/base/keycodes/ui.keyboard_code_conversion_x.o): previous
definition here
...

(from
http://build.chromium.org/p/tryserver.chromium/builders/linux_aura/builds/636...,
from https://codereview.chromium.org/20754005)

[sidharthms, 2013-07-27 07:50:23]

https://chromiumcodereview.appspot.com/18334003/diff/116001/chrome/browser/ui...
File chrome/browser/ui/libgtk2ui/gtk2_util.cc (right):

https://chromiumcodereview.appspot.com/18334003/diff/116001/chrome/browser/ui...
chrome/browser/ui/libgtk2ui/gtk2_util.cc:14: #include
"ui/base/keycodes/keyboard_code_conversion_x.cc"
Oh! I'm not sure how it got through the commit queue. Anyway I'll get it fixed
asap.

On 2013/07/27 04:38:20, Mark Seaborn wrote:
> This should be ".h", not ".cc"!
> 
> I just got the following link error on a linux_aura bot as a result:
> 
> ../../third_party/gold/gold64: error:
>
obj/chrome/browser/ui/libgtk2ui/libgtk2ui.a(obj/chrome/browser/ui/libgtk2ui/gtk2ui.gtk2_util.o):
> multiple definition of 'ui::KeyboardCodeFromXKeysym(unsigned int)'
> ../../third_party/gold/gold64:
> obj/ui/libui.a(obj/ui/base/keycodes/ui.keyboard_code_conversion_x.o): previous
> definition here
> ../../third_party/gold/gold64: error:
>
obj/chrome/browser/ui/libgtk2ui/libgtk2ui.a(obj/chrome/browser/ui/libgtk2ui/gtk2ui.gtk2_util.o):
> multiple definition of 'ui::KeyboardCodeFromXKeyEvent(_XEvent*)'
> ../../third_party/gold/gold64:
> obj/ui/libui.a(obj/ui/base/keycodes/ui.keyboard_code_conversion_x.o): previous
> definition here
> ...
> 
> (from
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_aura/builds/636...,
> from https://codereview.chromium.org/20754005)

[Mark Seaborn, 2013-07-27 16:28:43]

https://chromiumcodereview.appspot.com/18334003/diff/116001/chrome/browser/ui...
File chrome/browser/ui/libgtk2ui/gtk2_util.cc (right):

https://chromiumcodereview.appspot.com/18334003/diff/116001/chrome/browser/ui...
chrome/browser/ui/libgtk2ui/gtk2_util.cc:14: #include
"ui/base/keycodes/keyboard_code_conversion_x.cc"
On 2013/07/27 07:50:24, sidharthms wrote:
> Oh! I'm not sure how it got through the commit queue. Anyway I'll get
> it fixed asap.

It's OK, I've prepared a fix: https://codereview.chromium.org/20900002/

I should have just done that without complaining on this review first. :-)