Bug 137339 – Make it possible to test page overlays

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tim Horton]

We should test page overlays!

This patch is just a start (and the tests are fairly uninteresting), but it lays the groundwork for more testing in the future.

[Tim Horton]

Created attachment 239100 [details]
patch

[Tim Horton]

This won't apply because it depends on https://bugs.webkit.org/show_bug.cgi?id=137164.

[Tim Horton]

Created attachment 239164 [details]
patch

[Tim Horton]

Created attachment 239165 [details]
with images

[Tim Horton]

Created attachment 239234 [details]
patch

[Tim Horton]

Created attachment 239236 [details]
patch

Comment on attachment 239236 [details]
patch

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

> Source/WebCore/platform/graphics/GraphicsLayerClient.h:59
> +typedef unsigned LayerTreeAsTextBehavior;

:-/

Why not just move the definition of the enum from GraphicsLayer.h to this header?

> Source/WebCore/testing/Internals.cpp:2370
> +

Should we also throw an exception if overlayType is not "view" or "document"?

[Tim Horton]

Comment on attachment 239236 [details]
patch

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

Thank you!

>> Source/WebCore/platform/graphics/GraphicsLayerClient.h:59
>> +typedef unsigned LayerTreeAsTextBehavior;
> 
> :-/
> 
> Why not just move the definition of the enum from GraphicsLayer.h to this header?

Haha probably a better plan!

>> Source/WebCore/testing/Internals.cpp:2370
>> +
> 
> Should we also throw an exception if overlayType is not "view" or "document"?

This happens automatically!

[Tim Horton]

(In reply to comment #8)
> (From update of attachment 239236 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239236&action=review
> 
> Thank you!
> 
> >> Source/WebCore/platform/graphics/GraphicsLayerClient.h:59
> >> +typedef unsigned LayerTreeAsTextBehavior;
> > 
> > :-/
> > 
> > Why not just move the definition of the enum from GraphicsLayer.h to this header?
> 
> Haha probably a better plan!

Then again, it's not really a GraphicsLayerClient thing. The client can override some things, sometimes dependent on LayerTreeAsTextBehavior, but it's certainly not necessary to know about GraphicsLayerClient to need to use it.

But I'll move it anyway since GraphicsLayer.h pulls in GraphicsLayerClient.h. Probably best would be to put it in its own header...

[Tim Horton]

http://trac.webkit.org/changeset/174315

[Anders Carlsson]

(In reply to comment #7)
> (From update of attachment 239236 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239236&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsLayerClient.h:59
> > +typedef unsigned LayerTreeAsTextBehavior;
> 
> :-/
> 
> Why not just move the definition of the enum from GraphicsLayer.h to this header?

Or just make the enum strongly typed (enum class), so you can forward declare it!

[Csaba Osztrogonác]

(In reply to comment #10)
> http://trac.webkit.org/changeset/174315

It broke the Apple Windows build.

[Alexey Proskuryakov]

     1>WebCoreTestSupport.lib(Internals.obj) : error LNK2019: unresolved external symbol "public: void __thiscall WebCore::Document::updateLayout(void)" (?updateLayout@Document@WebCore@@QAEXXZ) referenced in function "public: class WTF::String __thiscall WebCore::Internals::pageOverlayLayerTreeAsText(int &)const " (?pageOverlayLayerTreeAsText@Internals@WebCore@@QBE?AVString@WTF@@AAH@Z)
     1>WebCoreTestSupport.lib(JSInternals.obj) : error LNK2019: unresolved external symbol "__int64 __cdecl WebCore::throwArgumentMustBeEnumError(class JSC::ExecState &,unsigned int,char const *,char const *,char const *,char const *)" (?throwArgumentMustBeEnumError@WebCore@@YA_JAAVExecState@JSC@@IPBD111@Z) referenced in function "public: float __thiscall <lambda_61e80db43a2fdfc51022831809c4282b>::operator()(class WebCore::LayoutUnit,class WebCore::LayoutUnit)const " (??R<lambda_61e80db43a2fdfc51022831809c4282b>@@QBEMVLayoutUnit@WebCore@@0@Z)
     1>WebCoreTestSupport.lib(MockPageOverlayClient.obj) : error LNK2019: unresolved external symbol "unsigned int __cdecl WebCore::makeRGB(int,int,int)" (?makeRGB@WebCore@@YAIHHH@Z) referenced in function "private: virtual void __thiscall WebCore::MockPageOverlayClient::drawRect(class WebCore::PageOverlay &,class WebCore::GraphicsContext &,class WebCore::IntRect const &)" (?drawRect@MockPageOverlayClient@WebCore@@EAEXAAVPageOverlay@2@AAVGraphicsContext@2@ABVIntRect@2@@Z)
     1>WebCoreTestSupport.lib(MockPageOverlayClient.obj) : error LNK2019: unresolved external symbol "public: static class WTF::PassRefPtr<class WebCore::PageOverlay> __cdecl WebCore::PageOverlay::create(class WebCore::PageOverlay::Client &,enum WebCore::PageOverlay::OverlayType)" (?create@PageOverlay@WebCore@@SA?AV?$PassRefPtr@VPageOverlay@WebCore@@@WTF@@AAVClient@12@W4OverlayType@12@@Z) referenced in function "public: void __thiscall WebCore::MockPageOverlayClient::installOverlay(class WebCore::MainFrame &,enum WebCore::PageOverlay::OverlayType)" (?installOverlay@MockPageOverlayClient@WebCore@@QAEXAAVMainFrame@2@W4OverlayType@PageOverlay@2@@Z)
     1>WebCoreTestSupport.lib(MockPageOverlayClient.obj) : error LNK2019: unresolved external symbol "public: virtual __thiscall WebCore::PageOverlay::~PageOverlay(void)" (??1PageOverlay@WebCore@@UAE@XZ) referenced in function "public: void __thiscall WebCore::MockPageOverlayClient::installOverlay(class WebCore::MainFrame &,enum WebCore::PageOverlay::OverlayType)" (?installOverlay@MockPageOverlayClient@WebCore@@QAEXAAVMainFrame@2@W4OverlayType@PageOverlay@2@@Z)

[Tim Horton]

Ah, should have run it through the EWS one more time after fixing the last thing it complained about. Will try to remember how to add exports on Windows shortly.

[Brian Burg]

(In reply to comment #14)
> Ah, should have run it through the EWS one more time after fixing the last thing it complained about. Will try to remember how to add exports on Windows shortly.

I believe you want WebkitExports.def.in, to re-export WebCore symbols to DRT.

[Tim Horton]

(In reply to comment #15)
> (In reply to comment #14)
> > Ah, should have run it through the EWS one more time after fixing the last thing it complained about. Will try to remember how to add exports on Windows shortly.
> 
> I believe you want WebkitExports.def.in, to re-export WebCore symbols to DRT.

Thanks! Maybe http://trac.webkit.org/changeset/174320 will help, will keep an eye.

[Tim Horton]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > Ah, should have run it through the EWS one more time after fixing the last thing it complained about. Will try to remember how to add exports on Windows shortly.
> > 
> > I believe you want WebkitExports.def.in, to re-export WebCore symbols to DRT.
> 
> Thanks! Maybe http://trac.webkit.org/changeset/174320 will help, will keep an eye.

Oh my now there are a lot more.

[Tim Horton]

Green after http://trac.webkit.org/changeset/174321.

[Brent Fulgham]

Almost! We still have 64-bit failures. I'm running a build now to get the 64-bit mangled symbol names, and I'll check in a build fix soon.

[Brent Fulgham]

64-bit Windows build fix checked in under r174368 <https://trac.webkit.org/r174368>.

[Roger Fong]

Still trying to figure out why the EWS bots aren't working.
I believe it has something to do with the machines themselves.
A clean rebuild is not fixing the issue. For some odd reason, on the EWS bots, the generate-bindings.pl script just does not output JSInternals.h/cpp. No error messages are given, it's just not there...

[Roger Fong]

Upon further inspection I've noticed the following in CodeGenerator.pm:

    my $interfaces = $useDocument->interfaces;
    foreach my $interface (@$interfaces) {
        //write stuff (presumably JSInternals.h/cpp) out to file.

On the trybot and local builds where this works fine $interface is equal to Internals.

However, on the EWS bots, the interfaces array is completely empty.
There doesn't seem to be anything suspicious about $useDocument

The docmument and the interfaces array respectively are
ARRAY(0x80392498), idlDocument=HASH(0x80236060)

Which seems reasonable?
Anyone out there that knows this how this code generation stuff works better wanna land a hand?