Bug 62432 – internals object should have createShadowContentElement()

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Hajime Morrita]

We need this to test it before making it web-facing.

[Hajime Morrita]

Created attachment 96936 [details]
Patch

[WebKit Review Bot]

Comment on attachment 96936 [details]
Patch

Attachment 96936 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8829967

[Hajime Morrita]

Created attachment 96937 [details]
Patch

[Hajime Morrita]

I know elementRenderTreeAsText() is a kind of unfortunate.
But I'd like to argue:

- Reftests doesn't reach its prime time, and
- Having non-text layout tests for each content test is worse because it will be
   almost all about render tree construction, not neither layout or DOM tree construction,

and of course love to hear your opinion!

[Collabora GTK+ EWS bot]

Comment on attachment 96937 [details]
Patch

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

[Dimitri Glazkov (Google)]

Comment on attachment 96937 [details]
Patch

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

I have no problem with exposing RenderTree representation like this. It's useful for building things that are more advanced than reftests.

> Source/WebCore/WebCore.exp.in:802
> +__ZN7WebCore6JSNode6s_infoE
> +__ZNK7WebCore6JSNode21pushEventHandlerScopeEPN3JSC9ExecStateEPNS1_14ScopeChainNodeE
> +__ZN7WebCore6JSNode13visitChildrenERN3JSC9MarkStackE
> +__ZN7WebCore6JSNode3putEPN3JSC9ExecStateERKNS1_10IdentifierENS1_7JSValueERNS1_15PutPropertySlotE
> +__ZN7WebCore10JSDocument3putEPN3JSC9ExecStateERKNS1_10IdentifierENS1_7JSValueERNS1_15PutPropertySlotE
> +__ZN7WebCore10toDocumentEN3JSC7JSValueE

These are not specific to this patch. In fact, Dominic is already exporting them in bug 61671.

[Dominic Cooney]

(In reply to comment #6)

> These are not specific to this patch. In fact, Dominic is already exporting them in bug 61671.

Whoever writes the first method will have to export these, so export on. Whoever lands second can deal with merging it.

[Hajime Morrita]

Hi Dimitri and Dominic, thank you for taking a look!

> I have no problem with exposing RenderTree representation like this. It's useful for building things that are more advanced than reftests.
This is good to hear!

> (In reply to comment #6)
> 
> > These are not specific to this patch. In fact, Dominic is already exporting them in bug 61671.
> 
> Whoever writes the first method will have to export these, so export on. Whoever lands second can deal with merging it.
Yes, someone need to do this first. And I'm happy to wait to land bug 61671 ;-)

[Dominic Cooney]

 > > > These are not specific to this patch. In fact, Dominic is already exporting them in bug 61671.
> > Whoever writes the first method will have to export these, so export on. Whoever lands second can deal with merging it.
> Yes, someone need to do this first. And I'm happy to wait to land bug 61671 ;-)

Only wait if you want to. These are in order so merging should be trivial.

[Hajime Morrita]

Created attachment 97073 [details]
Trying to fix linker errors

[Gustavo Noronha (kov)]

Comment on attachment 97073 [details]
Trying to fix linker errors

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

[Hajime Morrita]

Created attachment 97076 [details]
Passed Win. Attacking Gtk.

[Gustavo Noronha (kov)]

Comment on attachment 97076 [details]
Passed Win. Attacking Gtk.

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

[Hajime Morrita]

(In reply to comment #10)
> Created an attachment (id=97073) [details]
> Trying to fix linker errors
Hmm... It's tricky to figure out which symbol should be exported...

[Hajime Morrita]

Created attachment 97087 [details]
Another try

[Collabora GTK+ EWS bot]

Comment on attachment 97087 [details]
Another try

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

[Dominic Cooney]

Comment on attachment 97087 [details]
Another try

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

I am working through similar stuff for bug 61671. The easiest thing I found was:

1. WebCore.exp.in already has all of the symbols you need. So just search for TypeNameXmemberName where X is strlen("memberName") and copy the mangled symbols from there.

2. GTK build doesn't depend on symbols.filter correctly, so you may need to do a clean build to get accurate results. (I guess your results from bots may have spurious failures for this reason also.)

> Source/autotools/symbols.filter:29
>  _ZN7WebCore21getCachedDOMStructureEPNS_17JSDOMGlobalObjectEPKN3JSC9ClassInfoE;

So there is no trouble merging these let's M-x sort-lines from this line inclusive to local: exclusive when we add symbols for Internals.

[Dominic Cooney]

Comment on attachment 97087 [details]
Another try

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

> Source/WebCore/WebCore.exp.in:798
> +__ZNK7WebCore6JSNode21pushEventHandlerScopeEPN3JSC9ExecStateEPNS1_14ScopeChainNodeE

I am not sure that this file is globally ordered, but it looks locally ordered. Keep it ordered to make merging easier.

Note that 

__ZNK7WebCore10FooFactory < __ZNK7WebCore3Foo because '1' < '3'

and also be careful with ZN[^K] != ZNK

> Source/WebKit2/win/WebKit2.def:138
>          ?getCachedDOMStructure@WebCore@@YAPAVStructure@JSC@@PAVJSDOMGlobalObject@1@PBUClassInfo@3@@Z

Likewise we should M-x sort-lines this group too to make merging easier.

[Dimitri Glazkov (Google)]

(In reply to comment #17)
> (From update of attachment 97087 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97087&action=review
> 
> I am working through similar stuff for bug 61671. The easiest thing I found was:
> 
> 1. WebCore.exp.in already has all of the symbols you need. So just search for TypeNameXmemberName where X is strlen("memberName") and copy the mangled symbols from there.
> 
> 2. GTK build doesn't depend on symbols.filter correctly, so you may need to do a clean build to get accurate results. (I guess your results from bots may have spurious failures for this reason also.)
> 
> > Source/autotools/symbols.filter:29
> >  _ZN7WebCore21getCachedDOMStructureEPNS_17JSDOMGlobalObjectEPKN3JSC9ClassInfoE;
> 
> So there is no trouble merging these let's M-x sort-lines from this line inclusive to local: exclusive when we add symbols for Internals.

How hard would it be to write a tool around this?

[Dominic Cooney]

> How hard would it be to write a tool around this?

Not too hard, maybe a few days, if the ambition is suitably low.

Something like: Take a built WebCoreTestSupport and dump its unresolved symbols, or take build log output, and update the right files with suitable mangling for the Linux, OS X family and the Windows family. (Is translating the mangled names hard?)

Is it worth it? I expect the number of missing symbols will diminish very sharply soon (just look at the intersection between the symbols in this patch and in the patch on bug 61671.) However every new bit of window.internals will touch some new symbol.

There is still a couple of gotchas:

1. Mac already exports a boatload of symbols that GTK doesn't, so just getting the unresolved symbols on Mac might not be a big enough delta for GTK.

2. If you're building on GTK it prints the pretty name of the missing symbol, which makes life difficult since there's nothing to cut-and-paste from (except WebCore.exp.in...)

It would be nice if there was a tool to nix unused symbols too so these don't grow unboundedly, too.

The biggest bang for buck is probably in documenting all of the places you need to add symbols and how the formats relate to each other (you can trivially mangle WebCore.exp.in symbols to GTK symbols by deleting one leading _ and adding a semicolon, for example.)

[Martin Robinson]

(In reply to comment #19)
> How hard would it be to write a tool around this?

Another alternative would be to export the symbols manually with annoatations. I believe that Kevin Olliver is doing this now with JSC, to eliminate the symbol lists there.

[Hajime Morrita]

Created attachment 97252 [details]
Patch

[Hajime Morrita]

Hi guys, thank you for your advice.
I choose copy-from-exp strategy at this time.
Using annotation sounds promising though, especially for internals object
because there is no entry difference between ports in that case.

[Collabora GTK+ EWS bot]

Comment on attachment 97252 [details]
Patch

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

[Hajime Morrita]

Gtk bot looks having in trouble. He said he couldn't build it without a patch.

[Dominic Cooney]

Comment on attachment 97252 [details]
Patch

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

> Source/autotools/symbols.filter:37
> +_ZN7WebCore20ShadowContentElement6createEPNS_8DocumentE

You are missing a trailing semicolon here.

> Source/autotools/symbols.filter:43
> +_ZN7WebCore9JSElement3putEPN3JSC9ExecStateERKNS1_10IdentifierENS1_7JSValueERNS1_15PutPropertySlotE

...and here, and check below.

[Hajime Morrita]

(In reply to comment #26)
> (From update of attachment 97252 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97252&action=review
> 
> > Source/autotools/symbols.filter:37
> > +_ZN7WebCore20ShadowContentElement6createEPNS_8DocumentE
> 
> You are missing a trailing semicolon here.
> 
> > Source/autotools/symbols.filter:43
> > +_ZN7WebCore9JSElement3putEPN3JSC9ExecStateERKNS1_10IdentifierENS1_7JSValueERNS1_15PutPropertySlotE
> 
> ...and here, and check below.

Oops. Thank you for finding this. I'll fix them.

[Hajime Morrita]

Created attachment 97386 [details]
Another try

[Hajime Morrita]

It's green and ready to review!

[Dimitri Glazkov (Google)]

Comment on attachment 97386 [details]
Another try

Glorious.

Guys. We _really_ need to make adding all these exported symbols easier. This is a lot less plumbing code, but golly! so many exports.

[Darin Adler]

(In reply to comment #30)
> We _really_ need to make adding all these exported symbols easier. This is a lot less plumbing code, but golly! so many exports.

Switching to export macros instead of the export lists is a project that would resolve this. There are some pitfalls, though.

[Dimitri Glazkov (Google)]

(In reply to comment #31)
> (In reply to comment #30)
> > We _really_ need to make adding all these exported symbols easier. This is a lot less plumbing code, but golly! so many exports.
> 
> Switching to export macros instead of the export lists is a project that would resolve this. There are some pitfalls, though.

Is this something mac/win ports would be interested in? This sounds really horrifically complex... but exciting? :)

[Darin Adler]

(In reply to comment #32)
> Is this something mac/win ports would be interested in? This sounds really horrifically complex... but exciting? :)

To oversimplify, the answer is yes.

It would be helpful to have an easier way to control what’s exported that was less platform-specific.

It would be disappointing if the macro technique caused us to export a lot more things, although I’m not sure what the downsides are of exporting too much.

Not sure what the other issues are. A couple experts I’d talk to on this would be Mark Rowe and Maciej. There may be others in the Safari department at Apple with a view on this.

[WebKit Review Bot]

Comment on attachment 97386 [details]
Another try

Rejecting attachment 97386 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
file Source/WebCore/testing/Internals.h
patching file Source/WebCore/testing/Internals.idl
Hunk #1 FAILED at 25.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/testing/Internals.idl.rej
patching file Source/WebKit2/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit2/win/WebKit2.def
patching file Source/autotools/symbols.filter

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Dimitri Glazkov', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/8912313

[Hajime Morrita]

Committed r89230: <http://trac.webkit.org/changeset/89230>