Fix browser tests on Mac

Fix browser tests on Mac

BUG=687407
Review-Url: https://codereview.chromium.org/2778983002
Cr-Commit-Position: refs/heads/master@{#460303}
Committed: https://chromium.googlesource.com/chromium/src/+/41d64776a2936bd014ebed1340d3aaf9e5eeeede

[dvallet, 2017-03-28 05:59:39]

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

[commit-bot: I haz the power, 2017-03-28 06:00:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 06:26:59]

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

[commit-bot: I haz the power, 2017-03-28 06:27:00]

Dry run: This issue passed the CQ dry run.

[dvallet, 2017-03-28 07:21:35]

dvallet@chromium.org changed reviewers:
+ eseckler@chromium.org, skyostil@chromium.org

[dvallet, 2017-03-28 07:21:35]

PTAL

[Eric Seckler, 2017-03-28 07:39:48]

lgtm, but deferring to Sami for the DIR_SOURCE_ROOT fix.

https://codereview.chromium.org/2778983002/diff/20001/headless/lib/headless_w...
File headless/lib/headless_web_contents_browsertest.cc (right):

https://codereview.chromium.org/2778983002/diff/20001/headless/lib/headless_w...
headless/lib/headless_web_contents_browsertest.cc:91: // NOTE(dvallet): Mac
headless implementation already does this.
Interesting, do you know why that is? Is that simply "normal" on mac/cocoa, or
is it due to something in headless-mac setup?

[Sami, 2017-03-28 13:11:59]

lgtm with nits, thanks!

Could we also start running these tests now on Mac? This is where the linux
tests are enabled:

https://cs.chromium.org/chromium/src/testing/buildbot/chromium.linux.json?rcl...

https://codereview.chromium.org/2778983002/diff/20001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/2778983002/diff/20001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:360: // There is no concept of
screen on Mac, so this part of the test should
Would it be more accurate to say we don't override the size of the screen in
headless mode on Mac, so this returns the actual screen dimensions.

nit: Let's leave the query in but just #ifdef out the EXPECT_EQ part.

https://codereview.chromium.org/2778983002/diff/20001/headless/test/headless_...
File headless/test/headless_browser_test.cc (right):

https://codereview.chromium.org/2778983002/diff/20001/headless/test/headless_...
headless/test/headless_browser_test.cc:127: // On Mac the source root is not set
properly. We Override it by asumming
nit: s/Override/override/, typo: assuming

[dvallet, 2017-03-29 05:16:28]

Thanks for the review!
re: add browser_tests. I was planning to do that next! I have a feeling that
some other stuff will be broken so I'll submit in a separate CL

https://codereview.chromium.org/2778983002/diff/20001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/2778983002/diff/20001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:360: // There is no concept of
screen on Mac, so this part of the test should
On 2017/03/28 at 13:11:59, Sami wrote:
> Would it be more accurate to say we don't override the size of the screen in
headless mode on Mac, so this returns the actual screen dimensions.
> 
> nit: Let's leave the query in but just #ifdef out the EXPECT_EQ part.

Done. Yes, that's more accurate, thanks!

https://codereview.chromium.org/2778983002/diff/20001/headless/lib/headless_w...
File headless/lib/headless_web_contents_browsertest.cc (right):

https://codereview.chromium.org/2778983002/diff/20001/headless/lib/headless_w...
headless/lib/headless_web_contents_browsertest.cc:91: // NOTE(dvallet): Mac
headless implementation already does this.
On 2017/03/28 at 07:39:48, Eric Seckler wrote:
> Interesting, do you know why that is? Is that simply "normal" on mac/cocoa, or
is it due to something in headless-mac setup?

Yeah I think is a cocoa thing. The window is hidden not shown, but when we do
web_contents_delegate_->ActivateContents(...) then contents are set as focused
regardless of any other inactive windows. I'm happy that this is the case,
otherwise it could've been a problem to set up properly...

[dvallet, 2017-03-29 05:16:32]

The CQ bit was checked by dvallet@chromium.org

[dvallet, 2017-03-29 05:16:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, eseckler@chromium.org
Link to the patchset: https://codereview.chromium.org/2778983002/#ps60001
(title: "nit change on #if defined on DefaultSizes test")

[commit-bot: I haz the power, 2017-03-29 05:16:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 06:36:56]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490764592397230,
"parent_rev": "0611b01da7b4979fada8de6369f948cc3ba53dd2", "commit_rev":
"41d64776a2936bd014ebed1340d3aaf9e5eeeede"}

[commit-bot: I haz the power, 2017-03-29 06:37:43]

Description was changed from

==========
Fix browser tests on Mac

BUG=687407
==========

to

==========
Fix browser tests on Mac

BUG=687407

Review-Url: https://codereview.chromium.org/2778983002
Cr-Commit-Position: refs/heads/master@{#460303}
Committed:
https://chromium.googlesource.com/chromium/src/+/41d64776a2936bd014ebed1340d3...
==========

[commit-bot: I haz the power, 2017-03-29 06:37:44]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/41d64776a2936bd014ebed1340d3...