Bug 137339

Summary: Make it possible to test page overlays
Product: WebKit Reporter: Tim Horton <thorton>
Component: WebCore Misc.Assignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, bfulgham, burg, commit-queue, esprehn+autocc, glenn, gyuyoung.kim, jonlee, kondapallykalyan, mitz, ossy, rakuco, roger_fong, ryuan.choi, sam, sergio, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
none
with images
none
patch
none
patch mitz: review+

Description Tim Horton 2014-10-02 03:26:51 PDT
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.
Comment 1 Tim Horton 2014-10-02 03:28:17 PDT
Created attachment 239100 [details]
patch
Comment 2 Tim Horton 2014-10-02 03:30:46 PDT
This won't apply because it depends on https://bugs.webkit.org/show_bug.cgi?id=137164.
Comment 3 Tim Horton 2014-10-02 17:40:20 PDT
Created attachment 239164 [details]
patch
Comment 4 Tim Horton 2014-10-02 17:41:01 PDT
Created attachment 239165 [details]
with images
Comment 5 Tim Horton 2014-10-03 14:17:25 PDT
Created attachment 239234 [details]
patch
Comment 6 Tim Horton 2014-10-03 14:18:00 PDT
Created attachment 239236 [details]
patch
Comment 7 mitz 2014-10-03 20:11:32 PDT
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"?
Comment 8 Tim Horton 2014-10-03 20:19:57 PDT
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!
Comment 9 Tim Horton 2014-10-04 01:34:30 PDT
(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...
Comment 10 Tim Horton 2014-10-04 01:47:28 PDT
http://trac.webkit.org/changeset/174315
Comment 11 Anders Carlsson 2014-10-04 07:50:18 PDT
(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!
Comment 12 Csaba Osztrogonác 2014-10-04 11:38:51 PDT
(In reply to comment #10)
> http://trac.webkit.org/changeset/174315

It broke the Apple Windows build.
Comment 13 Alexey Proskuryakov 2014-10-04 11:41:49 PDT
     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)
Comment 14 Tim Horton 2014-10-04 12:09:26 PDT
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.
Comment 15 Brian Burg 2014-10-04 12:22:07 PDT
(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.
Comment 16 Tim Horton 2014-10-04 12:35:46 PDT
(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.
Comment 17 Tim Horton 2014-10-04 12:45:40 PDT
(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.
Comment 18 Tim Horton 2014-10-04 13:01:30 PDT
Green after http://trac.webkit.org/changeset/174321.
Comment 19 Brent Fulgham 2014-10-06 14:36:27 PDT
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.
Comment 20 Brent Fulgham 2014-10-06 14:54:00 PDT
64-bit Windows build fix checked in under r174368 <https://trac.webkit.org/r174368>.
Comment 21 Roger Fong 2014-10-08 13:27:48 PDT
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...
Comment 22 Roger Fong 2014-10-08 15:42:42 PDT
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?