Bug 27170 – [V8] drawImage method of HTMLCanvasElement to accept HTMLVideoElement as argument

• Component: Platform
• Status: RESOLVED FIXED

[Hin-Chung Lam]

Copied code from the same method and accepts HTMLVideoElement.

[Hin-Chung Lam]

Created attachment 32603 [details]
patch

[Dimitri Glazkov (Google)]

Comment on attachment 32603 [details]
patch

> +            if (ec != 0) {
> +                V8Proxy::setDOMException(ec);
> +                return notHandledByInterceptor();
> +            }

This probably should be

if (ec)
   return throwError(ec);

> +            if (ec != 0) {
> +                V8Proxy::setDOMException(ec);
> +                return notHandledByInterceptor();
> +            }

Ditto.

> +            break;
> +        default:
> +            V8Proxy::throwError(V8Proxy::SyntaxError, "drawImage: Invalid number of arguments");
> +            return v8::Undefined();

return throwError(V8Proxy::SyntaxError, "drawImage: Invalid number of arguments");

> Index: ChangeLog
> ===================================================================
> --- ChangeLog	(revision 45742)
> +++ ChangeLog	(working copy)
> @@ -1,3 +1,12 @@
> [phone]  Alpha Lam  <[e-mail]>
> +
> +        Reviewed by NOBODY(OOPS!).
> +
> +        Changed CanvasRenderingContext2DDrawImage to accept HTMLVideoElement
> +        as a parameter of drawImage of HTMLCanvasElement.
> +


Wrong ChangeLog? Should be WebCore/ChangeLog.

[Hin-Chung Lam]

Created attachment 32672 [details]
patch

V8Proxy::throwError takes an ErrorType rather than error code (i.e. |ec|). So I keep part where error code is useful.

[Darin Adler]

Is this a V8-only issue, or a V8-only fix to an issue that affects WebKit even without V8?

[Hin-Chung Lam]

(In reply to comment #4)
> Is this a V8-only issue, or a V8-only fix to an issue that affects WebKit even
> without V8?

This is a V8-only issue. It only applies when V8 is used with WebKit.

[Darin Adler]

(In reply to comment #5)
> This is a V8-only issue. It only applies when V8 is used with WebKit.

OK. Please mention that in the bug title in the future.

[Dimitri Glazkov (Google)]

Here are the throwError helpers I was suggesting to use:

http://trac.webkit.org/browser/trunk/WebCore/bindings/v8/V8Proxy.h (look at the bottom, trac down atm).

[Hin-Chung Lam]

Created attachment 32678 [details]
patch

[Hin-Chung Lam]

Created attachment 32679 [details]
patch

[Hin-Chung Lam]

I have updated the patch using the throwError helper method.

[Dimitri Glazkov (Google)]

Note that "return throwError(...)" will give you v8::Undefined(), whereas "return notHandledByInterceptor()" will produce v8::Null() (which is a signal for the interceptor handler). I am not sure why we were using notHandledByInterceptor() here and worry if changing this may introduce layout test regressions?

[Hin-Chung Lam]

(In reply to comment #11)
> Note that "return throwError(...)" will give you v8::Undefined(), whereas
> "return notHandledByInterceptor()" will produce v8::Null() (which is a signal
> for the interceptor handler). I am not sure why we were using
> notHandledByInterceptor() here and worry if changing this may introduce layout
> test regressions?

It might break some layout tests, I'd be safe and do what is in the original code.

[Dimitri Glazkov (Google)]

Ok. Sorry for the run-around. Pls upload the updated patch and I'll r+. Can you also file a WebKit bug to investigate why we have these weird notHandledByInterceptor retvals?

[Hin-Chung Lam]

Created attachment 32726 [details]
patch

[Dimitri Glazkov (Google)]

Comment on attachment 32726 [details]
patch

Good to go.

[David Levin]

Committed as http://trac.webkit.org/changeset/45929
Fixed changelog to have a link to the bug in http://trac.webkit.org/changeset/45932