Bug 91208

Summary: Restructure V8Utilities::extractTransferables() with help of toV8Sequence()
Product: WebKit Reporter: Vineet Chaudhary (vineetc) <code.vineet>
Component: New BugsAssignee: 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 Flags
Patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-04
none
Updated Patch
haraken: review+
updated_patch none

Description Vineet Chaudhary (vineetc) 2012-07-13 02:40:56 PDT
With toV8Sequence() (r122555) now we can have generalised validation of the passed object for sequence type per WebIDL spec.
We can remove the specialised check for MessagePort from V8Utilities::extractTransferables() using toV8Sequence()
Comment 1 Vineet Chaudhary (vineetc) 2012-07-13 03:43:36 PDT
Created attachment 152208 [details]
Patch
Comment 2 Vineet Chaudhary (vineetc) 2012-07-13 03:47:57 PDT
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 3 Kentaro Hara 2012-07-13 04:29:01 PDT
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 4 WebKit Review Bot 2012-07-13 05:38:36 PDT
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
Comment 5 WebKit Review Bot 2012-07-13 05:38:39 PDT
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
Comment 6 Vineet Chaudhary (vineetc) 2012-07-13 06:45:23 PDT
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 7 Kentaro Hara 2012-07-13 08:05:58 PDT
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.
Comment 8 Kentaro Hara 2012-07-13 08:06:51 PDT
(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.
Comment 9 Vineet Chaudhary (vineetc) 2012-07-13 12:41:01 PDT
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 10 Kentaro Hara 2012-07-13 12:42:10 PDT
Comment on attachment 152321 [details]
updated_patch

OK!
Comment 11 WebKit Review Bot 2012-07-13 13:46:07 PDT
Comment on attachment 152321 [details]
updated_patch

Clearing flags on attachment: 152321

Committed r122628: <http://trac.webkit.org/changeset/122628>
Comment 12 WebKit Review Bot 2012-07-13 13:46:13 PDT
All reviewed patches have been landed.  Closing bug.