Add tests for explicit serialization values
Created attachment 164204 [details] Patch
Note that a subset of the functionality of these tests already exist in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/IDBKeyPathTest.cpp - I am removing them in bug 95409 because of a refactoring - rather than write a bunch of v8-specific tests, I just moved the tests to JS. Also note that I'm using Uint16Array in the JS. This may seem arbitrary and strange, but it is intentional, to be consistent with the actual encoding style in SerializedScriptValue, which packs bytes into UChars - this will make it easier to debug regressions.
haraken@ - this is the test split out from bug 95409, as discussed in e-mail a few days ago. Mind a quick review? (path for the other bug forthcoming...)
Comment on attachment 164204 [details] Patch Attachment 164204 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13854538
Comment on attachment 164204 [details] Patch Attachment 164204 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13855548
Comment on attachment 164204 [details] Patch Attachment 164204 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13841879
Comment on attachment 164204 [details] Patch Attachment 164204 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13841880
Comment on attachment 164204 [details] Patch Attachment 164204 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13859301 New failing tests: storage/ssv.html
Comment on attachment 164204 [details] Patch Attachment 164204 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13858333
Comment on attachment 164204 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164204&action=review The approach looks good. Let's fix build failures. r-ing due to bot failures. > Source/WebCore/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). Please describe what your patch is doing. > Source/WebCore/testing/Internals.cpp:1261 > + return ArrayBuffer::create(static_cast<const void*>(stringValue.impl()->characters()), > + stringValue.sizeInBytes()); Nit: We don't wrap lines. > LayoutTests/ChangeLog:13 > + * storage/resources/ssv.js: Added. Let's rename ssv.js to serialized-script-value.js, for clarification. > LayoutTests/storage/resources/ssv.js:1 > + Nit: remove this line. > LayoutTests/storage/resources/ssv.js:32 > + // test deserialization Nit: this comment is not needed (invalid?). > LayoutTests/storage/resources/ssv.js:42 > + // test serialization Nit: this comment is not needed. > LayoutTests/storage/resources/ssv.js:60 > +// we only test a few cases of the "old" serialization format because It looks like no tests are passing values to 'oldFormat'. Is it intended? > LayoutTests/storage/resources/ssv.js:98 > +testSerialization(undefined, [0x01ff, 0x003f, 0x005f]); > +testSerialization(true, [0x01ff, 0x003f, 0x0054]); > +testSerialization(false, [0x01ff, 0x003f, 0x0046]); > +testSerialization([1,2,3,4,5,6,7,8], [0x01ff, 0x003f, 0x0841, 0x013f, 0x0249, 0x013f, 0x0449, 0x013f, 0x0649, 0x013f, 0x0849, 0x013f, 0x0a49, 0x013f, 0x0c49, 0x013f, 0x0e49, 0x013f, 0x1049, 0x0024, 0x0008]); > +testSerialization(10, [0x01ff, 0x003f, 0x1449]); > +testSerialization(-10, [0x01ff, 0x003f, 0x1349]); > +testSerialization(Math.pow(2,30), [0x01ff, 0x003f, 0x8049, 0x8080, 0x0880]); > +testSerialization(Math.pow(2,55), [0x01ff, 0x003f, 0x004e, 0x0000, 0x0000, 0x6000, 0x0043]); > +testSerialization(null, [0x01ff, 0x003f, 0x0030]); > +testSerialization(/abc/, [0x01ff, 0x003f, 0x0352, 0x6261, 0x0063]); The coverage of added tests is not so good. Could you improve the coverage? e.g. "", "abc", 1.23, function(){}, etc... > LayoutTests/storage/resources/ssv.js:112 > +finishJSTest(); Nit: this is not needed. > LayoutTests/storage/ssv.html:8 > +<p id="description"></p> > +<div id="console"></div> Nit: These are not needed. > LayoutTests/storage/ssv.html:9 > +<script src="resources/ssv.js"></script> Nit: You can just directly write JavaScript here instead of including ssv.js.
Created attachment 174195 [details] Patch
abarth@ - r? We talked about this some months ago.. finally got around to fixing it using webkit_unit_tests note that I had to add webkit_test_support to the gyp to allow me to use the internals-injector helper.
Comment on attachment 174195 [details] Patch Attachment 174195 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14834409
Comment on attachment 174195 [details] Patch Attachment 174195 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14826683
Comment on attachment 174195 [details] Patch Attachment 174195 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14826687
Comment on attachment 174195 [details] Patch Attachment 174195 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14832438
Comment on attachment 174195 [details] Patch Woah, I don't think we want to link in the internals object into the unit tests. If you want to use internals, you should write a LayoutTest (perhaps somewhere in the platform/chromium directory). If you want to write a unit test, you should just call WebSerializedScriptValue::serialize and WebSerializedScriptValue::fromString directly from the unit test. Note: You can use WebFrame::executeScriptAndReturnValue to create interesting JavaScript objects to serialize.
Created attachment 174253 [details] Patch
I had no idea that there were *tests* in platform/chromium - that changes everything. New patch is much better (and fixes JSC build issues)
Comment on attachment 174253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174253&action=review > Source/WebCore/bindings/js/SerializedScriptValue.h:88 > String toString(); > + String toWireString() { return toString(); } What's the difference between toString and toWireString? Should we add a comment explaining the difference? > LayoutTests/platform/chromium/fast/storage/serialized-script-value.html:8 > + expectedV = expectedValues; expectedV <--- "V" isn't a great part of a variable name. we prefer complete words in variable names.
Comment on attachment 174253 [details] Patch Attachment 174253 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14837490
Comment on attachment 174253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174253&action=review >> Source/WebCore/bindings/js/SerializedScriptValue.h:88 >> + String toWireString() { return toString(); } > > What's the difference between toString and toWireString? Should we add a comment explaining the difference? In theory, there seems to be some intention of a serialization format saved somewhere, vs just passed around in memory. In practice, there is none. filed bug 102291
Created attachment 174272 [details] Patch for landing
Comment on attachment 174272 [details] Patch for landing Clearing flags on attachment: 174272 Committed r134691: <http://trac.webkit.org/changeset/134691>
All reviewed patches have been landed. Closing bug.
This has broken the Apple Windows build. http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/39479/steps/compile-webkit/logs/stdio
fix in bug 102302
Re-opened since this is blocked by bug 102337
As of 10:49 pm PST, the Apple Windows Debug build and GTK builds are broken. For completeness, the following are hyperlinks to the stdio from the bots as of r134733. Apple Windows Debug: <http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/57958/steps/compile-webkit/logs/stdio> GTK Linux 32-bit Release: <http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/28586/steps/compile-webkit/logs/stdio> GTK Linux 64-bit Debug: <http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/39171/steps/compile-webkit/logs/stdio> GTK Linux 64-bit Release: <http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/31012/steps/compile-webkit/logs/stdio>
Please don't proceed with landing a patch that will surely cause build breakage. Here's the patch required to fix the build on the GTK port: diff --git a/Source/autotools/symbols.filter b/Source/autotools/symbols.filter index 94d46cc..a3263de 100644 --- a/Source/autotools/symbols.filter +++ b/Source/autotools/symbols.filter @@ -210,6 +210,13 @@ _ZTVN7WebCore28InspectorFrontendClientLocal8SettingsE; _ZNK7WebCore5Frame15layerTreeAsTextEj; _ZN7WebCore9FrameView17setTracksRepaintsEb; _ZNK7WebCore5Frame25trackedRepaintRectsAsTextEv; +_ZN7WebCore4toJSEPN3JSC9ExecStateEPNS_17JSDOMGlobalObjectEPN3WTF11ArrayBufferE; +_ZN7WebCore13toArrayBufferEN3JSC7JSValueE; +_ZN7WebCore21SerializedScriptValue6createEPN3JSC9ExecStateENS1_7JSValueEPN3WTF6VectorINS5_6RefPtrINS_11MessagePortEEELm1EEEPNS6_INS7_INS5_11ArrayBufferEEELm1EEENS_22SerializationErrorModeE; +_ZN7WebCore21SerializedScriptValue6createERKN3WTF6StringE; +_ZN7WebCore21SerializedScriptValue8toStringEv; +_ZN7WebCore21SerializedScriptValue11deserializeEPN3JSC9ExecStateEPNS1_14JSGlobalObjectEPN3WTF6VectorINS6_6RefPtrINS_11MessagePortEEELm1EEENS_22SerializationErrorModeE; +_ZN7WebCore21SerializedScriptValueD1Ev; local: _Z*;
I decided to roll out this change in <http://trac.webkit.org/changeset/134747> (https://bugs.webkit.org/show_bug.cgi?id=102342) because the Apple Windows Debug build and GTK builds have been broken for over 5 hours. We should look to resolve the build failures offline and then re-land this change. I made an attempt to fix the Apple Windows Debug and GTK builds in <https://bugs.webkit.org/show_bug.cgi?id=102337>. This attempt didn't fix the build breakage and broke the Apple Windows Release build. For completeness, I reverted the following changesets (in order): http://trac.webkit.org/changeset/134691 (this bug) http://trac.webkit.org/changeset/134703 (Bug #102302) http://trac.webkit.org/changeset/134715 http://trac.webkit.org/changeset/134716 http://trac.webkit.org/changeset/134733 (Bug #102324)
Created attachment 174479 [details] Patch
Comment on attachment 174479 [details] Patch Running this through EWS again while I try to run as many local builds as I can.
Created attachment 174540 [details] Patch
Comment on attachment 174540 [details] Patch ok, this patch contains all of the subsequent patches, and it builds & runs on gtk, and last night I had the windows bots green. Here goes nothin'
Comment on attachment 174540 [details] Patch Clearing flags on attachment: 174540 Committed r134865: <http://trac.webkit.org/changeset/134865>
(In reply to comment #36) > (From update of attachment 174540 [details]) > Clearing flags on attachment: 174540 > > Committed r134865: <http://trac.webkit.org/changeset/134865> The Apple Windows Debug build is failing after this change: <http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/58018/steps/compile-webkit/logs/stdio>.
Re-opened since this is blocked by bug 102466
(In reply to comment #39) > Re-opened since this is blocked by bug 102466 As aforementioned in comment 38, this change broke the Apple Windows Debug build. I'm unclear how to resolve this breakage (*) and attempts to contact Alec Frett (alecf) and Adam Barth (abarth) on IRC were unsuccessful. Therefore, I decided to roll out this change so that we can fix the build offline. (*) I tried to fix identical build breakage yesterday. See comment 31 for more details.
The 2012-11-15 16:11 PST patch applies cleanly and builds on my Windows system in Debug mode. I'm building in Release mode and will see if that works as well.
Comment on attachment 174540 [details] Patch ok, so bfulgham tried the build himself, and didn't get an error, so we're thinking the windows buildbot may have just been flakey at the time. Retrying a landing.
Comment on attachment 174540 [details] Patch Clearing flags on attachment: 174540 Committed r135022: <http://trac.webkit.org/changeset/135022>
My release machine finished building successfully as well. If we continue to see Apple build failures, we might need someone at Apple to force it to regenerate the various files. These don't always get cleaned up properly from build-to-build if you don't manually clobber the directory. There is a trick they used to do (touching one of the IDL files), but I don't remember the details.