| Summary: | Add build logistics and plumbing for window.internals object. | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dimitri Glazkov (Google) <dglazkov> | ||||||||||||||||||||
| Component: | New Bugs | Assignee: | Dimitri Glazkov (Google) <dglazkov> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | ap, darin, dominicc, fishd, morrita, mrowe, sam | ||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||
| Bug Depends on: | 61139 | ||||||||||||||||||||||
| Bug Blocks: | 60277, 61071, 61073, 61074, 61076, 61671 | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Dimitri Glazkov (Google)
2011-05-05 16:41:48 PDT
Created attachment 92494 [details]
WIP: Mac build logistics.
Not yet ready for review, but wanted to ask your opinion on this approach: 1) build separate target called WebCoreTestSupport in WebCore project, which produces libWebCoreTestSupport.a 2) when building, InternalsInjector.h is copied to WebCoreTestSupport/PrivateHeaders/InternalsInjector.h; 3) the DumpRenderTree project links in libWebCoreTestSupport.a and refers to InternalsInjector.h; 4) allowing the DRT code to call into to InternalsInjector to inject window.internals. Created attachment 93212 [details]
WIP: windows.internals object works on mac WebKit1.
Wow, it's coming !!
> WIP: windows.internals object works on mac WebKit1.
And your patch is going to support WebKit2 - you are touching WebKitTestRunner.
Comment on attachment 93212 [details] WIP: windows.internals object works on mac WebKit1. View in context: https://bugs.webkit.org/attachment.cgi?id=93212&action=review Looking forward to this. > LayoutTests/fast/harness/internals-object.html:1 > +<html> Any reason not to use the test framework and simplify this to something like: <!DOCTYPE html> <script src="../js/resources/js-test-pre.js"></script> <pre id="console"> (Description) </pre> <script> shouldBeNonNull('window.internals'); var successfullyParsed = true; </script> <script src="../js/resources/js-test-post.js"></script> (In reply to comment #5) > (From update of attachment 93212 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93212&action=review > > Looking forward to this. > > > LayoutTests/fast/harness/internals-object.html:1 > > +<html> > > Any reason not to use the test framework and simplify this to something like: > > <!DOCTYPE html> > <script src="../js/resources/js-test-pre.js"></script> > <pre id="console"> > (Description) > </pre> > <script> > shouldBeNonNull('window.internals'); > var successfullyParsed = true; > </script> > <script src="../js/resources/js-test-post.js"></script> I hate the scripting framework, but I'll convert the test. (In reply to comment #4) > Wow, it's coming !! > > WIP: windows.internals object works on mac WebKit1. > And your patch is going to support WebKit2 - you are touching WebKitTestRunner. No, it won't. WebKit2 is a bit more difficult. Because the layout test controller code is in a bundle, I can't link in WebCore symbols there. I need to look more at how WebKit2 works -- or perhaps Sam can just point me at the right place. Created attachment 93830 [details]
WIP: Chromium and WebKit1/mac work.
Comment on attachment 93830 [details] WIP: Chromium and WebKit1/mac work. Attachment 93830 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8710261 Darin (Fisher), take a look at how the Chromium stuff is done. Let me know if I did something crazy. Comment on attachment 93830 [details] WIP: Chromium and WebKit1/mac work. View in context: https://bugs.webkit.org/attachment.cgi?id=93830&action=review > Source/WebKit/chromium/public/WebInternalsInjector.h:35 > + static void injectInternalsObject(WebFrame*); This needs to be marked WEBKIT_API so that it will get exported from WebKit.dll on Windows. nit: WebInternalsInjector::injectInternalsObject repeats the words "internals" and "inject"... you could get by with something shorter that only says these words once. it might be done by changing WebInternalsInjector to something like WebTestingSupport? or WebLayoutTestSupport? or WebDumpRenderTreeSupport? Maybe there are other methods that could be moved to this class too. (In reply to comment #11) > (From update of attachment 93830 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93830&action=review > > > Source/WebKit/chromium/public/WebInternalsInjector.h:35 > > + static void injectInternalsObject(WebFrame*); > > This needs to be marked WEBKIT_API so that it will get exported from WebKit.dll on Windows. Doh! thanks. > > nit: WebInternalsInjector::injectInternalsObject repeats the words "internals" and "inject"... you could get by with something shorter that only says these words once. it might be done by changing WebInternalsInjector to something like WebTestingSupport? or WebLayoutTestSupport? or WebDumpRenderTreeSupport? Maybe there are other methods that could be moved to this class too. I like WebTestingSupport, I'll go with that/ Created attachment 93990 [details]
Patch
This patch looks nice and green. Come review it. Comment on attachment 93990 [details]
Patch
LGTM for chromium bits.
Comment on attachment 93990 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93990&action=review Looks like a good start. > Source/WebCore/testing/v8/WebCoreTestSupport.h:32 > +namespace v8 { > +class Context; > +template <class T> class Local; > +} The stylebot is causing you to use bad formatting here ;-) If that's ok, I'll land this with a small change to WebCore.xcodeproj -- I'll add an extra "All" target, which depends on both WebCore and WebCoreTestSupport. This would make the project structure more consistent with DumpRenderTree.xcodeproj and WebKit2.xcodeproj. Committed r86869: <http://trac.webkit.org/changeset/86869> Looks like I broke Chromium component build, fixing... --------------------Configuration: webkit - Release Win32----------------------- Compiling... WebTestingSupport.cpp .\src\WebTestingSupport.cpp(29) : fatal error C1083: Cannot open include file: 'WebCoreTestSupport.h': No such file or directory Build log was saved at "file://e:\b\build\slave\Win_Shared_Builder__dbg_\build\src\build\Release\obj\webkit\BuildLog.htm" webkit - 1 error(s), 0 warning(s) Leopard build fixed in http://trac.webkit.org/changeset/86873. Rolled out (along with fixes) in : http://trac.webkit.org/changeset/86879. Also, Mark suggested to use xcconfig for the new target. Ultimately, the issue was in inability to manage symbols export. andersca and myself threw fixes at the tree for a while, but stopped after seeing this in the latest fix attempt. ERROR: WebCore has a weak vtable in it (/Volumes/Big/WebKit-BuildSlave/leopard-intel-release/build/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore) ERROR: Fix by making sure the first virtual function in each of these classes is not an inline: ERROR: class WebCore::JSDOMWrapper I fear we're solving a problem with another problem. This patch barely touched WebCore internals, and we still had way too much fun attempting to manage the exports. So instead of plumbing stuff through ports' WebKit layers, we would have to manage these ports' exports. Does anyone have any bright ideas on improving the situation? Created attachment 94331 [details]
Harder, better, faster, stronger.
> Does anyone have any bright ideas on improving the situation?
Following exciting intracranial discussion, I've concluded that this is still the right approach. Even though we will have to occasionally unpuzzle weird exported symbol problems, it's still better and less wasteful then plumbing this through the WebKit API layer.
(In reply to comment #24) > Created an attachment (id=94331) [details] > Harder, better, faster, stronger. This patch differs from the last one in: * Addition of WebCoreTestSupport.xcconfig file * A fix for the Chromium component build * Accumulating all of the exported symbol fixes that we threw at the tree while landing last time * A funky workaround for "weak external vtable" issue in JSDOMWrapper. I don't love it, but it appears to be the only rational way. Appreciate any um.. pointers! > better and less wasteful then plumbing this through the WebKit API layer.
I'd like to use this occasion to re-emphasize a point I've been making at the contributor meeting. This functionality should not be a replacement for tests that need to be end to end, but the contributor is unwilling to do WebKit plumbing for some reason.
I'm sure that we'll have exciting discussions for each specific case for a while, until we figure out the best practices. Let's try to have these early enough in the process, before code is written and a patch is up for review!
(In reply to comment #27) > > better and less wasteful then plumbing this through the WebKit API layer. > > I'd like to use this occasion to re-emphasize a point I've been making at the contributor meeting. This functionality should not be a replacement for tests that need to be end to end, but the contributor is unwilling to do WebKit plumbing for some reason. > > I'm sure that we'll have exciting discussions for each specific case for a while, until we figure out the best practices. Let's try to have these early enough in the process, before code is written and a patch is up for review! Totally agree. Comment on attachment 94331 [details] Harder, better, faster, stronger. View in context: https://bugs.webkit.org/attachment.cgi?id=94331&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:25783 > + COPY_PHASE_STRIP = NO; > + GCC_DYNAMIC_NO_PIC = NO; > + GCC_OPTIMIZATION_LEVEL = 0; These settings aren’t necessary AFAICT and should be removed. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:25795 > + COPY_PHASE_STRIP = YES; > + DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym"; > + GCC_ENABLE_FIX_AND_CONTINUE = NO; > + PRODUCT_NAME = All; > + ZERO_LINK = NO; Ditto here (except for PRODUCT_NAME). (In reply to comment #29) > (From update of attachment 94331 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94331&action=review > > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:25783 > > + COPY_PHASE_STRIP = NO; > > + GCC_DYNAMIC_NO_PIC = NO; > > + GCC_OPTIMIZATION_LEVEL = 0; > > These settings aren’t necessary AFAICT and should be removed. > > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:25795 > > + COPY_PHASE_STRIP = YES; > > + DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym"; > > + GCC_ENABLE_FIX_AND_CONTINUE = NO; > > + PRODUCT_NAME = All; > > + ZERO_LINK = NO; > > Ditto here (except for PRODUCT_NAME). Sounds good, will remove and test locally to make sure nothing breaks. Created attachment 94494 [details]
Event betterer, harderer, etc.
> Sounds good, will remove and test locally to make sure nothing breaks.
It worked! Thanks Mark. New patch uploaded.
Created attachment 94711 [details]
Using dylib approach EXTREME
Created attachment 94716 [details]
Dylib approach EXTREME rebased to HEAD
Bueller? Comment on attachment 94716 [details] Dylib approach EXTREME rebased to HEAD View in context: https://bugs.webkit.org/attachment.cgi?id=94716&action=review > Source/WebCore/bindings/js/JSDOMWrapper.h:53 > + // An inline function cannot be the first non-abstract virtual function declared > + // in the class as it results in the vtable being generated as a weak symbol. > + virtual void noWeakVtable(); This technique is fine. I would use a different name for the function that is more of a function name. Maybe virtualFunctionToPreventWeakVtable. > Source/WebCore/testing/Internals.cpp:33 > + return adoptRef(new Internals()); I’d leave out the () here. Committed r87926: <http://trac.webkit.org/changeset/87926> Reverted r87926 for reason: Fails to find WebCoreTestSupport.dylib on bots. Committed r87931: <http://trac.webkit.org/changeset/87931> Committed r87948: <http://trac.webkit.org/changeset/87948> Created attachment 95962 [details]
Patch
Comment on attachment 95962 [details]
Patch
yay!
Comment on attachment 95962 [details] Patch Whoops. Uploaded this to the wrong bug. I've landed it at https://bugs.webkit.org/show_bug.cgi?id=61071. |