New issue
Advanced search Search tips

Issue 594566 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Broken ability to use "--expose-internals-for-testing" in an interactive content-shell session

Project Member Reported by lukasza@chromium.org, Mar 14 2016

Issue description

Based on  https://crbug.com/587175#c10  it seems that 381b049a regressed the ability to use "--expose-internals-for-testing" in an interactive content-shell session.

 
I think the issue is caused by moving processing of some command line flags from ShellContentBrowserClient::AppendExtraCommandLineSwitches into LayoutTestContentBrowserClient::AppendExtraCommandLineSwitches (the latter will not be called if --run-layout-test switch is missing - see ShellMainDelegate::CreateContentBrowserClient).

The issue seems to affect more than --expose-internals-for-testing flag - it affects:
- kAlwaysUseComplexText
- kEnableFontAntialiasing
- kExposeInternalsForTesting
- kStableReleaseMode
- kEnableLeakDetection


Cc: -lukasza@chromium.org
Owner: lukasza@chromium.org
I've put together a fix for --expose-internals-for-testing at https://crrev.com/1799923002.

I've gone through the other 4 switches and I am pretty sure that they were only active when used together with --run-layout-test.  So - in theory they can stay this way / my previous CL didn't break anything here.  OTOH, this inconsistent treatment the switches got is a bit surprising (i.e. wouldn't devs want to use --stable-release-mode and/or --enable-font-antialiasing when testing via interactive content_shell?  is it ok if the answer is different than for --expose-internals-for-testing?).
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/756abda6b173d7b698bf43a0726e43eaab8c03dd

commit 756abda6b173d7b698bf43a0726e43eaab8c03dd
Author: lukasza <lukasza@chromium.org>
Date: Tue Mar 15 21:58:50 2016

Make kExposeInternalsForTesting usable again for interactive content_shell.

In https://crrev.com/1763113002 I've tried to separate
layout-test-specific cmdline switches into a separate header file and
handle some of them under
LayoutTestContentBrowserClient::AppendExtraCommandLineSwitches rather
than ShellContentBrowserClient::AppendExtraCommandLineSwitches.  This
was wrong - this meant that devs cannot pass these switches when
debugging via content_shell running in interactive / non-layout-test
mode.

So - the current CL undoes part of https://crrev.com/1763113002 and
restores processing of some of the switches back to
ShellContentBrowserClient:

- kExposeInternalsForTesting - this is useful for debugging layout tests
  (see  https://crbug.com/594566 ).

For now it is okay to leave propagation of the following switches in
LayoutTestContentBrowserClient (because moving them in
https://crrev.com/1763113002 was not a regression as explained below):

- kStableReleaseMode - it only has effect if used together with
  --run-layout-test switch (i.e. see how
  ShellMainDelegate::BasicStartupComplete first checks
  switches::kRunLayoutTest before checking switches::kStableReleaseMode
  in a nested if).

- kEnableLeakDetection only affects BlinkTestController and
  LayoutTestContentBrowserClient so it won't have any effect without
  --run-layout-test switch.

- kEnableFontAntialiasing and kAlwaysUseComplexText only affects
  LayoutTestContentBrowserClient and LayoutTestRenderProcessObserver so
  it won't have any effect without --run-layout-test switch.

BUG= 594566 

Review URL: https://codereview.chromium.org/1799923002

Cr-Commit-Position: refs/heads/master@{#381330}

[modify] https://crrev.com/756abda6b173d7b698bf43a0726e43eaab8c03dd/content/shell/browser/layout_test/layout_test_content_browser_client.cc
[modify] https://crrev.com/756abda6b173d7b698bf43a0726e43eaab8c03dd/content/shell/browser/shell_content_browser_client.cc
[modify] https://crrev.com/756abda6b173d7b698bf43a0726e43eaab8c03dd/content/shell/common/layout_test/layout_test_switches.cc
[modify] https://crrev.com/756abda6b173d7b698bf43a0726e43eaab8c03dd/content/shell/common/layout_test/layout_test_switches.h
[modify] https://crrev.com/756abda6b173d7b698bf43a0726e43eaab8c03dd/content/shell/common/shell_switches.cc
[modify] https://crrev.com/756abda6b173d7b698bf43a0726e43eaab8c03dd/content/shell/common/shell_switches.h
[modify] https://crrev.com/756abda6b173d7b698bf43a0726e43eaab8c03dd/content/shell/renderer/shell_render_view_observer.cc

Status: Fixed (was: Untriaged)

Comment 5 by sshru...@google.com, May 18 2016

Labels: Test-Layout

Sign in to add a comment