Initial gtk3 support

Initial gtk3 support

Currently we're sticking with deprecated apis to make it easier to support both gtk2 and gtk3. If Chrome ever drops gtk2 support, it might be a good idea to do a complete port.

BUG=132847
R=erg@chromium.org,estade@chromium.org

Committed: https://crrev.com/c694003c15dc6af21b453febd1463c5394c98c9e
Cr-Commit-Position: refs/heads/master@{#348339}

[Elliot Glaysher, 2015-07-20 19:31:40]

So in general, the changes look OK, though, as you mention, this is incomplete.

That said, please don't change libgtk2ui. Please make a parallel libgtk3ui so
that we can support both, and change a
ChromeBrowserMainExtraPartsAura::PreEarlyInitialization() so that you dlopen()
between the two libraries.

https://codereview.chromium.org/1234223005/diff/20001/build/linux/system.gyp
File build/linux/system.gyp (left):

https://codereview.chromium.org/1234223005/diff/20001/build/linux/system.gyp#...
build/linux/system.gyp:116: '<!@(<(pkg-config) --cflags gdk-2.0)',
Please don't do this. We need to support both GTK 2 and 3 in the same build.
Instead make a 'gdk2' and 'gdk3' targets, have libgtk2ui depend on gdk2 (&etc)
and have libgtk3ui depend on gdk3 (&etc).

https://codereview.chromium.org/1234223005/diff/20001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (left):

https://codereview.chromium.org/1234223005/diff/20001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:146:
views::BlueButton::kViewClassName;
So while I personally don't like this feature, this was a compromise with the
designers; our "BlueButton" is a special "call to action" element with an accent
blue color to have it visually stand out. Even though it's not what GTK usually
does, having a button with a background set to the GTK default color was how we
compromised here, and has to be replicated.

https://codereview.chromium.org/1234223005/diff/20001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc (right):

https://codereview.chromium.org/1234223005/diff/20001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc:497: }
Why was the thunk moved here? If you left it as a static member you wouldn't
have to move this.

https://codereview.chromium.org/1234223005/diff/20001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1234223005/diff/20001/content/browser/browser...
content/browser/browser_main_loop.cc:237: LOG(ERROR) << log_domain << ": " <<
message;
What messages were added here which tripped the DFATAL?

[knthzh, 2015-07-21 15:48:28]

https://codereview.chromium.org/1234223005/diff/20001/build/linux/system.gyp
File build/linux/system.gyp (left):

https://codereview.chromium.org/1234223005/diff/20001/build/linux/system.gyp#...
build/linux/system.gyp:116: '<!@(<(pkg-config) --cflags gdk-2.0)',
On 2015/07/20 19:31:40, Elliot Glaysher wrote:
> Please don't do this. We need to support both GTK 2 and 3 in the same build.
> Instead make a 'gdk2' and 'gdk3' targets, have libgtk2ui depend on gdk2 (&etc)
> and have libgtk3ui depend on gdk3 (&etc).

I can't. I'm too terrified of triggering another eight-hour rebuild to touch the
build system.

https://codereview.chromium.org/1234223005/diff/20001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (left):

https://codereview.chromium.org/1234223005/diff/20001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:146:
views::BlueButton::kViewClassName;
On 2015/07/20 19:31:40, Elliot Glaysher wrote:
> So while I personally don't like this feature, this was a compromise with the
> designers; our "BlueButton" is a special "call to action" element with an
accent
> blue color to have it visually stand out. Even though it's not what GTK
usually
> does, having a button with a background set to the GTK default color was how
we
> compromised here, and has to be replicated.

Okay. Where are the BlueButtons on the Chrome UI?

https://codereview.chromium.org/1234223005/diff/20001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc (right):

https://codereview.chromium.org/1234223005/diff/20001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc:497: }
On 2015/07/20 19:31:40, Elliot Glaysher wrote:
> Why was the thunk moved here? If you left it as a static member you wouldn't
> have to move this.

I think I just wanted to remove it from the public header interface. Should I
put it back?

https://codereview.chromium.org/1234223005/diff/20001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1234223005/diff/20001/content/browser/browser...
content/browser/browser_main_loop.cc:237: LOG(ERROR) << log_domain << ": " <<
message;
On 2015/07/20 19:31:40, Elliot Glaysher wrote:
> What messages were added here which tripped the DFATAL?

Whoops, didn't know git-cl doesn't preserve commit messages.

So, GtkPrintUnixDialog hasn't been ported properly to gtk3. It still uses the
deprecated property GtkButton:xalign, which means that calling
gtk_print_unix_dialog_new triggers a warning message, which results in those
crashes/aborts that I mentioned earlier.

I could strstr for "deprecated", but I don't know if cherry picking specific
non-fatal warning messages is a good idea. It just feels too much like an
unexpected gtk3 upgrade could cause completely avoidable aborts.

[Elliot Glaysher, 2015-07-28 17:54:36]

(Thought I hit send. Ooops.)

https://codereview.chromium.org/1234223005/diff/20001/build/linux/system.gyp
File build/linux/system.gyp (left):

https://codereview.chromium.org/1234223005/diff/20001/build/linux/system.gyp#...
build/linux/system.gyp:116: '<!@(<(pkg-config) --cflags gdk-2.0)',
On 2015/07/21 15:48:27, knthzh wrote:
> On 2015/07/20 19:31:40, Elliot Glaysher wrote:
> > Please don't do this. We need to support both GTK 2 and 3 in the same build.
> > Instead make a 'gdk2' and 'gdk3' targets, have libgtk2ui depend on gdk2
(&etc)
> > and have libgtk3ui depend on gdk3 (&etc).
> 
> I can't. I'm too terrified of triggering another eight-hour rebuild to touch
the
> build system.

(Just to be clear, I'm fine with you having this hack in while you're developing
the rest of the theme code, but this can not land as is; we need to be able to
continue to support gtk2 for the foreseeable future.)

[knthzh, 2015-07-28 18:59:00]

> (Just to be clear, I'm fine with you having this hack in while you're
developing
> the rest of the theme code, but this can not land as is; we need to be able to
> continue to support gtk2 for the foreseeable future.)

Ideally I'd like to just make it a build time option and pray that it won't need
too many ifdefs.

Speaking of themes, I came up with a good solution: Remove all GTK theme code in
favor of sourcing a community maintained script that detects the current DE and
converts its theme to a crx. Something like:


FILE* theme = popen("chrome-theme", "r");
if (theme) {
    LoadThemeFrom(theme);
    pclose(theme);
}
else {
    LoadDefaultTheme();
}


...which would basically make this patch meaningless, I guess. Oh well.

What are your thoughts? Should I bring it up on chromium-dev?

[Elliot Glaysher, 2015-07-28 19:36:28]

On 2015/07/28 18:59:00, knthzh wrote:
> > (Just to be clear, I'm fine with you having this hack in while you're
> developing
> > the rest of the theme code, but this can not land as is; we need to be able
to
> > continue to support gtk2 for the foreseeable future.)
> 
> Ideally I'd like to just make it a build time option and pray that it won't
need
> too many ifdefs.

Build options will probably not be accepted.

We have a giant build farm which builds chromium in a variety of different build
configurations. Adding more configurations isn't done lightly, especially for
something like the Linux port which doesn't have many users. (We are already
running into resource pressures as is.) However, having it part of the same
build wouldn't impose that cost. Likewise, we don't want to ship multiple
configurations of the Google Chrome deb; it's bad enough from a user perspective
that we already have 64/32 bit builds. Compilation flags don't actually fix
that.

Besides, I'm not convinced that the differences would be small--a large chunk of
gtk2_ui.cc is dealing with theme images.

> 
> Speaking of themes, I came up with a good solution: Remove all GTK theme code
in
> favor of sourcing a community maintained script that detects the current DE
and
> converts its theme to a crx. Something like:
> 
> 
> FILE* theme = popen("chrome-theme", "r");
> if (theme) {
>     LoadThemeFrom(theme);
>     pclose(theme);
> }
> else {
>     LoadDefaultTheme();
> }
> 
> 
> ...which would basically make this patch meaningless, I guess. Oh well.
> 
> What are your thoughts? Should I bring it up on chromium-dev?

Several things will be lost; there's no support in CRX files for changing:
- icons (so no more system icons)
- colors of text in fields; so we'll break the high contrast theme which is part
of the reason why any desktop integration exists in the first place.
- theming of things that aren't the main browser ui (bubbles, etc.)

In addition:
- This would add blocking latency to a third party program or script at a time
that we're already trying to speed chrome back up to what it used to be.
- (As written above, at least) This would move theme unpacking into the blocking
ui loop.

[knthzh, 2015-07-28 20:18:00]

> Build options will probably not be accepted.
> 
> We have a giant build farm which builds chromium in a variety of different
build
> configurations. Adding more configurations isn't done lightly, especially for
> something like the Linux port which doesn't have many users. (We are already
> running into resource pressures as is.) However, having it part of the same
> build wouldn't impose that cost. Likewise, we don't want to ship multiple
> configurations of the Google Chrome deb; it's bad enough from a user
perspective
> that we already have 64/32 bit builds. Compilation flags don't actually fix
> that.
Okay. In that case, libgtkui will be built twice, once for gtk2 linkage and once
for gtk3 linkage. That, or just keep the official build system at gtk2, and let
the distros choose whether or not to patch system.gyp for their builds.

> Besides, I'm not convinced that the differences would be small--a large chunk
of
> gtk2_ui.cc is dealing with theme images.
I'll report back if the differences end up being too cumbersome.

> Several things will be lost; there's no support in CRX files for changing:
> - icons (so no more system icons)
> - colors of text in fields; so we'll break the high contrast theme which is
part
> of the reason why any desktop integration exists in the first place.
> - theming of things that aren't the main browser ui (bubbles, etc.)
Oh geez. Okay, back to square one.

[knthzh, 2015-08-03 18:19:15]

Patchset #3 (id:40001) has been deleted

[knthzh, 2015-08-04 22:56:20]

Patchset #7 (id:140001) has been deleted

[knthzh, 2015-08-04 22:56:25]

Patchset #6 (id:120001) has been deleted

[knthzh, 2015-08-05 05:02:31]

It works on both gtk2 and gtk3 now, although these patches didn't turn out as
well as I would have liked. I honestly don't know about patchset 6. It was
supposed to make the gtk2/gtk3 transitions smoother, but it looks to be a lost
cause. Might have to stick with the deprecated APIs. Is -Werror part of Chrome's
release policy?

ChromeGtkFrame's existence is pretty questionable. It's better to pull colors
from existing widgets, especially since Adwaita on gtk3 doesn't supply
properties for Chrome (this was the actual reason for the broken themes). Is
there a reason for it, or can I remove it?

[Elliot Glaysher, 2015-08-06 18:03:07]

On 2015/08/05 05:02:31, knthzh wrote:
> It works on both gtk2 and gtk3 now, although these patches didn't turn out as
> well as I would have liked. I honestly don't know about patchset 6. It was
> supposed to make the gtk2/gtk3 transitions smoother, but it looks to be a lost
> cause. Might have to stick with the deprecated APIs. Is -Werror part of
Chrome's
> release policy?

"-Wno-deprecated-declarations" scoped to the local build targets should be fine.
("-Werror" won't be accepted; error suppressions should be as small and as local
as possible.)

> ChromeGtkFrame's existence is pretty questionable. It's better to pull colors
> from existing widgets, especially since Adwaita on gtk3 doesn't supply
> properties for Chrome (this was the actual reason for the broken themes). Is
> there a reason for it, or can I remove it?

Sadly, experimentally, it is not. IIRC, at least with gtk2 themes, people didn't
set the colors for GtkWindow to sane things. Hence adding an explicit class for
them to target if they wanted to opt in. (Which a surprising number of themes
did.) If there's a way to get the window border colors in gtk3, feel free to use
that.

(I continue to suspect that you are making things way too difficult on yourself
by trying to share theming code between gtk2 and gtk3.)

[knthzh, 2015-08-07 19:25:52]

> (I continue to suspect that you are making things way too difficult on
yourself
> by trying to share theming code between gtk2 and gtk3.)
Believe me, separating libgtk2ui and libgtk3ui would be much, much worse.

> "-Wno-deprecated-declarations" scoped to the local build targets should be
fine.
Okay, thanks.

As of patchset 10, libgtk2ui can be built for both gtk2 and gtk3, and works
pretty much the same for both (same colors, same icons, etc). There shouldn't be
any more serious problems left (crashes, white text on white background, etc),
either.

My laptop is too underpowered to play with the build system, so can you send
over a patch that changes system.gyp to compile it twice (once to libgtk2ui.so,
once to libgtk3ui.so) and changes
ChromeBrowserMainExtraPartsAura::PreEarlyInitialization() to dlopen the correct
library at runtime?

I still need to fix theming issues like the missing BlueButton. You were right,
it'll be much easier to test themes if I can compare the two versions side by
side.

[Elliot Glaysher, 2015-08-10 17:36:40]

(FYI, I'm out for most of this week and won't be able to poke at build
system issues until next week.)

On Fri, Aug 7, 2015 at 12:25 PM, <knthzh@gmail.com> wrote:

> (I continue to suspect that you are making things way too difficult on
>>
> yourself
>
>> by trying to share theming code between gtk2 and gtk3.)
>>
> Believe me, separating libgtk2ui and libgtk3ui would be much, much worse.
>
> "-Wno-deprecated-declarations" scoped to the local build targets should be
>>
> fine.
> Okay, thanks.
>
> As of patchset 10, libgtk2ui can be built for both gtk2 and gtk3, and works
> pretty much the same for both (same colors, same icons, etc). There
> shouldn't be
> any more serious problems left (crashes, white text on white background,
> etc),
> either.
>
> My laptop is too underpowered to play with the build system, so can you
> send
> over a patch that changes system.gyp to compile it twice (once to
> libgtk2ui.so,
> once to libgtk3ui.so) and changes
> ChromeBrowserMainExtraPartsAura::PreEarlyInitialization() to dlopen the
> correct
> library at runtime?
>
> I still need to fix theming issues like the missing BlueButton. You were
> right,
> it'll be much easier to test themes if I can compare the two versions side
> by
> side.
>
> https://codereview.chromium.org/1234223005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[knthzh, 2015-08-10 17:53:46]

On 2015/08/10 17:36:40, Elliot Glaysher wrote:
> (FYI, I'm out for most of this week and won't be able to poke at build
> system issues until next week.)

Sure thing, take your time.

[knthzh, 2015-08-21 03:21:47]

https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (left):

https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:144: bool call_to_action =
owning_button_->GetClassName() ==
Okay, I put in some test code that sets state to kPressed if this is true, but I
still can't find a place where this made a difference. I looked through the
interface code, and it seems like all of it (other than the frame, of course)
has been ported to html. Is preserving this still necessary?

https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (left):

https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:503: void
NativeThemeGtk2::PaintComboboxArrow(SkCanvas* canvas,
Same idea, does this still do anything?

[Evan Stade, 2015-08-21 15:24:18]

https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (left):

https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:144: bool call_to_action =
owning_button_->GetClassName() ==
On 2015/08/21 03:21:47, knthzh wrote:
> Okay, I put in some test code that sets state to kPressed if this is true, but
I
> still can't find a place where this made a difference. I looked through the
> interface code, and it seems like all of it (other than the frame, of course)
> has been ported to html. Is preserving this still necessary?

yes, it's still used:

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (left):

https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:503: void
NativeThemeGtk2::PaintComboboxArrow(SkCanvas* canvas,
On 2015/08/21 03:21:47, knthzh wrote:
> Same idea, does this still do anything?

yup:

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

[knthzh, 2015-08-22 19:26:00]

On 2015/08/21 15:24:18, Evan Stade wrote:
>
https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
> File chrome/browser/ui/libgtk2ui/gtk2_border.cc (left):
> 
>
https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
> chrome/browser/ui/libgtk2ui/gtk2_border.cc:144: bool call_to_action =
> owning_button_->GetClassName() ==
> On 2015/08/21 03:21:47, knthzh wrote:
> > Okay, I put in some test code that sets state to kPressed if this is true,
but
> I
> > still can't find a place where this made a difference. I looked through the
> > interface code, and it seems like all of it (other than the frame, of
course)
> > has been ported to html. Is preserving this still necessary?
> 
> yes, it's still used:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
> 
>
https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
> File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (left):
> 
>
https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
> chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:503: void
> NativeThemeGtk2::PaintComboboxArrow(SkCanvas* canvas,
> On 2015/08/21 03:21:47, knthzh wrote:
> > Same idea, does this still do anything?
> 
> yup:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

Okay, thanks. But can you tell me where to find BlueButton and ComboboxArrow on
Chrome's interface? I can't implement correct behavior for the above two
functions if I don't know where to look for changes.

[Evan Stade, 2015-08-22 20:18:30]

On 2015/08/22 19:26:00, knthzh wrote:
> On 2015/08/21 15:24:18, Evan Stade wrote:
> >
>
https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
> > File chrome/browser/ui/libgtk2ui/gtk2_border.cc (left):
> > 
> >
>
https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
> > chrome/browser/ui/libgtk2ui/gtk2_border.cc:144: bool call_to_action =
> > owning_button_->GetClassName() ==
> > On 2015/08/21 03:21:47, knthzh wrote:
> > > Okay, I put in some test code that sets state to kPressed if this is true,
> but
> > I
> > > still can't find a place where this made a difference. I looked through
the
> > > interface code, and it seems like all of it (other than the frame, of
> course)
> > > has been ported to html. Is preserving this still necessary?
> > 
> > yes, it's still used:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
> > 
> >
>
https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
> > File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (left):
> > 
> >
>
https://codereview.chromium.org/1234223005/diff/240001/chrome/browser/ui/libg...
> > chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:503: void
> > NativeThemeGtk2::PaintComboboxArrow(SkCanvas* canvas,
> > On 2015/08/21 03:21:47, knthzh wrote:
> > > Same idea, does this still do anything?
> > 
> > yup:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
> 
> Okay, thanks. But can you tell me where to find BlueButton and ComboboxArrow
on
> Chrome's interface? I can't implement correct behavior for the above two
> functions if I don't know where to look for changes.

Sure. I figured it out and here's how: I followed the above link for BlueButton.
Then I scrolled down to "instantiations". Then I saw it's used in
profile_chooser_view and it's called signin_current_profile_link. So I created a
new "person" that wasn't signed in. Now when I click on the button in the
toolbar that says "Person 1" it shows me a BlueButton. I'll leave the Combobox
one as an exercise for the reader :)

[knthzh, 2015-08-22 20:27:42]

> Sure. I figured it out and here's how: I followed the above link for
BlueButton.
> Then I scrolled down to "instantiations". Then I saw it's used in
> profile_chooser_view and it's called signin_current_profile_link. So I created
a
> new "person" that wasn't signed in. Now when I click on the button in the
> toolbar that says "Person 1" it shows me a BlueButton. I'll leave the Combobox
> one as an exercise for the reader :)
Okay, thank you very much!

[knthzh, 2015-08-22 23:05:50]

Patchset #16 (id:360001) has been deleted

[knthzh, 2015-08-22 23:05:56]

Patchset #15 (id:340001) has been deleted

[knthzh, 2015-08-24 03:39:34]

Patchset #17 (id:420001) has been deleted

[knthzh, 2015-08-24 03:58:18]

https://codereview.chromium.org/1234223005/diff/440001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1234223005/diff/440001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1022: #else
So, I ended up deciding that the best way to render the titlebar in gtk3 is
having it actually render a CSD titlebar, which thankfully exists.
Unfortunately, this means that incognito and regular windows will look exactly
the same. Is this okay?

[Elliot Glaysher, 2015-08-24 17:14:38]

https://codereview.chromium.org/1234223005/diff/440001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1234223005/diff/440001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1022: #else
On 2015/08/24 03:58:18, knthzh wrote:
> So, I ended up deciding that the best way to render the titlebar in gtk3 is
> having it actually render a CSD titlebar, which thankfully exists.
> Unfortunately, this means that incognito and regular windows will look exactly
> the same. Is this okay?

Probably not. You should still be able to do by rendering the CSD titlebar
though. So the gradient_size has a default value: 16. You could just hardcode
that if you don't want to make it customizable. You should be able to just take
your |surface| and put it into a canvas though.

(Also, does this render the CSD style buttons onto the image? If it does, that
might be a dealbreaker because I don't believe we'd be reliably painting our
close/min/max controls entirely over them.)

[knthzh, 2015-08-24 20:04:18]

https://codereview.chromium.org/1234223005/diff/440001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1234223005/diff/440001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1022: #else
On 2015/08/24 17:14:38, Elliot Glaysher wrote:
> Probably not. You should still be able to do by rendering the CSD titlebar
> though. So the gradient_size has a default value: 16. You could just hardcode
> that if you don't want to make it customizable. You should be able to just
take
> your |surface| and put it into a canvas though.
Wait, huh?

> (Also, does this render the CSD style buttons onto the image? If it does, that
> might be a dealbreaker because I don't believe we'd be reliably painting our
> close/min/max controls entirely over them.)
Nope. It draws just the background of a gtk3 titlebar
(https://developer.gnome.org/gtk3/stable/GtkHeaderBar.html, to be exact), which
means that integration should actually be better than the gtk2 version.

[Elliot Glaysher, 2015-08-24 20:14:23]

https://codereview.chromium.org/1234223005/diff/440001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1234223005/diff/440001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1022: #else
On 2015/08/24 20:04:18, knthzh wrote:
> On 2015/08/24 17:14:38, Elliot Glaysher wrote:
> > Probably not. You should still be able to do by rendering the CSD titlebar
> > though. So the gradient_size has a default value: 16. You could just
hardcode
> > that if you don't want to make it customizable. You should be able to just
> take
> > your |surface| and put it into a canvas though.
> Wait, huh?

I should not be allowed to write anything before my second cup of coffee.

So the old code above took a color and made a gradient of it and
HSLShift(that_color, kGtkFrameShift).

So, you are taking a GtkOffscreenWindow and rendering it to a cairo surface. You
should be able to take that surface and perform further modification on it if
you are in incognito mode (take a look at this function's input range of
ThemeProperties::COLOR_FRAME{,_INCOGNITO}{,_INACTIVE}.) In case of _INCOGNITO,
just perform additional tinting on the incoming cairo surface, possibly by
changing it to skia first. Window activeness should also be taken into account
here so that COLOR_FRAME and COLOR_FRAME_INACTIVE look different.

[knthzh, 2015-08-25 18:07:38]

On 2015/08/24 20:14:23, Elliot Glaysher wrote:
> So, you are taking a GtkOffscreenWindow and rendering it to a cairo surface.
You
> should be able to take that surface and perform further modification on it if
> you are in incognito mode (take a look at this function's input range of
> ThemeProperties::COLOR_FRAME{,_INCOGNITO}{,_INACTIVE}.) In case of _INCOGNITO,
> just perform additional tinting on the incoming cairo surface, possibly by
> changing it to skia first. Window activeness should also be taken into account
> here so that COLOR_FRAME and COLOR_FRAME_INACTIVE look different.

Done. Thank god for SkBitmapOperations::CreateHSLShiftedBitmap().


This is now complete and ready for review. There's still some cleanups that can
be done, but those should probably go to another patch.

[Elliot Glaysher, 2015-08-31 22:23:49]

On 2015/08/25 18:07:38, knthzh wrote:
> On 2015/08/24 20:14:23, Elliot Glaysher wrote:
> > So, you are taking a GtkOffscreenWindow and rendering it to a cairo surface.
> You
> > should be able to take that surface and perform further modification on it
if
> > you are in incognito mode (take a look at this function's input range of
> > ThemeProperties::COLOR_FRAME{,_INCOGNITO}{,_INACTIVE}.) In case of
_INCOGNITO,
> > just perform additional tinting on the incoming cairo surface, possibly by
> > changing it to skia first. Window activeness should also be taken into
account
> > here so that COLOR_FRAME and COLOR_FRAME_INACTIVE look different.
> 
> Done. Thank god for SkBitmapOperations::CreateHSLShiftedBitmap().
> 
> 
> This is now complete and ready for review. There's still some cleanups that
can
> be done, but those should probably go to another patch.

(Note: I haven't forgotten about this; I'm just trying to get the build system
patch landed first. Sorry about the wait.)

[Elliot Glaysher, 2015-09-01 22:43:58]

This patch is large enough that I'm going to have to read this several times.
These are some immediate first impressions. I've edited your description to
point to the linux aura gtk3 bug instead of the one you, and am dumping images
related there.

When compiling with 'use_gtk2=false':
- Something is wrong with the gtk frame tinting code here.
- The icons appear to be off-centered.

When compiling with 'use_gtk3=false':
- The new tab button is extremely dark. Is it being tinted correctly?

https://codereview.chromium.org/1234223005/diff/480001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (right):

https://codereview.chromium.org/1234223005/diff/480001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:81: for (int i = 0; i < 2; ++i) {
Probably want to keep the constant.

https://codereview.chromium.org/1234223005/diff/480001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:107: rect.height()));
So what's the motivation for this change here? The previous version appears to
create the 2x scaled version correctly, and this one just scales.

https://codereview.chromium.org/1234223005/diff/480001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1234223005/diff/480001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1135: stock_id,
Does this handle rtl-ness correctly?

https://developer.gnome.org/gtk3/stable/GtkIconTheme.html talks about how stock
ids are made of named theme icons, but
https://developer.gnome.org/gtk3/stable/gtk3-Stock-Items.html says that the
stock macros are deprecated (and that you should be using named icons, which you
are at the top of the file). Do these two things interact correctly for correct
behaviour?

https://codereview.chromium.org/1234223005/diff/480001/content/browser/browse...
File content/browser/browser_main_loop.cc (left):

https://codereview.chromium.org/1234223005/diff/480001/content/browser/browse...
content/browser/browser_main_loop.cc:243: LOG(DFATAL) << log_domain << ": " <<
message;
You'll want to revert back to DFATAL and whitelist the specific error messages
here.

[knthzh, 2015-09-02 05:48:29]

On 2015/09/01 22:43:58, Elliot Glaysher wrote:
> This patch is large enough that I'm going to have to read this several times.
Sorry about that, first time contributing to Chrome.

> When compiling with 'use_gtk2=false':
> - Something is wrong with the gtk frame tinting code here.
> - The icons appear to be off-centered.
> 
> When compiling with 'use_gtk3=false':
> - The new tab button is extremely dark. Is it being tinted correctly?
Which gtk and icon themes are those?

https://codereview.chromium.org/1234223005/diff/480001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (right):

https://codereview.chromium.org/1234223005/diff/480001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:81: for (int i = 0; i < 2; ++i) {
> Probably want to keep the constant.

I did it to match the declaration in gtk2_border.h,
gfx::ImageSkia button_images_[2][views::Button::STATE_COUNT];

How about replacing the 2's with a macro?

https://codereview.chromium.org/1234223005/diff/480001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:107: rect.height()));
> So what's the motivation for this change here? The previous version appears to
> create the 2x scaled version correctly, and this one just scales.

Oops, misunderstood the original code. I definitely broke hidpi borders. But
hidpi icons were already broken, so the Chrome/gtk/hidpi combo wasn't working in
the first place.

For the sake of not making this patch even larger, I'll fix hidpi in a separate
patch after this one gets committed.

https://codereview.chromium.org/1234223005/diff/480001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1234223005/diff/480001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1135: stock_id,
> Does this handle rtl-ness correctly?

I checked it, and surprisingly enough the old behavior was wrong. This actually
fixes it.

Seeing rtl flip all my windows was quite a trippy experience.

https://codereview.chromium.org/1234223005/diff/480001/content/browser/browse...
File content/browser/browser_main_loop.cc (left):

https://codereview.chromium.org/1234223005/diff/480001/content/browser/browse...
content/browser/browser_main_loop.cc:243: LOG(DFATAL) << log_domain << ": " <<
message;
> You'll want to revert back to DFATAL and whitelist the specific error messages
> here.

Why are glib logs being filtered this way? Can I convert log_level to a Chrome
log level instead and remove this whole whitelist?

[Elliot Glaysher, 2015-09-03 19:28:01]

> Which gtk and icon themes are those?

Murrina-Tangoesque (I believe this is an XFCE theme), and Humanity as the icon
theme.

https://codereview.chromium.org/1234223005/diff/480001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (right):

https://codereview.chromium.org/1234223005/diff/480001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:81: for (int i = 0; i < 2; ++i) {
On 2015/09/02 05:48:29, knthzh wrote:
> > Probably want to keep the constant.
> 
> I did it to match the declaration in gtk2_border.h,
> gfx::ImageSkia button_images_[2][views::Button::STATE_COUNT];
> 
> How about replacing the 2's with a macro?

Macros are highly, highly discouraged. (You'll still find some usages of
#define, but it shouldn't be routine.)

https://codereview.chromium.org/1234223005/diff/480001/content/browser/browse...
File content/browser/browser_main_loop.cc (left):

https://codereview.chromium.org/1234223005/diff/480001/content/browser/browse...
content/browser/browser_main_loop.cc:243: LOG(DFATAL) << log_domain << ": " <<
message;
On 2015/09/02 05:48:29, knthzh wrote:
> > You'll want to revert back to DFATAL and whitelist the specific error
messages
> > here.
> 
> Why are glib logs being filtered this way? Can I convert log_level to a Chrome
> log level instead and remove this whole whitelist?

Because of GTK's permissiveness.

GTK has the annoying property that a lot of things that should be fatal errors,
such as array out of bounds errors (!), are treated as non-fatal. If you open
pretty much any other program written with GTK on the console, you will usually
see a complete spew of state invariant errors.

(The correct action is almost always to crash as soon as you know that something
is wrong. Most attempts to recover from a state invariant failure just cause the
failure to migrate somewhere else.)

[knthzh, 2015-09-04 00:15:18]

For future images, can you make them side-by-side screenshots of a vanilla build
and a build with this patch, so it's easier for me to tell what the expected
behavior is?

> - Something is wrong with the gtk frame tinting code here.
What's it supposed to look like?

> - The icons appear to be off-centered.
Chrome's icons are 29x29 and XDG icon themes pretty much come entirely in even
sizes, so I'm guessing this is inevitable. Are the icons in the vanilla build
off-center? I'm seeing that on my side.

> - The new tab button is extremely dark. Is it being tinted correctly?
Is this the Murrina-Tangoesque you're using? It looks like it's a gtk2 theme.
http://gnome-look.org/content/show.php/Murrina-Tangoesque?content=66200

https://codereview.chromium.org/1234223005/diff/480001/content/browser/browse...
File content/browser/browser_main_loop.cc (left):

https://codereview.chromium.org/1234223005/diff/480001/content/browser/browse...
content/browser/browser_main_loop.cc:243: LOG(DFATAL) << log_domain << ": " <<
message;
> GTK has the annoying property that a lot of things that should be fatal
errors,
> such as array out of bounds errors (!), are treated as non-fatal.

Well then.

I'm still skeptical about this, though. I have a bad premonition of a future
gtk3 upgrade doing something that slips through this whitelist and forces
everyone to scramble to get an emergency patch out.

I also personally want to reduce the differences between all the various Linux
configurations (including ChromeOS). I don't think it's fair for the Chrome team
to have to integrate with every single toolkit like this when the Linux
ecosystem as a whole is already such a minority in the first place.

For developers working on the gtk port, there's --g-fatal-warnings. Is that good
enough? https://developer.gnome.org/gtk3/stable/gtk-running.html

[Elliot Glaysher, 2015-09-04 21:56:51]

On 2015/09/04 00:15:18, knthzh wrote:
> For future images, can you make them side-by-side screenshots of a vanilla
build
> and a build with this patch, so it's easier for me to tell what the expected
> behavior is?

I looked for a gtk theme that you're more likely to have and saw that the same
problem happens on Clearlooks. Also showed off the problem with icons.

(Monday is a public holiday; I won't be responding to email then.)

[Elliot Glaysher, 2015-09-04 21:57:10]

On 2015/09/04 21:56:51, Elliot Glaysher wrote:
> On 2015/09/04 00:15:18, knthzh wrote:
> > For future images, can you make them side-by-side screenshots of a vanilla
> build
> > and a build with this patch, so it's easier for me to tell what the expected
> > behavior is?
> 
> I looked for a gtk theme that you're more likely to have and saw that the same
> problem happens on Clearlooks. Also showed off the problem with icons.
> 
> (Monday is a public holiday; I won't be responding to email then.)

(Sorry, I meant to also say that the image is in the bug.)

[knthzh, 2015-09-05 05:24:01]

On 2015/09/04 21:56:51, Elliot Glaysher wrote:
> I looked for a gtk theme that you're more likely to have and saw that the same
> problem happens on Clearlooks.
Fixed in patchset 20.

> Also showed off the problem with icons.
Disabled icon behavior: gtk3 disables icons by setting alpha to 0.5, so I copied
that and used the same code for gtk2. Should I grayscale it for gtk2 instead?

Off center: It's Humanity's 16x16 icons that are off center.

[Elliot Glaysher, 2015-09-08 22:44:37]

On 2015/09/05 05:24:01, knthzh wrote:
> On 2015/09/04 21:56:51, Elliot Glaysher wrote:
> > I looked for a gtk theme that you're more likely to have and saw that the
same
> > problem happens on Clearlooks.
> Fixed in patchset 20.

It's still different. (To be clear, I think what you have here looks nicer.
However it is a difference, and I'm looking for no changes in the interface.) I
put comments where I suspect that the changes are.

> > Also showed off the problem with icons.
> Disabled icon behavior: gtk3 disables icons by setting alpha to 0.5, so I
copied
> that and used the same code for gtk2. Should I grayscale it for gtk2 instead?

Yes please. The gtk2 interface will still be the default for a while after this
lands, so I don't want any regressions which could get this backed out.

> Off center: It's Humanity's 16x16 icons that are off center.

Given that this isn't a problem before this patch, something has changed. See
the last row of the screenshots I did last time. Either we're currently
upscaling (ugh but ok), or centering the GtkImage in the GtkButton. Just port
what we're currently doing forward.

https://codereview.chromium.org/1234223005/diff/500001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1234223005/diff/500001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:853:
GetDefaultTint(ThemeProperties::TINT_FRAME));
You probably want kDefaultFrameShift here.

https://codereview.chromium.org/1234223005/diff/500001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:862:
GetDefaultTint(ThemeProperties::TINT_FRAME_INACTIVE));
You probably want to get the bg[INSENSITIVE] state here instead of bg[SELECTED].

(And use kDefaultFrameShift).

[knthzh, 2015-09-09 03:49:59]

On 2015/09/08 22:44:37, Elliot Glaysher wrote:
> The gtk2 interface will still be the default for a while after this
> lands, so I don't want any regressions which could get this backed out.

Okay. All regressions fixed in patchset 21.

Incidentally, if anything slips past review and only gets caught after this is
committed, throw me an email - I'll fix it.

[Elliot Glaysher, 2015-09-09 17:58:10]

Visually, everything looks fine now on gtk2 and anything in gtk3 can be fixed up
post patch since it isn't on by default, and I only have two final comments.

Thank you for putting up with my Extreme Nitpicking.

https://codereview.chromium.org/1234223005/diff/480001/content/browser/browse...
File content/browser/browser_main_loop.cc (left):

https://codereview.chromium.org/1234223005/diff/480001/content/browser/browse...
content/browser/browser_main_loop.cc:243: LOG(DFATAL) << log_domain << ": " <<
message;
On 2015/09/04 00:15:18, knthzh wrote:
> I'm still skeptical about this, though. I have a bad premonition of a future
> gtk3 upgrade doing something that slips through this whitelist and forces
> everyone to scramble to get an emergency patch out.

Yes. That is the story of this list.

> I also personally want to reduce the differences between all the various Linux
> configurations (including ChromeOS). I don't think it's fair for the Chrome
team
> to have to integrate with every single toolkit like this when the Linux
> ecosystem as a whole is already such a minority in the first place.

I agree with you that it isn't fair. That the entire situation on the Linux
desktop is ridiculous. I too wanted to minimize the differences between the
various chrome ports. Seeing that porting the full gtk2 chrome to gtk3 was
entirely infeasible was the main catalyst for why I jumped on replacing as much
of the linux UI with the cross-platform views UI, leaving only small stubs here
and there to deal with the things that have to stay gtk.

After spending several days looking at the linking issues, it'll be a long time
before we can really separate out this module and dlopen() different versions of
it (if ever). gtk2 vs gtk3 will probably have to stay a compile time flag.
Realistically, the various distros will probably ship gtk3 compiled chromium
first before Google ships a Chrome build with it switched on.

It would be nice to get rid of gtk entirely, but that's not realistic. There's
native dialogs, IMEs and printing support. And there's the integration with
theming which was our big goodwill gesture to the Linux community, which was why
we went to such trouble to get this working when we stopped using gtk2 natively
for all the UI.

> For developers working on the gtk port, there's --g-fatal-warnings. Is that
good
> enough? https://developer.gnome.org/gtk3/stable/gtk-running.html

There are two problems with g-fatal-warnings. We want to ignore some of these.
(GTK icon errors failures are actually surprisingly common. So are Ubuntu Menu
Proxy errors.) And then there's the issue that it isn't on by default and I
don't think the average chrome engineer will remember to pass --g-fatal-warnings
on top of all the other things they have to remember to do.

https://codereview.chromium.org/1234223005/diff/520001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right):

https://codereview.chromium.org/1234223005/diff/520001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:26:
GdkColorToSkColor(gtk_rc_get_style(gtkwidget)->fg[GTK_STATE_ ## gtkstate])
Prefer inline functions to #define.

[knthzh, 2015-09-09 19:40:29]

On 2015/09/09 17:58:10, Elliot Glaysher wrote:
> Visually, everything looks fine now on gtk2 and anything in gtk3 can be fixed
up
> post patch since it isn't on by default, and I only have two final comments.

Fixed.

> Thank you for putting up with my Extreme Nitpicking.

And thanks for putting up with my patch bomb, heh.

[Elliot Glaysher, 2015-09-09 19:54:42]

lgtm

(I'll leave clicking the CQ button to you.)

[knthzh, 2015-09-09 20:06:02]

The CQ bit was checked by knthzh@gmail.com

[commit-bot: I haz the power, 2015-09-09 20:06:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1234223005/560001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1234223005/560001

[commit-bot: I haz the power, 2015-09-09 20:16:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-09 20:16:49]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Elliot Glaysher, 2015-09-09 20:31:06]

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

[Elliot Glaysher, 2015-09-09 20:31:07]

jam: OWNERS stamp on content/browser/browser_main_loop.cc

[jam, 2015-09-10 19:22:11]

lgtm

[knthzh, 2015-09-10 21:11:55]

The CQ bit was checked by knthzh@gmail.com

[commit-bot: I haz the power, 2015-09-10 21:13:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1234223005/560001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1234223005/560001

[commit-bot: I haz the power, 2015-09-10 22:55:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-10 22:55:38]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[knthzh, 2015-09-10 23:38:17]

The CQ bit was checked by knthzh@gmail.com

[commit-bot: I haz the power, 2015-09-10 23:38:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1234223005/560001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1234223005/560001

[commit-bot: I haz the power, 2015-09-11 01:02:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-11 01:02:23]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[knthzh, 2015-09-11 04:44:12]

The CQ bit was checked by knthzh@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-11 04:44:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1234223005/600001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1234223005/600001

[commit-bot: I haz the power, 2015-09-11 05:40:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-11 05:40:27]

Dry run: This issue passed the CQ dry run.

[knthzh, 2015-09-11 05:42:10]

The CQ bit was checked by knthzh@gmail.com

[knthzh, 2015-09-11 05:42:10]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1234223005/#ps600001
(title: "Silence gtk memory leak")

[commit-bot: I haz the power, 2015-09-11 05:42:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1234223005/600001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1234223005/600001

[commit-bot: I haz the power, 2015-09-11 05:47:56]

Committed patchset #25 (id:600001)

[commit-bot: I haz the power, 2015-09-11 05:48:33]

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

[commit-bot: I haz the power, 2015-09-23 12:19:16]

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