Revert "Revert of Support device scale factor test with popups in LayoutTest (patchset #2 id:40001 …

Revert "Revert of Support device scale factor test with popups in LayoutTest (patchset #2 id:40001 of https://codereview.chromium.org/1525933002/ )"

Reland of https://codereview.chromium.org/1525933002 with hypothentical fix.
>> Support device scale factor test with popups in LayoutTest
>>
>> * Sends DSF to host so that host can send a resize request with requested DSF.
>> * Updated X11's screen so that displays contains specified DSF.
>>   This is used by RWH to construct WebScreenInfo with updated DSF.
>> - Win/Mac will use the DSF specified by command line flag.
>>   * Scale the popup location based on the device scale factor.
>>   This is done by compositor in normal scenario.

BUG=567837
TEST=added popu-menu-appearance-dsf2.html
TBR=mkwst@chromium.org,sadrul@chromium.org,tkent@chromium.org,wkorman@chromium.org,oshima@chromium.org
R=noel@chromium.org

[wkorman, 2016-01-04 21:51:29]

Can you briefly describe what changes were made to address the two issues that
came up when this patch was landed, note in my comments here:

http://crbug.com/570401#c61

[oshima, 2016-01-04 21:55:06]

On 2016/01/04 21:51:29, wkorman wrote:
> Can you briefly describe what changes were made to address the two issues that
> came up when this patch was landed, note in my comments here:
> 
> http://crbug.com/570401#c61

See the patch set 2. My current hypothesis is that test harness requires
initialization

content::SetDeviceScaleFactor(render_view(), 1.0f);

[oshima, 2016-01-04 21:55:50]

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

[commit-bot: I haz the power, 2016-01-04 21:56:44]

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

[wkorman, 2016-01-04 22:08:40]

On 2016/01/04 at 21:55:06, oshima wrote:
> On 2016/01/04 21:51:29, wkorman wrote:
> > Can you briefly describe what changes were made to address the two issues
that
> > came up when this patch was landed, note in my comments here:
> > 
> > http://crbug.com/570401#c61
> 
> See the patch set 2. My current hypothesis is that test harness requires
initialization
> 
> content::SetDeviceScaleFactor(render_view(), 1.0f);

I actually don't see that line in patch 2, maybe the change upload is stale?

With current patch 2 it doesn't look like we are doing any save/restore at
start/end of test, or at least a reset to a known safe starting value at end, so
if it is changed by one test that change will persist into subsequent tests run
by that test harness content shell instance, right? Or am I missing something
and the initialization call does happen for every subsequent run. See my
comments re: other places we do a save/restore in http://crrev.com/1525933002.

Resetting internal state mostly looks to happen today through
BlinkTestRunner::Reset() which ends up in
WebCoreTestsuppoer::resetInternalsObject() which calls into things like
Internals::resetToConsistentState() and
InternalSettings::from(*page)->resetToConsistentState().

So adding the test harness initialization (presuming change is just stale or I
am missing it and it does exist) seems like a good theory to fix #2 (missing
Windows images) but #1 is still at risk.

[oshima, 2016-01-04 22:52:55]

On 2016/01/04 22:08:40, wkorman wrote:
> On 2016/01/04 at 21:55:06, oshima wrote:
> > On 2016/01/04 21:51:29, wkorman wrote:
> > > Can you briefly describe what changes were made to address the two issues
> that
> > > came up when this patch was landed, note in my comments here:
> > > 
> > > http://crbug.com/570401#c61
> > 
> > See the patch set 2. My current hypothesis is that test harness requires
> initialization
> > 
> > content::SetDeviceScaleFactor(render_view(), 1.0f);
> 
> I actually don't see that line in patch 2, maybe the change upload is stale?
> 
> With current patch 2 it doesn't look like we are doing any save/restore at
> start/end of test, or at least a reset to a known safe starting value at end,
so
> if it is changed by one test that change will persist into subsequent tests
run
> by that test harness content shell instance, right? Or am I missing something
> and the initialization call does happen for every subsequent run. See my
> comments re: other places we do a save/restore in http://crrev.com/1525933002.


The original code was

void BlinkTestRunner::SetDeviceScaleFactor(float factor) {
  content::SetDeviceScaleFactor(render_view(), factor);
}

and I changed it to

void BlinkTestRunner::SetDeviceScaleFactor(float factor) {
  if (device_scale_factor_ == factor)
    return;
  content::SetDeviceScaleFactor(render_view(), factor);
  ...
}

believing that the renderer is created and initialized for 1.f for each test. I
was probably wrong, and
I think the test harness expects this is called at least once with 1.f.

> 
> Resetting internal state mostly looks to happen today through
> BlinkTestRunner::Reset() which ends up in
> WebCoreTestsuppoer::resetInternalsObject() which calls into things like
> Internals::resetToConsistentState() and
> InternalSettings::from(*page)->resetToConsistentState().
> 
> So adding the test harness initialization (presuming change is just stale or I
> am missing it and it does exist) seems like a good theory to fix #2 (missing
> Windows images) but #1 is still at risk.

The test harness is calling this from here:

https://code.google.com/p/chromium/codesearch#chromium/src/components/test_ru...

In patch set 2, I changed it to

void BlinkTestRunner::SetDeviceScaleFactor(float factor) {
  content::SetDeviceScaleFactor(render_view(), factor);
  if (device_scale_factor_ == factor)
    return;
  ...
}

to keep the original behavior.

[wkorman, 2016-01-04 23:03:37]

On 2016/01/04 at 22:52:55, oshima wrote:
> On 2016/01/04 22:08:40, wkorman wrote:
> > On 2016/01/04 at 21:55:06, oshima wrote:
> > > On 2016/01/04 21:51:29, wkorman wrote:
> > > > Can you briefly describe what changes were made to address the two
issues
> > that
> > > > came up when this patch was landed, note in my comments here:
> > > > 
> > > > http://crbug.com/570401#c61
> > > 
> > > See the patch set 2. My current hypothesis is that test harness requires
> > initialization
> > > 
> > > content::SetDeviceScaleFactor(render_view(), 1.0f);
> > 
> > I actually don't see that line in patch 2, maybe the change upload is stale?
> > 
> > With current patch 2 it doesn't look like we are doing any save/restore at
> > start/end of test, or at least a reset to a known safe starting value at
end, so
> > if it is changed by one test that change will persist into subsequent tests
run
> > by that test harness content shell instance, right? Or am I missing
something
> > and the initialization call does happen for every subsequent run. See my
> > comments re: other places we do a save/restore in
http://crrev.com/1525933002.
> 
> 
> The original code was
> 
> void BlinkTestRunner::SetDeviceScaleFactor(float factor) {
>   content::SetDeviceScaleFactor(render_view(), factor);
> }
> 
> and I changed it to
> 
> void BlinkTestRunner::SetDeviceScaleFactor(float factor) {
>   if (device_scale_factor_ == factor)
>     return;
>   content::SetDeviceScaleFactor(render_view(), factor);
>   ...
> }
> 
> believing that the renderer is created and initialized for 1.f for each test.
I was probably wrong, and
> I think the test harness expects this is called at least once with 1.f.
> 
> > 
> > Resetting internal state mostly looks to happen today through
> > BlinkTestRunner::Reset() which ends up in
> > WebCoreTestsuppoer::resetInternalsObject() which calls into things like
> > Internals::resetToConsistentState() and
> > InternalSettings::from(*page)->resetToConsistentState().
> > 
> > So adding the test harness initialization (presuming change is just stale or
I
> > am missing it and it does exist) seems like a good theory to fix #2 (missing
> > Windows images) but #1 is still at risk.
> 
> The test harness is calling this from here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/test_ru...
> 
> In patch set 2, I changed it to
> 
> void BlinkTestRunner::SetDeviceScaleFactor(float factor) {
>   content::SetDeviceScaleFactor(render_view(), factor);
>   if (device_scale_factor_ == factor)
>     return;
>   ...
> }
> 
> to keep the original behavior.

Ah, I see, thanks for the explanation. Since we still skip the newly-added:

Send(new ShellViewHostMsg_SetDeviceScaleFactor(routing_id(), factor));

which looks like it's what's responsible eventually for the call to:

host_view->SetSize(host_view->GetViewBounds().size());

Does something already make sure the host view is reset to a standard size? I
would not expect this to cause layout test failures if we're now resetting the
dsf, but it could lead to some other issue.

It could also be worth sanity-checking the layout-size variability issue we saw
previously by running all layout tests locally with --repeat-each=5 or similar
and making sure no unexpected errors occur, as that should reuse test runners in
a similar manner to what we see on the try bots.

[wkorman, 2016-01-04 23:03:45]

https://codereview.chromium.org/1555163002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (left):

https://codereview.chromium.org/1555163002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:226: # FIXME issue 570401, FIXME
dsf2 is an abbrv, fix the test name per style.
Part of this FIXME reads as if it involves renaming the tests so that 'dsf2' is
written out in words. Probably we should either leave that FIXME, open a
separate bug to track that, or do it as part of this change?

[commit-bot: I haz the power, 2016-01-04 23:12:21]

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

[commit-bot: I haz the power, 2016-01-04 23:12:21]

Dry run: This issue passed the CQ dry run.

[oshima, 2016-01-04 23:24:58]

On 2016/01/04 23:03:37, wkorman wrote:
> On 2016/01/04 at 22:52:55, oshima wrote:
> > On 2016/01/04 22:08:40, wkorman wrote:
> > > On 2016/01/04 at 21:55:06, oshima wrote:
> > > > On 2016/01/04 21:51:29, wkorman wrote:
> > > > > Can you briefly describe what changes were made to address the two
> issues
> > > that
> > > > > came up when this patch was landed, note in my comments here:
> > > > > 
> > > > > http://crbug.com/570401#c61
> > > > 
> > > > See the patch set 2. My current hypothesis is that test harness requires
> > > initialization
> > > > 
> > > > content::SetDeviceScaleFactor(render_view(), 1.0f);
> > > 
> > > I actually don't see that line in patch 2, maybe the change upload is
stale?
> > > 
> > > With current patch 2 it doesn't look like we are doing any save/restore at
> > > start/end of test, or at least a reset to a known safe starting value at
> end, so
> > > if it is changed by one test that change will persist into subsequent
tests
> run
> > > by that test harness content shell instance, right? Or am I missing
> something
> > > and the initialization call does happen for every subsequent run. See my
> > > comments re: other places we do a save/restore in
> http://crrev.com/1525933002.
> > 
> > 
> > The original code was
> > 
> > void BlinkTestRunner::SetDeviceScaleFactor(float factor) {
> >   content::SetDeviceScaleFactor(render_view(), factor);
> > }
> > 
> > and I changed it to
> > 
> > void BlinkTestRunner::SetDeviceScaleFactor(float factor) {
> >   if (device_scale_factor_ == factor)
> >     return;
> >   content::SetDeviceScaleFactor(render_view(), factor);
> >   ...
> > }
> > 
> > believing that the renderer is created and initialized for 1.f for each
test.
> I was probably wrong, and
> > I think the test harness expects this is called at least once with 1.f.
> > 
> > > 
> > > Resetting internal state mostly looks to happen today through
> > > BlinkTestRunner::Reset() which ends up in
> > > WebCoreTestsuppoer::resetInternalsObject() which calls into things like
> > > Internals::resetToConsistentState() and
> > > InternalSettings::from(*page)->resetToConsistentState().
> > > 
> > > So adding the test harness initialization (presuming change is just stale
or
> I
> > > am missing it and it does exist) seems like a good theory to fix #2
(missing
> > > Windows images) but #1 is still at risk.
> > 
> > The test harness is calling this from here:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/test_ru...
> > 
> > In patch set 2, I changed it to
> > 
> > void BlinkTestRunner::SetDeviceScaleFactor(float factor) {
> >   content::SetDeviceScaleFactor(render_view(), factor);
> >   if (device_scale_factor_ == factor)
> >     return;
> >   ...
> > }
> > 
> > to keep the original behavior.
> 
> Ah, I see, thanks for the explanation. Since we still skip the newly-added:
> 
> Send(new ShellViewHostMsg_SetDeviceScaleFactor(routing_id(), factor));
> 
> which looks like it's what's responsible eventually for the call to:
> 
> host_view->SetSize(host_view->GetViewBounds().size());
> 
> Does something already make sure the host view is reset to a standard size?

The Reset() is called after test run, and the next test won't start until
messages before
ResetDone messages are handled, so the host should be reset to 1.0f if it was
2.f.

The renderer size is reset by the call content::SetDeviceScaleFactor(1.f), which
calls

{RenderViewImpl|RenderWidget}::OnResize

> I
> would not expect this to cause layout test failures if we're now resetting the
> dsf, but it could lead to some other issue.
> 
> It could also be worth sanity-checking the layout-size variability issue we
saw
> previously by running all layout tests locally with --repeat-each=5 or similar
> and making sure no unexpected errors occur, as that should reuse test runners
in
> a similar manner to what we see on the try bots.

sure.

[wkorman, 2016-01-04 23:28:25]

lgtm

LGTM please also see my prior comment re: the dsf test rename FIXME. Could just
leave that bit in TestExpectations or open a new bug.

[Noel Gordon, 2016-01-04 23:31:45]

https://code.google.com/p/chromium/issues/detail?id=567837#c2 was good advice
for implementing popup DSF tests.  Why was that advice not followed?

[oshima, 2016-01-05 00:03:52]

https://codereview.chromium.org/1555163002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (left):

https://codereview.chromium.org/1555163002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:226: # FIXME issue 570401, FIXME
dsf2 is an abbrv, fix the test name per style.
On 2016/01/04 23:03:44, wkorman wrote:
> Part of this FIXME reads as if it involves renaming the tests so that 'dsf2'
is
> written out in words. Probably we should either leave that FIXME, open a
> separate bug to track that, or do it as part of this change?

I didn't know about the style, thanks. I'm happy to change, but let me file a
separate bug, as I need to update unrelated files.

[oshima, 2016-01-05 00:12:30]

Just passing command line didn't work, partly because the layout test
wasn't using command line flag, and also because the test harness was using
different code path to position the popup window, so I abandoned the idea
at that point. CL was the result of investigation and it eventually got
lgtm from tkent@, but I probably should have revisited the approach. I'm
happy to change if that's still desirable. One downside is that there are
now two ways to change the device scale factor in layout tests, depending
on what to test. Maybe all dsf tests should use command line flag instead.
I'll confirm with him.


On Mon, Jan 4, 2016 at 3:31 PM, <noel@chromium.org> wrote:

> https://code.google.com/p/chromium/issues/detail?id=567837#c2 was good
> advice
> for implementing popup DSF tests.  Why was that advice not followed?
>
> https://codereview.chromium.org/1555163002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[oshima, 2016-01-05 00:12:31]

Just passing command line didn't work, partly because the layout test
wasn't using command line flag, and also because the test harness was using
different code path to position the popup window, so I abandoned the idea
at that point. CL was the result of investigation and it eventually got
lgtm from tkent@, but I probably should have revisited the approach. I'm
happy to change if that's still desirable. One downside is that there are
now two ways to change the device scale factor in layout tests, depending
on what to test. Maybe all dsf tests should use command line flag instead.
I'll confirm with him.


On Mon, Jan 4, 2016 at 3:31 PM, <noel@chromium.org> wrote:

> https://code.google.com/p/chromium/issues/detail?id=567837#c2 was good
> advice
> for implementing popup DSF tests.  Why was that advice not followed?
>
> https://codereview.chromium.org/1555163002/
>

-- 
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.

[Noel Gordon, 2016-01-05 01:16:36]

On 2016/01/05 00:12:31, oshima wrote:
> Just passing command line didn't work, partly because the layout test
> wasn't using command line flag, and also because the test harness was using
> different code path to position the popup window, so I abandoned the idea
> at that point. CL was the result of investigation and it eventually got
> lgtm from tkent@, but I probably should have revisited the approach.
> I'm happy to change if that's still desirable. One downside is that there are
> now two ways to change the device scale factor in layout tests, depending
> on what to test.

Three thoughts.

1) re: renaming the 'dsf2' tests to avoid abbrv: the new test in this CL are
already in fast/hidpi so the 'dsf2' seems somewhat redundant.  Was there some
reason other you added 'dsf2' to these test names?

2) We have existing layout tests that set the device scale factor during a
layout test, right?
Are they broken somehow? 

3) A similar issue came up for color profile tests btw: setting the device color
profile during a layout test and ensuring that it is reset to a consistent state
at the end of each test, so that it does not interfere with layout tests that
follow. See from

https://code.google.com/p/chromium/issues/detail?id=369787#c4 ...

onwards. The layout test harness code paths I had to change to ensure that
consistency seem different to the ones you are changing, but it does work in
practice, my color profile tests do not interfere with other layout tests [1]

[1] https://codereview.chromium.org/1331533002

[oshima, 2016-01-05 01:34:07]

On Mon, Jan 4, 2016 at 5:16 PM, <noel@chromium.org> wrote:

> On 2016/01/05 00:12:31, oshima wrote:
>
>> Just passing command line didn't work, partly because the layout test
>> wasn't using command line flag, and also because the test harness was
>> using
>> different code path to position the popup window, so I abandoned the idea
>> at that point. CL was the result of investigation and it eventually got
>> lgtm from tkent@, but I probably should have revisited the approach.
>> I'm happy to change if that's still desirable. One downside is that there
>> are
>> now two ways to change the device scale factor in layout tests, depending
>> on what to test.
>>
>
> Three thoughts.
>
> 1) re: renaming the 'dsf2' tests to avoid abbrv: the new test in this CL
> are
> already in fast/hidpi so the 'dsf2' seems somewhat redundant.  Was there
> some
> reason other you added 'dsf2' to these test names?
>

I'm planning to add tests for fractional scale factors, so there will be
dsf1.25, dsf1.5 etc. (and I followed zoom100, zoom125 pattern)
I can just use -2 or -1.5 if that's ok.

2) We have existing layout tests that set the device scale factor during a
> layout test, right?
> Are they broken somehow?
>

Popup creates another RenderWidget, and its location is controlled on host.
This wasn't an issue just because there was no test for this configuration.


> 3) A similar issue came up for color profile tests btw: setting the device
> color
> profile during a layout test and ensuring that it is reset to a consistent
> state
> at the end of each test, so that it does not interfere with layout tests
> that
> follow. See from
>
> https://code.google.com/p/chromium/issues/detail?id=369787#c4 ...
>
> onwards. The layout test harness code paths I had to change to ensure that
> consistency seem different to the ones you are changing, but it does work
> in
> practice, my color profile tests do not interfere with other layout tests
> [1]
>
> [1] https://codereview.chromium.org/1331533002


> https://codereview.chromium.org/1555163002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[oshima, 2016-01-05 01:34:08]

On Mon, Jan 4, 2016 at 5:16 PM, <noel@chromium.org> wrote:

> On 2016/01/05 00:12:31, oshima wrote:
>
>> Just passing command line didn't work, partly because the layout test
>> wasn't using command line flag, and also because the test harness was
>> using
>> different code path to position the popup window, so I abandoned the idea
>> at that point. CL was the result of investigation and it eventually got
>> lgtm from tkent@, but I probably should have revisited the approach.
>> I'm happy to change if that's still desirable. One downside is that there
>> are
>> now two ways to change the device scale factor in layout tests, depending
>> on what to test.
>>
>
> Three thoughts.
>
> 1) re: renaming the 'dsf2' tests to avoid abbrv: the new test in this CL
> are
> already in fast/hidpi so the 'dsf2' seems somewhat redundant.  Was there
> some
> reason other you added 'dsf2' to these test names?
>

I'm planning to add tests for fractional scale factors, so there will be
dsf1.25, dsf1.5 etc. (and I followed zoom100, zoom125 pattern)
I can just use -2 or -1.5 if that's ok.

2) We have existing layout tests that set the device scale factor during a
> layout test, right?
> Are they broken somehow?
>

Popup creates another RenderWidget, and its location is controlled on host.
This wasn't an issue just because there was no test for this configuration.


> 3) A similar issue came up for color profile tests btw: setting the device
> color
> profile during a layout test and ensuring that it is reset to a consistent
> state
> at the end of each test, so that it does not interfere with layout tests
> that
> follow. See from
>
> https://code.google.com/p/chromium/issues/detail?id=369787#c4 ...
>
> onwards. The layout test harness code paths I had to change to ensure that
> consistency seem different to the ones you are changing, but it does work
> in
> practice, my color profile tests do not interfere with other layout tests
> [1]
>
> [1] https://codereview.chromium.org/1331533002


> https://codereview.chromium.org/1555163002/
>

-- 
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.

[Noel Gordon, 2016-01-05 01:50:40]

On 2016/01/05 01:34:08, oshima wrote:
> On Mon, Jan 4, 2016 at 5:16 PM, <mailto:noel@chromium.org> wrote:
> 
> > On 2016/01/05 00:12:31, oshima wrote:
> >
> >> Just passing command line didn't work, partly because the layout test
> >> wasn't using command line flag, and also because the test harness was
> >> using
> >> different code path to position the popup window, so I abandoned the idea
> >> at that point. CL was the result of investigation and it eventually got
> >> lgtm from tkent@, but I probably should have revisited the approach.
> >> I'm happy to change if that's still desirable. One downside is that there
> >> are
> >> now two ways to change the device scale factor in layout tests, depending
> >> on what to test.
> >>
> >
> > Three thoughts.
> >
> > 1) re: renaming the 'dsf2' tests to avoid abbrv: the new test in this CL
> > are
> > already in fast/hidpi so the 'dsf2' seems somewhat redundant.  Was there
> > some
> > reason other you added 'dsf2' to these test names?
> >
> 
> I'm planning to add tests for fractional scale factors, so there will be
> dsf1.25, dsf1.5 etc. (and I followed zoom100, zoom125 pattern)
> I can just use -2 or -1.5 if that's ok.

Thanks for planning new tests.  zoom-100, zoom-125 seem fine. So maybe
scale-factor-2.0 scale-factor-1.5 to make them easy to search for in chromium
code search.

[oshima, 2016-01-05 01:59:36]

On 2016/01/05 01:50:40, noel gordon wrote:
> On 2016/01/05 01:34:08, oshima wrote:
> > On Mon, Jan 4, 2016 at 5:16 PM, <mailto:noel@chromium.org> wrote:
> > 
> > > On 2016/01/05 00:12:31, oshima wrote:
> > >
> > >> Just passing command line didn't work, partly because the layout test
> > >> wasn't using command line flag, and also because the test harness was
> > >> using
> > >> different code path to position the popup window, so I abandoned the idea
> > >> at that point. CL was the result of investigation and it eventually got
> > >> lgtm from tkent@, but I probably should have revisited the approach.
> > >> I'm happy to change if that's still desirable. One downside is that there
> > >> are
> > >> now two ways to change the device scale factor in layout tests, depending
> > >> on what to test.
> > >>
> > >
> > > Three thoughts.
> > >
> > > 1) re: renaming the 'dsf2' tests to avoid abbrv: the new test in this CL
> > > are
> > > already in fast/hidpi so the 'dsf2' seems somewhat redundant.  Was there
> > > some
> > > reason other you added 'dsf2' to these test names?
> > >
> > 
> > I'm planning to add tests for fractional scale factors, so there will be
> > dsf1.25, dsf1.5 etc. (and I followed zoom100, zoom125 pattern)
> > I can just use -2 or -1.5 if that's ok.
> 
> Thanks for planning new tests.  zoom-100, zoom-125 seem fine. So maybe
> scale-factor-2.0 scale-factor-1.5 to make them easy to search for in chromium
> code search.

sounds good. I'll propose that in the crbug.com/574257.

[Noel Gordon, 2016-01-05 02:05:11]

On 2016/01/05 01:59:36, oshima wrote:
> On 2016/01/05 01:50:40, noel gordon wrote:
> > On 2016/01/05 01:34:08, oshima wrote:
> > > I'm planning to add tests for fractional scale factors, so there will be
> > > dsf1.25, dsf1.5 etc. (and I followed zoom100, zoom125 pattern)
> > > I can just use -2 or -1.5 if that's ok.
> > 
> > Thanks for planning new tests.  zoom-100, zoom-125 seem fine. So maybe
> > scale-factor-2.0 scale-factor-1.5 to make them easy to search for in
chromium
> > code search.
> 
> sounds good. I'll propose that in the crbug.com/574257.

Ok good.

[Noel Gordon, 2016-01-05 02:22:20]

On 2016/01/05 01:34:08, oshima wrote:
> On Mon, Jan 4, 2016 at 5:16 PM, <mailto:noel@chromium.org> wrote:
> 2) We have existing layout tests that set the device scale factor during a
> > layout test, right?
> > Are they broken somehow?
 
> This wasn't an issue just because there was no test for this configuration.

Glad we are adding tests, the device scale factor implementation in Blink needs
more tests :)

> Popup creates another RenderWidget, and its location is controlled on host.

IC. The Popup's RenderWidget will always be positioned somewhere over the main
page RenderWidget I expect, so perhaps you could just fake the location for
testing purposes?

Also, is the main page RenderWidget scale factor correctly propagated to the
Popup's RenderWidget?  I imagine they should be the same if the Popup is placed
over the main page.

[oshima, 2016-01-05 03:05:07]

On 2016/01/05 02:22:20, noel gordon wrote:
> On 2016/01/05 01:34:08, oshima wrote:
> > On Mon, Jan 4, 2016 at 5:16 PM, <mailto:noel@chromium.org> wrote:
> > 2) We have existing layout tests that set the device scale factor during a
> > > layout test, right?
> > > Are they broken somehow?
>  
> > This wasn't an issue just because there was no test for this configuration.
> 
> Glad we are adding tests, the device scale factor implementation in Blink
needs
> more tests :)
> 
> > Popup creates another RenderWidget, and its location is controlled on host.
> 
> IC. The Popup's RenderWidget will always be positioned somewhere over the main
> page RenderWidget I expect, so perhaps you could just fake the location for
> testing purposes?

Sorry I didn't fully understand what you meant by "fake the location". Can you
elaborate?

> Also, is the main page RenderWidget scale factor correctly propagated to the
> Popup's RenderWidget?  I imagine they should be the same if the Popup is
placed
> over the main page.

Yes. The popup's renderer is created with the main renderer's scale factor, but
final bounds, screen info
etc comes from its render host (RenderWidget::OnResize). In real situation,
these dsfs are the same. In layout tests,
existing device scale factor tests only change the device scale factor on
renderer, and that was one of reasons why
test wasn't working. (another reason is that the test harness code that computes
the location does not take the scale factor into consideration.)

[oshima, 2016-01-07 20:31:37]

We decided to go with VirtualTestSuite. I'll abandon this CL and create new one.
Thanks.

[oshima, 2016-01-07 20:41:52]

Description was changed from

==========
Revert "Revert of Support device scale factor test with popups in LayoutTest
(patchset #2 id:40001 of https://codereview.chromium.org/1525933002/ )"

Reland of https://codereview.chromium.org/1525933002 with hypothentical fix.
>> Support device scale factor test with popups in LayoutTest
>>
>> * Sends DSF to host so that host can send a resize request with requested
DSF.
>> * Updated X11's screen so that displays contains specified DSF.
>>   This is used by RWH to construct WebScreenInfo with updated DSF.
>> - Win/Mac will use the DSF specified by command line flag.
>>   * Scale the popup location based on the device scale factor.
>>   This is done by compositor in normal scenario.

BUG=567837
TEST=added popu-menu-appearance-dsf2.html
TBR=mkwst@chromium.org,sadrul@chromium.org,tkent@chromium.org,wkorman@chromiu...
R=noel@chromium.org
==========

to

==========
Revert "Revert of Support device scale factor test with popups in LayoutTest
(patchset #2 id:40001 of https://codereview.chromium.org/1525933002/ )"

Reland of https://codereview.chromium.org/1525933002 with hypothentical fix.
>> Support device scale factor test with popups in LayoutTest
>>
>> * Sends DSF to host so that host can send a resize request with requested
DSF.
>> * Updated X11's screen so that displays contains specified DSF.
>>   This is used by RWH to construct WebScreenInfo with updated DSF.
>> - Win/Mac will use the DSF specified by command line flag.
>>   * Scale the popup location based on the device scale factor.
>>   This is done by compositor in normal scenario.

BUG=567837
TEST=added popu-menu-appearance-dsf2.html
TBR=mkwst@chromium.org,sadrul@chromium.org,tkent@chromium.org,wkorman@chromiu...
R=noel@chromium.org
==========

[Noel Gordon, 2016-01-08 03:09:04]

On 2016/01/05 03:05:07, oshima wrote:
> On 2016/01/05 02:22:20, noel gordon wrote:
> > On 2016/01/05 01:34:08, oshima wrote:
> > > On Mon, Jan 4, 2016 at 5:16 PM, <mailto:noel@chromium.org> wrote:
> > > 2) We have existing layout tests that set the device scale factor during a
> > > > layout test, right?
> > > > Are they broken somehow?
> >  
> > > This wasn't an issue just because there was no test for this
configuration.
> > 
> > Glad we are adding tests, the device scale factor implementation in Blink
> needs
> > more tests :)
> > 
> > > Popup creates another RenderWidget, and its location is controlled on
host.
> > 
> > IC. The Popup's RenderWidget will always be positioned somewhere over the
main
> > page RenderWidget I expect, so perhaps you could just fake the location for
> > testing purposes?
> 
> Sorry I didn't fully understand what you meant by "fake the location". Can you
> elaborate?

I mean just invent the location (10px, 10px) say, an avoid needing to have host
involvement.

After looking at the approach in this CL is more detail, I see that you could
not just invent a location since you were communicating with the host to
determine the DPI-aware location.

[Noel Gordon, 2016-01-08 03:14:39]

On 2016/01/05 03:05:07, oshima wrote:
> On 2016/01/05 02:22:20, noel gordon wrote:
> > Also, is the main page RenderWidget scale factor correctly propagated to the
> > Popup's RenderWidget?  I imagine they should be the same if the Popup is
> placed
> > over the main page.
> 
> Yes. The popup's renderer is created with the main renderer's scale factor,
but
> final bounds, screen info
> etc comes from its render host (RenderWidget::OnResize). In real situation,
> these dsfs are the same.

Yes, RenderWidget::OnResize provides the DPI (device scale factor).

In layout tests,
> existing device scale factor tests only change the device scale factor on
> renderer, and that was one of reasons why
> test wasn't working.

It also does not set the scale factor by calling through RenderWidget::OnResize
as a real browser does.  RenderWidget::OnResize has all sorts of side-effects on
RenderView/Widget state.  We should investigate / address that issue in
subsequent CLs, ok?

> (another reason is that the test harness code that computes
> the location does not take the scale factor into consideration.)

Yeah, that's silly.

[Noel Gordon, 2016-01-08 03:16:21]

On 2016/01/07 20:31:37, oshima wrote:
> We decided to go with VirtualTestSuite. I'll abandon this CL and create new
one.
> Thanks.

Sounds like a plan.

[Noel Gordon, 2016-01-08 04:47:03]

Also, we should recall that we tried decoupling scale factor updates from the
Resize IPC message in the past.  All hell broke loose

https://code.google.com/p/chromium/issues/detail?id=366848#c4

and produced all the "drawn-an-quartered" bugs,
https://code.google.com/p/chromium/issues/detail?id=239624 is one example, but
there were many others,
https://code.google.com/p/chromium/issues/detail?id=338130 and related.

Those bugs eventually got "fixed".  No tests were added on the chromium-side. 
We could regress at any moment, sadly.

[Noel Gordon, 2016-01-08 05:00:38]

On 2016/01/08 04:47:03, noel gordon wrote:
> Also, we should recall that we tried decoupling scale factor updates from the
> Resize IPC message in the past.  All hell broke loose
> 
> https://code.google.com/p/chromium/issues/detail?id=366848#c4
> 
> and produced all the "drawn-an-quartered" bugs,
> https://code.google.com/p/chromium/issues/detail?id=239624 is one example, but
> there were many others,
> https://code.google.com/p/chromium/issues/detail?id=338130 and related.
> 
> Those bugs eventually got "fixed".  No tests were added on the chromium-side. 
> We could regress at any moment, sadly.

Anyway, for color profiles, that's what
https://code.google.com/p/chromium/issues/detail?id=369787#c4 is all about. 
Ensuring we can test device color profile updates in layout tests _using the
code paths that a real browser uses_

The browser's WasResized() IPC causes a RenderWidget/View::OnResize.  We can not
include color profile in the WasResize() IPC flow for techinical reasons [1],
but the WasResized() IPC flow must cause a color profile IPC flow if needed.

[1] https://code.google.com/p/chromium/issues/detail?id=368663#c18

[Noel Gordon, 2016-01-08 05:25:08]

Since you are working on multi-monitor DPI, best I can tell, what's the OOPIF
story?

https://code.google.com/p/chromium/issues/detail?id=487272#6 onwards tells me
there is no story, and as a result, the compositors will happily combine content
that has different DPI and present it to the screen.

Awesome, and it causes DPI "flashing" and color profile "flashing" problems,
because out-of-process content is delayed processing their WasResized() IPC, but
the main Blink page has already processed that message for example, when VSync
arrives (time to composite and present the page).  But the DPI and colors are
not synchronized at composite time.  That causes "flashing".

[Noel Gordon, 2016-01-10 22:50:03]

On 2016/01/08 04:47:03, noel gordon wrote:
> Also, we should recall that we tried decoupling scale factor updates from the
> Resize IPC message in the past.  All hell broke loose
> 
> https://code.google.com/p/chromium/issues/detail?id=366848#c4
> 
> and produced all the "drawn-an-quartered" bugs,
> https://code.google.com/p/chromium/issues/detail?id=239624 is one example, but
> there were many others,
> https://code.google.com/p/chromium/issues/detail?id=338130 and related.
> 
> Those bugs eventually got "fixed".  No tests were added on the chromium-side. 
> We could regress at any moment, sadly.

And sadly, https://code.google.com/p/chromium/issues/detail?id=338130#c52
suggest users still experience that problem.