Bug 60313

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

61139

Bug Blocks:

60277, 61071, 61073, 61074, 61076, 61671

Attachments:

Description	Flags
WIP: Mac build logistics.	none
WIP: windows.internals object works on mac WebKit1.	none
WIP: Chromium and WebKit1/mac work.	none
Patch	none
Harder, better, faster, stronger.	none
Event betterer, harderer, etc.	none
Using dylib approach EXTREME	none
Dylib approach EXTREME rebased to HEAD	none
Patch	none

Dimitri Glazkov (Google)

Reported 2011-05-05 16:41:48 PDT

Add build logistics and plumbing for window.internals object.

Attachments
WIP: Mac build logistics. (22.40 KB, patch) 2011-05-05 16:42 PDT, Dimitri Glazkov (Google)	no flags	Details Formatted Diff Diff
WIP: windows.internals object works on mac WebKit1. (38.84 KB, patch) 2011-05-11 16:44 PDT, Dimitri Glazkov (Google)	no flags	Details Formatted Diff Diff
WIP: Chromium and WebKit1/mac work. (52.74 KB, patch) 2011-05-17 15:18 PDT, Dimitri Glazkov (Google)	no flags	Details Formatted Diff Diff
Patch (52.74 KB, patch) 2011-05-18 14:57 PDT, Dimitri Glazkov (Google)	no flags	Details Formatted Diff Diff
Harder, better, faster, stronger. (61.09 KB, patch) 2011-05-21 18:06 PDT, Dimitri Glazkov (Google)	no flags	Details Formatted Diff Diff
Event betterer, harderer, etc. (60.98 KB, patch) 2011-05-23 14:51 PDT, Dimitri Glazkov (Google)	no flags	Details Formatted Diff Diff
Using dylib approach EXTREME (76.17 KB, patch) 2011-05-24 16:34 PDT, Dimitri Glazkov (Google)	no flags	Details Formatted Diff Diff
Dylib approach EXTREME rebased to HEAD (63.62 KB, patch) 2011-05-24 16:58 PDT, Dimitri Glazkov (Google)	no flags	Details Formatted Diff Diff
Patch (9.50 KB, patch) 2011-06-03 14:24 PDT, Martin Robinson	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Dimitri Glazkov (Google)

Comment 1 2011-05-05 16:42:15 PDT

Created attachment 92494 [details] WIP: Mac build logistics.

Dimitri Glazkov (Google)

Comment 2 2011-05-05 16:47:08 PDT

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.

Dimitri Glazkov (Google)

Comment 3 2011-05-11 16:44:23 PDT

Created attachment 93212 [details] WIP: windows.internals object works on mac WebKit1.

Hajime Morrita

Comment 4 2011-05-11 21:54:00 PDT

Wow, it's coming !! > WIP: windows.internals object works on mac WebKit1. And your patch is going to support WebKit2 - you are touching WebKitTestRunner.

Dominic Cooney

Comment 5 2011-05-11 23:41:27 PDT

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>

Dimitri Glazkov (Google)

Comment 6 2011-05-12 09:11:32 PDT

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

Dimitri Glazkov (Google)

Comment 7 2011-05-12 09:58:16 PDT

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

Dimitri Glazkov (Google)

Comment 8 2011-05-17 15:18:44 PDT

Created attachment 93830 [details] WIP: Chromium and WebKit1/mac work.

Early Warning System Bot

Comment 9 2011-05-17 15:45:41 PDT

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

Dimitri Glazkov (Google)

Comment 10 2011-05-17 17:43:27 PDT

Darin (Fisher), take a look at how the Chromium stuff is done. Let me know if I did something crazy.

Darin Fisher (:fishd, Google)

Comment 11 2011-05-18 11:24:46 PDT

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.

Dimitri Glazkov (Google)

Comment 12 2011-05-18 12:30:35 PDT

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

Dimitri Glazkov (Google)

Comment 13 2011-05-18 14:57:38 PDT

Created attachment 93990 [details] Patch

Dimitri Glazkov (Google)

Comment 14 2011-05-19 10:36:56 PDT

This patch looks nice and green. Come review it.

Darin Fisher (:fishd, Google)

Comment 15 2011-05-19 10:43:48 PDT

Comment on attachment 93990 [details] Patch LGTM for chromium bits.

Darin Adler

Comment 16 2011-05-19 11:47:02 PDT

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

Dimitri Glazkov (Google)

Comment 17 2011-05-19 11:50:50 PDT

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.

Dimitri Glazkov (Google)

Comment 18 2011-05-19 11:54:32 PDT

Committed r86869: <http://trac.webkit.org/changeset/86869>

Dimitri Glazkov (Google)

Comment 19 2011-05-19 12:30:36 PDT

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)

Dimitri Glazkov (Google)

Comment 20 2011-05-19 12:31:12 PDT

Leopard build fixed in http://trac.webkit.org/changeset/86873.

Dimitri Glazkov (Google)

Comment 21 2011-05-19 13:14:15 PDT

Rolled out (along with fixes) in : http://trac.webkit.org/changeset/86879.

Dimitri Glazkov (Google)

Comment 22 2011-05-19 13:15:10 PDT

Also, Mark suggested to use xcconfig for the new target.

Dimitri Glazkov (Google)

Comment 23 2011-05-19 13:21:26 PDT

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?

Dimitri Glazkov (Google)

Comment 24 2011-05-21 18:06:57 PDT

Created attachment 94331 [details] Harder, better, faster, stronger.

Dimitri Glazkov (Google)

Comment 25 2011-05-21 18:40:05 PDT

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

Dimitri Glazkov (Google)

Comment 26 2011-05-21 19:01:44 PDT

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

Alexey Proskuryakov

Comment 27 2011-05-21 21:52:01 PDT

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

Dimitri Glazkov (Google)

Comment 28 2011-05-21 22:00:44 PDT

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

Mark Rowe (bdash)

Comment 29 2011-05-23 13:37:22 PDT

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

Dimitri Glazkov (Google)

Comment 30 2011-05-23 13:42:00 PDT

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

Dimitri Glazkov (Google)

Comment 31 2011-05-23 14:51:13 PDT

Created attachment 94494 [details] Event betterer, harderer, etc.

Dimitri Glazkov (Google)

Comment 32 2011-05-23 14:52:07 PDT

> Sounds good, will remove and test locally to make sure nothing breaks. It worked! Thanks Mark. New patch uploaded.

Dimitri Glazkov (Google)

Comment 33 2011-05-24 16:34:19 PDT

Created attachment 94711 [details] Using dylib approach EXTREME

Dimitri Glazkov (Google)

Comment 34 2011-05-24 16:58:46 PDT

Created attachment 94716 [details] Dylib approach EXTREME rebased to HEAD

Dimitri Glazkov (Google)

Comment 35 2011-05-27 07:48:10 PDT

Bueller?

Darin Adler

Comment 36 2011-06-01 16:55:01 PDT

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.

Dimitri Glazkov (Google)

Comment 37 2011-06-02 10:30:59 PDT

Committed r87926: <http://trac.webkit.org/changeset/87926>

Dimitri Glazkov (Google)

Comment 38 2011-06-02 11:05:23 PDT

Reverted r87926 for reason: Fails to find WebCoreTestSupport.dylib on bots. Committed r87931: <http://trac.webkit.org/changeset/87931>

Dimitri Glazkov (Google)

Comment 39 2011-06-02 13:15:45 PDT

Committed r87948: <http://trac.webkit.org/changeset/87948>

Martin Robinson

Comment 40 2011-06-03 14:24:52 PDT

Created attachment 95962 [details] Patch