Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2762593002:
Add --headless flag to Windows (Closed)
DescriptionLaunch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916
Add --headless flag to Windows:
Fix browser_tests not compiling properly on windows
Fix headless/BUILD.gn by:
- Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true.
- Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/)
- Disabling breakpad in windows (will fix in a separate CL)
BUG=686608
Review-Url: https://codereview.chromium.org/2762593002
Cr-Original-Original-Commit-Position: refs/heads/master@{#466176}
Committed: https://chromium.googlesource.com/chromium/src/+/e3c745d4b37a659afaa0358c7de7b9bc881decb5
Review-Url: https://codereview.chromium.org/2762593002
Cr-Original-Commit-Position: refs/heads/master@{#466259}
Committed: https://chromium.googlesource.com/chromium/src/+/d238dae172d63175b562f99fc988e47e5213a244
Review-Url: https://codereview.chromium.org/2762593002
Cr-Commit-Position: refs/heads/master@{#471525}
Committed: https://chromium.googlesource.com/chromium/src/+/8303a91150b12ffda29e1d2d5e46dc1371730bfe
Patch Set 1 #Patch Set 2 : Fix Mac build #
Total comments: 4
Patch Set 3 : Fixed export in HeadlessShellMain #Patch Set 4 : Fix headless_browsertests #Patch Set 5 : Updated upstream #Patch Set 6 : reformatted main #Patch Set 7 : fix headless_shell build #
Total comments: 19
Patch Set 8 : Changed headless_lib to be a component library #Patch Set 9 : fixed build error #Patch Set 10 : added //content/public/app:both dep #Patch Set 11 : Componetized headless #Patch Set 12 : updated upstream #Patch Set 13 : Fix switches::KenableCrashReporter reference #Patch Set 14 : Fix RendererCommandPrefixTest #Patch Set 15 : readded headless_lib target #
Total comments: 18
Patch Set 16 : re-add missing HEADLESS_EXPORT and child_lib dep to headles_shell and headless_example #
Total comments: 3
Patch Set 17 : Set default value for Windows options #Patch Set 18 : Reorder Option parameters to avoid error in clang #Patch Set 19 : add crash/content dep #Patch Set 20 : Updated upstream #Patch Set 21 : Updated upstream #Patch Set 22 : add CommandLine::Init so that perf unit tests don't fail #Patch Set 23 : remove crash content app component #Patch Set 24 : Fixed disallowed dependencies for ContentRendererClient in Windows multi-dll #Patch Set 25 : Fix headless_example #Patch Set 26 : Fix breakpad not properly disabled #Patch Set 27 : Fixed Request job export #
Total comments: 10
Patch Set 28 : added extra documentations, nits #
Total comments: 1
Patch Set 29 : Added //net dependency for shell_child_lib to avoid Mac build error... #Patch Set 30 : Removed unnecessary deps from headless_shell_child_lib #Patch Set 31 : Updated upstream #Patch Set 32 : Updated upstream #Patch Set 33 : Working patch on windows #Patch Set 34 : Fixed linux build #Patch Set 35 : Fixed headless_unittests #
Total comments: 6
Patch Set 36 : added TODO #Messages
Total messages: 220 (153 generated)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
PTAL fix browser_tests pending.
dvallet@chromium.org changed reviewers: + alexclarke@chromium.org, skyostil@chromium.org
PTAL fix browser_tests pending I've solved the sandbox error by passing the sandbox_info from ChromeMain
PTAL fix browser_tests pending I've solved the sandbox error by passing the sandbox_info from ChromeMain
PTAL fix browser_tests pending I've solved the sandbox error by passing the sandbox_info from ChromeMain
PTAL fix browser_tests pending I've solved the sandbox error by passing the sandbox_info from ChromeMain
https://codereview.chromium.org/2762593002/diff/20001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2762593002/diff/20001/headless/BUILD.gn#newco... headless/BUILD.gn:483: "//headless:headless_shell_child_lib", Does the linker not complain about this? app/headless_shell.cc etc seem to be defined twice.
https://codereview.chromium.org/2762593002/diff/20001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2762593002/diff/20001/chrome/BUILD.gn#newcode365 chrome/BUILD.gn:365: configs += [ "//headless:headless_implementation" ] This doesn't seem right: the implementation build should only be enabled when building the headless library itself. Are you getting some linker errors without this? Perhaps there's an export macro missing for some function/symbol?
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@chromium.org 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...
Description was changed from ========== Add --headless flag to Windows BUG=686608 ========== to ========== Add --headless flag to Windows Fix browser_tests on windows BUG=686608 ==========
dvallet@chromium.org changed reviewers: + sky@chromium.org, wfh@chromium.org
PTAL @sky: Can you please review changes in chrome/? @wfh: Can you approve moving the sandbox DEPS from headless/lib to headless/? https://codereview.chromium.org/2762593002/diff/20001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2762593002/diff/20001/chrome/BUILD.gn#newcode365 chrome/BUILD.gn:365: configs += [ "//headless:headless_implementation" ] On 2017/03/20 11:53:10, Sami wrote: > This doesn't seem right: the implementation build should only be enabled when > building the headless library itself. > > Are you getting some linker errors without this? Perhaps there's an export macro > missing for some function/symbol? Fixed, the reason is that ChromeMain calls HeadlesShellMain which uses HEADLESS_EXPORT and this required exporting this in this library. But I don't really think that we need to add the export to the main method and so the config is not needed. https://codereview.chromium.org/2762593002/diff/20001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2762593002/diff/20001/headless/BUILD.gn#newco... headless/BUILD.gn:483: "//headless:headless_shell_child_lib", On 2017/03/20 08:41:29, alex clarke wrote: > Does the linker not complain about this? app/headless_shell.cc etc seem to be > defined twice. It doesn't. I was thinking of adding all the dependencies again, but this worked and it seems easier to maintain. Also the sources dependencies are still needed, since if not weird things happen when changing the code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2762593002/diff/120001/chrome/app/chrome_main.cc File chrome/app/chrome_main.cc (right): https://codereview.chromium.org/2762593002/diff/120001/chrome/app/chrome_main... chrome/app/chrome_main.cc:97: base::CommandLine::Init(0, NULL); Why do you have to add this? Aren't we already init'ing the command line? Assuming you need this, NULL -> nullptr and move this to be between 88/89 and line 95 to between 91/92. https://codereview.chromium.org/2762593002/diff/120001/chrome/app/chrome_main... chrome/app/chrome_main.cc:110: return headless::HeadlessShellMain(argc, argv); Can HeadlessShellMain take ContentMainParams so that you don't need all the ifdefs here?
lgtm for moving the dep to sandbox/win/src
https://codereview.chromium.org/2762593002/diff/120001/chrome/app/chrome_main.cc File chrome/app/chrome_main.cc (right): https://codereview.chromium.org/2762593002/diff/120001/chrome/app/chrome_main... chrome/app/chrome_main.cc:110: return headless::HeadlessShellMain(argc, argv); On 2017/03/22 16:05:03, sky wrote: > Can HeadlessShellMain take ContentMainParams so that you don't need all the > ifdefs here? We could add an override for that, but for users who embed the library we should also keep the old entrypoint for compatibility. https://codereview.chromium.org/2762593002/diff/120001/headless/BUILD.gn File headless/BUILD.gn (left): https://codereview.chromium.org/2762593002/diff/120001/headless/BUILD.gn#oldc... headless/BUILD.gn:296: "//content/public/browser", Why remove these? Only content/public/child seems unused. https://codereview.chromium.org/2762593002/diff/120001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2762593002/diff/120001/headless/BUILD.gn#newc... headless/BUILD.gn:435: configs += [ ":headless_implementation" ] I don't think this should be on the testing target -- it's testing the implementation defined by the library itself. https://codereview.chromium.org/2762593002/diff/120001/headless/BUILD.gn#newc... headless/BUILD.gn:547: configs += [ ":headless_implementation" ] Also this seems unnecessary -- the example isn't implementing any headless symbols. https://codereview.chromium.org/2762593002/diff/120001/headless/lib/headless_... File headless/lib/headless_browser_browsertest.cc (left): https://codereview.chromium.org/2762593002/diff/120001/headless/lib/headless_... headless/lib/headless_browser_browsertest.cc:628: #if defined(OS_POSIX) If this doesn't build on Windows, let's just put the whole test inside #ifdef OS_POSIX and remove the MAYBE_ business. https://codereview.chromium.org/2762593002/diff/120001/headless/lib/headless_... File headless/lib/headless_content_main_delegate.cc (right): https://codereview.chromium.org/2762593002/diff/120001/headless/lib/headless_... headless/lib/headless_content_main_delegate.cc:163: DCHECK(!breakpad::IsCrashReporterEnabled()); Breakpad should work on Windows. Maybe we just need to depend on the right libraries to make this build? https://codereview.chromium.org/2762593002/diff/120001/headless/lib/headless_... File headless/lib/headless_devtools_client_browsertest.cc (right): https://codereview.chromium.org/2762593002/diff/120001/headless/lib/headless_... headless/lib/headless_devtools_client_browsertest.cc:721: EXPECT_EQ(base::TERMINATION_STATUS_PROCESS_CRASHED, status); IIRC this is a side-effect of not having breakpad. https://codereview.chromium.org/2762593002/diff/120001/headless/public/headle... File headless/public/headless_browser.cc (right): https://codereview.chromium.org/2762593002/diff/120001/headless/public/headle... headless/public/headless_browser.cc:117: #endif Please add // defined(OS_WIN) for long #ifdef blocks like this.
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Add --headless flag to Windows Fix browser_tests on windows BUG=686608 ========== to ========== Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 ==========
PTAL https://codereview.chromium.org/2762593002/diff/120001/chrome/app/chrome_main.cc File chrome/app/chrome_main.cc (right): https://codereview.chromium.org/2762593002/diff/120001/chrome/app/chrome_main... chrome/app/chrome_main.cc:97: base::CommandLine::Init(0, NULL); On 2017/03/22 at 16:05:03, sky wrote: > Why do you have to add this? Aren't we already init'ing the command line? Assuming you need this, NULL -> nullptr and move this to be between 88/89 and line 95 to between 91/92. Done. You are right, it's not needed. I've moved the initialization of Linux to 91/92 https://codereview.chromium.org/2762593002/diff/120001/chrome/app/chrome_main... chrome/app/chrome_main.cc:110: return headless::HeadlessShellMain(argc, argv); On 2017/03/22 at 19:46:25, Sami wrote: > On 2017/03/22 16:05:03, sky wrote: > > Can HeadlessShellMain take ContentMainParams so that you don't need all the > > ifdefs here? > > We could add an override for that, but for users who embed the library we should also keep the old entrypoint for compatibility. Done https://codereview.chromium.org/2762593002/diff/120001/headless/BUILD.gn File headless/BUILD.gn (left): https://codereview.chromium.org/2762593002/diff/120001/headless/BUILD.gn#oldc... headless/BUILD.gn:296: "//content/public/browser", On 2017/03/22 at 19:46:25, Sami wrote: > Why remove these? Only content/public/child seems unused. The issue is that chrome in windows has restrictions on what dependencies can be pulled for chrome_main and chrome_child DLLs, in this case fro chrome main there cannot be any content/browser dependencies. These are moved to headless_shell_lib and headless_shell_child_lib. The app:both dependency is moved to the actual executables that need both child and main dependencies. https://codereview.chromium.org/2762593002/diff/120001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2762593002/diff/120001/headless/BUILD.gn#newc... headless/BUILD.gn:435: configs += [ ":headless_implementation" ] On 2017/03/22 at 19:46:25, Sami wrote: > I don't think this should be on the testing target -- it's testing the implementation defined by the library itself. Done https://codereview.chromium.org/2762593002/diff/120001/headless/BUILD.gn#newc... headless/BUILD.gn:547: configs += [ ":headless_implementation" ] On 2017/03/22 at 19:46:25, Sami wrote: > Also this seems unnecessary -- the example isn't implementing any headless symbols. Done. https://codereview.chromium.org/2762593002/diff/120001/headless/lib/headless_... File headless/lib/headless_browser_browsertest.cc (left): https://codereview.chromium.org/2762593002/diff/120001/headless/lib/headless_... headless/lib/headless_browser_browsertest.cc:628: #if defined(OS_POSIX) On 2017/03/22 at 19:46:25, Sami wrote: > If this doesn't build on Windows, let's just put the whole test inside #ifdef OS_POSIX and remove the MAYBE_ business. Done https://codereview.chromium.org/2762593002/diff/120001/headless/lib/headless_... File headless/lib/headless_content_main_delegate.cc (right): https://codereview.chromium.org/2762593002/diff/120001/headless/lib/headless_... headless/lib/headless_content_main_delegate.cc:163: DCHECK(!breakpad::IsCrashReporterEnabled()); On 2017/03/22 at 19:46:25, Sami wrote: > Breakpad should work on Windows. Maybe we just need to depend on the right libraries to make this build? There's a bunch of code that needs to be changed (most of the code is hardcoded to use breakpad's linux implementation). I have plans to add crash report support for Mac and Windows in separate CLs. https://codereview.chromium.org/2762593002/diff/120001/headless/lib/headless_... File headless/lib/headless_devtools_client_browsertest.cc (right): https://codereview.chromium.org/2762593002/diff/120001/headless/lib/headless_... headless/lib/headless_devtools_client_browsertest.cc:721: EXPECT_EQ(base::TERMINATION_STATUS_PROCESS_CRASHED, status); On 2017/03/22 at 19:46:25, Sami wrote: > IIRC this is a side-effect of not having breakpad. Ditto, I'll fix this in a separate CL. https://codereview.chromium.org/2762593002/diff/120001/headless/public/headle... File headless/public/headless_browser.cc (right): https://codereview.chromium.org/2762593002/diff/120001/headless/public/headle... headless/public/headless_browser.cc:117: #endif On 2017/03/22 at 19:46:25, Sami wrote: > Please add // defined(OS_WIN) for long #ifdef blocks like this. Done
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
PTAL, some telemetry tests failing and presubmit due to headless_example. Otherwise it looks like it's good to go.
Thanks! Couple of thoughts below. https://codereview.chromium.org/2762593002/diff/280001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2762593002/diff/280001/headless/BUILD.gn#newc... headless/BUILD.gn:191: component("headless") { Can we still call this headless_lib? That's the component that others would want to depend on, while "headless" is just the top-level name for this folder. https://codereview.chromium.org/2762593002/diff/280001/headless/BUILD.gn#newc... headless/BUILD.gn:459: static_library("headless_shell_child_lib") { Is "child" the right name to be using here since code runs in the browser (parent) process? https://codereview.chromium.org/2762593002/diff/280001/headless/BUILD.gn#newc... headless/BUILD.gn:472: "//content/public/child:child", Where's this dependency coming from? I didn't find it under headless/app/. https://codereview.chromium.org/2762593002/diff/280001/headless/app/headless_... File headless/app/headless_shell_main.cc (right): https://codereview.chromium.org/2762593002/diff/280001/headless/app/headless_... headless/app/headless_shell_main.cc:4: #include "build/build_config.h" nit: Blank line after the license block please. https://codereview.chromium.org/2762593002/diff/280001/headless/app/headless_... File headless/app/headless_shell_switches.cc (right): https://codereview.chromium.org/2762593002/diff/280001/headless/app/headless_... headless/app/headless_shell_switches.cc:11: const char kEnableCrashReporter[] = "enable-chrash-reporter"; typo: crash (in the comment too) https://codereview.chromium.org/2762593002/diff/280001/headless/lib/headless_... File headless/lib/headless_content_main_delegate.cc (right): https://codereview.chromium.org/2762593002/diff/280001/headless/lib/headless_... headless/lib/headless_content_main_delegate.cc:5: #include <utility> This should go in a new block after "headless_content_main_delegate.h" https://codereview.chromium.org/2762593002/diff/280001/headless/public/headle... File headless/public/headless_browser.h (right): https://codereview.chromium.org/2762593002/diff/280001/headless/public/headle... headless/public/headless_browser.h:173: HINSTANCE instance; Looks like both of these are uninitialized.
This looks good now, but I have one question. How do we plan on using this for windows?
On 2017/03/27 15:56:19, sky wrote: > This looks good now, but I have one question. How do we plan on using this for > windows? We expect folks to use this in one of two ways: 1. Running a regular Chrome binary with --headless --remote-debugging-port=9222 and driving the browser with Selenium, Node.js, DevTools, etc. 2. Embedding the headless library into their own applications and driving it with the C++ API. Hopefully that answered your question? There's some more information here: https://chromium.googlesource.com/chromium/src/+/lkgr/headless/README.md
Ok, thanks for the pointer. Did headless go through design review or launch process? On Mon, Mar 27, 2017 at 9:22 AM, skyostil@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > On 2017/03/27 15:56:19, sky wrote: >> This looks good now, but I have one question. How do we plan on using this >> for >> windows? > > We expect folks to use this in one of two ways: > > 1. Running a regular Chrome binary with --headless > --remote-debugging-port=9222 > and driving the browser with Selenium, Node.js, DevTools, etc. > > 2. Embedding the headless library into their own applications and driving it > with the C++ API. > > Hopefully that answered your question? There's some more information here: > https://chromium.googlesource.com/chromium/src/+/lkgr/headless/README.md > > https://codereview.chromium.org/2762593002/ -- 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.
On 2017/03/27 at 19:15:43, sky wrote: > Ok, thanks for the pointer. Did headless go through design review or > launch process? > > On Mon, Mar 27, 2017 at 9:22 AM, skyostil@chromium.org via > codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> > wrote: > > On 2017/03/27 15:56:19, sky wrote: > >> This looks good now, but I have one question. How do we plan on using this > >> for > >> windows? > > > > We expect folks to use this in one of two ways: > > > > 1. Running a regular Chrome binary with --headless > > --remote-debugging-port=9222 > > and driving the browser with Selenium, Node.js, DevTools, etc. > > > > 2. Embedding the headless library into their own applications and driving it > > with the C++ API. > > > > Hopefully that answered your question? There's some more information here: > > https://chromium.googlesource.com/chromium/src/+/lkgr/headless/README.md > > > > https://codereview.chromium.org/2762593002/ > > -- > 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. > > Here's the bug created for Linux: https://bugs.chromium.org/p/chromium/issues/detail?id=612904, and the LGTM'd CL: https://codereview.chromium.org/1991953002 which points to this Design Doc: https://docs.google.com/document/d/1aIJUzQr3eougZQp90bp4mqGr5gY6hdUice8UPa-Ys... Sami can answer if this went through design review, I'm not really sure
And specifically if windows was covered in the matrix. On Mon, Mar 27, 2017 at 3:12 PM, <dvallet@chromium.org> wrote: > On 2017/03/27 at 19:15:43, sky wrote: >> Ok, thanks for the pointer. Did headless go through design review or >> launch process? >> >> On Mon, Mar 27, 2017 at 9:22 AM, skyostil@chromium.org via >> codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> >> wrote: >> > On 2017/03/27 15:56:19, sky wrote: >> >> This looks good now, but I have one question. How do we plan on using >> >> this >> >> for >> >> windows? >> > >> > We expect folks to use this in one of two ways: >> > >> > 1. Running a regular Chrome binary with --headless >> > --remote-debugging-port=9222 >> > and driving the browser with Selenium, Node.js, DevTools, etc. >> > >> > 2. Embedding the headless library into their own applications and >> > driving it >> > with the C++ API. >> > >> > Hopefully that answered your question? There's some more information >> > here: >> > https://chromium.googlesource.com/chromium/src/+/lkgr/headless/README.md >> > >> > https://codereview.chromium.org/2762593002/ >> >> -- >> 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. >> >> > Here's the bug created for Linux: > https://bugs.chromium.org/p/chromium/issues/detail?id=612904, and the LGTM'd > CL: > https://codereview.chromium.org/1991953002 > > which points to this Design Doc: > https://docs.google.com/document/d/1aIJUzQr3eougZQp90bp4mqGr5gY6hdUice8UPa-Ys... > > Sami can answer if this went through design review, I'm not really sure > > > https://codereview.chromium.org/2762593002/ -- 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.
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2762593002/diff/280001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2762593002/diff/280001/headless/BUILD.gn#newc... headless/BUILD.gn:191: component("headless") { On 2017/03/27 at 14:07:58, Sami wrote: > Can we still call this headless_lib? That's the component that others would want to depend on, while "headless" is just the top-level name for this folder. From the component doc, it looks like it's preferable that the component name matches the top level folder's name: https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md I've still left the group def as a kind of alias (and also the trybots are configured for headless_lib). I don't mind changing it back, your call. https://codereview.chromium.org/2762593002/diff/280001/headless/BUILD.gn#newc... headless/BUILD.gn:459: static_library("headless_shell_child_lib") { On 2017/03/27 at 14:07:58, Sami wrote: > Is "child" the right name to be using here since code runs in the browser (parent) process? This is only linked in chrome_child DLL, that's why I called it child, it's basically headless minus the forbidden DEPS for child processes in Win https://codereview.chromium.org/2762593002/diff/280001/headless/BUILD.gn#newc... headless/BUILD.gn:472: "//content/public/child:child", On 2017/03/27 at 14:07:58, Sami wrote: > Where's this dependency coming from? I didn't find it under headless/app/. This pools the Ppapi plugging, I found it fitting to add it here since is the child lib. I actually forgot to add a dependency for headless_shell and headless_example, which is needed for non component build, see latest patch https://codereview.chromium.org/2762593002/diff/280001/headless/app/headless_... File headless/app/headless_shell_main.cc (right): https://codereview.chromium.org/2762593002/diff/280001/headless/app/headless_... headless/app/headless_shell_main.cc:4: #include "build/build_config.h" On 2017/03/27 at 14:07:58, Sami wrote: > nit: Blank line after the license block please. Done https://codereview.chromium.org/2762593002/diff/280001/headless/app/headless_... File headless/app/headless_shell_switches.cc (right): https://codereview.chromium.org/2762593002/diff/280001/headless/app/headless_... headless/app/headless_shell_switches.cc:11: const char kEnableCrashReporter[] = "enable-chrash-reporter"; On 2017/03/27 at 14:07:58, Sami wrote: > typo: crash (in the comment too) Oops, good catch :) https://codereview.chromium.org/2762593002/diff/280001/headless/lib/headless_... File headless/lib/headless_content_main_delegate.cc (right): https://codereview.chromium.org/2762593002/diff/280001/headless/lib/headless_... headless/lib/headless_content_main_delegate.cc:5: #include <utility> On 2017/03/27 at 14:07:58, Sami wrote: > This should go in a new block after "headless_content_main_delegate.h" Done. https://codereview.chromium.org/2762593002/diff/280001/headless/public/headle... File headless/public/headless_browser.h (right): https://codereview.chromium.org/2762593002/diff/280001/headless/public/headle... headless/public/headless_browser.h:173: HINSTANCE instance; On 2017/03/27 at 14:07:58, Sami wrote: > Looks like both of these are uninitialized. They are set in HeadlessShellMain and headlessExample, would you rather force a builder for these? HINSTANCE can be unitialized as it defaults to a value that works.
On 2017/03/27 19:15:43, sky wrote: > Ok, thanks for the pointer. Did headless go through design review or > launch process? We didn't go through a formal design review, but rather worked directly with content/, net/ and Blink owners. go/ghost-rider has some more background in case you're curious. Now that we're about to gain runtime headless support for all the major platforms (for headless anyway), it's probably a good idea to kick off a launch review. I'll get that going.
On 2017/03/28 10:02:12, Sami wrote: > On 2017/03/27 19:15:43, sky wrote: > > Ok, thanks for the pointer. Did headless go through design review or > > launch process? > > We didn't go through a formal design review, but rather worked directly with > content/, net/ and Blink owners. go/ghost-rider has some more background in case > you're curious. > > Now that we're about to gain runtime headless support for all the major > platforms (for headless anyway), it's probably a good idea to kick off a launch > review. I'll get that going. Update: here's the bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916
lgtm with the change to Options::Options https://codereview.chromium.org/2762593002/diff/280001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2762593002/diff/280001/headless/BUILD.gn#newc... headless/BUILD.gn:191: component("headless") { On 2017/03/28 02:35:59, dvallet wrote: > On 2017/03/27 at 14:07:58, Sami wrote: > > Can we still call this headless_lib? That's the component that others would > want to depend on, while "headless" is just the top-level name for this folder. > > From the component doc, it looks like it's preferable that the component name > matches the top level folder's name: > https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md > I've still left the group def as a kind of alias (and also the trybots are > configured for headless_lib). > > I don't mind changing it back, your call. Okay, let's follow established convention then. https://codereview.chromium.org/2762593002/diff/280001/headless/BUILD.gn#newc... headless/BUILD.gn:459: static_library("headless_shell_child_lib") { On 2017/03/28 02:35:59, dvallet wrote: > On 2017/03/27 at 14:07:58, Sami wrote: > > Is "child" the right name to be using here since code runs in the browser > (parent) process? > > This is only linked in chrome_child DLL, that's why I called it child, it's > basically headless minus the forbidden DEPS for child processes in Win Ah, a little confusing but okay :) https://codereview.chromium.org/2762593002/diff/280001/headless/public/headle... File headless/public/headless_browser.h (right): https://codereview.chromium.org/2762593002/diff/280001/headless/public/headle... headless/public/headless_browser.h:173: HINSTANCE instance; On 2017/03/28 02:36:00, dvallet wrote: > On 2017/03/27 at 14:07:58, Sami wrote: > > Looks like both of these are uninitialized. > > They are set in HeadlessShellMain and headlessExample, would you rather force a > builder for these? HINSTANCE can be unitialized as it defaults to a value that > works. I meant Options::Options should set them to null like we do with the other fields. Otherwise these would just be garbage pointers. https://codereview.chromium.org/2762593002/diff/300001/headless/lib/browser/h... File headless/lib/browser/headless_browser_impl.cc (right): https://codereview.chromium.org/2762593002/diff/300001/headless/lib/browser/h... headless/lib/browser/headless_browser_impl.cc:208: #endif nit: // !defined(OS_WIN)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2762593002/diff/280001/headless/public/headle... File headless/public/headless_browser.h (right): https://codereview.chromium.org/2762593002/diff/280001/headless/public/headle... headless/public/headless_browser.h:173: HINSTANCE instance; On 2017/03/28 at 11:49:32, Sami wrote: > On 2017/03/28 02:36:00, dvallet wrote: > > On 2017/03/27 at 14:07:58, Sami wrote: > > > Looks like both of these are uninitialized. > > > > They are set in HeadlessShellMain and headlessExample, would you rather force a > > builder for these? HINSTANCE can be unitialized as it defaults to a value that > > works. > > I meant Options::Options should set them to null like we do with the other fields. Otherwise these would just be garbage pointers. Gotcha, Done. https://codereview.chromium.org/2762593002/diff/300001/headless/lib/browser/h... File headless/lib/browser/headless_browser_impl.cc (right): https://codereview.chromium.org/2762593002/diff/300001/headless/lib/browser/h... headless/lib/browser/headless_browser_impl.cc:208: #endif On 2017/03/28 at 11:49:33, Sami wrote: > nit: // !defined(OS_WIN) Done. https://codereview.chromium.org/2762593002/diff/300001/headless/lib/browser/h... headless/lib/browser/headless_browser_impl.cc:208: #endif On 2017/03/28 at 11:49:33, Sami wrote: > nit: // !defined(OS_WIN) Done.
Description was changed from ========== Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 ========== to ========== Launch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916 Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 ==========
The CQ bit was checked by dvallet@chromium.org 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 checked by dvallet@chromium.org 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...
@sky: Can we land this (given that it's already available in Mac/Linux) or you prefer to wait for the launch bug to be fully resolved? https://bugs.chromium.org/p/chromium/issues/detail?id=705916
Please wait until the launch bug has been resolved. Thanks, -Scott On Wed, Mar 29, 2017 at 7:21 PM, <dvallet@chromium.org> wrote: > @sky: Can we land this (given that it's already available in Mac/Linux) or > you > prefer to wait for the launch bug to be fully resolved? > https://bugs.chromium.org/p/chromium/issues/detail?id=705916 > > https://codereview.chromium.org/2762593002/ > -- 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.
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 dvallet@chromium.org 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by dvallet@chromium.org 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 checked by dvallet@chromium.org 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 checked by dvallet@chromium.org 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_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by dvallet@chromium.org 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...
dvallet@chromium.org changed reviewers: + jzfeng@chromium.org
PTAL, the launch bug is approved, so this CL is ready to submit! @sky: please LGTM changes in chrome/ @sami: Please check changes in headless/BUILD.gn. I had to redo this due to headless_client_content_renderer and headless_contain_main_delegate pooling dependencies for compontent/renderer, which is not allowd in chrome:main_dll: - For non component builds: I've created a new source_set 'headless_renderer' that contains these dependencied, this is use for chrome_shell_child_lib (used in chrome_child), headless_shell_lib. - For component builds: All sources must be in the headless component, so these are added in the headless component in this case. For convenience, I keep the headless_renderer source_set that does not pull additional sources/deps if it is a component build - Added also some extra fixes from upstream @jzfeng: Please check changes in headless_content_main_delegate, to avoid refering to render client in chrome:main_dll
I think Sami should look at the build file too but I don't see any red flags. https://codereview.chromium.org/2762593002/diff/520001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2762593002/diff/520001/headless/BUILD.gn#newc... headless/BUILD.gn:507: "app/headless_shell.h", Please add a comment explaining why these three targets include a bunch of the same files. Pretty sure somebody new to the project will scratch their head when they see that :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with some small nits. https://codereview.chromium.org/2762593002/diff/520001/headless/app/headless_... File headless/app/headless_shell_switches.cc (right): https://codereview.chromium.org/2762593002/diff/520001/headless/app/headless_... headless/app/headless_shell_switches.cc:16: const char kEnableCrashReporter[] = "enable-chrash-reporter"; Typo: crash https://codereview.chromium.org/2762593002/diff/520001/headless/lib/headless_... File headless/lib/headless_content_main_delegate.cc (right): https://codereview.chromium.org/2762593002/diff/520001/headless/lib/headless_... headless/lib/headless_content_main_delegate.cc:279: return NULL; nullptr https://codereview.chromium.org/2762593002/diff/520001/headless/lib/headless_... headless/lib/headless_content_main_delegate.cc:290: return NULL; nullptr
LGTM - thanks for going through launch process and all that.
Hi I tried running binaries from this patch with --headless (I just wanted to double check the sandbox was working properly) and I get an INT 3 in chrome_child!headless::HeadlessBrowserMain+0x11b in the renderer process I'm using binaries from https://chromium-swarm.appspot.com/task?id=359e18a6d4fdf810 and command line --headless --disable-gpu
are there any tests/bots using --headless flag?
On 2017/04/19 16:55:34, Will Harris wrote: > Hi I tried running binaries from this patch with --headless (I just wanted to > double check the sandbox was working properly) and I get an INT 3 in > chrome_child!headless::HeadlessBrowserMain+0x11b in the renderer process > > I'm using binaries from > https://chromium-swarm.appspot.com/task?id=359e18a6d4fdf810 and command line > --headless --disable-gpu Thanks for checking Will. Was this binary built with is_debug=true? I know there's an issue with logging failing to properly initialized for the renderer, and a DCHEK failing. I did some tests and it seems that other DCHECKs might be failing in the renderer. My plan is to submit this and try and solve the issue on another CL.
On 2017/04/19 17:28:49, Will Harris wrote: > are there any tests/bots using --headless flag? There are tests for headless_shell, but not --headless. We run headless_unittests and headless_browsertests, but I do need to enable this for Windows.
lgtm https://codereview.chromium.org/2762593002/diff/520001/headless/public/headle... File headless/public/headless_browser.h (right): https://codereview.chromium.org/2762593002/diff/520001/headless/public/headle... headless/public/headless_browser.h:112: // Set with sandboc information. This has to be already initialized. Typo: sandboc => sandbox
lgtm
The CQ bit was checked by dvallet@chromium.org to run a CQ dry run
https://codereview.chromium.org/2762593002/diff/520001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2762593002/diff/520001/headless/BUILD.gn#newc... headless/BUILD.gn:507: "app/headless_shell.h", On 2017/04/19 07:47:09, alex clarke wrote: > Please add a comment explaining why these three targets include a bunch of the > same files. Pretty sure somebody new to the project will scratch their head > when they see that :) Thanks Alex! I added some documentation https://codereview.chromium.org/2762593002/diff/520001/headless/app/headless_... File headless/app/headless_shell_switches.cc (right): https://codereview.chromium.org/2762593002/diff/520001/headless/app/headless_... headless/app/headless_shell_switches.cc:16: const char kEnableCrashReporter[] = "enable-chrash-reporter"; On 2017/04/19 11:19:39, Sami wrote: > Typo: crash Whoops thanks, I probably reintroduced this doing the rebase. https://codereview.chromium.org/2762593002/diff/520001/headless/lib/headless_... File headless/lib/headless_content_main_delegate.cc (right): https://codereview.chromium.org/2762593002/diff/520001/headless/lib/headless_... headless/lib/headless_content_main_delegate.cc:279: return NULL; On 2017/04/19 11:19:39, Sami wrote: > nullptr Done. https://codereview.chromium.org/2762593002/diff/520001/headless/lib/headless_... headless/lib/headless_content_main_delegate.cc:290: return NULL; On 2017/04/19 11:19:39, Sami wrote: > nullptr Done. https://codereview.chromium.org/2762593002/diff/520001/headless/public/headle... File headless/public/headless_browser.h (right): https://codereview.chromium.org/2762593002/diff/520001/headless/public/headle... headless/public/headless_browser.h:112: // Set with sandboc information. This has to be already initialized. On 2017/04/20 01:01:45, jzfeng wrote: > Typo: sandboc => sandbox 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: This issue passed the CQ dry run.
The CQ bit was checked by dvallet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org, sky@chromium.org, skyostil@chromium.org, jzfeng@chromium.org Link to the patchset: https://codereview.chromium.org/2762593002/#ps540001 (title: "added extra documentations, nits")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Launch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916 Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 ========== to ========== Launch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916 Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 ==========
dvallet@chromium.org changed reviewers: + nick@chromium.org
@nick, could you please LGTM adding a dependence for '+content/public/renderer'? This is needed for Headless chrome to include print-to-pdf functionality.
lgtm https://codereview.chromium.org/2762593002/diff/540001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2762593002/diff/540001/headless/BUILD.gn#newc... headless/BUILD.gn:375: # that depend on the reenderer. These are not added in case of a component build s/reenderer/renderer :)
On 2017/04/20 00:45:22, dvallet wrote: > On 2017/04/19 17:28:49, Will Harris wrote: > > are there any tests/bots using --headless flag? > > There are tests for headless_shell, but not --headless. We run > headless_unittests and headless_browsertests, but I do need to enable this for > Windows. I'm not sure I'm entirely happy committing this without tests, and with a dcheck hitting, given we compile with dcheck enabled on waterfall.
On 2017/04/20 22:20:44, Will Harris wrote: > On 2017/04/20 00:45:22, dvallet wrote: > > On 2017/04/19 17:28:49, Will Harris wrote: > > > are there any tests/bots using --headless flag? > > > > There are tests for headless_shell, but not --headless. We run > > headless_unittests and headless_browsertests, but I do need to enable this for > > Windows. > > I'm not sure I'm entirely happy committing this without tests, and with a dcheck > hitting, given we compile with dcheck enabled on waterfall. My plan is to add headless_browsertests and headless_unittests to the waterfall once this CL is submitted, since the targets do not exist for Windows until I land it. ANd for that I'll need to fix the DCHECK. The alternative is to fix DCHECK in this cl and do the waterfall right away. The reason I'd rather just commit this and fix DCHECK later is that we keep braking the Windows build and every time I need to update the upstream it takes a while to include additional fixes. With this CL in at least we won't brake the build anymore. If you feel strongly about it please say so I I'll try and fix the DCHECK issues here
okay sure given the mitigating circumstances and that this is behind a flag I'm happy committing this and standing up the tests later - lgtm. Thanks for your detailed explanation.
new DEP on content/renderer lgtm
On 2017/04/20 22:44:52, Will Harris wrote: > okay sure given the mitigating circumstances and that this is behind a flag I'm > happy committing this and standing up the tests later - lgtm. Thanks for your > detailed explanation. Thanks for your understanding Will! I'll try to add the tests ASAP
The CQ bit was checked by dvallet@chromium.org
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": 540001, "attempt_start_ts": 1492728690208460,
"parent_rev": "3c3d810d9753f4c0138a84e0a4965880866304ed", "commit_rev":
"e3c745d4b37a659afaa0358c7de7b9bc881decb5"}
Message was sent while issue was closed.
Description was changed from ========== Launch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916 Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 ========== to ========== Launch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916 Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 Review-Url: https://codereview.chromium.org/2762593002 Cr-Commit-Position: refs/heads/master@{#466176} Committed: https://chromium.googlesource.com/chromium/src/+/e3c745d4b37a659afaa0358c7de7... ==========
Message was sent while issue was closed.
Committed patchset #28 (id:540001) as https://chromium.googlesource.com/chromium/src/+/e3c745d4b37a659afaa0358c7de7...
Message was sent while issue was closed.
A revert of this CL (patchset #28 id:540001) has been created in https://codereview.chromium.org/2835603002/ by findit-for-me@appspot.gserviceaccount.com. The reason for reverting is: Findit(https://goo.gl/kROfz5) identified CL at revision 466176 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....
Message was sent while issue was closed.
Description was changed from ========== Launch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916 Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 Review-Url: https://codereview.chromium.org/2762593002 Cr-Commit-Position: refs/heads/master@{#466176} Committed: https://chromium.googlesource.com/chromium/src/+/e3c745d4b37a659afaa0358c7de7... ========== to ========== Launch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916 Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 Review-Url: https://codereview.chromium.org/2762593002 Cr-Commit-Position: refs/heads/master@{#466176} Committed: https://chromium.googlesource.com/chromium/src/+/e3c745d4b37a659afaa0358c7de7... ==========
The CQ bit was checked by dvallet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, skyostil@chromium.org, jzfeng@chromium.org, sky@chromium.org, alexclarke@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2762593002/#ps560001 (title: "Added //net dependency for shell_child_lib to avoid Mac build error...")
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": 560001, "attempt_start_ts": 1492745144050370,
"parent_rev": "5ed39274a52c6e34e35e7a9c95aff9ef64a5ca32", "commit_rev":
"d238dae172d63175b562f99fc988e47e5213a244"}
Message was sent while issue was closed.
Description was changed from ========== Launch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916 Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 Review-Url: https://codereview.chromium.org/2762593002 Cr-Commit-Position: refs/heads/master@{#466176} Committed: https://chromium.googlesource.com/chromium/src/+/e3c745d4b37a659afaa0358c7de7... ========== to ========== Launch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916 Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 Review-Url: https://codereview.chromium.org/2762593002 Cr-Original-Commit-Position: refs/heads/master@{#466176} Committed: https://chromium.googlesource.com/chromium/src/+/e3c745d4b37a659afaa0358c7de7... Review-Url: https://codereview.chromium.org/2762593002 Cr-Commit-Position: refs/heads/master@{#466259} Committed: https://chromium.googlesource.com/chromium/src/+/d238dae172d63175b562f99fc988... ==========
Message was sent while issue was closed.
Committed patchset #29 (id:560001) as https://chromium.googlesource.com/chromium/src/+/d238dae172d63175b562f99fc988...
Message was sent while issue was closed.
hans@chromium.org changed reviewers: + hans@chromium.org
Message was sent while issue was closed.
Looks like this grew chrome_child.dll by about 5MB, or 10%. Don't know if this link works: https://chromeperf.appspot.com/report?sid=7778cd057883709478caf1f8ed5af083422... otherwise here's a screenshot: https://screenshot.googleplex.com/1xM9pHfzWq9 I'm surprised no perf system caught this.
Message was sent while issue was closed.
On 2017/04/24 22:45:03, hans wrote: > Looks like this grew chrome_child.dll by about 5MB, or 10%. > > Don't know if this link works: > https://chromeperf.appspot.com/report?sid=7778cd057883709478caf1f8ed5af083422... > otherwise here's a screenshot: https://screenshot.googleplex.com/1xM9pHfzWq9 > > I'm surprised no perf system caught this. Filed crbug.com/714841 for this one.
Message was sent while issue was closed.
A revert of this CL (patchset #29 id:560001) has been created in https://codereview.chromium.org/2837093003/ by dvallet@chromium.org. The reason for reverting is: Testing revert on --headless flag, due to increase in child_dll size: https://bugs.chromium.org/p/chromium/issues/detail?id=714841.
Message was sent while issue was closed.
Description was changed from ========== Launch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916 Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 Review-Url: https://codereview.chromium.org/2762593002 Cr-Original-Commit-Position: refs/heads/master@{#466176} Committed: https://chromium.googlesource.com/chromium/src/+/e3c745d4b37a659afaa0358c7de7... Review-Url: https://codereview.chromium.org/2762593002 Cr-Commit-Position: refs/heads/master@{#466259} Committed: https://chromium.googlesource.com/chromium/src/+/d238dae172d63175b562f99fc988... ========== to ========== Launch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916 Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 Review-Url: https://codereview.chromium.org/2762593002 Cr-Original-Commit-Position: refs/heads/master@{#466176} Committed: https://chromium.googlesource.com/chromium/src/+/e3c745d4b37a659afaa0358c7de7... Review-Url: https://codereview.chromium.org/2762593002 Cr-Commit-Position: refs/heads/master@{#466259} Committed: https://chromium.googlesource.com/chromium/src/+/d238dae172d63175b562f99fc988... ==========
The CQ bit was checked by dvallet@chromium.org 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dvallet@chromium.org 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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-...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dvallet@chromium.org 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: 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-...)
The CQ bit was checked by dvallet@chromium.org 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 checked by dvallet@chromium.org 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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL This patch is ready to be relanded. @skyostil, alexclarke: Please check additional changes to headless hans@: Please check if size increase estimate looks good. brucedawson@ and scottmg@ FYI, feel free to comment if you find any issue Size Increase estimate: - ~70KB increase in chrome_child_dll and ~165KB increase in chrome_dll. See https://bugs.chromium.org/p/chromium/issues/detail?id=714841 for more info Addittional stuff added: - More fixes for Windows breakages : Mostly specific Link & Compile errors, adding proper HEADLESS_EXPORT directives - Remove code that refers to WebContentsImpl in Headless_shell and HeadlessContentMainDelegate. This is done by using !define(CHROME_MULTIPLE_DLL_CHILD) where appropiate and modifying headless/BUILD.gn so that this defined is used appropiately during compilation and linking - Additional fixes due PinterManager pulling browser dependencies in chrome_child. - Included changes so that debug build in Windows works What is not working - There is an issue with Windows not logging properly to stdout. It does log fine to the default log file though. I'll investigate this in a different CL. - There seems to be an issue with not being able to navigate to WebContents view in localhost:9222. It could be related to https://codereview.chromium.org/2839553002. I'll investigate once both CLs land. Thanks!
PTAL This patch is ready to be relanded. @skyostil, alexclarke: Please check additional changes to headless hans@: Please check if size increase estimate looks good. brucedawson@ and scottmg@ FYI, feel free to comment if you find any issue Size Increase estimate: - ~70KB increase in chrome_child_dll and ~165KB increase in chrome_dll. See https://bugs.chromium.org/p/chromium/issues/detail?id=714841 for more info Addittional stuff added: - More fixes for Windows breakages : Mostly specific Link & Compile errors, adding proper HEADLESS_EXPORT directives - Remove code that refers to WebContentsImpl in Headless_shell and HeadlessContentMainDelegate. This is done by using !define(CHROME_MULTIPLE_DLL_CHILD) where appropiate and modifying headless/BUILD.gn so that this defined is used appropiately during compilation and linking - Additional fixes due PinterManager pulling browser dependencies in chrome_child. - Included changes so that debug build in Windows works What is not working - There is an issue with Windows not logging properly to stdout. It does log fine to the default log file though. I'll investigate this in a different CL. - There seems to be an issue with not being able to navigate to WebContents view in localhost:9222. It could be related to https://codereview.chromium.org/2839553002. I'll investigate once both CLs land. Thanks!
https://codereview.chromium.org/2762593002/diff/680001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2762593002/diff/680001/headless/app/headless_... headless/app/headless_shell.cc:70: #if !defined(CHROME_MULTIPLE_DLL_CHILD) It's kind of a shame about all the #ifs although that might just be the way of things. Anyway I have't really thought this through but I wonder if it's worth making this virtual and adding a headless_shell_win.cc (GN supports platform specific compilation based on file suffix) which overrides it with an empty implementation? Can do something similar for Shutdown and DevToolsTargetReady. WDYT? https://codereview.chromium.org/2762593002/diff/680001/headless/public/util/d... File headless/public/util/deterministic_http_protocol_handler.cc (right): https://codereview.chromium.org/2762593002/diff/680001/headless/public/util/d... headless/public/util/deterministic_http_protocol_handler.cc:13: #include "net/http/http_response_headers.h" Is this needed? https://codereview.chromium.org/2762593002/diff/680001/headless/public/util/t... File headless/public/util/testing/generic_url_request_mocks.cc (right): https://codereview.chromium.org/2762593002/diff/680001/headless/public/util/t... headless/public/util/testing/generic_url_request_mocks.cc:11: #include "net/http/http_response_headers.h" Is this needed?
lgtm from my point of view but Alex makes a good point (could be a TODO too).
On 2017/05/12 06:19:11, dvallet wrote: > hans@: Please check if size increase estimate looks good. [..] > Size Increase estimate: > - ~70KB increase in chrome_child_dll and ~165KB increase in chrome_dll. See > https://bugs.chromium.org/p/chromium/issues/detail?id=714841 for more info As commented on that bug, this sounds in line with what you got on Linux, so seems reasonable. lgtm
The CQ bit was checked by dvallet@chromium.org
Thanks for the review! https://codereview.chromium.org/2762593002/diff/680001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2762593002/diff/680001/headless/app/headless_... headless/app/headless_shell.cc:70: #if !defined(CHROME_MULTIPLE_DLL_CHILD) On 2017/05/12 at 11:31:28, alex clarke wrote: > It's kind of a shame about all the #ifs although that might just be the way of things. > > Anyway I have't really thought this through but I wonder if it's worth making this virtual and adding a headless_shell_win.cc (GN supports platform specific compilation based on file suffix) which overrides it with an empty implementation? Can do something similar for Shutdown and DevToolsTargetReady. WDYT? That's a good idea Alex! I'll look into it and will definitely think of making this more streamlined. I still want to land as is to check that this solves the size regression and will cleanup afterwards. Also, I want to avoid keep breaking the Windows build. I added a TODO https://codereview.chromium.org/2762593002/diff/680001/headless/public/util/d... File headless/public/util/deterministic_http_protocol_handler.cc (right): https://codereview.chromium.org/2762593002/diff/680001/headless/public/util/d... headless/public/util/deterministic_http_protocol_handler.cc:13: #include "net/http/http_response_headers.h" On 2017/05/12 at 11:31:28, alex clarke wrote: > Is this needed? Yes, Windows compiler complains (error C2027: use of undefined type 'net::HttpResonseHeaders' https://codereview.chromium.org/2762593002/diff/680001/headless/public/util/t... File headless/public/util/testing/generic_url_request_mocks.cc (right): https://codereview.chromium.org/2762593002/diff/680001/headless/public/util/t... headless/public/util/testing/generic_url_request_mocks.cc:11: #include "net/http/http_response_headers.h" On 2017/05/12 at 11:31:28, alex clarke wrote: > Is this needed? ditto
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, jzfeng@chromium.org, sky@chromium.org, wfh@chromium.org, alexclarke@chromium.org, hans@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2762593002/#ps700001 (title: "added TODO")
The CQ bit was checked by dvallet@chromium.org
Thanks for the review!
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dvallet@chromium.org
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": 700001, "attempt_start_ts": 1494633044664620,
"parent_rev": "dfc611172a969049848899a34ce553990373b67c", "commit_rev":
"8303a91150b12ffda29e1d2d5e46dc1371730bfe"}
Message was sent while issue was closed.
Description was changed from ========== Launch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916 Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 Review-Url: https://codereview.chromium.org/2762593002 Cr-Original-Commit-Position: refs/heads/master@{#466176} Committed: https://chromium.googlesource.com/chromium/src/+/e3c745d4b37a659afaa0358c7de7... Review-Url: https://codereview.chromium.org/2762593002 Cr-Commit-Position: refs/heads/master@{#466259} Committed: https://chromium.googlesource.com/chromium/src/+/d238dae172d63175b562f99fc988... ========== to ========== Launch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916 Add --headless flag to Windows: Fix browser_tests not compiling properly on windows Fix headless/BUILD.gn by: - Changing headless_lib to a headless component, following steps in https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md and fixing the build for linux, since it now compiles as a shared_library when component build is true. - Adding neccesary HEADLESS_EXPORT (submitted in https://codereview.chromium.org/2775693003/) - Disabling breakpad in windows (will fix in a separate CL) BUG=686608 Review-Url: https://codereview.chromium.org/2762593002 Cr-Original-Original-Commit-Position: refs/heads/master@{#466176} Committed: https://chromium.googlesource.com/chromium/src/+/e3c745d4b37a659afaa0358c7de7... Review-Url: https://codereview.chromium.org/2762593002 Cr-Original-Commit-Position: refs/heads/master@{#466259} Committed: https://chromium.googlesource.com/chromium/src/+/d238dae172d63175b562f99fc988... Review-Url: https://codereview.chromium.org/2762593002 Cr-Commit-Position: refs/heads/master@{#471525} Committed: https://chromium.googlesource.com/chromium/src/+/8303a91150b12ffda29e1d2d5e46... ==========
Message was sent while issue was closed.
Committed patchset #36 (id:700001) as https://chromium.googlesource.com/chromium/src/+/8303a91150b12ffda29e1d2d5e46...
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
This breaks mac bots that build all targets: https://build.chromium.org/p/chromium.fyi/builders/ClangToTMac?numbuilds=200 https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.fyi%2FClangToTM... FAILED: obj/headless/libheadless_renderer.a export DEVELOPER_DIR=/b/c/builder/ClangToTMac/src/build/mac_files/Xcode.app; rm -f obj/headless/libheadless_renderer.a && TOOL_VERSION=1494365172 python ../../build/toolchain/mac/filter_libtool.py libtool -static -o obj/headless/libheadless_renderer.a error: /b/c/builder/ClangToTMac/src/build/mac_files/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: no files specified Usage: /b/c/builder/ClangToTMac/src/build/mac_files/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool -static [-] file [...] [-filelist listfile[,dirname]] [-arch_only arch] [-sacLT] Usage: /b/c/builder/ClangToTMac/src/build/mac_files/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool -dynamic [-] file [...] [-filelist listfile[,dirname]] [-arch_only arch] [-o output] [-install_name name] [-compatibility_version #] [-current_version #] [-seg1addr 0x#] [-segs_read_only_addr 0x#] [-segs_read_write_addr 0x#] [-seg_addr_table <filename>] [-seg_addr_table_filename <file_system_path>] [-all_load] [-noall_load] You can't have static libraries without any sources. Make this an empty group() on platforms where there are no sources.
Message was sent while issue was closed.
On 2017/05/15 at 15:53:26, thakis wrote: > This breaks mac bots that build all targets: > > https://build.chromium.org/p/chromium.fyi/builders/ClangToTMac?numbuilds=200 > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.fyi%2FClangToTM... > > FAILED: obj/headless/libheadless_renderer.a > export DEVELOPER_DIR=/b/c/builder/ClangToTMac/src/build/mac_files/Xcode.app; rm -f obj/headless/libheadless_renderer.a && TOOL_VERSION=1494365172 python ../../build/toolchain/mac/filter_libtool.py libtool -static -o obj/headless/libheadless_renderer.a > error: /b/c/builder/ClangToTMac/src/build/mac_files/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: no files specified > Usage: /b/c/builder/ClangToTMac/src/build/mac_files/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool -static [-] file [...] [-filelist listfile[,dirname]] [-arch_only arch] [-sacLT] > Usage: /b/c/builder/ClangToTMac/src/build/mac_files/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool -dynamic [-] file [...] [-filelist listfile[,dirname]] [-arch_only arch] [-o output] [-install_name name] [-compatibility_version #] [-current_version #] [-seg1addr 0x#] [-segs_read_only_addr 0x#] [-segs_read_write_addr 0x#] [-seg_addr_table <filename>] [-seg_addr_table_filename <file_system_path>] [-all_load] [-noall_load] > > > You can't have static libraries without any sources. Make this an empty group() on platforms where there are no sources. Thanks, I'll track the change in this other related bug: https://bugs.chromium.org/p/chromium/issues/detail?id=722338 |
