Towards making PopupMenuClient more testable
Created attachment 110151 [details] Patch
Comment on attachment 110151 [details] Patch Attachment 110151 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10007233 New failing tests: fast/dom/select-element-height.html
Comment on attachment 110151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110151&action=review I like the general pattern, but see comments for nits. > Source/WebCore/rendering/RenderObject.h:301 > + virtual PopupMenuClient* popupMenuClient() { return 0; } popupMenuClient() should only be present in derived classes, not here. > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:618 > + return absoluteBoundingBoxRect(true); Aren't we using an enum now? > Source/WebCore/testing/Internals.idl:69 > + int popupClientPaddingLeft(in Element element) raises (DOMException); > + int popupClientPaddingRight(in Element element) raises (DOMException); Is it really useful to test paddingLeft and paddingRight?
Comment on attachment 110151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110151&action=review >> Source/WebCore/rendering/RenderObject.h:301 >> + virtual PopupMenuClient* popupMenuClient() { return 0; } > > popupMenuClient() should only be present in derived classes, not here. I'm not sure how to get around this. There is an issue with casting and multiple inheritance. RenderMenuList uses multiple inheritance. If I cast RenderObject to PopupMenuClient in Internals.cpp, it might not do the necessary thunking: http://en.wikipedia.org/wiki/Virtual_method_table because it has no idea where the appropriate vtable is. I suppose if I rearrange public RenderDeprecatedFlexibleBox, public ListPopupMenuClient to public ListPopupMenuClient, public RenderDeprecatedFlexibleBox the problem will fix itself (depending on compiler) but this would make the code very sensitive to breakage in the future that will be very hard to debug. >> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:618 >> + return absoluteBoundingBoxRect(true); > > Aren't we using an enum now? No. The signature is absoluteBoundingBoxRect(bool useTransforms = true), alternatively there's an absoluteBoundingBoxRectIgnoringTransforms(). See https://bugs.webkit.org/show_bug.cgi?id=69009 >> Source/WebCore/testing/Internals.idl:69 >> + int popupClientPaddingRight(in Element element) raises (DOMException); > > Is it really useful to test paddingLeft and paddingRight? I believe so, at some point you asked me to make sure they take page scale factor into account for this patch: https://bugs.webkit.org/show_bug.cgi?id=66062
Created attachment 110592 [details] Patch
Comment on attachment 110592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110592&action=review > Source/WebCore/rendering/RenderMenuList.cpp:307 > + m_popup->show(boundingBoxRect(), document()->view(), > select->optionToListIndex(select->selectedIndex())); You could unwrap this line. > Source/WebCore/testing/Internals.cpp:457 > + return reinterpret_cast<PopupMenuClient*>(object); This should be a static_cast too. > Source/WebCore/testing/Internals.cpp:472 > + return 0; > + > +} Extra blank line. > Source/WebCore/testing/Internals.h:107 > + static const PopupMenuClient* toPopupMenuClient(RenderObject*); I don't think there's any benefit in this being a class method. It could just be static in the impl.
Created attachment 110601 [details] Patch for landing
Comment on attachment 110592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110592&action=review >> Source/WebCore/rendering/RenderMenuList.cpp:307 >> select->optionToListIndex(select->selectedIndex())); > > You could unwrap this line. Done. >> Source/WebCore/testing/Internals.cpp:457 >> + return reinterpret_cast<PopupMenuClient*>(object); > > This should be a static_cast too. This can't be a static cast. A PopupMenuClient is not a subclass of RenderObject. >> Source/WebCore/testing/Internals.cpp:472 >> +} > > Extra blank line. Done. >> Source/WebCore/testing/Internals.h:107 >> + static const PopupMenuClient* toPopupMenuClient(RenderObject*); > > I don't think there's any benefit in this being a class method. It could just be static in the impl. Done.
Comment on attachment 110601 [details] Patch for landing Clearing flags on attachment: 110601 Committed r97202: <http://trac.webkit.org/changeset/97202>
All reviewed patches have been landed. Closing bug.
This patch broke Chromium Windows build. Build fix attempted in http://trac.webkit.org/changeset/97207.
This patch also broke Mac builds: http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/34669/steps/compile-webkit/logs/stdio Undefined symbols: "__ZN7WebCore9InlineBox14selectionStateEv", referenced from: __ZTVN7WebCore9InlineBoxE in Internals.o "__ZN7WebCore9InlineBox11nodeAtPointERKNS_14HitTestRequestERNS_13HitTestResultERKNS_8IntPointES8_ii", referenced from: __ZTVN7WebCore9InlineBoxE in Internals.o "__ZN7WebCore9InlineBox11extractLineEv", referenced from: __ZTVN7WebCore9InlineBoxE in Internals.o "__ZN7WebCore9InlineBox5paintERNS_9PaintInfoERKNS_8IntPointEii", referenced from: __ZTVN7WebCore9InlineBoxE in Internals.o "__ZN7WebCore9InlineBox7destroyEPNS_11RenderArenaE", referenced from: __ZTVN7WebCore9InlineBoxE in Internals.o "__ZN7WebCore9InlineBox14adjustPositionEff", referenced from: __ZTVN7WebCore9InlineBoxE in Internals.o "__ZN7WebCore9InlineBoxdlEPvm", referenced from: __ZN7WebCore9InlineBoxD0Ev in Internals.o "__ZNK7WebCore9InlineBox14caretMaxOffsetEv", referenced from: __ZTVN7WebCore9InlineBoxE in Internals.o "__ZN7WebCore9InlineBox22canAccommodateEllipsisEbii", referenced from: __ZTVN7WebCore9InlineBoxE in Internals.o "__ZN7WebCore9InlineBox10attachLineEv", referenced from: __ZTVN7WebCore9InlineBoxE in Internals.o "__ZN7WebCore9InlineBox16placeEllipsisBoxEbfffRb", referenced from: __ZTVN7WebCore9InlineBoxE in Internals.o "__ZN7WebCore9InlineBox10deleteLineEPNS_11RenderArenaE", referenced from: __ZTVN7WebCore9InlineBoxE in Internals.o "__ZNK7WebCore9InlineBox14caretMinOffsetEv", referenced from: __ZTVN7WebCore9InlineBoxE in Internals.o I don't think we wan to expose all these symbols from WebCore.
Note, looking at http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29, changesets listed on http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/34668 and http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/34669 are to be blamed.
Apparently including RenderMenuList.h causes Internals.cpp to use InlineBox's destructor.
Reopen the bug since the patch was rolled out.
Comment on attachment 110601 [details] Patch for landing Attachment 110601 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10028827
(In reply to comment #12) > This patch also broke Mac builds: > http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/34669/steps/compile-webkit/logs/stdio > > Undefined symbols: > "__ZN7WebCore9InlineBox14selectionStateEv", referenced from: > __ZTVN7WebCore9InlineBoxE in Internals.o > "__ZN7WebCore9InlineBox11nodeAtPointERKNS_14HitTestRequestERNS_13HitTestResultERKNS_8IntPointES8_ii", referenced from: > __ZTVN7WebCore9InlineBoxE in Internals.o > "__ZN7WebCore9InlineBox11extractLineEv", referenced from: > __ZTVN7WebCore9InlineBoxE in Internals.o > "__ZN7WebCore9InlineBox5paintERNS_9PaintInfoERKNS_8IntPointEii", referenced from: > __ZTVN7WebCore9InlineBoxE in Internals.o > "__ZN7WebCore9InlineBox7destroyEPNS_11RenderArenaE", referenced from: > __ZTVN7WebCore9InlineBoxE in Internals.o > "__ZN7WebCore9InlineBox14adjustPositionEff", referenced from: > __ZTVN7WebCore9InlineBoxE in Internals.o > "__ZN7WebCore9InlineBoxdlEPvm", referenced from: > __ZN7WebCore9InlineBoxD0Ev in Internals.o > "__ZNK7WebCore9InlineBox14caretMaxOffsetEv", referenced from: > __ZTVN7WebCore9InlineBoxE in Internals.o > "__ZN7WebCore9InlineBox22canAccommodateEllipsisEbii", referenced from: > __ZTVN7WebCore9InlineBoxE in Internals.o > "__ZN7WebCore9InlineBox10attachLineEv", referenced from: > __ZTVN7WebCore9InlineBoxE in Internals.o > "__ZN7WebCore9InlineBox16placeEllipsisBoxEbfffRb", referenced from: > __ZTVN7WebCore9InlineBoxE in Internals.o > "__ZN7WebCore9InlineBox10deleteLineEPNS_11RenderArenaE", referenced from: > __ZTVN7WebCore9InlineBoxE in Internals.o > "__ZNK7WebCore9InlineBox14caretMinOffsetEv", referenced from: > __ZTVN7WebCore9InlineBoxE in Internals.o > > I don't think we wan to expose all these symbols from WebCore. Odd this seems to only happen on release builds. Ryosuke and I had a discussion last night to move access to RenderMenuList to HTMLSelectElement and then export those symbols.. rather than export the symbols above.
Created attachment 110918 [details] Patch
Created attachment 110927 [details] Patch
Created attachment 110955 [details] Patch
Created attachment 110957 [details] Patch
Created attachment 110978 [details] Patch
Created attachment 111014 [details] Patch
Created attachment 111054 [details] Patch