GTK: Step 1 of tab strip refresh.

GTK: Step 1 of tab strip refresh.

willchan pointed out that while idling, the chrome browser process can take up
to 25% CPU time on one of our Z600s, which is ridiculous. There's a lot of
causes here, but the first one I'm focusing on is how slow our tabstrip
rendering code is. Previously, each frame we stuffed bitmaps across the X11
connection instead of using cairo primitives for serverside accelerated
drawing.

This patch changes the code in tab_renderer_gtk.h from skia rendering (with
manually composited bitmaps) to cairo rendering (which should be a set of
commands to the x server that should composite bitmaps already living
serverside).

From here we can:
- Look for other ways to minimize the amount of work in an expose event.
- Only redraw the dirty rectangles or otherwise minimize the expose area.
- Finally fix that theme rendering bug where dragging a tab along the tabstrip
  doesn't update to reflect the new background.

BUG=100803
TEST=Manual testing looking for graphical regressions.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107514

[Evan Stade, 2011-10-25 23:33:30]

http://codereview.chromium.org/8392011/diff/1013/chrome/browser/ui/gtk/tabs/t...
File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right):

http://codereview.chromium.org/8392011/diff/1013/chrome/browser/ui/gtk/tabs/t...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:462: theme_id =
IDR_THEME_TAB_BACKGROUND;
ternary operator

http://codereview.chromium.org/8392011/diff/1013/chrome/browser/ui/gtk/tabs/t...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:1110: tab_active_r_width_ =
rb.GetBitmapNamed(IDR_TAB_ACTIVE_RIGHT)->width();
the active and inactive widths are different?

http://codereview.chromium.org/8392011/diff/1013/chrome/browser/ui/gtk/tabs/t...
File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.h (right):

http://codereview.chromium.org/8392011/diff/1013/chrome/browser/ui/gtk/tabs/t...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.h:141: void Paint(GtkWidget* widget,
cairo_t* cr);
docs

http://codereview.chromium.org/8392011/diff/1013/chrome/browser/ui/gtk/tabs/t...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.h:294: void DrawTabBackground(
docs

http://codereview.chromium.org/8392011/diff/1013/chrome/browser/ui/gtk/tabs/t...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.h:348: static int
tab_active_l_width_;
docs

http://codereview.chromium.org/8392011/diff/1013/chrome/browser/ui/gtk/tabs/t...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.h:349: static int
tab_active_l_height_;
you might use gfx::Size here

http://codereview.chromium.org/8392011/diff/1013/ui/gfx/pango_util.cc
File ui/gfx/pango_util.cc (right):

http://codereview.chromium.org/8392011/diff/1013/ui/gfx/pango_util.cc#newcode270
ui/gfx/pango_util.cc:270: if (fade_width > (bounds_.width() / 2)) {
remove excess parens

http://codereview.chromium.org/8392011/diff/1013/ui/gfx/pango_util.h
File ui/gfx/pango_util.h (right):

http://codereview.chromium.org/8392011/diff/1013/ui/gfx/pango_util.h#newcode36
ui/gfx/pango_util.h:36: class UI_EXPORT PangoDrawString {
seems weird to do

PangoDrawString draw(...);
draw.Draw(color);

why is this a class at all? why do you pass color into Draw instead of the
constructor? I see that it may be a class because you want that other class to
inherit from it, that said, it seems either the other class should use this one
(rather than inherit) or there should at least be some sort of easier way to
draw, i.e. a static function that creates the object and draws for you.

[Elliot Glaysher, 2011-10-26 21:06:57]

http://codereview.chromium.org/8392011/diff/1013/chrome/browser/ui/gtk/tabs/t...
File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right):

http://codereview.chromium.org/8392011/diff/1013/chrome/browser/ui/gtk/tabs/t...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:462: theme_id =
IDR_THEME_TAB_BACKGROUND;
On 2011/10/25 23:33:30, Evan Stade wrote:
> ternary operator

Done.

http://codereview.chromium.org/8392011/diff/1013/chrome/browser/ui/gtk/tabs/t...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:1110: tab_active_r_width_ =
rb.GetBitmapNamed(IDR_TAB_ACTIVE_RIGHT)->width();
On 2011/10/25 23:33:30, Evan Stade wrote:
> the active and inactive widths are different?

Done.

http://codereview.chromium.org/8392011/diff/1013/chrome/browser/ui/gtk/tabs/t...
File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.h (right):

http://codereview.chromium.org/8392011/diff/1013/chrome/browser/ui/gtk/tabs/t...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.h:141: void Paint(GtkWidget* widget,
cairo_t* cr);
On 2011/10/25 23:33:30, Evan Stade wrote:
> docs

Done.

http://codereview.chromium.org/8392011/diff/1013/ui/gfx/pango_util.cc
File ui/gfx/pango_util.cc (right):

http://codereview.chromium.org/8392011/diff/1013/ui/gfx/pango_util.cc#newcode270
ui/gfx/pango_util.cc:270: if (fade_width > (bounds_.width() / 2)) {
On 2011/10/25 23:33:30, Evan Stade wrote:
> remove excess parens

I believe we should make order of operations explicit.

http://codereview.chromium.org/8392011/diff/1013/ui/gfx/pango_util.h
File ui/gfx/pango_util.h (right):

http://codereview.chromium.org/8392011/diff/1013/ui/gfx/pango_util.h#newcode36
ui/gfx/pango_util.h:36: class UI_EXPORT PangoDrawString {
On 2011/10/25 23:33:30, Evan Stade wrote:
> seems weird to do
> 
> PangoDrawString draw(...);
> draw.Draw(color);

First, I agree this is weird.

The class is split at which drawing interface is used. This class all uses cairo
drawing and takes no gfx::Canvas objects, while the subclass specifically takes
a skia canvas and its methods use the additional mixed mode drawing.

I've uploaded a different version with one exported method for clients to use,
at the price of some code duplication.

> why do you pass color into Draw instead of the constructor?

Because in pango the the text (along with most other properties) is a property
of the PangoLayout while the color is part of the cairo pattern.

[Evan Stade, 2011-10-26 21:46:05]

lgtm

http://codereview.chromium.org/8392011/diff/1013/ui/gfx/pango_util.cc
File ui/gfx/pango_util.cc (right):

http://codereview.chromium.org/8392011/diff/1013/ui/gfx/pango_util.cc#newcode270
ui/gfx/pango_util.cc:270: if (fade_width > (bounds_.width() / 2)) {
On 2011/10/26 21:06:57, Elliot Glaysher wrote:
> On 2011/10/25 23:33:30, Evan Stade wrote:
> > remove excess parens
> 
> I believe we should make order of operations explicit.

the C++ style guide doesn't touch on this, but the js style guide does: "Use
[parentheses] sparingly and in general only where required by the syntax and
semantics."

I think that the problem with adding too many parens (besides clutter) is
consistency. You don't have parens on line 265 -- why not? Same for line 274.
You assume the reader has some knowledge of the order of operations, where do
you draw the line?

http://codereview.chromium.org/8392011/diff/6001/chrome/browser/ui/gtk/tabs/t...
File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.h (right):

http://codereview.chromium.org/8392011/diff/6001/chrome/browser/ui/gtk/tabs/t...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.h:350: static int
tab_active_l_width_;
I'm not sure if you saw my previous comment about making these gfx::Size and
adding docs? Also I had wondered aloud whether active_l and inactive_l were
different, not whether active_l and active_r were different (although l and r
being the same is another cool thing). Is it possible to simplify all these
types to something like

static gfx::Size tab_edge_size_;

http://codereview.chromium.org/8392011/diff/6001/ui/gfx/pango_util.cc
File ui/gfx/pango_util.cc (right):

http://codereview.chromium.org/8392011/diff/6001/ui/gfx/pango_util.cc#newcode149
ui/gfx/pango_util.cc:149: 
^H

[Elliot Glaysher, 2011-10-26 23:23:52]

http://codereview.chromium.org/8392011/diff/6001/chrome/browser/ui/gtk/tabs/t...
File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.h (right):

http://codereview.chromium.org/8392011/diff/6001/chrome/browser/ui/gtk/tabs/t...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.h:350: static int
tab_active_l_width_;
On 2011/10/26 21:46:05, Evan Stade wrote:
> I'm not sure if you saw my previous comment about making these gfx::Size and
> adding docs? Also I had wondered aloud whether active_l and inactive_l were
> different, not whether active_l and active_r were different (although l and r
> being the same is another cool thing). Is it possible to simplify all these
> types to something like
> 
> static gfx::Size tab_edge_size_;

gfx::Size would make a static initializer, no?

[commit-bot: I haz the power, 2011-10-26 23:24:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/8392011/9013

[commit-bot: I haz the power, 2011-10-27 00:43:31]

Can't apply patch for file chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc
Hunk #12 FAILED at 432.
Hunk #18 FAILED at 794.
Hunk #19 FAILED at 850.
Hunk #20 succeeded at 1073 (offset -1 lines).
Hunk #21 succeeded at 1112 (offset -1 lines).
3 out of 21 hunks FAILED -- saving rejects to file
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc.rej

[commit-bot: I haz the power, 2011-10-27 00:57:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/8392011/13001

[commit-bot: I haz the power, 2011-10-27 02:10:04]

Change committed as 107514

[dpapad, 2011-10-28 16:56:52]

http://codereview.chromium.org/8392011/diff/13001/chrome/browser/ui/gtk/tabs/...
File chrome/browser/ui/gtk/tabs/dragged_view_gtk.cc (right):

http://codereview.chromium.org/8392011/diff/13001/chrome/browser/ui/gtk/tabs/...
chrome/browser/ui/gtk/tabs/dragged_view_gtk.cc:439: int widget_width) {
Drive by comment. |widget_width| argument can now be removed, since |widget|
itself is passed as an arg.