Use page popup pixel readback for calendar-picker-appearance.html

Use page popup pixel readback for calendar-picker-appearance.html

Use openPickerWithoutMock to test using real page popups and not the iframe mock.

window.internals.pagePopupWindow returns the window object of the page popup.

BUG=409503
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182909

[keishi, 2014-09-01 04:36:28]

keishi@chromium.org changed reviewers:
+ tkent@chromium.org

[keishi, 2014-09-01 04:36:29]

CL will be landed in this order
https://codereview.chromium.org/513723003/
https://codereview.chromium.org/508063002/
https://codereview.chromium.org/530653002/ (this)

[tkent, 2014-09-01 05:38:50]

On 2014/09/01 04:36:29, keishi wrote:
> CL will be landed in this order
> https://codereview.chromium.org/513723003/
> https://codereview.chromium.org/508063002/
> https://codereview.chromium.org/530653002/ (this)

Looks like this is a subset of the first one.

[keishi, 2014-09-04 13:14:51]

On 2014/09/01 05:38:50, tkent wrote:
> On 2014/09/01 04:36:29, keishi wrote:
> > CL will be landed in this order
> > https://codereview.chromium.org/513723003/
> > https://codereview.chromium.org/508063002/
> > https://codereview.chromium.org/530653002/ (this)
> 
> Looks like this is a subset of the first one.

Sorry, fixed.

[keishi, 2014-09-19 10:00:11]

I had to use some hacks because resize events aren't firing for page popups.

In a layout test owner page is dispatched like this.
Document::enqueueResizeEvent calls ScriptedAnimationController::enqueueEvent
which calls ScriptedAnimationController::scheduleAnimationIfNeeded so
WebWidgetClient::scheduleAnimation gets called.
WebTestProxyBase::ScheduleAnimation sets up a task to call
WebViewImpl::beginFrame which will begin an animation frame.
And ScriptedAnimationController::dispatchEvents gets called.

blink::ScriptedAnimationController::dispatchEvents()
blink::ScriptedAnimationController::serviceScriptedAnimations(double)
blink::Document::serviceScriptedAnimations(double)
blink::PageAnimator::serviceScriptedAnimations(double)
blink::PageWidgetDelegate::animate(blink::Page*, double, blink::LocalFrame*)
blink::WebViewImpl::beginFrame(blink::WebBeginFrameArgs const&)
blink::WebWidget::animate(double)
content::WebTestProxyBase::AnimateNow()
content::(anonymous namespace)::HostMethodTask::RunIfValid()
content::WebMethodTask<content::WebTestProxyBase>::run()
base::internal::RunnableAdapter<void
(content::WebTask::*)()>::Run(content::WebTask*)

WebTestProxy doesn't exist for the page popup so resize events are not firing.

I'm still trying to understand why we need to do this special handling for
layout tests only.

[keishi, 2014-09-25 06:34:19]

The resize event needs to fire properly to be acceptable as a test, so I made
this new patch set.
I need this chromium side change to go with this patch set.
https://codereview.chromium.org/603873002

When the resize event is enqueued to the ScriptedAnimationController,
PagePopupChromeClient::scheduleAnimation is called.
We need it to schedule an animation frame so I changed
PagePopupChromeClient::scheduleAnimation to call scheduleAnimation on the owner
web view which causes WebTestProxy::scheduleAnimation to be called.

With this chromium side change, WebTestProxy will begin a frame for itself and
the page popup.
https://codereview.chromium.org/603873002

When beginFrame is called ScriptedAnimationController::dispatchEvents is called
and the resize event is fired.

PTAL

[keishi, 2014-09-29 06:02:45]

Uploaded new patch that uses LayoutTestSupport. PTAL. Thanks.

[tkent, 2014-09-29 06:20:45]

https://codereview.chromium.org/530653002/diff/200001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/530653002/diff/200001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:149: #crbug.com/340522
fast/forms/calendar-picker/calendar-picker-appearance.html [ NeedsRebaseline ]
Do you want to do rebaseline, or not?

https://codereview.chromium.org/530653002/diff/200001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/calendar-picker/calendar-picker-appearance.html
(left):

https://codereview.chromium.org/530653002/diff/200001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/calendar-picker/calendar-picker-appearance.html:8:
<script src="../../forms/resources/picker-common.js"></script>
We should have additional utility JS functions to support non-mock case.  e.g. 
openPickerWithoutMock().

https://codereview.chromium.org/530653002/diff/200001/public/platform/WebComp...
File public/platform/WebCompositeAndReadbackAsyncCallback.h (right):

https://codereview.chromium.org/530653002/diff/200001/public/platform/WebComp...
public/platform/WebCompositeAndReadbackAsyncCallback.h:38: void setIsPopup(bool
isPopup) { m_isPopup = isPopup; };
Is this necessary?

[keishi, 2014-09-29 12:48:45]

https://codereview.chromium.org/530653002/diff/200001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/530653002/diff/200001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:149: #crbug.com/340522
fast/forms/calendar-picker/calendar-picker-appearance.html [ NeedsRebaseline ]
On 2014/09/29 06:20:45, tkent (high review load) wrote:
> Do you want to do rebaseline, or not?
> 

Removed. Appearance should remain the same.

https://codereview.chromium.org/530653002/diff/200001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/calendar-picker/calendar-picker-appearance.html
(left):

https://codereview.chromium.org/530653002/diff/200001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/calendar-picker/calendar-picker-appearance.html:8:
<script src="../../forms/resources/picker-common.js"></script>
On 2014/09/29 06:20:45, tkent (high review load) wrote:
> We should have additional utility JS functions to support non-mock case.  e.g.

> openPickerWithoutMock().
> 

Done.

https://codereview.chromium.org/530653002/diff/200001/public/platform/WebComp...
File public/platform/WebCompositeAndReadbackAsyncCallback.h (right):

https://codereview.chromium.org/530653002/diff/200001/public/platform/WebComp...
public/platform/WebCompositeAndReadbackAsyncCallback.h:38: void setIsPopup(bool
isPopup) { m_isPopup = isPopup; };
On 2014/09/29 06:20:45, tkent (high review load) wrote:
> Is this necessary?

Sorry, got mixed in while rebase.

[tkent, 2014-09-29 13:08:00]

lgtm

We need to disable the test until https://codereview.chromium.org/603873002 is
landed.

> Use page popup pixel readback for calendar-picker-appearance.html

Please add more explanation.  For example, adding openPickerWithoutMock() is a
notable change.

[keishi, 2014-09-30 07:27:43]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-09-30 07:27:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/530653002/220001

[commit-bot: I haz the power, 2014-09-30 08:31:47]

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

[commit-bot: I haz the power, 2014-09-30 08:31:48]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[keishi, 2014-09-30 09:06:30]

https://codereview.chromium.org/530653002/diff/200001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/530653002/diff/200001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:149: #crbug.com/340522
fast/forms/calendar-picker/calendar-picker-appearance.html [ NeedsRebaseline ]
On 2014/09/29 12:48:44, keishi wrote:
> On 2014/09/29 06:20:45, tkent (high review load) wrote:
> > Do you want to do rebaseline, or not?
> > 
> 
> Removed. Appearance should remain the same.

Turned out that the triangle icon in the next/prev month button was off by one
pixel in the mock. I confirmed that the new result after this CL reproduces the
actual result.

[keishi, 2014-09-30 09:06:36]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-09-30 09:07:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/530653002/240001

[commit-bot: I haz the power, 2014-09-30 11:20:42]

Committed patchset #13 (id:240001) as 182909