Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2628043002:
Gtk3: Render a GtkHeaderBar as the background of the tab strip (Closed)
|
Created:
3 years, 11 months ago by Tom (Use chromium acct) Modified:
3 years, 11 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGtk3: Render a GtkHeaderBar as the background of the tab strip
This CL changes the Gtk3 theme to render a GtkHeaderBar background behind the
tab strip. This should give a more native look-and-feel for Linux UI. A
demonstration of the change can be seen in:
https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c42
BUG=132847, 590301
R=pkasting@chromium.org,erg@chromium.org,sky@chromium.org
Review-Url: https://codereview.chromium.org/2628043002
Cr-Commit-Position: refs/heads/master@{#445818}
Committed: https://chromium.googlesource.com/chromium/src/+/2bbf4303d8af1e49e748bb2d7e131d114daa7354
Patch Set 1 #
Total comments: 18
Patch Set 2 : sky@'s comments #Patch Set 3 : Move set_* into OnPaint #
Total comments: 2
Patch Set 4 : Handle incognito #
Total comments: 2
Patch Set 5 : . #Patch Set 6 : Handle custom frames #
Total comments: 2
Patch Set 7 : . #Patch Set 8 : Rebase #Patch Set 9 : Guard kFrameTopArea with #if defined(OS_LINUX) && !defined(OS_CHROMEOS) #
Dependent Patchsets: Messages
Total messages: 46 (22 generated)
Description was changed from ========== Gtk3: Render a GtkHeaderBar as the background of the tab strip BUG=462689 R=pkasting@chromium.org,erg@chromium.org,sky@chromium.org ========== to ========== Gtk3: Render a GtkHeaderBar as the background of the tab strip BUG=132847 R=pkasting@chromium.org,erg@chromium.org,sky@chromium.org ==========
Description was changed from ========== Gtk3: Render a GtkHeaderBar as the background of the tab strip BUG=132847 R=pkasting@chromium.org,erg@chromium.org,sky@chromium.org ========== to ========== Gtk3: Render a GtkHeaderBar as the background of the tab strip This CL changes the Gtk3 theme to render a GtkHeaderBar background behind the tab strip. This should give a more native look-and-feel for Linux UI. A demonstration of the change can be seen in: https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c42 BUG=132847 R=pkasting@chromium.org,erg@chromium.org,sky@chromium.org ==========
reviewers PTAL erg: chrome/browser/ui/libgtkui/gtk_ui.cc chrome/browser/ui/libgtkui/gtk_util.h chrome/browser/ui/libgtkui/native_theme_gtk3.cc chrome/browser/ui/libgtkui/native_theme_gtk3.h pkasting: chrome/browser/ui/views/frame/opaque_browser_frame_view.cc ui/native_theme/native_theme.h ui/native_theme/native_theme_base.cc ui/native_theme/native_theme_base.h sky: ui/views/window/custom_frame_view.cc ui/views/window/frame_background.cc ui/views/window/frame_background.h https://codereview.chromium.org/2628043002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right): https://codereview.chromium.org/2628043002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:511: top_area_height = GetBoundsForTabStrip(browser_view()->tabstrip()).bottom(); pkasting@: The "kHeight = 64" was messing this up. The top area height should be 44 when restored and 29 when maximized, but this code was causing it to always be 64, which was causing the GtkHeaderBar to render stretched out. Is the fix just a matter of hard-coding these new constants? https://codereview.chromium.org/2628043002/diff/1/ui/native_theme/native_theme.h File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/2628043002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme.h:115: SkColor default_background_color; // The theme can ignore this color. pkasting@: I was thinking of alternative ways to do this and one thing that I thought of was to get rid of this constant, have FrameBackground always render the solid color, change NativeThemeBase::PaintFrameTopArea be a no-op, and have the NativeThemeGtk3::PaintFrameTopArea override do the actual drawing. The disadvantage to this would be redundantly drawing the top area in FrameBackground on Linux. wdyt?
I just want to verify: this doesn't cause any problems with chrome browser themed painting, right? You're just changing the painting from a solid color to a pixmap drawn by GTK, and there isn't a difference in the output of the frame when a chrome theme is installed because chrome themes aren't going through NativeThemeGtk3?
https://codereview.chromium.org/2628043002/diff/1/ui/views/window/custom_fram... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/2628043002/diff/1/ui/views/window/custom_fram... ui/views/window/custom_frame_view.cc:223: Any reason not to call set_is_active here? Calling set_is_active here seems less error prone. https://codereview.chromium.org/2628043002/diff/1/ui/views/window/frame_backg... File ui/views/window/frame_background.cc (right): https://codereview.chromium.org/2628043002/diff/1/ui/views/window/frame_backg... ui/views/window/frame_background.cc:129: if (top_area_height_ > theme_frame_bottom) { no {} https://codereview.chromium.org/2628043002/diff/1/ui/views/window/frame_backg... File ui/views/window/frame_background.h (right): https://codereview.chromium.org/2628043002/diff/1/ui/views/window/frame_backg... ui/views/window/frame_background.h:84: bool is_active_; = true here or initialize in member initialize list
On 2017/01/11 20:52:32, Elliot Glaysher wrote: > I just want to verify: this doesn't cause any problems with chrome browser > themed painting, right? You're just changing the painting from a solid color to > a pixmap drawn by GTK, and there isn't a difference in the output of the frame > when a chrome theme is installed because chrome themes aren't going through > NativeThemeGtk3? I tested browser-themed painting and it works fine. I think it just draws on top of the GtkHeaderBar https://codereview.chromium.org/2628043002/diff/1/ui/views/window/custom_fram... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/2628043002/diff/1/ui/views/window/custom_fram... ui/views/window/custom_frame_view.cc:223: On 2017/01/11 21:54:52, sky wrote: > Any reason not to call set_is_active here? Calling set_is_active here seems less > error prone. I added the set_is_active right after set_frame_color, which is what used to handle active/nonactive colors. Maybe move all of the set_* here? https://codereview.chromium.org/2628043002/diff/1/ui/views/window/frame_backg... File ui/views/window/frame_background.cc (right): https://codereview.chromium.org/2628043002/diff/1/ui/views/window/frame_backg... ui/views/window/frame_background.cc:129: if (top_area_height_ > theme_frame_bottom) { On 2017/01/11 21:54:52, sky wrote: > no {} Done. https://codereview.chromium.org/2628043002/diff/1/ui/views/window/frame_backg... File ui/views/window/frame_background.h (right): https://codereview.chromium.org/2628043002/diff/1/ui/views/window/frame_backg... ui/views/window/frame_background.h:84: bool is_active_; On 2017/01/11 21:54:52, sky wrote: > = true here or initialize in member initialize list Done.
chrome/browser/ui/libgtkui/ lgtm
https://codereview.chromium.org/2628043002/diff/1/ui/views/window/custom_fram... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/2628043002/diff/1/ui/views/window/custom_fram... ui/views/window/custom_frame_view.cc:223: On 2017/01/11 22:44:48, Tom Anderson wrote: > On 2017/01/11 21:54:52, sky wrote: > > Any reason not to call set_is_active here? Calling set_is_active here seems > less > > error prone. > > I added the set_is_active right after set_frame_color, which is what used to > handle active/nonactive colors. Maybe move all of the set_* here? SGTM
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/2628043002/diff/1/ui/views/window/custom_fram... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/2628043002/diff/1/ui/views/window/custom_fram... ui/views/window/custom_frame_view.cc:223: On 2017/01/12 00:58:14, sky wrote: > On 2017/01/11 22:44:48, Tom Anderson wrote: > > On 2017/01/11 21:54:52, sky wrote: > > > Any reason not to call set_is_active here? Calling set_is_active here seems > > less > > > error prone. > > > > I added the set_is_active right after set_frame_color, which is what used to > > handle active/nonactive colors. Maybe move all of the set_* here? > > SGTM Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
LGTM
https://codereview.chromium.org/2628043002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right): https://codereview.chromium.org/2628043002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:511: top_area_height = GetBoundsForTabStrip(browser_view()->tabstrip()).bottom(); On 2017/01/11 20:07:27, Tom Anderson wrote: > pkasting@: The "kHeight = 64" was messing this up. The top area height should > be 44 when restored and 29 when maximized, but this code was causing it to > always be 64, which was causing the GtkHeaderBar to render stretched out. Is > the fix just a matter of hard-coding these new constants? 44 and 29 are what the GetBoundsForTabStrip() call will end up returning, right? I don't think we want to hardcode constants like those. It's not those specific values you want, it's "always use the height of the tabstrip bottom". In other words, I think you basically want what my TODO above suggests: to just remove kHeight. Looking at how this value will be used, I think it's safe to go ahead and do this. Effectively, you'll want to do: int top_area_height = frame_image.height(); // Returns 0 if isNull() ...and then leave the old body of this conditional. What you'll want to check is what happens in windows with no tabstrip. Reading the above, I think this will return 0 then, and I'm not sure whether that's right. It depends how we draw things afterwards. https://codereview.chromium.org/2628043002/diff/1/ui/native_theme/native_theme.h File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/2628043002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme.h:115: SkColor default_background_color; // The theme can ignore this color. On 2017/01/11 20:07:27, Tom Anderson wrote: > pkasting@: I was thinking of alternative ways to do this and one thing that I > thought of was to get rid of this constant, have FrameBackground always render > the solid color, change NativeThemeBase::PaintFrameTopArea be a no-op, and have > the NativeThemeGtk3::PaintFrameTopArea override do the actual drawing. The > disadvantage to this would be redundantly drawing the top area in > FrameBackground on Linux. wdyt? I think if I were going to change this, it would be to try to avoid routing through NativeTheme at all. It feels weird somehow that we're calling this a "theme part". Maybe that's just because of my Windows-centric view of the world, where this isn't a theme part. I was imagining FrameBackground::PaintFrameTopArea() doing this painting directly (via ifdefs), or directly calling code in platform-specific files that would do this, rather than indirecting through NativeTheme. But maybe we can't easily do the right thing on Linux through that route. If we need to go through NativeTheme to begin with, then I think the existing way you've done it is OK. I wouldn't make the change you propose. Separately, I find the comment "The theme can ignore this color" confusing. NativeTheme doesn't ignore this color, and it's not really clear which "theme" you're saying can ignore this and why. https://codereview.chromium.org/2628043002/diff/40001/ui/views/window/frame_b... File ui/views/window/frame_background.h (right): https://codereview.chromium.org/2628043002/diff/40001/ui/views/window/frame_b... ui/views/window/frame_background.h:32: // Sets whether the frame to be drawn should have focus. Nit: This sounds as if the frame will be given focus. Maybe "Distinguishes between active (foreground) and inactive (background) window frame styles", or something.
d'oh! I forgot about incognito. Fixed in latest patch set https://codereview.chromium.org/2628043002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right): https://codereview.chromium.org/2628043002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:511: top_area_height = GetBoundsForTabStrip(browser_view()->tabstrip()).bottom(); On 2017/01/12 18:53:52, Peter Kasting wrote: > On 2017/01/11 20:07:27, Tom Anderson wrote: > > pkasting@: The "kHeight = 64" was messing this up. The top area height > should > > be 44 when restored and 29 when maximized, but this code was causing it to > > always be 64, which was causing the GtkHeaderBar to render stretched out. Is > > the fix just a matter of hard-coding these new constants? > > 44 and 29 are what the GetBoundsForTabStrip() call will end up returning, right? > Yes > I don't think we want to hardcode constants like those. It's not those specific > values you want, it's "always use the height of the tabstrip bottom". In other > words, I think you basically want what my TODO above suggests: to just remove > kHeight. > > Looking at how this value will be used, I think it's safe to go ahead and do > this. > > Effectively, you'll want to do: > > int top_area_height = frame_image.height(); // Returns 0 if isNull() > > ...and then leave the old body of this conditional. > Done > What you'll want to check is what happens in windows with no tabstrip. Reading > the above, I think this will return 0 then, and I'm not sure whether that's > right. It depends how we draw things afterwards. I tested with a fullscreen window and this function never gets called. https://codereview.chromium.org/2628043002/diff/1/ui/native_theme/native_theme.h File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/2628043002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme.h:115: SkColor default_background_color; // The theme can ignore this color. On 2017/01/12 18:53:53, Peter Kasting wrote: > On 2017/01/11 20:07:27, Tom Anderson wrote: > > pkasting@: I was thinking of alternative ways to do this and one thing that I > > thought of was to get rid of this constant, have FrameBackground always render > > the solid color, change NativeThemeBase::PaintFrameTopArea be a no-op, and > have > > the NativeThemeGtk3::PaintFrameTopArea override do the actual drawing. The > > disadvantage to this would be redundantly drawing the top area in > > FrameBackground on Linux. wdyt? > > I think if I were going to change this, it would be to try to avoid routing > through NativeTheme at all. It feels weird somehow that we're calling this a > "theme part". Maybe that's just because of my Windows-centric view of the > world, where this isn't a theme part. I was imagining > FrameBackground::PaintFrameTopArea() doing this painting directly (via ifdefs), > or directly calling code in platform-specific files that would do this, rather > than indirecting through NativeTheme. But maybe we can't easily do the right > thing on Linux through that route. > > If we need to go through NativeTheme to begin with, then I think the existing > way you've done it is OK. I wouldn't make the change you propose. > > Separately, I find the comment "The theme can ignore this color" confusing. > NativeTheme doesn't ignore this color, and it's not really clear which "theme" > you're saying can ignore this and why. Changed the comment, hopefully this one's better https://codereview.chromium.org/2628043002/diff/40001/ui/views/window/frame_b... File ui/views/window/frame_background.h (right): https://codereview.chromium.org/2628043002/diff/40001/ui/views/window/frame_b... ui/views/window/frame_background.h:32: // Sets whether the frame to be drawn should have focus. On 2017/01/12 18:53:53, Peter Kasting wrote: > Nit: This sounds as if the frame will be given focus. Maybe "Distinguishes > between active (foreground) and inactive (background) window frame styles", or > something. Done.
Description was changed from ========== Gtk3: Render a GtkHeaderBar as the background of the tab strip This CL changes the Gtk3 theme to render a GtkHeaderBar background behind the tab strip. This should give a more native look-and-feel for Linux UI. A demonstration of the change can be seen in: https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c42 BUG=132847 R=pkasting@chromium.org,erg@chromium.org,sky@chromium.org ========== to ========== Gtk3: Render a GtkHeaderBar as the background of the tab strip This CL changes the Gtk3 theme to render a GtkHeaderBar background behind the tab strip. This should give a more native look-and-feel for Linux UI. A demonstration of the change can be seen in: https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c42 BUG=132847,590301 R=pkasting@chromium.org,erg@chromium.org,sky@chromium.org ==========
https://codereview.chromium.org/2628043002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right): https://codereview.chromium.org/2628043002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:511: top_area_height = GetBoundsForTabStrip(browser_view()->tabstrip()).bottom(); On 2017/01/12 21:51:09, Tom Anderson wrote: > On 2017/01/12 18:53:52, Peter Kasting wrote: > > What you'll want to check is what happens in windows with no tabstrip. > Reading > > the above, I think this will return 0 then, and I'm not sure whether that's > > right. It depends how we draw things afterwards. > > I tested with a fullscreen window and this function never gets called. Fullscreen wouldn't be the thing to check; you'd want a case where this gets called with IsTabStripVisible() false, so a window without a tabstrip. An app window ought to work for this, I think? https://codereview.chromium.org/2628043002/diff/60001/ui/native_theme/native_... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/2628043002/diff/60001/ui/native_theme/native_... ui/native_theme/native_theme.h:115: // (background) window frame styles Nit: Trailing period.
https://codereview.chromium.org/2628043002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right): https://codereview.chromium.org/2628043002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:511: top_area_height = GetBoundsForTabStrip(browser_view()->tabstrip()).bottom(); On 2017/01/12 22:02:56, Peter Kasting wrote: > On 2017/01/12 21:51:09, Tom Anderson wrote: > > On 2017/01/12 18:53:52, Peter Kasting wrote: > > > What you'll want to check is what happens in windows with no tabstrip. > > Reading > > > the above, I think this will return 0 then, and I'm not sure whether that's > > > right. It depends how we draw things afterwards. > > > > I tested with a fullscreen window and this function never gets called. > > Fullscreen wouldn't be the thing to check; you'd want a case where this gets > called with IsTabStripVisible() false, so a window without a tabstrip. An app > window ought to work for this, I think? Hm.. I tried with many types of windows and this still doesn't get called :S I tried with: * Chrome Remote Desktop window * gnubbyd window * Wrench menu * Context menus * Tooltips * Chrome task manager * JS alerts https://codereview.chromium.org/2628043002/diff/60001/ui/native_theme/native_... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/2628043002/diff/60001/ui/native_theme/native_... ui/native_theme/native_theme.h:115: // (background) window frame styles On 2017/01/12 22:02:56, Peter Kasting wrote: > Nit: Trailing period. Done.
https://codereview.chromium.org/2628043002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right): https://codereview.chromium.org/2628043002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:511: top_area_height = GetBoundsForTabStrip(browser_view()->tabstrip()).bottom(); On 2017/01/12 22:54:13, Tom Anderson wrote: > On 2017/01/12 22:02:56, Peter Kasting wrote: > > On 2017/01/12 21:51:09, Tom Anderson wrote: > > > On 2017/01/12 18:53:52, Peter Kasting wrote: > > > > What you'll want to check is what happens in windows with no tabstrip. > > > Reading > > > > the above, I think this will return 0 then, and I'm not sure whether > that's > > > > right. It depends how we draw things afterwards. > > > > > > I tested with a fullscreen window and this function never gets called. > > > > Fullscreen wouldn't be the thing to check; you'd want a case where this gets > > called with IsTabStripVisible() false, so a window without a tabstrip. An app > > window ought to work for this, I think? > > Hm.. I tried with many types of windows and this still doesn't get called :S > I tried with: > * Chrome Remote Desktop window > * gnubbyd window > * Wrench menu > * Context menus > * Tooltips > * Chrome task manager > * JS alerts Things like menus, tooltips, and alerts wouldn't get a frame, so you'd be looking at app windows mostly; I dunno whether the remote desktop/gnubbyd cases would fit this. I'd do app>More Tools>Add To Desktop and make sure "open as window" is checked. Make sure to test in an environment where the opaque frame is the native frame; I think for example Win 7 Aero Basic (non-glass) is this case.
d'oh! I also forgot about "use system titlebar and borders" in chrome:settings. Added a case for this in the latest patch set. erg@ can you re-review libgtkui? https://codereview.chromium.org/2628043002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right): https://codereview.chromium.org/2628043002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:511: top_area_height = GetBoundsForTabStrip(browser_view()->tabstrip()).bottom(); On 2017/01/12 23:14:04, Peter Kasting wrote: > On 2017/01/12 22:54:13, Tom Anderson wrote: > > On 2017/01/12 22:02:56, Peter Kasting wrote: > > > On 2017/01/12 21:51:09, Tom Anderson wrote: > > > > On 2017/01/12 18:53:52, Peter Kasting wrote: > > > > > What you'll want to check is what happens in windows with no tabstrip. > > > > Reading > > > > > the above, I think this will return 0 then, and I'm not sure whether > > that's > > > > > right. It depends how we draw things afterwards. > > > > > > > > I tested with a fullscreen window and this function never gets called. > > > > > > Fullscreen wouldn't be the thing to check; you'd want a case where this gets > > > called with IsTabStripVisible() false, so a window without a tabstrip. An > app > > > window ought to work for this, I think? > > > > Hm.. I tried with many types of windows and this still doesn't get called :S > > I tried with: > > * Chrome Remote Desktop window > > * gnubbyd window > > * Wrench menu > > * Context menus > > * Tooltips > > * Chrome task manager > > * JS alerts > > Things like menus, tooltips, and alerts wouldn't get a frame, so you'd be > looking at app windows mostly; I dunno whether the remote desktop/gnubbyd cases > would fit this. I'd do app>More Tools>Add To Desktop and make sure "open as > window" is checked. Make sure to test in an environment where the opaque frame > is the native frame; I think for example Win 7 Aero Basic (non-glass) is this > case. Ok that seemed to work. It does return 0, but nothing is visually wrong
lgtm
lgtm lgtm
LGTM https://codereview.chromium.org/2628043002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right): https://codereview.chromium.org/2628043002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:513: int top_area_height = frame_image.height(); // Returns 0 if isNull() Nit: Trailing period if you're keeping this comment.
https://codereview.chromium.org/2628043002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right): https://codereview.chromium.org/2628043002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:513: int top_area_height = frame_image.height(); // Returns 0 if isNull() On 2017/01/17 21:05:16, Peter Kasting wrote: > Nit: Trailing period if you're keeping this comment. Done.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, erg@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2628043002/#ps120001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Now that kFrameTopArea is Linux-only, and FrameBackground::PaintFrameTopArea() has #ifdefs, is there any way to avoid touching the NativeTheme stuff at all and just have PaintFrameTopArea() make some kind of direct call like: PaintGtkTopArea(... all the params you were stuffing into a struct ...); And have that symbol defined in a Linux-specific source file? This would be simpler than touching NativeTheme at all. If not, then still LGTM
On 2017/01/24 19:35:12, Peter Kasting wrote: > Now that kFrameTopArea is Linux-only, and FrameBackground::PaintFrameTopArea() > has #ifdefs, is there any way to avoid touching the NativeTheme stuff at all and > just have PaintFrameTopArea() make some kind of direct call like: > > PaintGtkTopArea(... all the params you were stuffing into a struct ...); > > And have that symbol defined in a Linux-specific source file? This would be > simpler than touching NativeTheme at all. > > If not, then still LGTM I think that's possible, but Linux users have the option to disable the Gtk theme in chrome:settings. So we need to know at runtime whether the NativeTheme is an instance of NativeThemeBase or NativeThemeGtk3
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2628043002/#ps160001 (title: "Guard kFrameTopArea with #if defined(OS_LINUX) && !defined(OS_CHROMEOS)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485290480531970,
"parent_rev": "86f05da3c1fe8ca3d242e96c8717dcdb69950f6c", "commit_rev":
"2bbf4303d8af1e49e748bb2d7e131d114daa7354"}
Message was sent while issue was closed.
Description was changed from ========== Gtk3: Render a GtkHeaderBar as the background of the tab strip This CL changes the Gtk3 theme to render a GtkHeaderBar background behind the tab strip. This should give a more native look-and-feel for Linux UI. A demonstration of the change can be seen in: https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c42 BUG=132847,590301 R=pkasting@chromium.org,erg@chromium.org,sky@chromium.org ========== to ========== Gtk3: Render a GtkHeaderBar as the background of the tab strip This CL changes the Gtk3 theme to render a GtkHeaderBar background behind the tab strip. This should give a more native look-and-feel for Linux UI. A demonstration of the change can be seen in: https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c42 BUG=132847,590301 R=pkasting@chromium.org,erg@chromium.org,sky@chromium.org Review-Url: https://codereview.chromium.org/2628043002 Cr-Commit-Position: refs/heads/master@{#445818} Committed: https://chromium.googlesource.com/chromium/src/+/2bbf4303d8af1e49e748bb2d7e13... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/2bbf4303d8af1e49e748bb2d7e13... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
