Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(53)

Issue 2683343002: Notify listeners on bitmaprenderer context changes (Closed)

Created:
3 years, 10 months ago by emircan
Modified:
3 years, 10 months ago
Reviewers:
mcasas, Justin Novosad
CC:
chromium-reviews, dshwang, ajuma+watch-canvas_chromium.org, blink-reviews-html_chromium.org, jam, feature-media-reviews_chromium.org, dglazkov+blink, Rik, darin-cc_chromium.org, blink-reviews, emircan+watch+capturefromdom_chromium.org, mcasas+watch+capturefromdom_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Notify listeners on bitmaprenderer context changes This CL addresses the bug in HTMLCanvasElement::getSourceImageForCanvas() caused by the uninitialized value of |status|. With the corrected status, we can send captured frame to canvas listeners. BUG=679610 TEST=Added layout test checking for a single frame capture and content_browsertest for a continuous capture. Review-Url: https://codereview.chromium.org/2683343002 Cr-Commit-Position: refs/heads/master@{#452743} Committed: https://chromium.googlesource.com/chromium/src/+/fab6dba98d3356e685ffee7ca2cd6acadb2fb4f9

Patch Set 1 : #

Total comments: 9

Patch Set 2 : #

Patch Set 3 : Rebase #

Patch Set 4 : Change didDraw() call. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -5 lines) Patch
M content/browser/webrtc/webrtc_capture_from_element_browsertest.cc View 1 2 2 chunks +7 lines, -1 line 0 comments Download
M content/test/data/media/canvas_capture.html View 1 2 2 chunks +11 lines, -1 line 0 comments Download
A third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html View 1 1 chunk +47 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp View 1 2 1 chunk +4 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.cpp View 1 2 3 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 40 (27 generated)
emircan
PTAL.
3 years, 10 months ago (2017-02-10 23:36:04 UTC) #6
Justin Novosad
lgtm for webkit
3 years, 10 months ago (2017-02-14 21:20:33 UTC) #9
emircan
Thanks. mcasas@ can you PTAL at /content/*?
3 years, 10 months ago (2017-02-14 21:25:58 UTC) #11
mcasas
tests and content/... lgtm with suggestions https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html File third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html (right): https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html#newcode25 third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html:25: var offscreen = ...
3 years, 10 months ago (2017-02-15 01:20:28 UTC) #12
emircan
https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html File third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html (right): https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html#newcode25 third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html:25: var offscreen = new OffscreenCanvas(400, 200); On 2017/02/15 01:20:27, ...
3 years, 10 months ago (2017-02-15 18:40:31 UTC) #14
emircan
junov@ these tests no longer pass after https://codereview.chromium.org/2653933003/. I can see that there are didDraw() ...
3 years, 10 months ago (2017-02-15 21:51:04 UTC) #18
Justin Novosad
On 2017/02/15 21:51:04, emircan wrote: > junov@ these tests no longer pass after > https://codereview.chromium.org/2653933003/. ...
3 years, 10 months ago (2017-02-15 22:34:29 UTC) #19
emircan
On 2017/02/15 22:34:29, Justin Novosad wrote: > On 2017/02/15 21:51:04, emircan wrote: > > junov@ ...
3 years, 10 months ago (2017-02-15 22:43:47 UTC) #20
Justin Novosad
I think the missing link is that ImageBitmapRenderingContext::transferFromImageBitmap needs to call CanvasRanderingContext::didDraw() instead of HTMLCanvasElement::didDraw() ...
3 years, 10 months ago (2017-02-21 19:16:35 UTC) #21
emircan
On 2017/02/21 19:16:35, Justin Novosad wrote: > I think the missing link is that > ...
3 years, 10 months ago (2017-02-23 22:33:35 UTC) #33
Justin Novosad
Awesome! lgtm.
3 years, 10 months ago (2017-02-23 22:39:42 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2683343002/100001
3 years, 10 months ago (2017-02-24 01:01:23 UTC) #37
commit-bot: I haz the power
3 years, 10 months ago (2017-02-24 03:47:30 UTC) #40
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/fab6dba98d3356e685ffee7ca2cd...

Powered by Google App Engine
This is Rietveld 408576698