Move layout-test-specific switches from shell_switches to layout_test_switches.h

Move layout-test-specific switches from shell_switches to layout_test_switches.h

This partially addresses a "soft" layering violation pointed out in
https://codereview.chromium.org/1755123004/#msg4
("soft" = honour-based, not DEPS-enforced).

BUG=587175
Committed: https://crrev.com/381b049a926507ef30b493deb57e12528d0ef3fd
Cr-Commit-Position: refs/heads/master@{#380420}

[Łukasz Anforowicz, 2016-03-04 17:35:45]

Description was changed from

==========
Move layout-test-specific switches into from shell_switches into
layout_test_switches.h.

This partially addresses a "soft" layering violation pointed out in
https://codereview.chromium.org/1755123004/#msg4
("soft" = honour-based, not DEPS-enforced).

BUG=587175
==========

to

==========
Move layout-test-specific switches from shell_switches to
layout_test_switches.h.

This partially addresses a "soft" layering violation pointed out in
https://codereview.chromium.org/1755123004/#msg4
("soft" = honour-based, not DEPS-enforced).

BUG=587175
==========

[Łukasz Anforowicz, 2016-03-04 17:36:08]

Description was changed from

==========
Move layout-test-specific switches from shell_switches to
layout_test_switches.h.

This partially addresses a "soft" layering violation pointed out in
https://codereview.chromium.org/1755123004/#msg4
("soft" = honour-based, not DEPS-enforced).

BUG=587175
==========

to

==========
Move layout-test-specific switches from shell_switches to layout_test_switches.h

This partially addresses a "soft" layering violation pointed out in
https://codereview.chromium.org/1755123004/#msg4
("soft" = honour-based, not DEPS-enforced).

BUG=587175
==========

[Łukasz Anforowicz, 2016-03-04 17:51:14]

lukasza@chromium.org changed reviewers:
+ jochen@chromium.org

[Łukasz Anforowicz, 2016-03-04 17:51:15]

jochen@, could you please take a look? (this is a follow-up to your comment @
https://codereview.chromium.org/1755123004/#msg5)

https://codereview.chromium.org/1763113002/diff/1/content/shell/app/shell_mai...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/1763113002/diff/1/content/shell/app/shell_mai...
content/shell/app/shell_main_delegate.cc:29: #include
"content/shell/common/layout_test/layout_test_switches.h"
1 out of 3 remaining shell -> layout_test includes.  I think this one is ok,
because it mostly switches between generic and layout-test-specific
functionality.  Example below:

ContentBrowserClient* ShellMainDelegate::CreateContentBrowserClient() {
  browser_client_.reset(base::CommandLine::ForCurrentProcess()->HasSwitch(
                            switches::kRunLayoutTest)
                            ? new LayoutTestContentBrowserClient
                            : new ShellContentBrowserClient);

  return browser_client_.get();
}

https://codereview.chromium.org/1763113002/diff/1/content/shell/browser/layou...
File content/shell/browser/layout_test/layout_test_browser_main.cc (left):

https://codereview.chromium.org/1763113002/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_browser_main.cc:27: #include
"content/shell/common/shell_switches.h"
shell_switches.h include needs to be retained for kContentShellDataPath

https://codereview.chromium.org/1763113002/diff/1/content/shell/common/shell_...
File content/shell/common/shell_switches.h (right):

https://codereview.chromium.org/1763113002/diff/1/content/shell/common/shell_...
content/shell/common/shell_switches.h:29: bool IsRunLayoutTestSwitchPresent();
This is a bit of a weird method, but I think it is worth introducing it, because
otherwise we would still need to drag in the whole layout_test_switches.h into 
content/shell/browser/shell.cc, content/shell/common/shell_content_client.cc.

I guess one can look at this as 1 out of 3 remaining shell -> layout_test
dependencies (the include is hidden in shell_switches.cc).

https://codereview.chromium.org/1763113002/diff/1/content/shell/renderer/shel...
File content/shell/renderer/shell_render_view_observer.cc (right):

https://codereview.chromium.org/1763113002/diff/1/content/shell/renderer/shel...
content/shell/renderer/shell_render_view_observer.cc:10: #include
"content/shell/common/layout_test/layout_test_switches.h"
1/3 of places that still have a shell -> layout_test include - this one is for:

void ShellRenderViewObserver::DidClearWindowObject(
    blink::WebLocalFrame* frame) {
  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
          switches::kExposeInternalsForTesting)) {
    blink::WebTestingSupport::injectInternalsObject(frame);
  }
}

[Łukasz Anforowicz, 2016-03-04 17:59:38]

https://codereview.chromium.org/1763113002/diff/1/content/shell/app/shell_mai...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/1763113002/diff/1/content/shell/app/shell_mai...
content/shell/app/shell_main_delegate.cc:29: #include
"content/shell/common/layout_test/layout_test_switches.h"
On 2016/03/04 17:51:15, Łukasz Anforowicz wrote:
> 1 out of 3 remaining shell -> layout_test includes.  I think this one is ok,
> because it mostly switches between generic and layout-test-specific
> functionality.  Example below:
> 
> ContentBrowserClient* ShellMainDelegate::CreateContentBrowserClient() {
>   browser_client_.reset(base::CommandLine::ForCurrentProcess()->HasSwitch(
>                             switches::kRunLayoutTest)
>                             ? new LayoutTestContentBrowserClient
>                             : new ShellContentBrowserClient);
> 
>   return browser_client_.get();
> }

BTW: To get rid of the include of layout_test_switches.h here we would have to
handle:

- kRunLayoutTest (easy - can use switches::IsRunLayoutTestSwitchPresent)

- kCheckLayoutTestSysDeps

- kStableReleaseMode

I kind of arbitrarily decided that I do not want to widen the layering hole in
shell_switches.h and introduce IsCheckLayoutTestSysDepsSwitchPresent and/or
IsStableReleaseModeSwitchPresent.

https://codereview.chromium.org/1763113002/diff/1/content/shell/common/shell_...
File content/shell/common/shell_switches.h (left):

https://codereview.chromium.org/1763113002/diff/1/content/shell/common/shell_...
content/shell/common/shell_switches.h:15: extern const char
kAllowExternalPages[];
I tried to see where the switches are used and moved ones mostly used only under
layout_tests.  OTOH, some of them seemed rather generic (i.e.
kEnableAccelerated2DCanvas)... hmmm...

[jochen (gone - plz use gerrit), 2016-03-09 14:53:19]

lgtm

[Łukasz Anforowicz, 2016-03-09 16:32:07]

Thanks for reviewing (in a new patchset I am rebasing and making a small change
in content/public/test/content_browser_test.cc - this is same change as made
elsewhere in the CL).

[Łukasz Anforowicz, 2016-03-09 16:32:14]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-09 16:32:25]

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

[commit-bot: I haz the power, 2016-03-09 16:45:39]

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

[commit-bot: I haz the power, 2016-03-09 16:45:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2016-03-09 17:25:44]

Aaaand I had to add switches::IsRunLayoutTestSwitchPresent() call to a bit more
places under content/shell (win- and mac-specific delegates).

BTW - interestingly the "run-layout-test" string is also defined in
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java
(and used in
shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java:89).

[Łukasz Anforowicz, 2016-03-09 17:25:53]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-09 17:26:07]

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

[commit-bot: I haz the power, 2016-03-09 18:37:40]

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

[commit-bot: I haz the power, 2016-03-09 18:37:41]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Łukasz Anforowicz, 2016-03-09 18:42:23]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-03-09 18:42:24]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1763113002/#ps80001
(title: "Adding IsRunLayoutTestSwitchPresent() call to Win- and Mac-specific
code.")

[commit-bot: I haz the power, 2016-03-09 18:42:47]

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

[commit-bot: I haz the power, 2016-03-09 20:09:49]

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

[commit-bot: I haz the power, 2016-03-09 20:09:50]

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

[Łukasz Anforowicz, 2016-03-09 22:18:44]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-09 22:19:11]

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

[commit-bot: I haz the power, 2016-03-09 22:29:56]

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

[commit-bot: I haz the power, 2016-03-09 22:29:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Łukasz Anforowicz, 2016-03-09 23:30:20]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-09 23:30:42]

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

[commit-bot: I haz the power, 2016-03-10 02:02:36]

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

[commit-bot: I haz the power, 2016-03-10 02:02:37]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Łukasz Anforowicz, 2016-03-10 03:25:34]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-10 03:25:53]

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

[Łukasz Anforowicz, 2016-03-10 03:28:24]

jochen@, this CL has gone through 3 extra patchsets since your l-g-t-m (fixing
rather silly mistakes in the original CL - fixing compile issues beyond the
targets I've tested locally and fixing accidental reversal of a condition).

I think the CL is still landable, but I thought that before I'll push it to CQ,
I'll give you a chance to shout first (feel free to push it to CQ if everything
still looks ok).

[commit-bot: I haz the power, 2016-03-10 04:36:44]

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

[commit-bot: I haz the power, 2016-03-10 04:36:45]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-03-10 16:43:14]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-03-10 16:43:15]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1763113002/#ps100001
(title: "Fix accidental condition reversal in
Shell::ToggleFullscreenModeForTab.")

[commit-bot: I haz the power, 2016-03-10 16:43:36]

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

[commit-bot: I haz the power, 2016-03-10 16:48:50]

Description was changed from

==========
Move layout-test-specific switches from shell_switches to layout_test_switches.h

This partially addresses a "soft" layering violation pointed out in
https://codereview.chromium.org/1755123004/#msg4
("soft" = honour-based, not DEPS-enforced).

BUG=587175
==========

to

==========
Move layout-test-specific switches from shell_switches to layout_test_switches.h

This partially addresses a "soft" layering violation pointed out in
https://codereview.chromium.org/1755123004/#msg4
("soft" = honour-based, not DEPS-enforced).

BUG=587175
==========

[commit-bot: I haz the power, 2016-03-10 16:48:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-10 16:49:56]

Description was changed from

==========
Move layout-test-specific switches from shell_switches to layout_test_switches.h

This partially addresses a "soft" layering violation pointed out in
https://codereview.chromium.org/1755123004/#msg4
("soft" = honour-based, not DEPS-enforced).

BUG=587175
==========

to

==========
Move layout-test-specific switches from shell_switches to layout_test_switches.h

This partially addresses a "soft" layering violation pointed out in
https://codereview.chromium.org/1755123004/#msg4
("soft" = honour-based, not DEPS-enforced).

BUG=587175

Committed: https://crrev.com/381b049a926507ef30b493deb57e12528d0ef3fd
Cr-Commit-Position: refs/heads/master@{#380420}
==========

[commit-bot: I haz the power, 2016-03-10 16:49:59]

Patchset 6 (id:??) landed as
https://crrev.com/381b049a926507ef30b493deb57e12528d0ef3fd
Cr-Commit-Position: refs/heads/master@{#380420}