Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2186813002:
Linux: Support the --class argument (Reland) (Closed)
|
Created:
4 years, 4 months ago by Tom (Use chromium acct) Modified:
4 years, 4 months ago CC:
chromium-reviews, tfarina, jennb, jianli, Dmitry Titov, dcheng, sky, Elliot Glaysher, benwells Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinux: Support the --class argument (Reland)
It appears we lost support for --class when we stopped using gtk for our
windowing. This CL adds that feature back.
BUG=118613
Committed: https://crrev.com/b4a38edc75149162fcbdf01e42d5411cf8e72520
Committed: https://crrev.com/12d8758651726f96ba4d34d63fb9ce39623ca215
Cr-Original-Commit-Position: refs/heads/master@{#408709}
Cr-Commit-Position: refs/heads/master@{#408756}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add unit test #
Total comments: 4
Patch Set 3 : Add internal namespace #
Total comments: 15
Patch Set 4 : Refactor #
Total comments: 6
Patch Set 5 : Revert gtk2_util.cc #
Messages
Total messages: 54 (27 generated)
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...
thomasanderson@google.com changed reviewers: + erg@chromium.org, sky@chromium.org
erg needed for: chrome/browser/shell_integration_linux.cc chrome/browser/shell_integration_linux.h sky: chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc chrome/browser/ui/views/frame/desktop_browser_frame_auralinux.cc chrome/browser/ui/views/panels/panel_view.cc
Description was changed from ========== Linux: Support the --class argument BUG=118613 ========== to ========== Linux: Support the --class argument It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 ==========
Description was changed from ========== Linux: Support the --class argument It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 ========== to ========== Linux: Support the --class argument It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2186813002/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/2186813002/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_linux.cc:211: std::string GetDesktopBaseName() { Please add comment. https://codereview.chromium.org/2186813002/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration_linux.h (right): https://codereview.chromium.org/2186813002/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_linux.h:34: std::string GetProgramClassName(); How about test coverage of these functions?
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/2186813002/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/2186813002/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_linux.cc:211: std::string GetDesktopBaseName() { On 2016/07/27 15:17:00, sky wrote: > Please add comment. Done. https://codereview.chromium.org/2186813002/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration_linux.h (right): https://codereview.chromium.org/2186813002/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_linux.h:34: std::string GetProgramClassName(); On 2016/07/27 15:17:00, sky wrote: > How about test coverage of these functions? Added ShellIntegrationTest.WmClass https://codereview.chromium.org/2186813002/diff/20001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux.h (right): https://codereview.chromium.org/2186813002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.h:41: std::string GetProgramClassClass(); It feels messy having this be overloaded, but I need the arguments for the unit test.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2186813002/diff/20001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux.h (right): https://codereview.chromium.org/2186813002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.h:41: std::string GetProgramClassClass(); On 2016/07/27 19:33:09, Tom Anderson wrote: > It feels messy having this be overloaded, but I need the arguments for the unit > test. Can't the test replace the command line?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2186813002/diff/20001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux.h (right): https://codereview.chromium.org/2186813002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.h:41: std::string GetProgramClassClass(); On 2016/07/27 19:58:04, sky wrote: > On 2016/07/27 19:33:09, Tom Anderson wrote: > > It feels messy having this be overloaded, but I need the arguments for the > unit > > test. > > Can't the test replace the command line? Yes, but what about the desktop file name? The values are hard-coded here https://cs.chromium.org/chromium/src/chrome/browser/shell_integration_linux.c...
https://codereview.chromium.org/2186813002/diff/20001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux.h (right): https://codereview.chromium.org/2186813002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.h:41: std::string GetProgramClassClass(); On 2016/07/27 22:04:34, Tom Anderson wrote: > On 2016/07/27 19:58:04, sky wrote: > > On 2016/07/27 19:33:09, Tom Anderson wrote: > > > It feels messy having this be overloaded, but I need the arguments for the > > unit > > > test. > > > > Can't the test replace the command line? > > Yes, but what about the desktop file name? The values are hard-coded here > https://cs.chromium.org/chromium/src/chrome/browser/shell_integration_linux.c... Agreed, for that you need to pass in the desktop_file_name.
On 2016/07/27 22:40:21, sky wrote: > https://codereview.chromium.org/2186813002/diff/20001/chrome/browser/shell_in... > File chrome/browser/shell_integration_linux.h (right): > > https://codereview.chromium.org/2186813002/diff/20001/chrome/browser/shell_in... > chrome/browser/shell_integration_linux.h:41: std::string GetProgramClassClass(); > On 2016/07/27 22:04:34, Tom Anderson wrote: > > On 2016/07/27 19:58:04, sky wrote: > > > On 2016/07/27 19:33:09, Tom Anderson wrote: > > > > It feels messy having this be overloaded, but I need the arguments for the > > > unit > > > > test. > > > > > > Can't the test replace the command line? > > > > Yes, but what about the desktop file name? The values are hard-coded here > > > https://cs.chromium.org/chromium/src/chrome/browser/shell_integration_linux.c... > > Agreed, for that you need to pass in the desktop_file_name. Ok. In that case, should we have two overloaded functions: one with no arguments and one that takes desktop_file_name, or just the latter and callers will have to supply the file name?
What would help is if you made it clear the one that takes the desktop name is internal and not meant for public consumption. Some people use an internal namespace for that, or rename it to make it more obvious. I'm out until Monday, thestig could likely pick this up. -Scott On Wed, Jul 27, 2016 at 4:42 PM, <thomasanderson@google.com> wrote: > On 2016/07/27 22:40:21, sky wrote: >> > https://codereview.chromium.org/2186813002/diff/20001/chrome/browser/shell_in... >> File chrome/browser/shell_integration_linux.h (right): >> >> > https://codereview.chromium.org/2186813002/diff/20001/chrome/browser/shell_in... >> chrome/browser/shell_integration_linux.h:41: std::string > GetProgramClassClass(); >> On 2016/07/27 22:04:34, Tom Anderson wrote: >> > On 2016/07/27 19:58:04, sky wrote: >> > > On 2016/07/27 19:33:09, Tom Anderson wrote: >> > > > It feels messy having this be overloaded, but I need the arguments >> > > > for > the >> > > unit >> > > > test. >> > > >> > > Can't the test replace the command line? >> > >> > Yes, but what about the desktop file name? The values are hard-coded >> > here >> > >> > https://cs.chromium.org/chromium/src/chrome/browser/shell_integration_linux.c... >> >> Agreed, for that you need to pass in the desktop_file_name. > > Ok. In that case, should we have two overloaded functions: one with no > arguments > and one that takes desktop_file_name, or just the latter and callers will > have > to supply the file name? > > https://codereview.chromium.org/2186813002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
thomasanderson@google.com changed reviewers: + thestig@chromium.org
thomasanderson@google.com changed reviewers: - erg@chromium.org, sky@chromium.org
+thestig On 2016/07/28 14:52:49, sky wrote: > What would help is if you made it clear the one that takes the desktop > name is internal and not meant for public consumption. Some people use > an internal namespace for that, or rename it to make it more obvious. > > I'm out until Monday, thestig could likely pick this up. > > -Scott > done
Looking good. https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:217: std::size_t last = desktop_file_name.find(".desktop"); BTW, std::string::find() in the worst case has a much worse run time vs base::EndsWith(). That's what we really want to check here, right? https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:597: if (command_line.HasSwitch(switches::kUserDataDir)) { I wouldn't bother with this. I'd just call command_line.GetSwitchValueNative() and handle the non-empty case instead. base::CommandLine will happily take --class <-- no value! --> and then you'd end up with "google-chrome()". https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:611: std::string class_class = Call this after handling the switch? https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux.h (right): https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.h:142: std::string GetProgramClassName(const base::CommandLine& command_line, Mention these are exposed only for testing? https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux_unittest.cc (right): https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_linux_unittest.cc:659: EXPECT_EQ(internal::GetProgramClassName(command_line, "foo.desktop"), "foo"); EXPECT_EQ(expected_val, actual_val) https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/gtk2_util.cc (right): https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/gtk2_util.cc:59: // TODO(erg): This method was copied out of shell_integration_linux.cc. Because Any chance we can deal with this TODO here? I'm annoyed the two copies of GetDesktopName() has gotten out of sync. At the minimunm, we should add comments in both places to say they should stay in sync. https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/desktop_browser_frame_auralinux.cc (right): https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/desktop_browser_frame_auralinux.cc:40: // This window is a hosted app or v1 packaged app. benwells: Please help refresh my memory, are either of these two types of apps still around? https://codereview.chromium.org/2186813002/diff/40001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2186813002/diff/40001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:1141: // Sets WM_CLASS with this value. You may want to mention this is similar to -class in X applications. The comments you add here eventually shows up on http://peter.sh/experiments/chromium-command-line-switches/ BTW.
benwells@chromium.org changed reviewers: + benwells@chromium.org
https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/desktop_browser_frame_auralinux.cc (right): https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/desktop_browser_frame_auralinux.cc:40: // This window is a hosted app or v1 packaged app. On 2016/07/28 21:28:46, Lei Zhang wrote: > benwells: Please help refresh my memory, are either of these two types of apps > still around? Yes, they're still around.
https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:217: std::size_t last = desktop_file_name.find(".desktop"); On 2016/07/28 21:28:46, Lei Zhang wrote: > BTW, std::string::find() in the worst case has a much worse run time vs > base::EndsWith(). That's what we really want to check here, right? Done. https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:597: if (command_line.HasSwitch(switches::kUserDataDir)) { On 2016/07/28 21:28:46, Lei Zhang wrote: > I wouldn't bother with this. I'd just call command_line.GetSwitchValueNative() > and handle the non-empty case instead. > > base::CommandLine will happily take --class <-- no value! --> and then you'd > end up with "google-chrome()". Done. https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:611: std::string class_class = On 2016/07/28 21:28:46, Lei Zhang wrote: > Call this after handling the switch? Done. https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux.h (right): https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.h:142: std::string GetProgramClassName(const base::CommandLine& command_line, On 2016/07/28 21:28:46, Lei Zhang wrote: > Mention these are exposed only for testing? Done. https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux_unittest.cc (right): https://codereview.chromium.org/2186813002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_linux_unittest.cc:659: EXPECT_EQ(internal::GetProgramClassName(command_line, "foo.desktop"), "foo"); On 2016/07/28 21:28:46, Lei Zhang wrote: > EXPECT_EQ(expected_val, actual_val) Done. https://codereview.chromium.org/2186813002/diff/40001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2186813002/diff/40001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:1141: // Sets WM_CLASS with this value. On 2016/07/28 21:28:46, Lei Zhang wrote: > You may want to mention this is similar to -class in X applications. The > comments you add here eventually shows up on > http://peter.sh/experiments/chromium-command-line-switches/ BTW. done That's much better than grepping through *_switches.cc :)
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: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2186813002/diff/60001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/2186813002/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:613: if (command_line.HasSwitch(switches::kWmClass)) Do we really want to return "" if the user passed in "--class" ? https://codereview.chromium.org/2186813002/diff/60001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2186813002/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:1141: // The same as the --class argument in X applications. Overrides the WM_CLASS Just -class, as seen in "man xterm"
https://codereview.chromium.org/2186813002/diff/60001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/2186813002/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:613: if (command_line.HasSwitch(switches::kWmClass)) On 2016/07/29 00:41:43, Lei Zhang wrote: > Do we really want to return "" if the user passed in "--class" ? The empty string is a valid class, but if I run gedit, for example, "gedit --class" is an error, "gedit --class=" is a class with the empty string, and '--class=""' is also the empty string. The only inconsistency here is that Chrome will accept "chrome --class" without error. Is there any way to replicate this behavior? https://codereview.chromium.org/2186813002/diff/60001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2186813002/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:1141: // The same as the --class argument in X applications. Overrides the WM_CLASS On 2016/07/29 00:41:43, Lei Zhang wrote: > Just -class, as seen in "man xterm" -class in xterm is different. xterm does not support --class Try "gedit --class=Foo" and then "xprop WM_CLASS" and click on the gedit window to test.
https://codereview.chromium.org/2186813002/diff/60001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/2186813002/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:613: if (command_line.HasSwitch(switches::kWmClass)) On 2016/07/29 18:52:34, Tom Anderson wrote: > On 2016/07/29 00:41:43, Lei Zhang wrote: > > Do we really want to return "" if the user passed in "--class" ? > > The empty string is a valid class, but if I run gedit, for example, "gedit > --class" is an error, "gedit --class=" is a class with the empty string, and > '--class=""' is also the empty string. So it is. Nevermind then.
https://codereview.chromium.org/2186813002/diff/60001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2186813002/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:1141: // The same as the --class argument in X applications. Overrides the WM_CLASS On 2016/07/29 18:52:34, Tom Anderson wrote: > On 2016/07/29 00:41:43, Lei Zhang wrote: > > Just -class, as seen in "man xterm" > > -class in xterm is different. xterm does not support --class > > Try "gedit --class=Foo" and then "xprop WM_CLASS" and click on the gedit window > to test. I thought more X apps supported -class, but it looks more like an xterm-ism. OTOH, --class is probably a GTKism, since konqueror and other KDE apps don't support it. It's fine to leave it as is. Users who need it will figure it out.
The CQ bit was checked by thomasanderson@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Linux: Support the --class argument It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 ========== to ========== Linux: Support the --class argument It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Linux: Support the --class argument It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 ========== to ========== Linux: Support the --class argument It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 Committed: https://crrev.com/b4a38edc75149162fcbdf01e42d5411cf8e72520 Cr-Commit-Position: refs/heads/master@{#408709} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b4a38edc75149162fcbdf01e42d5411cf8e72520 Cr-Commit-Position: refs/heads/master@{#408709}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2193393002/ by pavely@chromium.org. The reason for reverting is: Change breaks compile on Linux x64: https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux....
Message was sent while issue was closed.
Description was changed from ========== Linux: Support the --class argument It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 Committed: https://crrev.com/b4a38edc75149162fcbdf01e42d5411cf8e72520 Cr-Commit-Position: refs/heads/master@{#408709} ========== to ========== Linux: Support the --class argument It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 Committed: https://crrev.com/b4a38edc75149162fcbdf01e42d5411cf8e72520 Cr-Commit-Position: refs/heads/master@{#408709} ==========
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2186813002/#ps80001 (title: "Revert gtk2_util.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Linux: Support the --class argument It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 Committed: https://crrev.com/b4a38edc75149162fcbdf01e42d5411cf8e72520 Cr-Commit-Position: refs/heads/master@{#408709} ========== to ========== Linux: Support the --class argument It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 Committed: https://crrev.com/b4a38edc75149162fcbdf01e42d5411cf8e72520 Cr-Commit-Position: refs/heads/master@{#408709} ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Linux: Support the --class argument It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 Committed: https://crrev.com/b4a38edc75149162fcbdf01e42d5411cf8e72520 Cr-Commit-Position: refs/heads/master@{#408709} ========== to ========== Linux: Support the --class argument It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 Committed: https://crrev.com/b4a38edc75149162fcbdf01e42d5411cf8e72520 Committed: https://crrev.com/12d8758651726f96ba4d34d63fb9ce39623ca215 Cr-Original-Commit-Position: refs/heads/master@{#408709} Cr-Commit-Position: refs/heads/master@{#408756} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/12d8758651726f96ba4d34d63fb9ce39623ca215 Cr-Commit-Position: refs/heads/master@{#408756}
Message was sent while issue was closed.
Patch set 5 is the reland, right? Please say something before relanding so it's more obvious. And why revert gtk2_util.cc? Why not just include the missing header?
Message was sent while issue was closed.
On 2016/07/29 21:24:47, Lei Zhang wrote: > Patch set 5 is the reland, right? Please say something before relanding so it's > more obvious. > Yes, this is the reland. > And why revert gtk2_util.cc? Why not just include the missing header? libgtk2ui.so does not link against the necessary dependencies.
Message was sent while issue was closed.
On 2016/07/29 22:51:05, Tom Anderson wrote: > On 2016/07/29 21:24:47, Lei Zhang wrote: > > And why revert gtk2_util.cc? Why not just include the missing header? > > libgtk2ui.so does not link against the necessary dependencies. But we usually don't do Chrome builds with component=shared_library and it only fails in Chrome branded builds. If you did a Chrome branded component=shared_library build... does it actually break?
Message was sent while issue was closed.
Description was changed from ========== Linux: Support the --class argument It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 Committed: https://crrev.com/b4a38edc75149162fcbdf01e42d5411cf8e72520 Committed: https://crrev.com/12d8758651726f96ba4d34d63fb9ce39623ca215 Cr-Original-Commit-Position: refs/heads/master@{#408709} Cr-Commit-Position: refs/heads/master@{#408756} ========== to ========== Linux: Support the --class argument (Reland) It appears we lost support for --class when we stopped using gtk for our windowing. This CL adds that feature back. BUG=118613 Committed: https://crrev.com/b4a38edc75149162fcbdf01e42d5411cf8e72520 Committed: https://crrev.com/12d8758651726f96ba4d34d63fb9ce39623ca215 Cr-Original-Commit-Position: refs/heads/master@{#408709} Cr-Commit-Position: refs/heads/master@{#408756} ==========
Message was sent while issue was closed.
On 2016/07/29 22:57:10, Lei Zhang wrote: > On 2016/07/29 22:51:05, Tom Anderson wrote: > > On 2016/07/29 21:24:47, Lei Zhang wrote: > > > And why revert gtk2_util.cc? Why not just include the missing header? > > > > libgtk2ui.so does not link against the necessary dependencies. > > But we usually don't do Chrome builds with component=shared_library and it only > fails in Chrome branded builds. > > If you did a Chrome branded component=shared_library build... does it actually > break? Good call New CL is here https://codereview.chromium.org/2195063003/ Tested on component debug build and an official build |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
