Bug 62432

Created attachment 96936 [details] Patch

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

Created attachment 96937 [details] Patch

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!

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

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.

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

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 ;-)

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

Created attachment 97073 [details] Trying to fix linker errors

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

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

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

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

Created attachment 97087 [details] Another try

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

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.

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.

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

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

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

Created attachment 97252 [details] Patch

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.

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

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

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.

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

Created attachment 97386 [details] Another try

It's green and ready to review!

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.

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

(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? :)

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

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

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