Gtk3: Refactor NativeThemeGtk3 to use foreign drawing only

Gtk3: Refactor NativeThemeGtk3 to use foreign drawing only

BUG=132847
Committed: https://crrev.com/bc47ad6fd02f3de36046899ab9f484cf2fbbcf3c
Cr-Commit-Position: refs/heads/master@{#441531}

[Tom (Use chromium acct), 2016-12-20 00:37:34]

thomasanderson@google.com changed reviewers:
+ estade@chromium.org

[Tom (Use chromium acct), 2016-12-20 00:37:35]

estade@ please review

[Evan Stade, 2016-12-20 00:40:29]

estade@chromium.org changed reviewers:
+ erg@chromium.org

[Evan Stade, 2016-12-20 00:40:30]

+erg is a better reviewer for this one

[Evan Stade, 2016-12-20 00:40:37]

Description was changed from

==========
Gtk3: Replace widget code with GtkStyleContext code

BUG=132847
==========

to

==========
Gtk3: Replace widget code with GtkStyleContext code

BUG=132847
==========

[Evan Stade, 2016-12-20 00:40:37]

estade@chromium.org changed reviewers:
- estade@chromium.org

[Tom (Use chromium acct), 2016-12-20 00:51:18]

On 2016/12/20 00:40:30, Evan Stade wrote:
> +erg is a better reviewer for this one

erg is OOO this week.  estade@ do you think you can review?

[Tom (Use chromium acct), 2016-12-21 22:41:24]

Patchset #1 (id:1) has been deleted

[Tom (Use chromium acct), 2016-12-21 22:41:30]

Patchset #1 (id:20001) has been deleted

[Tom (Use chromium acct), 2016-12-21 22:44:09]

thomasanderson@google.com changed reviewers:
+ pkasting@chromium.org
- erg@chromium.org

[Tom (Use chromium acct), 2016-12-21 22:44:10]

pkasting@ Can you review this?  The gtk folks seem to be OOO

[Tom (Use chromium acct), 2016-12-21 22:44:21]

Description was changed from

==========
Gtk3: Replace widget code with GtkStyleContext code

BUG=132847
==========

to

==========
Gtk3: Refactor NativeThemeGtk3 to use foreign drawing

BUG=132847
==========

[Peter Kasting, 2016-12-22 00:39:58]

To be blunt, no.  I could do a style review, but I've literally never worked
with GTK and I think you need someone who has.  I would just wait for erg to get
back in-office.

[Evan Stade, 2016-12-22 01:42:09]

On 2016/12/20 00:51:18, Tom Anderson wrote:
> On 2016/12/20 00:40:30, Evan Stade wrote:
> > +erg is a better reviewer for this one
> 
> erg is OOO this week.  estade@ do you think you can review?

yea, unfortunately most people are OOO and to some extent development on
Chromium comes to a halt until the new year.

[Tom (Use chromium acct), 2016-12-29 22:42:00]

thomasanderson@google.com changed reviewers:
+ erg@chromium.org
- pkasting@chromium.org

[Tom (Use chromium acct), 2016-12-29 22:56:54]

Description was changed from

==========
Gtk3: Refactor NativeThemeGtk3 to use foreign drawing

BUG=132847
==========

to

==========
Gtk3: Refactor NativeThemeGtk3 to use foreign drawing only

BUG=132847
==========

[Elliot Glaysher, 2017-01-03 18:56:54]

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtkui/gtk_util.cc (right):

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtkui/gtk_util.cc:197: GtkStyleContext*
AppendNode(GtkStyleContext* context, std::string css_node) {
Any reason why |css_node| isn't const& ? It doesn't look like it's mutated.

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtkui/gtk_util.cc:370: // TODO(thomasanderson): Refactor
these functions.
What exactly needs refactoring here?

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtkui/native_theme_gtk3.cc (right):

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtkui/native_theme_gtk3.cc:78:
g_type_class_unref(g_type_class_ref(gtk_button_get_type()));
It's been awhile since I did any gobject work, but is this safe to do? Are there
examples elsewhere of doing something like this?

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtkui/native_theme_gtk3.cc:114: return
GetBGColor("GtkMenu.menu");
Previously, it looks like GetBGColor was just fetching a color from the style
context. Now it's doing a render pass behind the back of the caller. Does this
have a negative performance impact?

[Tom (Use chromium acct), 2017-01-04 19:39:58]

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtkui/gtk_util.cc (right):

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtkui/gtk_util.cc:197: GtkStyleContext*
AppendNode(GtkStyleContext* context, std::string css_node) {
On 2017/01/03 18:56:54, Elliot Glaysher wrote:
> Any reason why |css_node| isn't const& ? It doesn't look like it's mutated.

No reason.  Changed to const&

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtkui/gtk_util.cc:370: // TODO(thomasanderson): Refactor
these functions.
On 2017/01/03 18:56:54, Elliot Glaysher wrote:
> What exactly needs refactoring here?

Oh, I forgot to upload the patch set where the TODO is actually done :P
I didn't like the way code was duplicated between GetBGColor and GetBorderColor.

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtkui/native_theme_gtk3.cc (right):

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtkui/native_theme_gtk3.cc:78:
g_type_class_unref(g_type_class_ref(gtk_button_get_type()));
On 2017/01/03 18:56:54, Elliot Glaysher wrote:
> It's been awhile since I did any gobject work, but is this safe to do? Are
there
> examples elsewhere of doing something like this?

They do it in
https://github.com/GNOME/gtk/blob/5ea69a136bd7e4970b3a800390e20314665aaed2/te...

We could get rid of the g_type_class_[un]ref if the compiler just didn't
optimize away the code.  Using (void)gtk_label_get_type() doesn't seem to work. 
The trick to use is
#define UNUSED(x) asm volatile("" ::"r"(x))
Maybe it's worth it getting something like that added to base/macros.h?  or is
this hack ok for now?

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtkui/native_theme_gtk3.cc:114: return
GetBGColor("GtkMenu.menu");
On 2017/01/03 18:56:54, Elliot Glaysher wrote:
> Previously, it looks like GetBGColor was just fetching a color from the style
> context. Now it's doing a render pass behind the back of the caller. Does this
> have a negative performance impact?

Probably..  maybe we could add a cache for LookupGtkThemeColor, and invalidate
it when the theme changes?

[Elliot Glaysher, 2017-01-04 20:18:21]

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtkui/native_theme_gtk3.cc (right):

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtkui/native_theme_gtk3.cc:78:
g_type_class_unref(g_type_class_ref(gtk_button_get_type()));
On 2017/01/04 19:39:58, Tom Anderson wrote:
> On 2017/01/03 18:56:54, Elliot Glaysher wrote:
> > It's been awhile since I did any gobject work, but is this safe to do? Are
> there
> > examples elsewhere of doing something like this?
> 
> They do it in
>
https://github.com/GNOME/gtk/blob/5ea69a136bd7e4970b3a800390e20314665aaed2/te...
> 
> We could get rid of the g_type_class_[un]ref if the compiler just didn't
> optimize away the code.  Using (void)gtk_label_get_type() doesn't seem to
work. 
> The trick to use is
> #define UNUSED(x) asm volatile("" ::"r"(x))
> Maybe it's worth it getting something like that added to base/macros.h?  or is
> this hack ok for now?

I still think this sorta sucks, but if the upstream gtk people are doing it,  I
guess it's fine.

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtkui/native_theme_gtk3.cc:114: return
GetBGColor("GtkMenu.menu");
On 2017/01/04 19:39:58, Tom Anderson wrote:
> On 2017/01/03 18:56:54, Elliot Glaysher wrote:
> > Previously, it looks like GetBGColor was just fetching a color from the
style
> > context. Now it's doing a render pass behind the back of the caller. Does
this
> > have a negative performance impact?
> 
> Probably..  maybe we could add a cache for LookupGtkThemeColor, and invalidate
> it when the theme changes?

That sounds like a good plan.

[Tom (Use chromium acct), 2017-01-04 21:39:09]

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtkui/native_theme_gtk3.cc (right):

https://codereview.chromium.org/2588993002/diff/60001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtkui/native_theme_gtk3.cc:114: return
GetBGColor("GtkMenu.menu");
On 2017/01/04 20:18:21, Elliot Glaysher wrote:
> On 2017/01/04 19:39:58, Tom Anderson wrote:
> > On 2017/01/03 18:56:54, Elliot Glaysher wrote:
> > > Previously, it looks like GetBGColor was just fetching a color from the
> style
> > > context. Now it's doing a render pass behind the back of the caller. Does
> this
> > > have a negative performance impact?
> > 
> > Probably..  maybe we could add a cache for LookupGtkThemeColor, and
invalidate
> > it when the theme changes?
> 
> That sounds like a good plan.

Done.

[Elliot Glaysher, 2017-01-04 21:53:00]

lgtm

[Tom (Use chromium acct), 2017-01-04 23:42:45]

The CQ bit was checked by thomasanderson@google.com

[commit-bot: I haz the power, 2017-01-04 23:43:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 00:39:07]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483573365421650,
"parent_rev": "b6177d9ca825e72f0e9292f7b255711d56cbe562", "commit_rev":
"b615d1c0fbf9f0c9404b392153cc0e6767962182"}

[commit-bot: I haz the power, 2017-01-05 00:39:37]

Description was changed from

==========
Gtk3: Refactor NativeThemeGtk3 to use foreign drawing only

BUG=132847
==========

to

==========
Gtk3: Refactor NativeThemeGtk3 to use foreign drawing only

BUG=132847

Review-Url: https://codereview.chromium.org/2588993002
==========

[commit-bot: I haz the power, 2017-01-05 00:39:38]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2017-01-05 00:44:34]

Description was changed from

==========
Gtk3: Refactor NativeThemeGtk3 to use foreign drawing only

BUG=132847

Review-Url: https://codereview.chromium.org/2588993002
==========

to

==========
Gtk3: Refactor NativeThemeGtk3 to use foreign drawing only

BUG=132847

Committed: https://crrev.com/bc47ad6fd02f3de36046899ab9f484cf2fbbcf3c
Cr-Commit-Position: refs/heads/master@{#441531}
==========

[commit-bot: I haz the power, 2017-01-05 00:44:35]

Patchset 4 (id:??) landed as
https://crrev.com/bc47ad6fd02f3de36046899ab9f484cf2fbbcf3c
Cr-Commit-Position: refs/heads/master@{#441531}