Bug 84827 - Make calendar pickers testable
Summary: Make calendar pickers testable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on: 92605 92609
Blocks: 53961
  Show dependency treegraph
 
Reported: 2012-04-24 22:33 PDT by Kent Tamura
Modified: 2012-07-30 20:50 PDT (History)
8 users (show)

See Also:


Attachments
WIP (50.50 KB, patch)
2012-04-24 23:02 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch (66.22 KB, patch)
2012-04-25 02:10 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (70.10 KB, patch)
2012-04-25 02:26 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (70.28 KB, patch)
2012-04-26 04:32 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 4 (58.03 KB, patch)
2012-05-30 03:51 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 5 (60.40 KB, patch)
2012-07-12 01:25 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 6 (60.47 KB, patch)
2012-07-12 01:56 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch for landing (60.51 KB, patch)
2012-07-13 02:32 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-04-24 22:33:27 PDT
Make calendar pickers testable
Comment 1 Kent Tamura 2012-04-24 23:02:51 PDT
Created attachment 138741 [details]
WIP
Comment 2 Kent Tamura 2012-04-25 02:10:43 PDT
Created attachment 138762 [details]
Patch
Comment 3 Early Warning System Bot 2012-04-25 02:16:14 PDT
Comment on attachment 138762 [details]
Patch

Attachment 138762 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12535200
Comment 4 Gustavo Noronha (kov) 2012-04-25 02:16:35 PDT
Comment on attachment 138762 [details]
Patch

Attachment 138762 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12490496
Comment 5 Early Warning System Bot 2012-04-25 02:17:40 PDT
Comment on attachment 138762 [details]
Patch

Attachment 138762 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12524615
Comment 6 Build Bot 2012-04-25 02:19:22 PDT
Comment on attachment 138762 [details]
Patch

Attachment 138762 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12520496
Comment 7 Kent Tamura 2012-04-25 02:26:02 PDT
Created attachment 138765 [details]
Patch 2

build fix
Comment 8 WebKit Review Bot 2012-04-25 02:29:33 PDT
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.
Comment 9 Build Bot 2012-04-25 02:35:02 PDT
Comment on attachment 138765 [details]
Patch 2

Attachment 138765 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12526509
Comment 10 Kentaro Hara 2012-04-25 09:06:08 PDT
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()?
Comment 11 Kent Tamura 2012-04-26 04:03:27 PDT
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.
Comment 12 Kent Tamura 2012-04-26 04:32:32 PDT
Created attachment 138975 [details]
Patch 2

Mac build fix, rebaseline
Comment 13 Kent Tamura 2012-05-29 17:49:24 PDT
Comment on attachment 138975 [details]
Patch 2

Need to update the patch because window.setValueAndClosePopup() was moved.
Comment 14 Kent Tamura 2012-05-30 03:51:27 PDT
Created attachment 144777 [details]
Patch 4
Comment 15 Hajime Morrita 2012-05-31 18:14:22 PDT
(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.
Comment 16 Kent Tamura 2012-07-12 01:25:52 PDT
Created attachment 151884 [details]
Patch 5
Comment 17 Kent Tamura 2012-07-12 01:30:11 PDT
(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.
Comment 18 Build Bot 2012-07-12 01:46:25 PDT
Comment on attachment 151884 [details]
Patch 5

Attachment 151884 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13221139
Comment 19 Build Bot 2012-07-12 01:48:21 PDT
Comment on attachment 151884 [details]
Patch 5

Attachment 151884 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13182707
Comment 20 Early Warning System Bot 2012-07-12 01:56:05 PDT
Comment on attachment 151884 [details]
Patch 5

Attachment 151884 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13180742
Comment 21 Kent Tamura 2012-07-12 01:56:18 PDT
Created attachment 151888 [details]
Patch 6

Build fix for !ENABLE_PAGE_POPUP
Comment 22 Hajime Morrita 2012-07-13 01:57:21 PDT
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 ":"
Comment 23 Kent Tamura 2012-07-13 02:23:47 PDT
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
Comment 24 Kent Tamura 2012-07-13 02:32:29 PDT
Created attachment 152193 [details]
Patch for landing
Comment 25 WebKit Review Bot 2012-07-13 03:36:10 PDT
Comment on attachment 152193 [details]
Patch for landing

Clearing flags on attachment: 152193

Committed r122558: <http://trac.webkit.org/changeset/122558>
Comment 26 WebKit Review Bot 2012-07-13 03:36:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 Peter Kasting 2012-07-28 10:54:44 PDT
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.
Comment 28 Kent Tamura 2012-07-30 20:50:56 PDT
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