Bug 84827 – Make calendar pickers testable

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Kent Tamura]

Make calendar pickers testable

[Kent Tamura]

Created attachment 138741 [details]
WIP

[Kent Tamura]

Created attachment 138762 [details]
Patch

[Early Warning System Bot]

Comment on attachment 138762 [details]
Patch

Attachment 138762 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12535200

[Gustavo Noronha (kov)]

Comment on attachment 138762 [details]
Patch

Attachment 138762 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12490496

[Early Warning System Bot]

Comment on attachment 138762 [details]
Patch

Attachment 138762 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12524615

[Build Bot]

Comment on attachment 138762 [details]
Patch

Attachment 138762 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12520496

[Kent Tamura]

Created attachment 138765 [details]
Patch 2

build fix

[WebKit Review Bot]

Attachment 138765 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/ChangeLog:21:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 138765 [details]
Patch 2

Attachment 138765 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12526509

[Kentaro Hara]

Comment on attachment 138765 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=138765&action=review

Just nit-picking comments.

> Source/WebCore/ChangeLog:14
> +        Make Internals a PagePopupDriver. It cretes a MockPagePopup, and the

Nit: cre*a*tes

> Source/WebCore/page/PagePopupDriver.h:44
> +    virtual ~PagePopupDriver() { }

Nit: We write '{' and '}' in the separate line. Although the coding style is a bit unclear from the description in "Other Punctuation - 1.", I was often told to do so.

> Source/WebCore/testing/Internals.cpp:1024
> +        closePagePopup(m_mockPagePopup.get());

Nit: You can just call m_mockPagePopup.clear()?

[Kent Tamura]

Comment on attachment 138765 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=138765&action=review

>> Source/WebCore/page/PagePopupDriver.h:44
>> +    virtual ~PagePopupDriver() { }
> 
> Nit: We write '{' and '}' in the separate line. Although the coding style is a bit unclear from the description in "Other Punctuation - 1.", I was often told to do so.

I don't think we should put them in the separated line.  This is common style to define one-line function.

>> Source/WebCore/testing/Internals.cpp:1024
>> +        closePagePopup(m_mockPagePopup.get());
> 
> Nit: You can just call m_mockPagePopup.clear()?

Yes for now.  However we might add something in closePagePopup().  So this code is safer.

[Kent Tamura]

Created attachment 138975 [details]
Patch 2

Mac build fix, rebaseline

[Kent Tamura]

Comment on attachment 138975 [details]
Patch 2

Need to update the patch because window.setValueAndClosePopup() was moved.

[Kent Tamura]

Created attachment 144777 [details]
Patch 4

[Hajime Morrita]

(In reply to comment #14)
> Created an attachment (id=144777) [details]
> Patch 4

Why not make PagePopup (or MockPagePopup) directly accessible as a JS API?
It's clearly eligible for having an object.

[Kent Tamura]

Created attachment 151884 [details]
Patch 5

[Kent Tamura]

(In reply to comment #15)
> (In reply to comment #14)
> > Created an attachment (id=144777) [details] [details]
> > Patch 4
> 
> Why not make PagePopup (or MockPagePopup) directly accessible as a JS API?
> It's clearly eligible for having an object.

I don't think exposing PagePopup is helpful because it has no member functions and its implementation is platform-dependent.

[Build Bot]

Comment on attachment 151884 [details]
Patch 5

Attachment 151884 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13221139

[Build Bot]

Comment on attachment 151884 [details]
Patch 5

Attachment 151884 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13182707

[Early Warning System Bot]

Comment on attachment 151884 [details]
Patch 5

Attachment 151884 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13180742

[Kent Tamura]

Created attachment 151888 [details]
Patch 6

Build fix for !ENABLE_PAGE_POPUP

[Hajime Morrita]

Comment on attachment 151888 [details]
Patch 6

View in context: https://bugs.webkit.org/attachment.cgi?id=151888&action=review

Cool! this is a great example of "creating mocks in internals" approach.

> Source/WebCore/testing/MockPagePopupDriver.cpp:77
> +    WebCoreTestSupport::injectPagePopupController(contentFrame, m_controller.get());

Why not just make "pagePopupController" an attribute of Internals? Then we can get rid of V8 custom part.

> Source/WebKit/chromium/src/WebViewImpl.h:117
> +                    public WebLayerTreeViewClient,

Nit: commas should be aligned to ":"

[Kent Tamura]

Comment on attachment 151888 [details]
Patch 6

View in context: https://bugs.webkit.org/attachment.cgi?id=151888&action=review

>> Source/WebCore/testing/MockPagePopupDriver.cpp:77
>> +    WebCoreTestSupport::injectPagePopupController(contentFrame, m_controller.get());
> 
> Why not just make "pagePopupController" an attribute of Internals? Then we can get rid of V8 custom part.

You mean
 - Add internals.pagePopupController
 - inject the following JS code to the popup document
        window.pagePopupController = internals.pagePopupController;
right?

Hmm, it might be possible.  Let me try it in a separated patch.

>> Source/WebKit/chromium/src/WebViewImpl.h:117
>> +                    public WebLayerTreeViewClient,
> 
> Nit: commas should be aligned to ":"

will do.  thanks

[Kent Tamura]

Created attachment 152193 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 152193 [details]
Patch for landing

Clearing flags on attachment: 152193

Committed r122558: <http://trac.webkit.org/changeset/122558>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Peter Kasting]

I was going to rebaseline this for all Chromium ports, but the "actual" images look strange -- at least in garden-o-matic, all the bots show pickers with different opacities, as if the picker was fading in or out and then got snapshotted in the midst of that.

Also, the Mac 10.6 dbg and Win dbg canaries show a form field with the picker below it, while the other bots just show the picker.  This seems inconsistent.

Can you check whether this is really working correctly and perhaps go ahead and rebaseline if it actually is?  Reopening so this doesn't get lost.

[Kent Tamura]

The test was fixed by:
  http://trac.webkit.org/changeset/123993
  http://trac.webkit.org/changeset/124144

Rebaseline was done by http://trac.webkit.org/changeset/124170