Summary: | Restructure V8Utilities::extractTransferables() with help of toV8Sequence() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vineet Chaudhary (vineetc) <code.vineet> | ||||||||||
Component: | New Bugs | Assignee: | Vineet Chaudhary (vineetc) <code.vineet> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, haraken, japhet, jochen, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Vineet Chaudhary (vineetc)
2012-07-13 02:40:56 PDT
Created attachment 152208 [details]
Patch
Comment on attachment 152208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152208&action=review > LayoutTests/ChangeLog:11 > + * platform/chromium-win/fast/workers/worker-context-multi-port-expected.txt: Removed. Currently this and below tests are in TextExpections of chromium as BUGWK74459 SKIP : fast/workers/worker-context-multi-port.html = PASS Comment on attachment 152208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152208&action=review > Source/WebCore/bindings/v8/V8Utilities.cpp:-105 > - if (value->IsArray()) { > - v8::Local<v8::Array> array = v8::Local<v8::Array>::Cast(value); > - length = array->Length(); > - } else { Is it OK to remove this part? I mean, the code might be: if (value->IsArray()) { v8::Local<v8::Array> array = v8::Local<v8::Array>::Cast(value); length = array->Length(); } else { if (toV8Sequence(value, length).IsEmpty()) return false; } > LayoutTests/ChangeLog:15 > + * platform/chromium-win/fast/workers/worker-context-multi-port-expected.txt: Removed. > + * platform/chromium-win/fast/workers/worker-multi-port-expected.txt: Removed. > + * platform/chromium/fast/dom/Window/window-postmessage-args-expected.txt: Removed. > + * platform/chromium/fast/events/constructors/message-event-constructor-expected.txt: Removed. > + * platform/chromium/fast/events/message-port-multi-expected.txt: Rebaselined. Great! Comment on attachment 152208 [details] Patch Attachment 152208 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13230186 New failing tests: http/tests/w3c/webperf/approved/navigation-timing/html5/test_performance_attributes_exist_in_object.html Created attachment 152228 [details]
Archive of layout-test-results from gce-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 152247 [details] Updated Patch (In reply to comment #3) > (From update of attachment 152208 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152208&action=review > > > Source/WebCore/bindings/v8/V8Utilities.cpp:-105 > > - if (value->IsArray()) { > > - v8::Local<v8::Array> array = v8::Local<v8::Array>::Cast(value); > > - length = array->Length(); > > - } else { > > Is it OK to remove this part? I mean, the code might be: > > if (value->IsArray()) { > v8::Local<v8::Array> array = v8::Local<v8::Array>::Cast(value); > length = array->Length(); > } else { > if (toV8Sequence(value, length).IsEmpty()) > return false; > } Though removing IsArray() check doesn't cause any behavioural change but having it will surely faster than every time calling toV8Sequence(). Thanks!! > New failing tests: > http/tests/w3c/webperf/approved/navigation-timing/html5/test_performance_attributes_exist_in_object.html Regarding failing perf test though I have rebased test expected but I really not sure if that is expected? Comment on attachment 152247 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152247&action=review > LayoutTests/ChangeLog:11 > + * http/tests/w3c/webperf/approved/navigation-timing/html5/test_performance_attributes_exist_in_object-expected.txt: Rebased failing perf-test This is not related to this patch. Let's keep it as is and ping committers who caused the regression. (In reply to comment #7) > This is not related to this patch. Let's keep it as is and ping committers who caused the regression. Just in case: "keep it as is" means please do not include the change in this patch. Created attachment 152321 [details] updated_patch Thanks haraken for review. > > LayoutTests/ChangeLog:11 > > + * http/tests/w3c/webperf/approved/navigation-timing/html5/test_performance_attributes_exist_in_object-expected.txt: Rebased failing perf-test > This is not related to this patch. Let's keep it as is and ping committers who caused the regression. I think this regression of r122528. Comment on attachment 152321 [details]
updated_patch
OK!
Comment on attachment 152321 [details] updated_patch Clearing flags on attachment: 152321 Committed r122628: <http://trac.webkit.org/changeset/122628> All reviewed patches have been landed. Closing bug. |