Bug 69631 – Towards making PopupMenuClient more testable

• Component: New Bugs
• Status: REOPENED

[Fady Samuel]

Towards making PopupMenuClient more testable

[Fady Samuel]

Created attachment 110151 [details]
Patch

[WebKit Review Bot]

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

[Simon Fraser (smfr)]

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?

[Fady Samuel]

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

[Fady Samuel]

Created attachment 110592 [details]
Patch

[Simon Fraser (smfr)]

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.

[Fady Samuel]

Created attachment 110601 [details]
Patch for landing

[Fady Samuel]

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.

[WebKit Review Bot]

Comment on attachment 110601 [details]
Patch for landing

Clearing flags on attachment: 110601

Committed r97202: <http://trac.webkit.org/changeset/97202>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch broke Chromium Windows build. Build fix attempted in http://trac.webkit.org/changeset/97207.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Apparently including RenderMenuList.h causes Internals.cpp to use InlineBox's destructor.

[Ryosuke Niwa]

Reopen the bug since the patch was rolled out.

[Collabora GTK+ EWS bot]

Comment on attachment 110601 [details]
Patch for landing

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

[Fady Samuel]

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

[Fady Samuel]

Created attachment 110918 [details]
Patch

[Fady Samuel]

Created attachment 110927 [details]
Patch

[Fady Samuel]

Created attachment 110955 [details]
Patch

[Fady Samuel]

Created attachment 110957 [details]
Patch

[Fady Samuel]

Created attachment 110978 [details]
Patch

[Fady Samuel]

Created attachment 111014 [details]
Patch

[Fady Samuel]

Created attachment 111054 [details]
Patch