Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
230813002:
Make it possible to have <object>'s scriptableObject as a v8 object instead of NPObject. (Closed)
|
Created:
6 years, 8 months ago by Krzysztof Olczyk Modified:
6 years, 4 months ago CC:
adamk+blink_chromium.org, arv+blink, blink-reviews, Inactive, dglazkov+blink, haraken, jamesr, Nate Chapin, jsbell+bindings_chromium.org, kojih, kouhei+bindings_chromium.org, marja+watch_chromium.org, Mostyn Bramley-Moore, sof Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionMake it possible to have <object>'s scriptableObject as a v8 object instead of NPObject.
It is well-known that Chromium is gradually deprecating NPAPI
with the goal of removing it completely by the end of 2014.
This is said to include NPObject class as well.
However, currently, for blink::WebPlugin implementators
(for <object> tag), it is only possible to return the corresponding
scriptable object (to back properties of the <object> tag
as visible from JavaScript) as an NPObject*.
This CL introduces the possibility to provide the plain v8 object,
instead. This lets the embedder to provide scripting facilities
for plugins (or whatever else provides <object> tags)
using v8 api or modern means like gin.
On top of it, the compatibility with current NPObject-based
features like nsplugins is kept
which simply can be removed once NPAPI is fully deprecated.
BUG=351636
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179794
Patch Set 1 #Patch Set 2 : Rebased to current ToT (by raymes - fetched from https://codereview.chromium.org/426853002/) #
Total comments: 28
Patch Set 3 : Fixes after code review. #
Total comments: 4
Patch Set 4 : Final (hopefully) fixes. #Patch Set 5 : Fixed broken layout tests. #
Messages
Total messages: 36 (0 generated)
Hi, I've added you as reviewers as I've found you as reviewers of random somewhat-related reviews. Could you please take look at this CL related to NPAPI deprecation?
i'm not the one to make the call on whether this makes sense or not for blink, but from a v8 api perspective, you should always return a Local<> and never pass in a Local<>* other than that it looks okay
Can you explain more about the broader context for this change? We've talked about replacing the use of NPObject in PP_Var_Deprecated with Gin. Is this change part of that effort? I guess it would be helpful to reference a bug that describes the overall direction we're planning to take this code. I guess a change like this might make sense. My hesitation is that plugin scripting is a sore spot, and I want to make sure we're headed in a coherent direction here.
On 2014/04/09 15:42:56, abarth wrote: > Can you explain more about the broader context for this change? We've talked > about replacing the use of NPObject in PP_Var_Deprecated with Gin. Is this > change part of that effort? I guess it would be helpful to reference a bug that > describes the overall direction we're planning to take this code. > > I guess a change like this might make sense. My hesitation is that plugin > scripting is a sore spot, and I want to make sure we're headed in a coherent > direction here. yes, in an ideal world, we don't have synchronous scripting between the page and the plugin at all, so I also wonder what the overall goal is.
On 2014/04/09 15:42:56, abarth wrote: > Can you explain more about the broader context for this change? We've talked > about replacing the use of NPObject in PP_Var_Deprecated with Gin. Is this > change part of that effort? I guess it would be helpful to reference a bug that > describes the overall direction we're planning to take this code. The background for this change is an intention to rebase our JavaScript extensions required in certain projects, which were originally based on NPAPI which I described a bit in https://codereview.chromium.org/19861006/ (which was rejected as it was about the NPAPI interfaces). I'm submitting this change as I believe it can benefit Chromium in general. I'm not very familiar with ppapi, but I've seen https://code.google.com/p/chromium/issues/detail?id=351636 and it looks like this change can be a part of that effort. Currently, ppapi also returns scriptable object as an npobject [1] and this is the only way currently in Blink to "communicate" a scriptable object to Blink. So if we wanted to use gin for ppapi, this or equivalent change is a prerequisite anyway. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... > I guess a change like this might make sense. My hesitation is that plugin > scripting is a sore spot, and I want to make sure we're headed in a coherent > direction here. Yes, I'm aware of this being a sore point. But from what I observed from chromium-dev threads, etc., this CL should not diverge from the common direction.
On 2014/04/10 07:48:44, jochen wrote: > On 2014/04/09 15:42:56, abarth wrote: > > Can you explain more about the broader context for this change? We've talked > > about replacing the use of NPObject in PP_Var_Deprecated with Gin. Is this > > change part of that effort? I guess it would be helpful to reference a bug > that > > describes the overall direction we're planning to take this code. > > > > I guess a change like this might make sense. My hesitation is that plugin > > scripting is a sore spot, and I want to make sure we're headed in a coherent > > direction here. > > yes, in an ideal world, we don't have synchronous scripting between the page and > the plugin at all, so I also wonder what the overall goal is. But I believe that, between the ideal world and current world, there is a state where we would like to have NPAPI removed, but still want scripting capabilities of ppapi available (which https://code.google.com/p/chromium/issues/detail?id=351636 suggest we do).
On 2014/04/10 14:48:39, Krzysztof Olczyk wrote: > I'm submitting this change as I believe it can benefit Chromium in general. I'm > not very > familiar with ppapi, but I've seen > https://code.google.com/p/chromium/issues/detail?id=351636 > and it looks like this change can be a part of that effort. Yes, that makes sense. However, it would make more sense to start from the PPAPI side and figure out what we need to do in order to remove PPAPI's dependency on NPObject. It's likely to include a change like this one, but before we do the work of figuring out how to move PPAPI off NPObject, we won't know for sure. If we accept this CL without planning to complete that project, we'll have some dead code that isn't used in trunk. The normal thing we do with dead code is to remove it... > Yes, I'm aware of this being a sore point. But from what I observed from > chromium-dev threads, etc., this CL should not diverge from the common > direction. Yes, that seems likely to me, but my hesitation is that we don't have a complete plan for moving PPAPI off NPObject an onto this approach. I realize that's pretty far afield from how you'd like to use this code, but generally speaking, we try not to constrain trunk with dependencies from branches. (We have similar discussions with teams inside Google who try to add code to trunk that benefits only their branch.)
On 2014/04/10 21:12:19, abarth wrote: > On 2014/04/10 14:48:39, Krzysztof Olczyk wrote: > > I'm submitting this change as I believe it can benefit Chromium in general. > I'm > > not very > > familiar with ppapi, but I've seen > > https://code.google.com/p/chromium/issues/detail?id=351636 > > and it looks like this change can be a part of that effort. > > Yes, that makes sense. However, it would make more sense to start from the > PPAPI side and figure out what we need to do in order to remove PPAPI's > dependency on NPObject. It's likely to include a change like this one, but > before we do the work of figuring out how to move PPAPI off NPObject, we won't > know for sure. > > If we accept this CL without planning to complete that project, we'll have some > dead code that isn't used in trunk. The normal thing we do with dead code is to > remove it... That sounds reasonable, although it is not completely dead code, the current users of this code (through NPObject*) will still execute the code as it just moves the place where NPObject is wrapped into v8::Object while giving possibility to just use v8::Object. Simple graph here: http://people.opera.com/~kolczyk/illustrations/chromium_230813002.png illustrates it. But yes, if mainline Chromium decides to completely remove sync scripting in ppapi, the code will indeed be dead. So, I guess this CL needs to wait for the outcome of further discussions regarding ppapi, like https://code.google.com/p/chromium/issues/detail?id=351636. I'm linking this CL to that issue. > > Yes, I'm aware of this being a sore point. But from what I observed from > > chromium-dev threads, etc., this CL should not diverge from the common > > direction. > > Yes, that seems likely to me, but my hesitation is that we don't have a complete > plan for moving PPAPI off NPObject an onto this approach. I realize that's > pretty far afield from how you'd like to use this code, but generally speaking, > we try not to constrain trunk with dependencies from branches. (We have similar > discussions with teams inside Google who try to add code to trunk that benefits > only their branch.)
On 2014/04/11 11:05:16, Krzysztof Olczyk wrote: > On 2014/04/10 21:12:19, abarth wrote: > > On 2014/04/10 14:48:39, Krzysztof Olczyk wrote: > > > I'm submitting this change as I believe it can benefit Chromium in general. > > I'm > > > not very > > > familiar with ppapi, but I've seen > > > https://code.google.com/p/chromium/issues/detail?id=351636 > > > and it looks like this change can be a part of that effort. > > > > Yes, that makes sense. However, it would make more sense to start from the > > PPAPI side and figure out what we need to do in order to remove PPAPI's > > dependency on NPObject. It's likely to include a change like this one, but > > before we do the work of figuring out how to move PPAPI off NPObject, we won't > > know for sure. > > > > If we accept this CL without planning to complete that project, we'll have > some > > dead code that isn't used in trunk. The normal thing we do with dead code is > to > > remove it... > > > That sounds reasonable, although it is not completely dead code, > the current users of this code (through NPObject*) will still execute the code > as it just moves the place where NPObject is wrapped into v8::Object > while giving possibility to just use v8::Object. > Simple graph here: > http://people.opera.com/~kolczyk/illustrations/chromium_230813002.png > illustrates it. > > But yes, if mainline Chromium decides to completely remove sync scripting > in ppapi, the code will indeed be dead. > > So, I guess this CL needs to wait for the outcome of further discussions > regarding ppapi, like > https://code.google.com/p/chromium/issues/detail?id=351636. > > I'm linking this CL to that issue. > > > > > > Yes, I'm aware of this being a sore point. But from what I observed from > > > chromium-dev threads, etc., this CL should not diverge from the common > > > direction. > > > > Yes, that seems likely to me, but my hesitation is that we don't have a > complete > > plan for moving PPAPI off NPObject an onto this approach. I realize that's > > pretty far afield from how you'd like to use this code, but generally > speaking, > > we try not to constrain trunk with dependencies from branches. (We have > similar > > discussions with teams inside Google who try to add code to trunk that > benefits > > only their branch.) Without thinking about it too hard, this change looks very plausible to me as a first step to getting PPAPI off of NPObject. But nobody on Pepper team is working on it right now; I was going to try for it in 37. I can understand the hesitation to check in a speculative CL. It might be best to port some part of PPAPI in a Chromium side change to prove this out. Krzysztof: If you want to take a look at the Chromium/Pepper side, I can help out with reviews. Otherwise, I'll probably start looking at it in a few weeks time, targeting M37.
On 2014/04/15 18:13:11, dmichael wrote: > On 2014/04/11 11:05:16, Krzysztof Olczyk wrote: > > On 2014/04/10 21:12:19, abarth wrote: > > > On 2014/04/10 14:48:39, Krzysztof Olczyk wrote: > > > > I'm submitting this change as I believe it can benefit Chromium in > general. > > > I'm > > > > not very > > > > familiar with ppapi, but I've seen > > > > https://code.google.com/p/chromium/issues/detail?id=351636 > > > > and it looks like this change can be a part of that effort. > > > > > > Yes, that makes sense. However, it would make more sense to start from the > > > PPAPI side and figure out what we need to do in order to remove PPAPI's > > > dependency on NPObject. It's likely to include a change like this one, but > > > before we do the work of figuring out how to move PPAPI off NPObject, we > won't > > > know for sure. > > > > > > If we accept this CL without planning to complete that project, we'll have > > some > > > dead code that isn't used in trunk. The normal thing we do with dead code > is > > to > > > remove it... > > > > > > That sounds reasonable, although it is not completely dead code, > > the current users of this code (through NPObject*) will still execute the code > > as it just moves the place where NPObject is wrapped into v8::Object > > while giving possibility to just use v8::Object. > > Simple graph here: > > http://people.opera.com/~kolczyk/illustrations/chromium_230813002.png > > illustrates it. > > > > But yes, if mainline Chromium decides to completely remove sync scripting > > in ppapi, the code will indeed be dead. > > > > So, I guess this CL needs to wait for the outcome of further discussions > > regarding ppapi, like > > https://code.google.com/p/chromium/issues/detail?id=351636. > > > > I'm linking this CL to that issue. > > > > > > > > > > Yes, I'm aware of this being a sore point. But from what I observed from > > > > chromium-dev threads, etc., this CL should not diverge from the common > > > > direction. > > > > > > Yes, that seems likely to me, but my hesitation is that we don't have a > > complete > > > plan for moving PPAPI off NPObject an onto this approach. I realize that's > > > pretty far afield from how you'd like to use this code, but generally > > speaking, > > > we try not to constrain trunk with dependencies from branches. (We have > > similar > > > discussions with teams inside Google who try to add code to trunk that > > benefits > > > only their branch.) > > Without thinking about it too hard, this change looks very plausible to me as a > first step to getting PPAPI off of NPObject. But nobody on Pepper team is > working on it right now; I was going to try for it in 37. > > I can understand the hesitation to check in a speculative CL. It might be best > to port some part of PPAPI in a Chromium side change to prove this out. > > Krzysztof: If you want to take a look at the Chromium/Pepper side, I can help > out with reviews. Otherwise, I'll probably start looking at it in a few weeks > time, targeting M37. Unfortunately, I'm heading for the vacation now, so I won't be able to look into it soon. However, I may have some time when I'm back (mid-May), unless you start before, to take a look at it to try to come up with some demonstration of the usage in Pepper plugins. This will probably mean an attempt to port MessageChannel (src/content/renderer/pepper/message_channel.h) from the NPObject to gin. Next (bigger) step would be probably to port scriptable_object_deprecated infrastructure - unless it is meant for deprecation before NPObject is dead (which is not, I think, as I think it must be used for PepperFlash).
Hi everyone, I have uploaded the new patch set with the CL rebased to the current master (as done by raymes in https://codereview.chromium.org/426853002/). He has successfully reused it also it to transition ppapi plugins to gin. Does it mean we can process with the review of this CL now? abarth? dmichael?
On 2014/07/29 at 06:42:30, kolczyk wrote: > He has successfully reused it also it to transition ppapi plugins to gin. > Does it mean we can process with the review of this CL now? > abarth? dmichael? Great! I'm happy that we've figured out the PPAPI side of this equation.
Mostly naming nits, but a few substantive comments. https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... File Source/bindings/core/v8/ScriptController.cpp (right): https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... Source/bindings/core/v8/ScriptController.cpp:239: v8::HandleScope handleScope(m_isolate); Why is this handle scope needed? https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8DOMActivityLogger.cpp (right): https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... Source/bindings/core/v8/V8DOMActivityLogger.cpp:95: v8::HandleScope handleScope(isolate); Why is this handle scope needed? https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... File Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp (right): https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp:48: template <typename TElement, typename TPropertyId> TElement -> ElementType TPropertyId -> PropertyType propertyId -> property https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp:49: void GetScriptableObjectProperty(TPropertyId propertyId, const v8::PropertyCallbackInfo<v8::Value>& info) GetScriptableObjectProperty -> getScriptableObjectProperty https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp:70: template <typename TElement, typename TPropertyId> ditto https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp:71: void SetScriptableObjectProperty(TPropertyId propertyId, v8::Local<v8::Value> value, const v8::PropertyCallbackInfo<v8::Value>& info) SetScriptableObjectProperty -> setScriptableObjectProperty https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp:150: void InvokeOnScriptableObject(const v8::FunctionCallbackInfo<v8::Value>& info) InvokeOnScriptableObject -> invokeOnScriptableObject https://codereview.chromium.org/230813002/diff/10001/Source/core/plugins/Plug... File Source/core/plugins/PluginView.h (right): https://codereview.chromium.org/230813002/diff/10001/Source/core/plugins/Plug... Source/core/plugins/PluginView.h:54: virtual void getScriptableObject(v8::Isolate*, v8::Local<v8::Object>*) { } This should just return a v8::Handle<v8::Object> https://codereview.chromium.org/230813002/diff/10001/Source/web/WebPluginCont... File Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/230813002/diff/10001/Source/web/WebPluginCont... Source/web/WebPluginContainerImpl.cpp:458: v8::EscapableHandleScope handleScope(isolate); Why do we need a handle scope here? The caller must already have one if we're returning a v8::Local https://codereview.chromium.org/230813002/diff/10001/Source/web/WebPluginCont... Source/web/WebPluginContainerImpl.cpp:464: v8::Handle<v8::Value> v8value = toV8(m_element, v8::Handle<v8::Object>(), isolate); We deleted all callers of toV8 with empty handles in the second parameter. We shouldn't add any new ones. Can we pass the right object? https://codereview.chromium.org/230813002/diff/10001/Source/web/WebPluginCont... Source/web/WebPluginContainerImpl.cpp:466: return v8::Local<v8::Object>(); How could the v8value fail to be an object? Should this be an ASSERT? https://codereview.chromium.org/230813002/diff/10001/Source/web/WebPluginCont... File Source/web/WebPluginContainerImpl.h (right): https://codereview.chromium.org/230813002/diff/10001/Source/web/WebPluginCont... Source/web/WebPluginContainerImpl.h:111: virtual v8::Local<v8::Object> getV8ObjectForElement() OVERRIDE; getV8ObjectForElement -> v8ObjectForElement https://codereview.chromium.org/230813002/diff/10001/public/web/WebPlugin.h File public/web/WebPlugin.h (right): https://codereview.chromium.org/230813002/diff/10001/public/web/WebPlugin.h#n... public/web/WebPlugin.h:82: virtual bool getScriptableObject(v8::Isolate*, v8::Local<v8::Object>*) { return false; } Why not just return a v8::Handle? Can't we use the empty handle to signal failure? https://codereview.chromium.org/230813002/diff/10001/public/web/WebPluginCont... File public/web/WebPluginContainer.h (right): https://codereview.chromium.org/230813002/diff/10001/public/web/WebPluginCont... public/web/WebPluginContainer.h:92: virtual v8::Local<v8::Object> getV8ObjectForElement() = 0; We generally don't use "get" as a prefix for getters. Instead, we just name the function as a noun: v8ObjectForElement
Thanks for the review. I've applied your suggestions. https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... File Source/bindings/core/v8/ScriptController.cpp (right): https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... Source/bindings/core/v8/ScriptController.cpp:239: v8::HandleScope handleScope(m_isolate); On 2014/07/29 17:34:21, abarth wrote: > Why is this handle scope needed? Hmm. It happened to appear in raymes' rebase. Haven't noticed it. I don't think it's needed. Will remove it. https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8DOMActivityLogger.cpp (right): https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... Source/bindings/core/v8/V8DOMActivityLogger.cpp:95: v8::HandleScope handleScope(isolate); On 2014/07/29 17:34:21, abarth wrote: > Why is this handle scope needed? Done. https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... File Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp (right): https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp:48: template <typename TElement, typename TPropertyId> On 2014/07/29 17:34:21, abarth wrote: > TElement -> ElementType > TPropertyId -> PropertyType > propertyId -> property Done. https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp:49: void GetScriptableObjectProperty(TPropertyId propertyId, const v8::PropertyCallbackInfo<v8::Value>& info) On 2014/07/29 17:34:21, abarth wrote: > GetScriptableObjectProperty -> getScriptableObjectProperty Done. https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp:70: template <typename TElement, typename TPropertyId> On 2014/07/29 17:34:21, abarth wrote: > ditto Done. https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp:71: void SetScriptableObjectProperty(TPropertyId propertyId, v8::Local<v8::Value> value, const v8::PropertyCallbackInfo<v8::Value>& info) On 2014/07/29 17:34:21, abarth wrote: > SetScriptableObjectProperty -> setScriptableObjectProperty Done. https://codereview.chromium.org/230813002/diff/10001/Source/bindings/core/v8/... Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp:150: void InvokeOnScriptableObject(const v8::FunctionCallbackInfo<v8::Value>& info) On 2014/07/29 17:34:21, abarth wrote: > InvokeOnScriptableObject -> invokeOnScriptableObject Done. https://codereview.chromium.org/230813002/diff/10001/Source/core/plugins/Plug... File Source/core/plugins/PluginView.h (right): https://codereview.chromium.org/230813002/diff/10001/Source/core/plugins/Plug... Source/core/plugins/PluginView.h:54: virtual void getScriptableObject(v8::Isolate*, v8::Local<v8::Object>*) { } On 2014/07/29 17:34:21, abarth wrote: > This should just return a v8::Handle<v8::Object> Done. https://codereview.chromium.org/230813002/diff/10001/Source/web/WebPluginCont... File Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/230813002/diff/10001/Source/web/WebPluginCont... Source/web/WebPluginContainerImpl.cpp:458: v8::EscapableHandleScope handleScope(isolate); On 2014/07/29 17:34:22, abarth wrote: > Why do we need a handle scope here? The caller must already have one if we're > returning a v8::Local Done. https://codereview.chromium.org/230813002/diff/10001/Source/web/WebPluginCont... Source/web/WebPluginContainerImpl.cpp:464: v8::Handle<v8::Value> v8value = toV8(m_element, v8::Handle<v8::Object>(), isolate); On 2014/07/29 17:34:22, abarth wrote: > We deleted all callers of toV8 with empty handles in the second parameter. We > shouldn't add any new ones. Can we pass the right object? Done. https://codereview.chromium.org/230813002/diff/10001/Source/web/WebPluginCont... Source/web/WebPluginContainerImpl.cpp:466: return v8::Local<v8::Object>(); On 2014/07/29 17:34:22, abarth wrote: > How could the v8value fail to be an object? Should this be an ASSERT? Done. https://codereview.chromium.org/230813002/diff/10001/Source/web/WebPluginCont... File Source/web/WebPluginContainerImpl.h (right): https://codereview.chromium.org/230813002/diff/10001/Source/web/WebPluginCont... Source/web/WebPluginContainerImpl.h:111: virtual v8::Local<v8::Object> getV8ObjectForElement() OVERRIDE; On 2014/07/29 17:34:22, abarth wrote: > getV8ObjectForElement -> v8ObjectForElement Done. https://codereview.chromium.org/230813002/diff/10001/public/web/WebPlugin.h File public/web/WebPlugin.h (right): https://codereview.chromium.org/230813002/diff/10001/public/web/WebPlugin.h#n... public/web/WebPlugin.h:82: virtual bool getScriptableObject(v8::Isolate*, v8::Local<v8::Object>*) { return false; } On 2014/07/29 17:34:22, abarth wrote: > Why not just return a v8::Handle? Can't we use the empty handle to signal > failure? Done. https://codereview.chromium.org/230813002/diff/10001/public/web/WebPluginCont... File public/web/WebPluginContainer.h (right): https://codereview.chromium.org/230813002/diff/10001/public/web/WebPluginCont... public/web/WebPluginContainer.h:92: virtual v8::Local<v8::Object> getV8ObjectForElement() = 0; On 2014/07/29 17:34:22, abarth wrote: > We generally don't use "get" as a prefix for getters. Instead, we just name the > function as a noun: > > v8ObjectForElement Done.
Very close. One memory safety bug though. https://codereview.chromium.org/230813002/diff/30001/Source/web/WebPluginCont... File Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/230813002/diff/30001/Source/web/WebPluginCont... Source/web/WebPluginContainerImpl.cpp:461: ScriptState::Scope scope(scriptState); Oh noes! ScriptState::Scope contains a v8::HandleScope, which means the handle you get on line 463 is scoped to this function and can't be returned to the caller. Can you delete this line? https://codereview.chromium.org/230813002/diff/30001/public/web/WebPluginCont... File public/web/WebPluginContainer.h (right): https://codereview.chromium.org/230813002/diff/30001/public/web/WebPluginCont... public/web/WebPluginContainer.h:43: } I'd just #include <v8.h>
https://codereview.chromium.org/230813002/diff/30001/Source/web/WebPluginCont... File Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/230813002/diff/30001/Source/web/WebPluginCont... Source/web/WebPluginContainerImpl.cpp:461: ScriptState::Scope scope(scriptState); On 2014/07/30 17:52:25, abarth wrote: > Oh noes! ScriptState::Scope contains a v8::HandleScope, which means the handle > you get on line 463 is scoped to this function and can't be returned to the > caller. Can you delete this line? oh dear, right. Done. https://codereview.chromium.org/230813002/diff/30001/public/web/WebPluginCont... File public/web/WebPluginContainer.h (right): https://codereview.chromium.org/230813002/diff/30001/public/web/WebPluginCont... public/web/WebPluginContainer.h:43: } On 2014/07/30 17:52:25, abarth wrote: > I'd just #include <v8.h> Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kolczyk@opera.com/230813002/50001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/17884) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/19890)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/17899)
The CQ bit was checked by kolczyk@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kolczyk@opera.com/230813002/50001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/18031) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20072)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20096)
Sorry for keeping this stalled for couple of days, after failing trybot. It looks like setters still need some extra detour when scriptable object is an NPObject, for existing plugins before raymes integrates his CLs. As, without it, it is not possible to distinguish before "return undefined" and "property not found" for NPObject, and default setters for HTMLPluginObjects were never called. Uploaded new CL that fixes the problem.
The CQ bit was checked by kolczyk@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kolczyk@opera.com/230813002/70001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
On 2014/08/06 13:48:45, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_blink_dbg on tryserver.blink > (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...) Now it looks like random failures - the ones already noticed in https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/Status$20...
The CQ bit was checked by kolczyk@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kolczyk@opera.com/230813002/70001
Message was sent while issue was closed.
Change committed as 179794 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
