Bug 84827

Created attachment 138741 [details] WIP

Created attachment 138762 [details] Patch

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

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

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

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

Created attachment 138765 [details] Patch 2 build fix

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 on attachment 138765 [details] Patch 2 Attachment 138765 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12526509

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

Created attachment 138975 [details] Patch 2 Mac build fix, rebaseline

Comment on attachment 138975 [details] Patch 2 Need to update the patch because window.setValueAndClosePopup() was moved.

Created attachment 144777 [details] Patch 4

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

Created attachment 151884 [details] Patch 5

(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 on attachment 151884 [details] Patch 5 Attachment 151884 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13221139

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

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

Created attachment 151888 [details] Patch 6 Build fix for !ENABLE_PAGE_POPUP

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

Created attachment 152193 [details] Patch for landing

Comment on attachment 152193 [details] Patch for landing Clearing flags on attachment: 152193 Committed r122558: <http://trac.webkit.org/changeset/122558>

All reviewed patches have been landed. Closing bug.

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.

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