Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
47303005:
Implement native bindings for cast extensions API (Closed)
|
Created:
7 years, 1 month ago by Alpha Left Google Modified:
7 years, 1 month ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, feature-media-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement native bindings for cast extensions API
This is the native code implementation of the cast extensions API that
glues the JS API to Cast objects.
Also fixes the named/refactored types and functions to be consistent between
JS and C++ code.
BUG=301920
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232887
Patch Set 1 #
Total comments: 34
Patch Set 2 : fixed nits #
Total comments: 8
Patch Set 3 : fixed nits #
Total comments: 13
Patch Set 4 : renderer api only #Patch Set 5 : fix build #Patch Set 6 : api_renderer #Patch Set 7 : revert to original #Patch Set 8 : dependency #Patch Set 9 : git cl upload #Patch Set 10 : fix build again #Messages
Total messages: 43 (0 generated)
scherkus: chrome/renderer/media justinlin: everything else
https://codereview.chromium.org/47303005/diff/1/chrome/common/extensions/api/... File chrome/common/extensions/api/webrtc_cast_send_transport.idl (right): https://codereview.chromium.org/47303005/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/webrtc_cast_send_transport.idl:94: GetCapsCallback callback); either move this to previous line or move transportId to this line similar to createParams() below https://codereview.chromium.org/47303005/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/webrtc_cast_send_transport.idl:109: RtpParams params); ditto https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... File chrome/renderer/extensions/webrtc_native_handler.cc (right): https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:29: const char kSendTransportNotFound[] = can this fit on one line? https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:31: const char kUdpTransportNotFound[] = "The udp transport cannot be found"; s/udp/UDP/ https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:51: // Convert from extension API's RtpParams to CastRtpCaps. s/CastRtpCaps/CastRtpParams/ https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:57: // Convert from extension API's RtpParams to CastRtpCaps. this comment is backwards in fact, I'm not sure how valuable these comments are -- I'd almost consider having a block comment before all of them that describe the following as helpers to convert to/from extension API types and internal types https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:107: CHECK(args[0]->IsInt32()); what about args[1]? also, is crashing really the right thing to do if you were passed the wrong # / invalid object types? it seems like you're throwing V8 exceptions elsewhere when unable to convert to/from types ... should we be doing that here? https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:110: int inner_transport_id = static_cast<int>( can this fit on one line? https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:271: create_info.transport_id = last_transport_id_++; do we need to initialize create_info.params in any sort of way? https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:298: CHECK(args[0]->IsInt32()); what about args[1]? https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... File chrome/renderer/extensions/webrtc_native_handler.h (right): https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.h:51: // If the transport is not found then return NULL and throw a v8 as much as it's nice to save a handful of lines of code, I'm not too hot having helpers (marked as const, even!) modify V8 state by throwing an exception when I was reading the code it looked strange returning from Get{Send/Udp}Transport() calls w/o doing anything I'd almost prefer the extra bit of V8::ThrowException() code for clarity or at least the very least renaming these to Get{Udp,Send}TransportOrThrowException(). WDYT? https://codereview.chromium.org/47303005/diff/1/chrome/renderer/media/cast_se... File chrome/renderer/media/cast_send_transport.h (right): https://codereview.chromium.org/47303005/diff/1/chrome/renderer/media/cast_se... chrome/renderer/media/cast_send_transport.h:85: WebKit::WebMediaStreamTrack* track); considering you're passing in NULL here and not even saving |track| to a member variable, I'd revert this change until you actually have it wired up https://codereview.chromium.org/47303005/diff/1/chrome/renderer/media/cast_se... chrome/renderer/media/cast_send_transport.h:95: CastRtpParams CreateParams(CastRtpCaps remote_caps); any reason to pass by value versus const-ref? https://codereview.chromium.org/47303005/diff/1/chrome/renderer/media/cast_se... chrome/renderer/media/cast_send_transport.h:99: void Start(CastRtpParams params); ditto https://codereview.chromium.org/47303005/diff/1/chrome/test/data/extensions/a... File chrome/test/data/extensions/api_test/webrtc_cast/basics.js (right): https://codereview.chromium.org/47303005/diff/1/chrome/test/data/extensions/a... chrome/test/data/extensions/api_test/webrtc_cast/basics.js:9: {address: "127.0.0.1", port: 60613}); indent needs fixing -- I'd drop info.transportId to this line at a 4-space indent
https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... File chrome/renderer/extensions/webrtc_native_handler.cc (right): https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:286: CHECK(args[0]->IsInt32()); I don't think you want to CHECK these. They're user-provided. Validate and return is probably what you want. https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... File chrome/renderer/extensions/webrtc_native_handler.h (right): https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.h:56: int last_transport_id_; You probably want to take a look at ApiResourceManager: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... It's what's commonly used in extension API's to create objects that are keyed by some id passed back to js clients (socket api's can serve as inspiration).
https://codereview.chromium.org/47303005/diff/1/chrome/common/extensions/api/... File chrome/common/extensions/api/webrtc_cast_send_transport.idl (right): https://codereview.chromium.org/47303005/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/webrtc_cast_send_transport.idl:94: GetCapsCallback callback); On 2013/10/29 17:38:17, scherkus wrote: > either move this to previous line or move transportId to this line similar to > createParams() below Done. https://codereview.chromium.org/47303005/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/webrtc_cast_send_transport.idl:109: RtpParams params); On 2013/10/29 17:38:17, scherkus wrote: > ditto Done. https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... File chrome/renderer/extensions/webrtc_native_handler.cc (right): https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:29: const char kSendTransportNotFound[] = On 2013/10/29 17:38:17, scherkus wrote: > can this fit on one line? Done. https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:31: const char kUdpTransportNotFound[] = "The udp transport cannot be found"; On 2013/10/29 17:38:17, scherkus wrote: > s/udp/UDP/ Done. https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:51: // Convert from extension API's RtpParams to CastRtpCaps. On 2013/10/29 17:38:17, scherkus wrote: > s/CastRtpCaps/CastRtpParams/ Done. https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:57: // Convert from extension API's RtpParams to CastRtpCaps. On 2013/10/29 17:38:17, scherkus wrote: > this comment is backwards > > in fact, I'm not sure how valuable these comments are -- I'd almost consider > having a block comment before all of them that describe the following as helpers > to convert to/from extension API types and internal types Okay. I removed them all. https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:107: CHECK(args[0]->IsInt32()); On 2013/10/29 17:38:17, scherkus wrote: > what about args[1]? > > also, is crashing really the right thing to do if you were passed the wrong # / > invalid object types? > > it seems like you're throwing V8 exceptions elsewhere when unable to convert > to/from types ... should we be doing that here? args[1] is a MediaStreamTrack and the extension system should check for us. The extension system should check for the types and this seems to be the style used: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex... https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:110: int inner_transport_id = static_cast<int>( On 2013/10/29 17:38:17, scherkus wrote: > can this fit on one line? Done. https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:271: create_info.transport_id = last_transport_id_++; On 2013/10/29 17:38:17, scherkus wrote: > do we need to initialize create_info.params in any sort of way? It will initialized with 0s which is appropriate for now. https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:286: CHECK(args[0]->IsInt32()); On 2013/10/29 18:22:29, justinlin wrote: > I don't think you want to CHECK these. They're user-provided. Validate and > return is probably what you want. This seems to be the style used for the extension system: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex... https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:298: CHECK(args[0]->IsInt32()); On 2013/10/29 17:38:17, scherkus wrote: > what about args[1]? Checked for IsObject(). https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... File chrome/renderer/extensions/webrtc_native_handler.h (right): https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.h:51: // If the transport is not found then return NULL and throw a v8 On 2013/10/29 17:38:17, scherkus wrote: > as much as it's nice to save a handful of lines of code, I'm not too hot having > helpers (marked as const, even!) modify V8 state by throwing an exception > > when I was reading the code it looked strange returning from > Get{Send/Udp}Transport() calls w/o doing anything > > I'd almost prefer the extra bit of V8::ThrowException() code for clarity or at > least the very least renaming these to Get{Udp,Send}TransportOrThrowException(). > > WDYT? Okay I'll move the exception to each caller. https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.h:56: int last_transport_id_; On 2013/10/29 18:22:29, justinlin wrote: > You probably want to take a look at ApiResourceManager: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > It's what's commonly used in extension API's to create objects that are keyed by > some id passed back to js clients (socket api's can serve as inspiration). I don't think that's doable because ApiResourceManager is used in the browser process specifically. https://codereview.chromium.org/47303005/diff/1/chrome/renderer/media/cast_se... File chrome/renderer/media/cast_send_transport.h (right): https://codereview.chromium.org/47303005/diff/1/chrome/renderer/media/cast_se... chrome/renderer/media/cast_send_transport.h:85: WebKit::WebMediaStreamTrack* track); On 2013/10/29 17:38:17, scherkus wrote: > considering you're passing in NULL here and not even saving |track| to a member > variable, I'd revert this change until you actually have it wired up Done. https://codereview.chromium.org/47303005/diff/1/chrome/renderer/media/cast_se... chrome/renderer/media/cast_send_transport.h:95: CastRtpParams CreateParams(CastRtpCaps remote_caps); On 2013/10/29 17:38:17, scherkus wrote: > any reason to pass by value versus const-ref? Changed to use const &. https://codereview.chromium.org/47303005/diff/1/chrome/renderer/media/cast_se... chrome/renderer/media/cast_send_transport.h:99: void Start(CastRtpParams params); On 2013/10/29 17:38:17, scherkus wrote: > ditto Done.
OOC for the transportId stuff would sergeyu@'s http://src.chromium.org/viewvc/chrome?revision=226160&view=revision be of use when generating IDs, or is that intended for a different style of API? https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... File chrome/renderer/extensions/webrtc_native_handler.cc (right): https://codereview.chromium.org/47303005/diff/1/chrome/renderer/extensions/we... chrome/renderer/extensions/webrtc_native_handler.cc:107: CHECK(args[0]->IsInt32()); On 2013/10/29 19:52:18, Alpha wrote: > On 2013/10/29 17:38:17, scherkus wrote: > > what about args[1]? > > > > also, is crashing really the right thing to do if you were passed the wrong # > / > > invalid object types? > > > > it seems like you're throwing V8 exceptions elsewhere when unable to convert > > to/from types ... should we be doing that here? > > args[1] is a MediaStreamTrack and the extension system should check for us. > > The extension system should check for the types and this seems to be the style > used: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex... I've found at least one counter-example :) chrome/renderer/extensions/webstore_bindings.cc can you reach out to someone more familiar w/ extensions code for guidance? I haven't written any such code but if we treat it like we drive-by-web in Blink we should fail gracefully.
On 2013/10/29 23:47:21, scherkus wrote: > OOC for the transportId stuff would sergeyu@'s > http://src.chromium.org/viewvc/chrome?revision=226160&view=revision be of use > when generating IDs, or is that intended for a different style of API? > The code you're referring to seems to allow javascript to generate new IDs for whatever purpose. The IDs generated here is a resource ID. They serve different purpose. > I've found at least one counter-example :) > chrome/renderer/extensions/webstore_bindings.cc > > can you reach out to someone more familiar w/ extensions code for guidance? I > haven't written any such code but if we treat it like we drive-by-web in Blink > we should fail gracefully. This change will go through kalman@ for owners review. He'll look at the native bindings as well.
On 2013/10/30 01:17:20, Alpha wrote: > On 2013/10/29 23:47:21, scherkus wrote: > > OOC for the transportId stuff would sergeyu@'s > > http://src.chromium.org/viewvc/chrome?revision=226160&view=revision be of use > > when generating IDs, or is that intended for a different style of API? > > > > The code you're referring to seems to allow javascript to generate new IDs for > whatever purpose. > > The IDs generated here is a resource ID. They serve different purpose. > > > I've found at least one counter-example :) > > chrome/renderer/extensions/webstore_bindings.cc > > > > can you reach out to someone more familiar w/ extensions code for guidance? I > > haven't written any such code but if we treat it like we drive-by-web in Blink > > we should fail gracefully. > > This change will go through kalman@ for owners review. He'll look at the native > bindings as well. kalman: can you give guidance as to whether we should be CHECK()ing arguments in chrome/renderer/extensions/* bindings code?
On 2013/10/30 04:17:56, scherkus wrote: > On 2013/10/30 01:17:20, Alpha wrote: > > On 2013/10/29 23:47:21, scherkus wrote: > > > OOC for the transportId stuff would sergeyu@'s > > > http://src.chromium.org/viewvc/chrome?revision=226160&view=revision be of > use > > > when generating IDs, or is that intended for a different style of API? > > > > > > > The code you're referring to seems to allow javascript to generate new IDs for > > whatever purpose. > > > > The IDs generated here is a resource ID. They serve different purpose. > > > > > I've found at least one counter-example :) > > > chrome/renderer/extensions/webstore_bindings.cc > > > > > > can you reach out to someone more familiar w/ extensions code for guidance? > I > > > haven't written any such code but if we treat it like we drive-by-web in > Blink > > > we should fail gracefully. > > > > This change will go through kalman@ for owners review. He'll look at the > native > > bindings as well. > > kalman: can you give guidance as to whether we should be CHECK()ing arguments in > chrome/renderer/extensions/* bindings code? I can't follow the discussion here, so I'll say what I think you're asking. API function calls should go through schema-based argument validation. From memory 'setHandleRequest' actually overrides this unfortunately (something I've been meaning to fix) but in the meantime you can call argument validation yourself (or maybe we do actually validate arguments?). Argument validation will give you nice messages like "expected string for argument 1, got boolean". Once nice argument validation is in place (implemented in JS), CHECK that the correct arguments are being passed to C++, since anything to the contrary there would be a bug in our own code. Indeed, we should never crash the renderer for anything other than our own bug. Any code which doesn't CHECK is wrong in some way.
n https://codereview.chromium.org/47303005/diff/150001/chrome/renderer/resource... File chrome/renderer/resources/extensions/webrtc_cast_send_transport_custom_bindings.js (right): https://codereview.chromium.org/47303005/diff/150001/chrome/renderer/resource... chrome/renderer/resources/extensions/webrtc_cast_send_transport_custom_bindings.js:13: apiFunctions.setHandleRequest('create', kalman: are you saying that we should be doing argument validation here, meaning the C++ code in webrtc_native_handler.cc is permitted to CHECK() fail as that would indicate missing/incorrect validation here?
https://codereview.chromium.org/47303005/diff/150001/chrome/renderer/resource... File chrome/renderer/resources/extensions/webrtc_cast_send_transport_custom_bindings.js (right): https://codereview.chromium.org/47303005/diff/150001/chrome/renderer/resource... chrome/renderer/resources/extensions/webrtc_cast_send_transport_custom_bindings.js:13: apiFunctions.setHandleRequest('create', On 2013/10/30 17:56:35, scherkus wrote: > kalman: are you saying that we should be doing argument validation here, meaning > the C++ code in webrtc_native_handler.cc is permitted to CHECK() fail as that > would indicate missing/incorrect validation here? Actually it looks like validation does happen by default: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... so you don't have to change this code, and you should CHECK in the C++.
On 2013/10/30 18:13:48, kalman wrote: > https://codereview.chromium.org/47303005/diff/150001/chrome/renderer/resource... > File > chrome/renderer/resources/extensions/webrtc_cast_send_transport_custom_bindings.js > (right): > > https://codereview.chromium.org/47303005/diff/150001/chrome/renderer/resource... > chrome/renderer/resources/extensions/webrtc_cast_send_transport_custom_bindings.js:13: > apiFunctions.setHandleRequest('create', > On 2013/10/30 17:56:35, scherkus wrote: > > kalman: are you saying that we should be doing argument validation here, > meaning > > the C++ code in webrtc_native_handler.cc is permitted to CHECK() fail as that > > would indicate missing/incorrect validation here? > > Actually it looks like validation does happen by default: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... > > so you don't have to change this code, and you should CHECK in the C++. Thanks for clearing it up! LGTM for media stuff
LGTM https://codereview.chromium.org/47303005/diff/150001/chrome/renderer/extensio... File chrome/renderer/extensions/webrtc_native_handler.cc (right): https://codereview.chromium.org/47303005/diff/150001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:267: int transport_id = static_cast<int>(args[0]->ToInt32()->Value()); nit: const or don't need this temp https://codereview.chromium.org/47303005/diff/150001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:292: context()->CallFunction(v8::Handle<v8::Function>::Cast(args[0]), 1, nit: indent looks weird, you probably want to just put everything on the next line, or align the "1" https://codereview.chromium.org/47303005/diff/150001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:302: CastUdpTransport* transport = GetUdpTransport(transport_id); nit: don't need this temp
kalman: owner review please. https://codereview.chromium.org/47303005/diff/150001/chrome/renderer/extensio... File chrome/renderer/extensions/webrtc_native_handler.cc (right): https://codereview.chromium.org/47303005/diff/150001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:267: int transport_id = static_cast<int>(args[0]->ToInt32()->Value()); On 2013/10/30 20:08:38, justinlin wrote: > nit: const or don't need this temp Done. https://codereview.chromium.org/47303005/diff/150001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:292: context()->CallFunction(v8::Handle<v8::Function>::Cast(args[0]), 1, On 2013/10/30 20:08:38, justinlin wrote: > nit: indent looks weird, you probably want to just put everything on the next > line, or align the "1" Done. https://codereview.chromium.org/47303005/diff/150001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:302: CastUdpTransport* transport = GetUdpTransport(transport_id); On 2013/10/30 20:08:38, justinlin wrote: > nit: don't need this temp Done.
https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... File chrome/renderer/extensions/webrtc_native_handler.cc (right): https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:9: #include "chrome/common/extensions/api/webrtc_cast_udp_transport.h" you can't use the generated objects on the renderer unfortunately. this will at some point mysteriously fail to compile because renderer.gyp doesn't depend on api.gyp; and we can't make renderer.gyp depend on api.gyp because that would end up pulling in a lot of code. if you want to make this work then you could pull out just the parts you need into a separate rule in api.gyp, like api.gyp:renderer_apis or something. make sure the binary size of the renderer doesn't grow too much. or, just hand-write the conversion. it's a long-standing TODO (bug somewhere) to generate a direct v8 conversion rather than going via v8::Value, but, that's probably never going to happen, value hasn't been high enough. https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:108: const int inner_transport_id = static_cast<int>(args[0]->ToInt32()->Value()); use args[0]->Cast<v8::Int32> here, more efficient than doing a conversion. slightly. well, it's 0 effort rather than >0. same elsewhere. https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:355: CastSendTransport* WebRtcNativeHandler::GetSendTransport( maybe this method should actually throw the v8::exception, since every caller is doing that, use something like: CastSendTransport* cast_send_transport = NULL; if (!(cast_send_transport = GetSendTransport(id)) return; https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:364: CastUdpTransport* WebRtcNativeHandler::GetUdpTransport( ditto https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... File chrome/renderer/extensions/webrtc_native_handler.h (right): https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.h:51: // If the transport is not found then return NULL. Comment: Gets the Send or UDP transport indexed by |transport_id|. If not found, returns NULL. https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.h:59: UdpTransportMap udp_transport_map_; can you separate these with new lines? it's hard to read. like int last_transport_id_; typedef... SendTransportMap... typedef... UdpTransportMap
https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... File chrome/renderer/extensions/webrtc_native_handler.cc (right): https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:108: const int inner_transport_id = static_cast<int>(args[0]->ToInt32()->Value()); On 2013/10/30 20:41:57, kalman wrote: > use args[0]->Cast<v8::Int32> here, more efficient than doing a conversion. > slightly. well, it's 0 effort rather than >0. > > same elsewhere. actually you should probably use args[0]->IsInteger and use ToInt32 after all :)
https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... File chrome/renderer/extensions/webrtc_native_handler.cc (right): https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:9: #include "chrome/common/extensions/api/webrtc_cast_udp_transport.h" On 2013/10/30 20:41:57, kalman wrote: > you can't use the generated objects on the renderer unfortunately. this will at > some point mysteriously fail to compile because renderer.gyp doesn't depend on > api.gyp; and we can't make renderer.gyp depend on api.gyp because that would end > up pulling in a lot of code. > > if you want to make this work then you could pull out just the parts you need > into a separate rule in api.gyp, like api.gyp:renderer_apis or something. make > sure the binary size of the renderer doesn't grow too much. I observed that chrome_common.gypi depends on api.gyp:api. This means both renderer and browser link to the API. Is this a future work to isolate the browser API from renderer? Anyway I have created a api_renderer target that includes only the two files I need. They are both very small. > > or, just hand-write the conversion. I would like to avoid doing this. It's too tedious for future updates and easy to get wrong. https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:108: const int inner_transport_id = static_cast<int>(args[0]->ToInt32()->Value()); On 2013/10/30 20:44:15, kalman wrote: > On 2013/10/30 20:41:57, kalman wrote: > > use args[0]->Cast<v8::Int32> here, more efficient than doing a conversion. > > slightly. well, it's 0 effort rather than >0. > > > > same elsewhere. > > actually you should probably use args[0]->IsInteger and use ToInt32 after all :) There's only IsInt32() but not IsInteger(). I dropped the static_cast though. https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:355: CastSendTransport* WebRtcNativeHandler::GetSendTransport( On 2013/10/30 20:41:57, kalman wrote: > maybe this method should actually throw the v8::exception, since every caller is > doing that, use something like: > > CastSendTransport* cast_send_transport = NULL; > if (!(cast_send_transport = GetSendTransport(id)) > return; I think this is a difference in style. scherkus@ prefers to have the caller to throw the exception. It's probably clearer that way. https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... File chrome/renderer/extensions/webrtc_native_handler.h (right): https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.h:51: // If the transport is not found then return NULL. On 2013/10/30 20:41:57, kalman wrote: > Comment: Gets the Send or UDP transport indexed by |transport_id|. > If not found, returns NULL. Done. https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.h:59: UdpTransportMap udp_transport_map_; On 2013/10/30 20:41:57, kalman wrote: > can you separate these with new lines? it's hard to read. like > > int last_transport_id_; > > typedef... > SendTransportMap... > > typedef... > UdpTransportMap Done.
Isolating the API just for the renderer is much harder than I thought. Still trying.
Ok - I had my facts with the API dependency a little oversimplified - the relevant bug is https://code.google.com/p/chromium/issues/detail?id=298380. https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... File chrome/renderer/extensions/webrtc_native_handler.cc (right): https://codereview.chromium.org/47303005/diff/410001/chrome/renderer/extensio... chrome/renderer/extensions/webrtc_native_handler.cc:355: CastSendTransport* WebRtcNativeHandler::GetSendTransport( On 2013/10/30 23:58:26, Alpha wrote: > On 2013/10/30 20:41:57, kalman wrote: > > maybe this method should actually throw the v8::exception, since every caller > is > > doing that, use something like: > > > > CastSendTransport* cast_send_transport = NULL; > > if (!(cast_send_transport = GetSendTransport(id)) > > return; > > I think this is a difference in style. scherkus@ prefers to have the caller to > throw the exception. It's probably clearer that way. I understand your point, but I think it would be better to both have less code and an obviously consistent error reporting mechanism. Perhaps if you called this method GetSendTransportOrThrow (or similar) it would be clearer at the call site what was happening?
On 2013/10/31 02:44:43, kalman wrote: > Ok - I had my facts with the API dependency a little oversimplified - the > relevant bug is https://code.google.com/p/chromium/issues/detail?id=298380. It's a bit more complicated than that. I see a couple problems here: 1. chrome_common.gypi depends on chrome/common/extensions/api which means the API is already linked to both renderer and browser. Is there a plan to isolate it? 2. build/json_schema_bundle_compile.gypi uses tools/json_schema_compiler/compiler.py which produces generated_api.{cc, h}. This code is supposed to be in chrome/common. But there's a couple problems with this compiler.py. a. It uses a fixed path for the generated dir: https://code.google.com/p/chromium/codesearch#chromium/src/tools/json_schema_... b. It includes chrome/browser/extensions! https://code.google.com/p/chromium/codesearch#chromium/src/tools/json_schema_... Because I don't need to build the entire bundle since it's all renderer side and I'm handling my own native bindings. I think I won't have problem with the generated_api.{cc, h}. But I think the dependency problem needs to be fixed separately to completely isolate browser and renderer native bindings.
On 2013/10/31 02:52:08, Alpha wrote: > On 2013/10/31 02:44:43, kalman wrote: > > Ok - I had my facts with the API dependency a little oversimplified - the > > relevant bug is https://code.google.com/p/chromium/issues/detail?id=298380. > > It's a bit more complicated than that. > > I see a couple problems here: > 1. chrome_common.gypi depends on chrome/common/extensions/api which means the > API is already linked to both renderer and browser. Is there a plan to isolate > it? Is it linked? Hm ok. I thought these used dlls and that hard dependencies did something... else... to that? Out of my depth here. > > 2. build/json_schema_bundle_compile.gypi uses > tools/json_schema_compiler/compiler.py which produces generated_api.{cc, h}. > This code is supposed to be in chrome/common. But there's a couple problems with > this compiler.py. > > a. It uses a fixed path for the generated dir: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/json_schema_... > b. It includes chrome/browser/extensions! > https://code.google.com/p/chromium/codesearch#chromium/src/tools/json_schema_... Yeah known issue. Annoying. > > Because I don't need to build the entire bundle since it's all renderer side and > I'm handling my own native bindings. I think I won't have problem with the > generated_api.{cc, h}. But I think the dependency problem needs to be fixed > separately to completely isolate browser and renderer native bindings. Ok. If it compiles then it's fine with me. It may cause build flakiness in that bug though? But you're right - this is a separate issue - and I can look into separating out the build targets in a better way later.
kalman: please look again. The latest change isolate the renderer API into a separate target without depending on chrome/browser. I removed generating the bundle because it's not needed.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/47303005/670001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/47303005/670001
I tried to make it a separate target but it didn't work.. I'm now reverting it back to the original. I'll try to separate in a later pass.
Failed to trigger a try job on ios_rel_device HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/47303005/1100001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/47303005/1270001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/47303005/1270001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/47303005/1450001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/47303005/1450001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/47303005/1700001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/47303005/1700001
Message was sent while issue was closed.
Change committed as 232887
Message was sent while issue was closed.
This CL seems to break an internal WebRTC bot which builds with enable_webrtc set to 0. http://chromegw.corp.google.com/i/chromium.webrtc/builders/Linux%20Daily%20We...
Message was sent while issue was closed.
On 2013/11/06 08:14:34, henrika wrote: > This CL seems to break an internal WebRTC bot which builds with enable_webrtc > set to 0. > > http://chromegw.corp.google.com/i/chromium.webrtc/builders/Linux%2520Daily%25... Hey sorry I forgot to reply. I landed a fix https://codereview.chromium.org/58993004/. Does the compilation with enable_webrtc == 0 still happes?
Message was sent while issue was closed.
Our bot builds daily. I will force use of your patch and get back. Thanks! |
