Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1926873002:
MediaStream Image Capture (3): Adding mojo and browser impl for dummy takePhoto(). (Closed)
|
Created:
4 years, 7 months ago by mcasas Modified:
4 years, 7 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, nasko+codewatch_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaStream Image Capture (3): Adding mojo and browser impl for dummy takePhoto().
This CL adds idl and mojo definitions of JS method
takePhoto(). It also adds the necessary browser side
files: context and implementation. finally, it plugs them
into RendererBlinkPlatformImpl.
(The idea is to land all the necessary files and
gyp/gn/DEPS changes without having to dig deep
into the functionality, since that would make the CL
intractable, and would ramify into
MediaStreamManager, VideoCaptureManager etc.)
A LayoutTest is also added for takePhoto().
BUG=518807
TEST= run demo html in
https://cdn.rawgit.com/Miguelao/demos/master/imagecapture.html
with flag --enable-blink-features=ImageCapture
(click on "Open Camera 320x240", then on
"Create Image Capturer" and finally, repeatedly,
on "takePhoto").
Committed: https://crrev.com/5b6b8cb7dce823cd25c9ff0aa695559b4bc3a432
Cr-Commit-Position: refs/heads/master@{#391163}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : reillyg@ comments #
Total comments: 11
Patch Set 3 : comments; rebased; removed ImageCaptureContext. #
Total comments: 2
Patch Set 4 : Removed isActive() in favour of checking m_serviceRequests.isEmpty() #
Total comments: 4
Patch Set 5 : haraken@ comments #
Total comments: 6
Patch Set 6 : miu@s comments and minor rebase. #
Total comments: 4
Patch Set 7 : dcheng@ comments: |string data| --> array<uint8_t> #
Total comments: 2
Patch Set 8 : const auto& #Messages
Total messages: 45 (14 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== [WIP] MediaStream Image Capture (3): Adding mojo and browser impl for dummy takePhoto(). BUG=518807 TEST= run demo html in https://cdn.rawgit.com/Miguelao/demos/master/imagecapture.html with flag --enable-blink-features=ImageCapture (click on "Open Camera 320x240", then on "Create Image Capturer" and finally, repeatedly, on "takePhoto"). ========== to ========== MediaStream Image Capture (3): Adding mojo and browser impl for dummy takePhoto(). This CL adds idl and mojo definitions of JS method takePhoto(). It also adds the necessary browser side files: context and implementation. finally, it plugs them into RendererBlinkPlatformImpl. (The idea is to land all the necessary files and gyp/gn/DEPS changes without having to dig deep into the functionality, since that would make the CL intractable, and would ramify into MediaStreamManager, VideoCaptureManager etc.) A LayoutTest is also added for takePhoto(). BUG=518807 TEST= run demo html in https://cdn.rawgit.com/Miguelao/demos/master/imagecapture.html with flag --enable-blink-features=ImageCapture (click on "Open Camera 320x240", then on "Create Image Capturer" and finally, repeatedly, on "takePhoto"). ==========
reillyg@chromium.org changed reviewers: + reillyg@chromium.org
https://codereview.chromium.org/1926873002/diff/60001/content/browser/media/c... File content/browser/media/capture/image_capture_context.h (right): https://codereview.chromium.org/1926873002/diff/60001/content/browser/media/c... content/browser/media/capture/image_capture_context.h:8: #include "base/memory/scoped_vector.h" ScopedVector is deprecated in favor of std::vector<std::unique_ptr>. https://codereview.chromium.org/1926873002/diff/60001/content/browser/media/c... File content/browser/media/capture/image_capture_impl.h (right): https://codereview.chromium.org/1926873002/diff/60001/content/browser/media/c... content/browser/media/capture/image_capture_impl.h:25: mojo::Binding<blink::mojom::ImageCapture> binding_; We should not just leak ImageCaptureImpls indefinitely. Why not make this a StrongBinding?
PTAL https://codereview.chromium.org/1926873002/diff/60001/content/browser/media/c... File content/browser/media/capture/image_capture_context.h (right): https://codereview.chromium.org/1926873002/diff/60001/content/browser/media/c... content/browser/media/capture/image_capture_context.h:8: #include "base/memory/scoped_vector.h" On 2016/04/28 00:44:03, Reilly Grant wrote: > ScopedVector is deprecated in favor of std::vector<std::unique_ptr>. Done. https://codereview.chromium.org/1926873002/diff/60001/content/browser/media/c... File content/browser/media/capture/image_capture_impl.h (right): https://codereview.chromium.org/1926873002/diff/60001/content/browser/media/c... content/browser/media/capture/image_capture_impl.h:25: mojo::Binding<blink::mojom::ImageCapture> binding_; On 2016/04/28 00:44:03, Reilly Grant wrote: > We should not just leak ImageCaptureImpls indefinitely. Why not make this a > StrongBinding? Done.
https://codereview.chromium.org/1926873002/diff/80001/content/browser/media/c... File content/browser/media/capture/image_capture_context.cc (right): https://codereview.chromium.org/1926873002/diff/80001/content/browser/media/c... content/browser/media/capture/image_capture_context.cc:17: services_.push_back(std::unique_ptr<ImageCaptureImpl>( Sorry, I should have been clearer. Now that ImageCaptureImpl has a StrongBinding you don't need to maintain this vector. The ImageCaptureImpl instances are owned by the StrongBinding they contain and will be destroyed when the pipe is closed by the renderer.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:141: if (isActive(resolver)) Why do we need to check isActive? The m_serviceRequests should be active here because you're clearing out the m_serviceRequests in contextDestroyed, right?
https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:141: if (isActive(resolver)) On 2016/04/28 at 15:19:59, haraken wrote: > Why do we need to check isActive? The m_serviceRequests should be active here because you're clearing out the m_serviceRequests in contextDestroyed, right? I cargo culted this check from CallbackPromiseAdapter. Notice that contextDestroyed() clears m_serviceRequests without rejecting them because it knows that the context is no longer active. On the other hand here the pipe was closed unexpectedly and so the context may still be active.
https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:141: if (isActive(resolver)) On 2016/04/28 15:27:56, Reilly Grant wrote: > On 2016/04/28 at 15:19:59, haraken wrote: > > Why do we need to check isActive? The m_serviceRequests should be active here > because you're clearing out the m_serviceRequests in contextDestroyed, right? > > I cargo culted this check from CallbackPromiseAdapter. Notice that > contextDestroyed() clears m_serviceRequests without rejecting them because it > knows that the context is no longer active. On the other hand here the pipe was > closed unexpectedly and so the context may still be active. I'm saying the context is guaranteed to be alive here. If the context is dead, contextDestroyed should have been already called (and thus m_serviceRequests should be empty).
https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:128: resolver->reject(DOMException::create(AbortError, "Operation was aborted")); If the context is inactive you don't need to resolve or reject at all. https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:141: if (isActive(resolver)) On 2016/04/28 at 15:45:00, haraken wrote: > On 2016/04/28 15:27:56, Reilly Grant wrote: > > On 2016/04/28 at 15:19:59, haraken wrote: > > > Why do we need to check isActive? The m_serviceRequests should be active here > > because you're clearing out the m_serviceRequests in contextDestroyed, right? > > > > I cargo culted this check from CallbackPromiseAdapter. Notice that > > contextDestroyed() clears m_serviceRequests without rejecting them because it > > knows that the context is no longer active. On the other hand here the pipe was > > closed unexpectedly and so the context may still be active. > > I'm saying the context is guaranteed to be alive here. If the context is dead, contextDestroyed should have been already called (and thus m_serviceRequests should be empty). Ah, I see now.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/imagecapture/image_capture.mojom (right): https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/imagecapture/image_capture.mojom:1: // Copyright 2016 The Chromium Authors. All rights reserved. Please add a noparent IPC OWNERS rule to cover the mojom files, thanks!
guys PTAL https://codereview.chromium.org/1926873002/diff/80001/content/browser/media/c... File content/browser/media/capture/image_capture_context.cc (right): https://codereview.chromium.org/1926873002/diff/80001/content/browser/media/c... content/browser/media/capture/image_capture_context.cc:17: services_.push_back(std::unique_ptr<ImageCaptureImpl>( On 2016/04/28 01:09:57, Reilly Grant wrote: > Sorry, I should have been clearer. Now that ImageCaptureImpl has a StrongBinding > you don't need to maintain this vector. The ImageCaptureImpl instances are owned > by the StrongBinding they contain and will be destroyed when the pipe is closed > by the renderer. Ah OK, then I'm removing ImageCaptureContext altogether. https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:128: resolver->reject(DOMException::create(AbortError, "Operation was aborted")); On 2016/04/28 16:19:40, Reilly Grant wrote: > If the context is inactive you don't need to resolve or reject at all. Done. https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:141: if (isActive(resolver)) On 2016/04/28 16:19:40, Reilly Grant wrote: > On 2016/04/28 at 15:45:00, haraken wrote: > > On 2016/04/28 15:27:56, Reilly Grant wrote: > > > On 2016/04/28 at 15:19:59, haraken wrote: > > > > Why do we need to check isActive? The m_serviceRequests should be active > here > > > because you're clearing out the m_serviceRequests in contextDestroyed, > right? > > > > > > I cargo culted this check from CallbackPromiseAdapter. Notice that > > > contextDestroyed() clears m_serviceRequests without rejecting them because > it > > > knows that the context is no longer active. On the other hand here the pipe > was > > > closed unexpectedly and so the context may still be active. > > > > I'm saying the context is guaranteed to be alive here. If the context is dead, > contextDestroyed should have been already called (and thus m_serviceRequests > should be empty). > > Ah, I see now. Remove the check for isActive() then. https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/imagecapture/image_capture.mojom (right): https://codereview.chromium.org/1926873002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/imagecapture/image_capture.mojom:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/04/28 16:57:28, dcheng wrote: > Please add a noparent IPC OWNERS rule to cover the mojom files, thanks! Done.
https://codereview.chromium.org/1926873002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): https://codereview.chromium.org/1926873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:148: if (!isActive(resolver)) Would you instead check if(m_serviceRequests.isEmpty()) and remove the isActive method? (My point is that we should remove the isActive method -- if you want to observe the stop event of the execution context, you should observe ContextLifecycleObserver::contextDestroyed. And you're already doing that in ImageCapture::contextDestroyed.)
https://codereview.chromium.org/1926873002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): https://codereview.chromium.org/1926873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:148: if (!isActive(resolver)) On 2016/04/28 19:41:36, haraken wrote: > > Would you instead check if(m_serviceRequests.isEmpty()) and remove the isActive > method? > > (My point is that we should remove the isActive method -- if you want to observe > the stop event of the execution context, you should observe > ContextLifecycleObserver::contextDestroyed. And you're already doing that in > ImageCapture::contextDestroyed.) Done.
haraken@chromium.org changed reviewers: + yhirano@chromium.org
https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:72: m_serviceRequests.clear(); I'm just curious but don't you need to reject the promises when the execution context is gone? +yhirano https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:142: if (m_serviceRequests.isEmpty()) Nit: A better check might be if(!m_serviceRequests.contains(resolver)).
guys PTAL https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:72: m_serviceRequests.clear(); On 2016/04/29 06:50:20, haraken wrote: > > I'm just curious but don't you need to reject the promises when the execution > context is gone? > I'm checking other callers like USB::contextDestroyed() [1] PaymentRequest::contextDestroyed() [2] MediaKeys::contextDestroyed() [3] and none of them do anything special for the promises around. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [2] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [3] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > +yhirano https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:142: if (m_serviceRequests.isEmpty()) On 2016/04/29 06:50:20, haraken wrote: > > Nit: A better check might be if(!m_serviceRequests.contains(resolver)). Done.
On 2016/04/29 21:03:14, mcasas wrote: > guys PTAL > > https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): > > https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:72: > m_serviceRequests.clear(); > On 2016/04/29 06:50:20, haraken wrote: > > > > I'm just curious but don't you need to reject the promises when the execution > > context is gone? > > > > I'm checking other callers like > USB::contextDestroyed() [1] > PaymentRequest::contextDestroyed() [2] > MediaKeys::contextDestroyed() [3] > > and none of them do anything special > for the promises around. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > [3] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > +yhirano I'm wondering why it doesn't hit an assert. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... All promises must be resolved, rejected or detached before the corresponding ScriptPromiseResolver gets destructed. But given that next week is a holiday week in Tokyo and I'm not sure if yhirano@ is available or not, LGTM to land this for now. Let's clarify the point once yhirano@ is back.
On 2016/04/30 04:35:54, haraken wrote: > On 2016/04/29 21:03:14, mcasas wrote: > > guys PTAL > > > > > https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): > > > > > https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:72: > > m_serviceRequests.clear(); > > On 2016/04/29 06:50:20, haraken wrote: > > > > > > I'm just curious but don't you need to reject the promises when the > execution > > > context is gone? > > > > > > > I'm checking other callers like > > USB::contextDestroyed() [1] > > PaymentRequest::contextDestroyed() [2] > > MediaKeys::contextDestroyed() [3] > > > > and none of them do anything special > > for the promises around. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > [2] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > [3] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > +yhirano > > I'm wondering why it doesn't hit an assert. > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > All promises must be resolved, rejected or detached before the corresponding > ScriptPromiseResolver gets destructed. > > But given that next week is a holiday week in Tokyo and I'm not sure if yhirano@ > is available or not, LGTM to land this for now. Let's clarify the point once > yhirano@ is back. That assertion passes when the execution context is stopped. IIUC it should be stopped before contextDestroyed is called.
On 2016/05/02 03:17:52, yhirano wrote: > On 2016/04/30 04:35:54, haraken wrote: > > On 2016/04/29 21:03:14, mcasas wrote: > > > guys PTAL > > > > > > > > > https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp > (right): > > > > > > > > > https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:72: > > > m_serviceRequests.clear(); > > > On 2016/04/29 06:50:20, haraken wrote: > > > > > > > > I'm just curious but don't you need to reject the promises when the > > execution > > > > context is gone? > > > > > > > > > > I'm checking other callers like > > > USB::contextDestroyed() [1] > > > PaymentRequest::contextDestroyed() [2] > > > MediaKeys::contextDestroyed() [3] > > > > > > and none of them do anything special > > > for the promises around. > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > [2] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > [3] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > > > > +yhirano > > > > I'm wondering why it doesn't hit an assert. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > All promises must be resolved, rejected or detached before the corresponding > > ScriptPromiseResolver gets destructed. > > > > But given that next week is a holiday week in Tokyo and I'm not sure if > yhirano@ > > is available or not, LGTM to land this for now. Let's clarify the point once > > yhirano@ is back. > > That assertion passes when the execution context is stopped. IIUC it should be > stopped before contextDestroyed is called. ah, got it. Is it okay that we don't reject the promises when the execution context is stopped?
lgtm
mcasas@chromium.org changed reviewers: + miu@chromium.org
miu@ PTAL at content/browser/media/capture plz
lgtm, with advice: https://codereview.chromium.org/1926873002/diff/140001/content/browser/media/... File content/browser/media/capture/image_capture_impl.cc (right): https://codereview.chromium.org/1926873002/diff/140001/content/browser/media/... content/browser/media/capture/image_capture_impl.cc:31: : binding_(this, std::move(request)) {} Weird! But, I guess that's the way the designers of mojo::StrongBinding<> intended this to work. https://codereview.chromium.org/1926873002/diff/140001/content/browser/media/... File content/browser/media/capture/image_capture_impl.h (right): https://codereview.chromium.org/1926873002/diff/140001/content/browser/media/... content/browser/media/capture/image_capture_impl.h:21: void TakePhoto(const mojo::String& sourceid, Should the argument name be source_id (or is this SOP for mojo?)? https://codereview.chromium.org/1926873002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/imagecapture/ImageCapture-takePhoto.html (right): https://codereview.chromium.org/1926873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/imagecapture/ImageCapture-takePhoto.html:21: capturer.takePhoto() This has probably already been decided elsewhere, and I know I don't have all the context, but: It seems inaccurate to name the method "takePhoto" since photo is short for photograph and implies some kind of image capture of the real world. Suggestion: How about "takeFrameSnapshot" or just "takeSnapshot"?
mcasas@chromium.org changed reviewers: + piman@chromium.org
piman@ plz RS content/browser/DEPS content/browser/renderer_host/render_process_host_impl.cc https://codereview.chromium.org/1926873002/diff/140001/content/browser/media/... File content/browser/media/capture/image_capture_impl.cc (right): https://codereview.chromium.org/1926873002/diff/140001/content/browser/media/... content/browser/media/capture/image_capture_impl.cc:31: : binding_(this, std::move(request)) {} On 2016/05/02 21:44:51, miu wrote: > Weird! But, I guess that's the way the designers of mojo::StrongBinding<> > intended this to work. Yeah! I think I have to add a comment (l.15) every time I use this syntax :P https://codereview.chromium.org/1926873002/diff/140001/content/browser/media/... File content/browser/media/capture/image_capture_impl.h (right): https://codereview.chromium.org/1926873002/diff/140001/content/browser/media/... content/browser/media/capture/image_capture_impl.h:21: void TakePhoto(const mojo::String& sourceid, On 2016/05/02 21:44:51, miu wrote: > Should the argument name be source_id (or is this SOP for mojo?)? Yes. Done. https://codereview.chromium.org/1926873002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/imagecapture/ImageCapture-takePhoto.html (right): https://codereview.chromium.org/1926873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/imagecapture/ImageCapture-takePhoto.html:21: capturer.takePhoto() On 2016/05/02 21:44:51, miu wrote: > This has probably already been decided elsewhere, and I know I don't have all > the context, but: It seems inaccurate to name the method "takePhoto" since photo > is short for photograph and implies some kind of image capture of the real > world. Suggestion: How about "takeFrameSnapshot" or just "takeSnapshot"? There are two methods in this API: grabFrame(), which essentially grabs a VideoFrame out of the MediaStream Track [1], and takePhoto() which will take momentarily control of the VideoCaptureDevice to produce a photo, eventually with larger resolution etc. [1] https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ...
lgtm
dcheng@ plz RS changes in third_party/WebKit/public/
https://codereview.chromium.org/1926873002/diff/160001/content/browser/media/... File content/browser/media/capture/image_capture_impl.cc (right): https://codereview.chromium.org/1926873002/diff/160001/content/browser/media/... content/browser/media/capture/image_capture_impl.cc:26: callback.Run("text/plain", "not implemented"); Please add a mojom reviewer when you implement the body of this function, since that should really be part of the security review as well. https://codereview.chromium.org/1926873002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): https://codereview.chromium.org/1926873002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:145: WTF::CString string = data.utf8(); Is this string binary data or text data? With the Blink variant, does std::string -> WTF::String use 8-bit or 16-bit characters? If it's just binary data, maybe this should actually be a Vector<uint8_t>, SharedBuffer, WebData, or something similar?
dcheng@ PTAL https://codereview.chromium.org/1926873002/diff/160001/content/browser/media/... File content/browser/media/capture/image_capture_impl.cc (right): https://codereview.chromium.org/1926873002/diff/160001/content/browser/media/... content/browser/media/capture/image_capture_impl.cc:26: callback.Run("text/plain", "not implemented"); On 2016/05/02 22:38:58, dcheng wrote: > Please add a mojom reviewer when you implement the body of this function, since > that should really be part of the security review as well. Acknowledged. https://codereview.chromium.org/1926873002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): https://codereview.chromium.org/1926873002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:145: WTF::CString string = data.utf8(); On 2016/05/02 22:38:58, dcheng wrote: > Is this string binary data or text data? With the Blink variant, does > std::string -> WTF::String use 8-bit or 16-bit characters? > > If it's just binary data, maybe this should actually be a Vector<uint8_t>, > SharedBuffer, WebData, or something similar? The data being returned from content/ should be binary, e.g. a JPEG blob. So, changed as per offline conversation.
lgtm https://codereview.chromium.org/1926873002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): https://codereview.chromium.org/1926873002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:146: const auto storage = data.storage(); const auto& to make it obvious this is bound as a ref
https://codereview.chromium.org/1926873002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right): https://codereview.chromium.org/1926873002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:146: const auto storage = data.storage(); On 2016/05/03 00:23:55, dcheng wrote: > const auto& to make it obvious this is bound as a ref Done.
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, reillyg@chromium.org, miu@chromium.org, piman@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1926873002/#ps200001 (title: "const auto&")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1926873002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1926873002/200001
Message was sent while issue was closed.
Description was changed from ========== MediaStream Image Capture (3): Adding mojo and browser impl for dummy takePhoto(). This CL adds idl and mojo definitions of JS method takePhoto(). It also adds the necessary browser side files: context and implementation. finally, it plugs them into RendererBlinkPlatformImpl. (The idea is to land all the necessary files and gyp/gn/DEPS changes without having to dig deep into the functionality, since that would make the CL intractable, and would ramify into MediaStreamManager, VideoCaptureManager etc.) A LayoutTest is also added for takePhoto(). BUG=518807 TEST= run demo html in https://cdn.rawgit.com/Miguelao/demos/master/imagecapture.html with flag --enable-blink-features=ImageCapture (click on "Open Camera 320x240", then on "Create Image Capturer" and finally, repeatedly, on "takePhoto"). ========== to ========== MediaStream Image Capture (3): Adding mojo and browser impl for dummy takePhoto(). This CL adds idl and mojo definitions of JS method takePhoto(). It also adds the necessary browser side files: context and implementation. finally, it plugs them into RendererBlinkPlatformImpl. (The idea is to land all the necessary files and gyp/gn/DEPS changes without having to dig deep into the functionality, since that would make the CL intractable, and would ramify into MediaStreamManager, VideoCaptureManager etc.) A LayoutTest is also added for takePhoto(). BUG=518807 TEST= run demo html in https://cdn.rawgit.com/Miguelao/demos/master/imagecapture.html with flag --enable-blink-features=ImageCapture (click on "Open Camera 320x240", then on "Create Image Capturer" and finally, repeatedly, on "takePhoto"). ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== MediaStream Image Capture (3): Adding mojo and browser impl for dummy takePhoto(). This CL adds idl and mojo definitions of JS method takePhoto(). It also adds the necessary browser side files: context and implementation. finally, it plugs them into RendererBlinkPlatformImpl. (The idea is to land all the necessary files and gyp/gn/DEPS changes without having to dig deep into the functionality, since that would make the CL intractable, and would ramify into MediaStreamManager, VideoCaptureManager etc.) A LayoutTest is also added for takePhoto(). BUG=518807 TEST= run demo html in https://cdn.rawgit.com/Miguelao/demos/master/imagecapture.html with flag --enable-blink-features=ImageCapture (click on "Open Camera 320x240", then on "Create Image Capturer" and finally, repeatedly, on "takePhoto"). ========== to ========== MediaStream Image Capture (3): Adding mojo and browser impl for dummy takePhoto(). This CL adds idl and mojo definitions of JS method takePhoto(). It also adds the necessary browser side files: context and implementation. finally, it plugs them into RendererBlinkPlatformImpl. (The idea is to land all the necessary files and gyp/gn/DEPS changes without having to dig deep into the functionality, since that would make the CL intractable, and would ramify into MediaStreamManager, VideoCaptureManager etc.) A LayoutTest is also added for takePhoto(). BUG=518807 TEST= run demo html in https://cdn.rawgit.com/Miguelao/demos/master/imagecapture.html with flag --enable-blink-features=ImageCapture (click on "Open Camera 320x240", then on "Create Image Capturer" and finally, repeatedly, on "takePhoto"). Committed: https://crrev.com/5b6b8cb7dce823cd25c9ff0aa695559b4bc3a432 Cr-Commit-Position: refs/heads/master@{#391163} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/5b6b8cb7dce823cd25c9ff0aa695559b4bc3a432 Cr-Commit-Position: refs/heads/master@{#391163}
Message was sent while issue was closed.
On 2016/05/02 03:45:57, haraken wrote: > On 2016/05/02 03:17:52, yhirano wrote: > > On 2016/04/30 04:35:54, haraken wrote: > > > On 2016/04/29 21:03:14, mcasas wrote: > > > > guys PTAL > > > > > > > > > > > > > > https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:72: > > > > m_serviceRequests.clear(); > > > > On 2016/04/29 06:50:20, haraken wrote: > > > > > > > > > > I'm just curious but don't you need to reject the promises when the > > > execution > > > > > context is gone? > > > > > > > > > > > > > I'm checking other callers like > > > > USB::contextDestroyed() [1] > > > > PaymentRequest::contextDestroyed() [2] > > > > MediaKeys::contextDestroyed() [3] > > > > > > > > and none of them do anything special > > > > for the promises around. > > > > > > > > [1] > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > [2] > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > [3] > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > > > > > > > +yhirano > > > > > > I'm wondering why it doesn't hit an assert. > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > All promises must be resolved, rejected or detached before the corresponding > > > ScriptPromiseResolver gets destructed. > > > > > > But given that next week is a holiday week in Tokyo and I'm not sure if > > yhirano@ > > > is available or not, LGTM to land this for now. Let's clarify the point once > > > yhirano@ is back. > > > > That assertion passes when the execution context is stopped. IIUC it should be > > stopped before contextDestroyed is called. > > ah, got it. > > Is it okay that we don't reject the promises when the execution context is > stopped? Currently ScriptPromiseResolver gets detached when the execution context is stopped: after that, calling resolve or reject do nothing. This is related to https://codereview.chromium.org/1897783002/ and I'm open to change the behavior if we agree on what we can do after the execution context is stopped.
Message was sent while issue was closed.
On 2016/05/06 03:43:49, yhirano wrote: > On 2016/05/02 03:45:57, haraken wrote: > > On 2016/05/02 03:17:52, yhirano wrote: > > > On 2016/04/30 04:35:54, haraken wrote: > > > > On 2016/04/29 21:03:14, mcasas wrote: > > > > > guys PTAL > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... > > > > > File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1926873002/diff/120001/third_party/WebKit/Sou... > > > > > third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:72: > > > > > m_serviceRequests.clear(); > > > > > On 2016/04/29 06:50:20, haraken wrote: > > > > > > > > > > > > I'm just curious but don't you need to reject the promises when the > > > > execution > > > > > > context is gone? > > > > > > > > > > > > > > > > I'm checking other callers like > > > > > USB::contextDestroyed() [1] > > > > > PaymentRequest::contextDestroyed() [2] > > > > > MediaKeys::contextDestroyed() [3] > > > > > > > > > > and none of them do anything special > > > > > for the promises around. > > > > > > > > > > [1] > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > [2] > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > [3] > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > > > > > > > > > > +yhirano > > > > > > > > I'm wondering why it doesn't hit an assert. > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > > All promises must be resolved, rejected or detached before the > corresponding > > > > ScriptPromiseResolver gets destructed. > > > > > > > > But given that next week is a holiday week in Tokyo and I'm not sure if > > > yhirano@ > > > > is available or not, LGTM to land this for now. Let's clarify the point > once > > > > yhirano@ is back. > > > > > > That assertion passes when the execution context is stopped. IIUC it should > be > > > stopped before contextDestroyed is called. > > > > ah, got it. > > > > Is it okay that we don't reject the promises when the execution context is > > stopped? > > Currently ScriptPromiseResolver gets detached when the execution context is > stopped: after that, calling resolve or reject do nothing. This is related to > https://codereview.chromium.org/1897783002/ and I'm open to change the behavior > if we agree on what we can do after the execution context is stopped. What's the speced behavior, and how do other browsers behave? Does Promise get rejected when the execution context stops? |
