Bug 91208 – Restructure V8Utilities::extractTransferables() with help of toV8Sequence()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vineet Chaudhary (vineetc)]

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

[Vineet Chaudhary (vineetc)]

Created attachment 152208 [details]
Patch

[Vineet Chaudhary (vineetc)]

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

[Kentaro Hara]

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!

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Vineet Chaudhary (vineetc)]

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?

[Kentaro Hara]

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.

[Kentaro Hara]

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

[Vineet Chaudhary (vineetc)]

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.

[Kentaro Hara]

Comment on attachment 152321 [details]
updated_patch

OK!

[WebKit Review Bot]

Comment on attachment 152321 [details]
updated_patch

Clearing flags on attachment: 152321

Committed r122628: <http://trac.webkit.org/changeset/122628>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.