headless: make initial screen/window sizes configurable.

headless: make initial screen/window sizes configurable.

BUG=546953
Committed: https://crrev.com/73c4ead9db3aad5e06d354c3091b6749a32e77a4
Cr-Commit-Position: refs/heads/master@{#409261}

[Eric Seckler, 2016-08-01 10:53:24]

eseckler@chromium.org changed reviewers:
+ skyostil@google.com

[Eric Seckler, 2016-08-01 10:53:25]

This is missing tests at the moment, primarily because there doesn't seem to be
a good way to test initialization options (?).

[Eric Seckler, 2016-08-01 10:54:09]

Description was changed from

==========
headless: make initial screen/window sizes configurable.

BUG=546953
==========

to

==========
headless: make initial screen/window sizes configurable.

BUG=546953
==========

[Eric Seckler, 2016-08-01 10:54:09]

eseckler@chromium.org changed reviewers:
+ skyostil@chromium.org
- skyostil@google.com

[Sami, 2016-08-01 12:22:17]

Looks good. Maybe we could have one test which checks that the default sizes are
what we'd expect by reading them from JS?

Does the screen size need to be configurable? I'm wondering if it could just be
an implementation detail or would someone actually want to configure it?

[Eric Seckler, 2016-08-02 08:50:39]

On 2016/08/01 12:22:17, Sami wrote:
> Looks good. Maybe we could have one test which checks that the default sizes
are
> what we'd expect by reading them from JS?
Done.

> Does the screen size need to be configurable? I'm wondering if it could just
be
> an implementation detail or would someone actually want to configure it?
It's visible from JS (screen.width/height), so I thought it'd be useful to make
the default configurable. But it's also possible via DevTools' Emulation domain.

[Sami, 2016-08-02 13:03:35]

On 2016/08/02 08:50:39, Eric Seckler wrote:
> On 2016/08/01 12:22:17, Sami wrote:
> > Looks good. Maybe we could have one test which checks that the default sizes
> are
> > what we'd expect by reading them from JS?
> Done.
> 
> > Does the screen size need to be configurable? I'm wondering if it could just
> be
> > an implementation detail or would someone actually want to configure it?
> It's visible from JS (screen.width/height), so I thought it'd be useful to
make
> the default configurable. But it's also possible via DevTools' Emulation
domain.

How about we rely on devtools for that and only expose it here if it turns out
we need it for something?

[Eric Seckler, 2016-08-02 15:34:28]

On 2016/08/02 13:03:35, Sami wrote:
> On 2016/08/02 08:50:39, Eric Seckler wrote:
> > On 2016/08/01 12:22:17, Sami wrote:
> > > Looks good. Maybe we could have one test which checks that the default
sizes
> > are
> > > what we'd expect by reading them from JS?
> > Done.
> > 
> > > Does the screen size need to be configurable? I'm wondering if it could
just
> > be
> > > an implementation detail or would someone actually want to configure it?
> > It's visible from JS (screen.width/height), so I thought it'd be useful to
> make
> > the default configurable. But it's also possible via DevTools' Emulation
> domain.
> 
> How about we rely on devtools for that and only expose it here if it turns out
> we need it for something?

Now using a single option for both window and screen size to avoid screen <
window.

[Sami, 2016-08-02 16:45:33]

lgtm with one comment.

https://codereview.chromium.org/2199773002/diff/60001/headless/public/headles...
File headless/public/headless_browser.cc (right):

https://codereview.chromium.org/2199773002/diff/60001/headless/public/headles...
headless/public/headless_browser.cc:16: const gfx::Size kDefaultWindowSize(800,
600);
We shouldn't use non-POD objects as static globals (since their constructors run
at process startup). Please change this to
kDefaultWindowWidth/kDefaultWindowHeight instead.

[Eric Seckler, 2016-08-02 18:07:06]

On 2016/08/02 16:45:33, Sami wrote:
> lgtm with one comment.
> 
>
https://codereview.chromium.org/2199773002/diff/60001/headless/public/headles...
> File headless/public/headless_browser.cc (right):
> 
>
https://codereview.chromium.org/2199773002/diff/60001/headless/public/headles...
> headless/public/headless_browser.cc:16: const gfx::Size
kDefaultWindowSize(800,
> 600);
> We shouldn't use non-POD objects as static globals (since their constructors
run
> at process startup). Please change this to
> kDefaultWindowWidth/kDefaultWindowHeight instead.

Changed to a constexpr instead. Also missed a stale reference to the old
screen_size in the test earlier, fixed too.

[Sami, 2016-08-02 18:09:11]

Still lgtm.

[altimin, 2016-08-02 18:09:22]

non-owner lgtm

[Eric Seckler, 2016-08-02 18:15:31]

The CQ bit was checked by eseckler@chromium.org

[commit-bot: I haz the power, 2016-08-02 18:16:30]

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, 2016-08-02 19:06:05]

Description was changed from

==========
headless: make initial screen/window sizes configurable.

BUG=546953
==========

to

==========
headless: make initial screen/window sizes configurable.

BUG=546953
==========

[commit-bot: I haz the power, 2016-08-02 19:06:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-02 19:08:36]

Description was changed from

==========
headless: make initial screen/window sizes configurable.

BUG=546953
==========

to

==========
headless: make initial screen/window sizes configurable.

BUG=546953

Committed: https://crrev.com/73c4ead9db3aad5e06d354c3091b6749a32e77a4
Cr-Commit-Position: refs/heads/master@{#409261}
==========

[commit-bot: I haz the power, 2016-08-02 19:08:37]

Patchset 5 (id:??) landed as
https://crrev.com/73c4ead9db3aad5e06d354c3091b6749a32e77a4
Cr-Commit-Position: refs/heads/master@{#409261}