GTK: Cache our GdkCursor objects, since building them hits the disk.

GTK: Cache our GdkCursor objects, since building them hits the disk.

TEST=strace chrome. Moving the mouse over the webpage shouldn't spew open() calls.
BUG=21623
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=26061

[Evan Stade, 2009-09-11 23:17:48]

Since you're really just exposing a single static function, it feels like this
belongs as a function in gtk_util, and the class itself can be defined in the
anonymous namespace in gtk_util.cc.

Although I'm not entirely sure the class is even necessary. What happens if you
don't free the cursors on shutdown?

http://codereview.chromium.org/195071/diff/1/7
File chrome/common/gdk_cursor_cache.cc (right):

http://codereview.chromium.org/195071/diff/1/7#newcode7
Line 7: GdkCursor* GdkCursorCache::GetCursor(GdkCursorType type) {
// static

http://codereview.chromium.org/195071/diff/1/8
File chrome/common/gdk_cursor_cache.h (right):

http://codereview.chromium.org/195071/diff/1/8#newcode29
Line 29: // A cache of mouse cursors. gdk_cursor_new() hits the disk in several
nit: this comment seems redundant with the class's comment.

[Elliot Glaysher, 2009-09-12 00:34:09]

On 2009/09/11 23:17:48, Evan Stade wrote:
> Since you're really just exposing a single static function, it feels like this
> belongs as a function in gtk_util, and the class itself can be defined in the
> anonymous namespace in gtk_util.cc.

Done.

> Although I'm not entirely sure the class is even necessary. What happens if
you
> don't free the cursors on shutdown?

valgrind reports leaked memory.

[Evan Stade, 2009-09-12 00:35:20]

lgtm, thanks

[Evan Martin, 2009-09-12 00:37:05]

http://codereview.chromium.org/195071/diff/4001/5004
File chrome/common/gtk_util.cc (right):

http://codereview.chromium.org/195071/diff/4001/5004#newcode86
Line 86: gdk_cursor_ref(cursor);
Won't this cause a +2 refcount on a cache miss?

[Evan Martin, 2009-09-12 00:38:58]

NM, I see the cache owns a ref as well.  LGTM

[Elliot Glaysher, 2009-09-12 00:52:48]

Clarified the comment in what I committed.

On Fri, Sep 11, 2009 at 5:38 PM,  <evan@chromium.org> wrote:
> NM, I see the cache owns a ref as well. =A0LGTM
>
> http://codereview.chromium.org/195071
>