Bug 27170 - [V8] drawImage method of HTMLCanvasElement to accept HTMLVideoElement as argument
Summary: [V8] drawImage method of HTMLCanvasElement to accept HTMLVideoElement as argu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Hin-Chung Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-10 19:32 PDT by Hin-Chung Lam
Modified: 2009-07-15 11:27 PDT (History)
3 users (show)

See Also:


Attachments
patch (2.61 KB, patch)
2009-07-10 19:33 PDT, Hin-Chung Lam
dglazkov: review-
Details | Formatted Diff | Diff
patch (2.60 KB, patch)
2009-07-13 13:06 PDT, Hin-Chung Lam
dglazkov: review-
Details | Formatted Diff | Diff
patch (deleted)
2009-07-13 14:50 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
patch (4.59 KB, patch)
2009-07-13 14:52 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
patch (3.08 KB, patch)
2009-07-14 11:54 PDT, Hin-Chung Lam
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hin-Chung Lam 2009-07-10 19:32:19 PDT
Copied code from the same method and accepts HTMLVideoElement.
Comment 1 Hin-Chung Lam 2009-07-10 19:33:23 PDT
Created attachment 32603 [details]
patch
Comment 2 Dimitri Glazkov (Google) 2009-07-10 21:54:24 PDT
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 @@
> +2009-07-07  Alpha Lam  <hclam@chromium.org>
> +
> +        Reviewed by NOBODY(OOPS!).
> +
> +        Changed CanvasRenderingContext2DDrawImage to accept HTMLVideoElement
> +        as a parameter of drawImage of HTMLCanvasElement.
> +


Wrong ChangeLog? Should be WebCore/ChangeLog.
Comment 3 Hin-Chung Lam 2009-07-13 13:06:31 PDT
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.
Comment 4 Darin Adler 2009-07-13 13:10:02 PDT
Is this a V8-only issue, or a V8-only fix to an issue that affects WebKit even without V8?
Comment 5 Hin-Chung Lam 2009-07-13 13:15:54 PDT
(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.
Comment 6 Darin Adler 2009-07-13 13:22:40 PDT
(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.
Comment 7 Dimitri Glazkov (Google) 2009-07-13 14:10:36 PDT
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).
Comment 8 Hin-Chung Lam 2009-07-13 14:50:54 PDT
Created attachment 32678 [details]
patch
Comment 9 Hin-Chung Lam 2009-07-13 14:52:22 PDT
Created attachment 32679 [details]
patch
Comment 10 Hin-Chung Lam 2009-07-13 14:53:37 PDT
I have updated the patch using the throwError helper method.
Comment 11 Dimitri Glazkov (Google) 2009-07-14 08:57:50 PDT
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?
Comment 12 Hin-Chung Lam 2009-07-14 10:36:57 PDT
(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.
Comment 13 Dimitri Glazkov (Google) 2009-07-14 10:41:24 PDT
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?
Comment 14 Hin-Chung Lam 2009-07-14 11:54:32 PDT
Created attachment 32726 [details]
patch
Comment 15 Dimitri Glazkov (Google) 2009-07-14 12:40:51 PDT
Comment on attachment 32726 [details]
patch

Good to go.
Comment 16 David Levin 2009-07-15 11:27:39 PDT
Committed as http://trac.webkit.org/changeset/45929
Fixed changelog to have a link to the bug in http://trac.webkit.org/changeset/45932