Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
964553003:
bindings: Use V8 MaybeLocal<> APIs in Dictionary class (Closed)
|
Created:
5 years, 9 months ago by bashi Modified:
5 years, 9 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, vivekg_samsung, arv+blink, vivekg Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Descriptionbindings: Use V8 MaybeLocal<> APIs in Dictionary class
We should use APIs which require explicit checks. No behavioral change.
BUG=462402
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192679
Patch Set 1 #
Total comments: 1
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 10
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #
Messages
Total messages: 25 (4 generated)
bashi@chromium.org changed reviewers: + dcarney@chromium.org, haraken@chromium.org
PTAL? This is a first attempt to use MaybeLocal<> APIs. We might want to have generic macros which call ToLocal in somewhere (e.g. V8BindingMacros.h), but I'd like to start this kind of case-by-case replacements to make sure we really need such generic macros. https://codereview.chromium.org/964553003/diff/1/Source/bindings/core/v8/Dict... File Source/bindings/core/v8/Dictionary.cpp (right): https://codereview.chromium.org/964553003/diff/1/Source/bindings/core/v8/Dict... Source/bindings/core/v8/Dictionary.cpp:138: if (v8Key.IsEmpty() || !object->Has(v8Key)) This seems redundant, but not sure we can omit IsEmpty() check. https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/Dictionary.h (right): https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/Dictionary.h:138: v8::Local<v8::Context> m_context; Should we hold ScriptState instead of v8::Local<v8::Context> ?
It looks like Dictionary is a bit too hard as a starting point. Shall we start with some simple custom bindings where we can do the replacement mechanically? Nit: I'd prefer landing the macro in an early phase even if it's not fully generalized. In other words, my suggestion would be to land the macro and a simple change to custom bindings in the first CL. https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/Dictionary.h (right): https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/Dictionary.h:48: class Dictionary final : public ScriptState::Observer { ScriptState::Observer is just intended to be used for some inspector-specific reason, and it's highly discouraged (because the lifetime of ScriptState is already complicated and we don't want to introduce something more complicated like observers). I don't think you need the observer. See my comment below. https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/Dictionary.h:138: v8::Local<v8::Context> m_context; On 2015/03/03 04:25:05, bashi1 wrote: > Should we hold ScriptState instead of v8::Local<v8::Context> ? A big YES. If a DOM object needs to keep some script state (including Context), the DOM object should hold RefPtr<ScriptState>. It is scary to hold a Context and I've replaced almost all of them with RefPtr<ScriptState> before :) However, in this particular case, Dictionary is guaranteed to be used on stack, which means that we're guaranteed to be in a correct ScriptState already when Dictionary is used. Thus what we should do here is: - Guarantee that Dictionary is used only on stack. I think this is already guaranteed in practice but we have no way to verify the fact. - Guarantee that we're always in a correct ScriptState. I hope this is already guaranteed. (As discussed offline, this is hard to verify.) - Just use m_isolate->GetCurrentContext() in Dictionary methods.
https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/Dictionary.h (right): https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/Dictionary.h:48: class Dictionary final : public ScriptState::Observer { On 2015/03/03 05:12:15, haraken wrote: > > ScriptState::Observer is just intended to be used for some inspector-specific > reason, and it's highly discouraged (because the lifetime of ScriptState is > already complicated and we don't want to introduce something more complicated > like observers). > > I don't think you need the observer. See my comment below. > I see. https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/Dictionary.h:138: v8::Local<v8::Context> m_context; On 2015/03/03 05:12:15, haraken wrote: > On 2015/03/03 04:25:05, bashi1 wrote: > > Should we hold ScriptState instead of v8::Local<v8::Context> ? > > A big YES. If a DOM object needs to keep some script state (including Context), > the DOM object should hold RefPtr<ScriptState>. It is scary to hold a Context > and I've replaced almost all of them with RefPtr<ScriptState> before :) > > However, in this particular case, Dictionary is guaranteed to be used on stack, > which means that we're guaranteed to be in a correct ScriptState already when > Dictionary is used. Thus what we should do here is: > > - Guarantee that Dictionary is used only on stack. I think this is already > guaranteed in practice but we have no way to verify the fact. > > - Guarantee that we're always in a correct ScriptState. I hope this is already > guaranteed. (As discussed offline, this is hard to verify.) > > - Just use m_isolate->GetCurrentContext() in Dictionary methods. Yeah, relying on RefPtr looks much better, and will follow your suggestion. But I have one concern: Won't we want to stop using GetCurrentContext() in the near future? If we want to stop using it, I guess that we would need this kind of weak reference (or make Dictionary take ScriptState each time get() called). One of the reasons why I choose Dictionary as the first instance was to make sure that this kind of our assumption is reasonable for the time being:)
https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/Dictionary.h (right): https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/Dictionary.h:138: v8::Local<v8::Context> m_context; On 2015/03/03 05:56:28, bashi1 wrote: > On 2015/03/03 05:12:15, haraken wrote: > > On 2015/03/03 04:25:05, bashi1 wrote: > > > Should we hold ScriptState instead of v8::Local<v8::Context> ? > > > > A big YES. If a DOM object needs to keep some script state (including > Context), > > the DOM object should hold RefPtr<ScriptState>. It is scary to hold a Context > > and I've replaced almost all of them with RefPtr<ScriptState> before :) > > > > However, in this particular case, Dictionary is guaranteed to be used on > stack, > > which means that we're guaranteed to be in a correct ScriptState already when > > Dictionary is used. Thus what we should do here is: > > > > - Guarantee that Dictionary is used only on stack. I think this is already > > guaranteed in practice but we have no way to verify the fact. > > > > - Guarantee that we're always in a correct ScriptState. I hope this is already > > guaranteed. (As discussed offline, this is hard to verify.) > > > > - Just use m_isolate->GetCurrentContext() in Dictionary methods. > > Yeah, relying on RefPtr looks much better, and will follow your suggestion. But > I have one concern: Won't we want to stop using GetCurrentContext() in the near > future? If we want to stop using it, I guess that we would need this kind of > weak reference (or make Dictionary take ScriptState each time get() called). > > One of the reasons why I choose Dictionary as the first instance was to make > sure that this kind of our assumption is reasonable for the time being:) I'm sorry for not being clear. Using GetCurrentContext() is completely fine (Note: We want to remove Isolate::GetCurrent(), but that's a totally different issue than Isolate::GetCurrentContext()). What we must make sure is that we are always in a correct context by entering a correct ScriptState::Scope. Being in a correct context is a mandatory requirement to make the system work correctly. And once it's guaranteed (I hope it's already guaranteed but it's very hard to guarantee the fact), GetCurrentContext() is guaranteed to return a correct context whenever it's called, so using GetCurrentContext() is totally fine. In short, what we should do is not to avoid GetCurrentContext but to guarantee that we're always in a correct context (by entering a correct ScriptState::Scope).
On 2015/03/03 06:12:14, haraken wrote: > https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... > File Source/bindings/core/v8/Dictionary.h (right): > > https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... > Source/bindings/core/v8/Dictionary.h:138: v8::Local<v8::Context> m_context; > On 2015/03/03 05:56:28, bashi1 wrote: > > On 2015/03/03 05:12:15, haraken wrote: > > > On 2015/03/03 04:25:05, bashi1 wrote: > > > > Should we hold ScriptState instead of v8::Local<v8::Context> ? > > > > > > A big YES. If a DOM object needs to keep some script state (including > > Context), > > > the DOM object should hold RefPtr<ScriptState>. It is scary to hold a > Context > > > and I've replaced almost all of them with RefPtr<ScriptState> before :) > > > > > > However, in this particular case, Dictionary is guaranteed to be used on > > stack, > > > which means that we're guaranteed to be in a correct ScriptState already > when > > > Dictionary is used. Thus what we should do here is: > > > > > > - Guarantee that Dictionary is used only on stack. I think this is already > > > guaranteed in practice but we have no way to verify the fact. > > > > > > - Guarantee that we're always in a correct ScriptState. I hope this is > already > > > guaranteed. (As discussed offline, this is hard to verify.) > > > > > > - Just use m_isolate->GetCurrentContext() in Dictionary methods. > > > > Yeah, relying on RefPtr looks much better, and will follow your suggestion. > But > > I have one concern: Won't we want to stop using GetCurrentContext() in the > near > > future? If we want to stop using it, I guess that we would need this kind of > > weak reference (or make Dictionary take ScriptState each time get() called). > > > > One of the reasons why I choose Dictionary as the first instance was to make > > sure that this kind of our assumption is reasonable for the time being:) > > I'm sorry for not being clear. > > Using GetCurrentContext() is completely fine (Note: We want to remove > Isolate::GetCurrent(), but that's a totally different issue than > Isolate::GetCurrentContext()). What we must make sure is that we are always in a > correct context by entering a correct ScriptState::Scope. Being in a correct > context is a mandatory requirement to make the system work correctly. And once > it's guaranteed (I hope it's already guaranteed but it's very hard to guarantee > the fact), GetCurrentContext() is guaranteed to return a correct context > whenever it's called, so using GetCurrentContext() is totally fine. > > In short, what we should do is not to avoid GetCurrentContext but to guarantee > that we're always in a correct context (by entering a correct > ScriptState::Scope). Hmm, then I'm confused at why ToXXX() need to take a Context. I understand that these ToXXX() methods need to have different signatures from the current ToXXX() methods for overload resolution, but it's totally unclear that why we need to pass a Context if we can get the correct Context from an Isolate. Solving overloading issue is the only reason to take a Context? I'm not fully sure this API change is the right thing to go.
On 2015/03/03 06:29:41, bashi1 wrote: > On 2015/03/03 06:12:14, haraken wrote: > > > https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... > > File Source/bindings/core/v8/Dictionary.h (right): > > > > > https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... > > Source/bindings/core/v8/Dictionary.h:138: v8::Local<v8::Context> m_context; > > On 2015/03/03 05:56:28, bashi1 wrote: > > > On 2015/03/03 05:12:15, haraken wrote: > > > > On 2015/03/03 04:25:05, bashi1 wrote: > > > > > Should we hold ScriptState instead of v8::Local<v8::Context> ? > > > > > > > > A big YES. If a DOM object needs to keep some script state (including > > > Context), > > > > the DOM object should hold RefPtr<ScriptState>. It is scary to hold a > > Context > > > > and I've replaced almost all of them with RefPtr<ScriptState> before :) > > > > > > > > However, in this particular case, Dictionary is guaranteed to be used on > > > stack, > > > > which means that we're guaranteed to be in a correct ScriptState already > > when > > > > Dictionary is used. Thus what we should do here is: > > > > > > > > - Guarantee that Dictionary is used only on stack. I think this is already > > > > guaranteed in practice but we have no way to verify the fact. > > > > > > > > - Guarantee that we're always in a correct ScriptState. I hope this is > > already > > > > guaranteed. (As discussed offline, this is hard to verify.) > > > > > > > > - Just use m_isolate->GetCurrentContext() in Dictionary methods. > > > > > > Yeah, relying on RefPtr looks much better, and will follow your suggestion. > > But > > > I have one concern: Won't we want to stop using GetCurrentContext() in the > > near > > > future? If we want to stop using it, I guess that we would need this kind of > > > weak reference (or make Dictionary take ScriptState each time get() called). > > > > > > One of the reasons why I choose Dictionary as the first instance was to make > > > sure that this kind of our assumption is reasonable for the time being:) > > > > I'm sorry for not being clear. > > > > Using GetCurrentContext() is completely fine (Note: We want to remove > > Isolate::GetCurrent(), but that's a totally different issue than > > Isolate::GetCurrentContext()). What we must make sure is that we are always in > a > > correct context by entering a correct ScriptState::Scope. Being in a correct > > context is a mandatory requirement to make the system work correctly. And once > > it's guaranteed (I hope it's already guaranteed but it's very hard to > guarantee > > the fact), GetCurrentContext() is guaranteed to return a correct context > > whenever it's called, so using GetCurrentContext() is totally fine. > > > > In short, what we should do is not to avoid GetCurrentContext but to guarantee > > that we're always in a correct context (by entering a correct > > ScriptState::Scope). > > Hmm, then I'm confused at why ToXXX() need to take a Context. I understand that > these ToXXX() methods need to have different signatures from the current ToXXX() > methods for overload resolution, but it's totally unclear that why we need to > pass a Context if we can get the correct Context from an Isolate. Solving > overloading issue is the only reason to take a Context? I'm not fully sure this > API change is the right thing to go. If I'm understanding things correctly, I think your understanding is correct. Solving the overloading issue is the only practical reason to take a Context. By letting the V8 API take a Context, it apparently improves the situation in that we can start passing a correct Context to the V8 API, but that actually doesn't improve (or degrade) the situation. We still always need to be in a correct Context and thus what we should do when calling the V8 API is just to call isolate->GetCurrentContext. In other words, we're just migrating isolate->GetCurrentContext from V8 to an embedder. Even if you aim at removing GetCurrentContext by storing ScriptState here and there, I guess that won't improve the situation either. Then you need to guarantee that you always store the correct ScriptState, but that would be as hard as guaranteeing that we always enter a correct ScriptState & use GetCurrentContext. In essence, we anyway need to somehow guarantee that we're always in a correct context. Entering ScriptState & using GetCurrentContext is one way to do that, and storing ScriptState & removing GetCurrentContext is another (a bit more expensive) way to do that. But the point is that, whichever we take, we won't be benefited by the V8 API that takes a Context. (If you think the V8 API is not a right way to go, we can push it back to the V8 team.)
On 2015/03/03 06:55:12, haraken wrote: > On 2015/03/03 06:29:41, bashi1 wrote: > > On 2015/03/03 06:12:14, haraken wrote: > > > > > > https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... > > > File Source/bindings/core/v8/Dictionary.h (right): > > > > > > > > > https://codereview.chromium.org/964553003/diff/20001/Source/bindings/core/v8/... > > > Source/bindings/core/v8/Dictionary.h:138: v8::Local<v8::Context> m_context; > > > On 2015/03/03 05:56:28, bashi1 wrote: > > > > On 2015/03/03 05:12:15, haraken wrote: > > > > > On 2015/03/03 04:25:05, bashi1 wrote: > > > > > > Should we hold ScriptState instead of v8::Local<v8::Context> ? > > > > > > > > > > A big YES. If a DOM object needs to keep some script state (including > > > > Context), > > > > > the DOM object should hold RefPtr<ScriptState>. It is scary to hold a > > > Context > > > > > and I've replaced almost all of them with RefPtr<ScriptState> before :) > > > > > > > > > > However, in this particular case, Dictionary is guaranteed to be used on > > > > stack, > > > > > which means that we're guaranteed to be in a correct ScriptState already > > > when > > > > > Dictionary is used. Thus what we should do here is: > > > > > > > > > > - Guarantee that Dictionary is used only on stack. I think this is > already > > > > > guaranteed in practice but we have no way to verify the fact. > > > > > > > > > > - Guarantee that we're always in a correct ScriptState. I hope this is > > > already > > > > > guaranteed. (As discussed offline, this is hard to verify.) > > > > > > > > > > - Just use m_isolate->GetCurrentContext() in Dictionary methods. > > > > > > > > Yeah, relying on RefPtr looks much better, and will follow your > suggestion. > > > But > > > > I have one concern: Won't we want to stop using GetCurrentContext() in the > > > near > > > > future? If we want to stop using it, I guess that we would need this kind > of > > > > weak reference (or make Dictionary take ScriptState each time get() > called). > > > > > > > > One of the reasons why I choose Dictionary as the first instance was to > make > > > > sure that this kind of our assumption is reasonable for the time being:) > > > > > > I'm sorry for not being clear. > > > > > > Using GetCurrentContext() is completely fine (Note: We want to remove > > > Isolate::GetCurrent(), but that's a totally different issue than > > > Isolate::GetCurrentContext()). What we must make sure is that we are always > in > > a > > > correct context by entering a correct ScriptState::Scope. Being in a correct > > > context is a mandatory requirement to make the system work correctly. And > once > > > it's guaranteed (I hope it's already guaranteed but it's very hard to > > guarantee > > > the fact), GetCurrentContext() is guaranteed to return a correct context > > > whenever it's called, so using GetCurrentContext() is totally fine. > > > > > > In short, what we should do is not to avoid GetCurrentContext but to > guarantee > > > that we're always in a correct context (by entering a correct > > > ScriptState::Scope). > > > > Hmm, then I'm confused at why ToXXX() need to take a Context. I understand > that > > these ToXXX() methods need to have different signatures from the current > ToXXX() > > methods for overload resolution, but it's totally unclear that why we need to > > pass a Context if we can get the correct Context from an Isolate. Solving > > overloading issue is the only reason to take a Context? I'm not fully sure > this > > API change is the right thing to go. > > If I'm understanding things correctly, I think your understanding is correct. > Solving the overloading issue is the only practical reason to take a Context. > > By letting the V8 API take a Context, it apparently improves the situation in > that we can start passing a correct Context to the V8 API, but that actually > doesn't improve (or degrade) the situation. We still always need to be in a > correct Context and thus what we should do when calling the V8 API is just to > call isolate->GetCurrentContext. In other words, we're just migrating > isolate->GetCurrentContext from V8 to an embedder. > > Even if you aim at removing GetCurrentContext by storing ScriptState here and > there, I guess that won't improve the situation either. Then you need to > guarantee that you always store the correct ScriptState, but that would be as > hard as guaranteeing that we always enter a correct ScriptState & use > GetCurrentContext. In essence, we anyway need to somehow guarantee that we're > always in a correct context. Entering ScriptState & using GetCurrentContext is > one way to do that, and storing ScriptState & removing GetCurrentContext is > another (a bit more expensive) way to do that. But the point is that, whichever > we take, we won't be benefited by the V8 API that takes a Context. > > (If you think the V8 API is not a right way to go, we can push it back to the V8 > team.) Thanks for the explanation. I have better understanding now. I'll put a link to this thread on the v8 tracking bug to provide feedback:)
https://codereview.chromium.org/964553003/diff/60001/Source/bindings/core/v8/... File Source/bindings/core/v8/Dictionary.cpp (right): https://codereview.chromium.org/964553003/diff/60001/Source/bindings/core/v8/... Source/bindings/core/v8/Dictionary.cpp:236: v8::MaybeLocal<v8::String> maybeKey = properties->Get(i)->ToString(context()); you want to use a macro here. basically, you should never have to write MaybeLocal almost anywhere https://codereview.chromium.org/964553003/diff/60001/Source/bindings/core/v8/... File Source/bindings/core/v8/DictionaryHelperForCore.cpp (right): https://codereview.chromium.org/964553003/diff/60001/Source/bindings/core/v8/... Source/bindings/core/v8/DictionaryHelperForCore.cpp:243: v8::Local<v8::Integer> v8Integer; same here - macros everywhere
bashi@chromium.org changed reviewers: + yukishiino@chromium.org
Updated. PTAL? https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... File Source/bindings/core/v8/DictionaryHelperForCore.cpp (left): https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... Source/bindings/core/v8/DictionaryHelperForCore.cpp:68: v8::Local<v8::Boolean> v8Bool = v8Value->ToBoolean(dictionary.isolate()); I think we can just use BooleanValue() here. Same for Int32Value etc. https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... Source/bindings/core/v8/DictionaryHelperForCore.cpp:108: v8::TryCatch block; Removed this TryCatch because we needed this to check whether an exception is thrown. Now we can check it via Maybe.
https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... File Source/bindings/core/v8/Dictionary.cpp (right): https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... Source/bindings/core/v8/Dictionary.cpp:218: return true; I'm not sure if this should always be true. If the empty handle means that there is no property name, then it should be true. However, if the empty handle means that some exception was thrown, then it should be false. In other words, this V8 API breaks the invariant that an empty handle is returned if and only if an exception was thrown. I guess we need to redesign the API? https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... Source/bindings/core/v8/Dictionary.cpp:256: bool Dictionary::toObject(v8::Local<v8::Object>* object) const Can we change v8::Local<v8::Object>* to v8::Local<v8::Object>? You don't need to worry about the copying overhead since v8::Local<v8::Object> just holds a pointer to the v8::Object. We basically don't want to use v8::Local<v8::Object>* in the binding layer to avoid complexity. https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... File Source/bindings/core/v8/DictionaryHelperForCore.cpp (right): https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... Source/bindings/core/v8/DictionaryHelperForCore.cpp:68: return getValueFromMaybe(v8Value->BooleanValue(dictionary.v8Context()), value); Can we rename getValueFromMaybe to something better? Other macros start with "v8Call".
https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... File Source/bindings/core/v8/Dictionary.cpp (right): https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... Source/bindings/core/v8/Dictionary.cpp:218: return true; On 2015/03/27 03:33:33, haraken wrote: > > I'm not sure if this should always be true. If the empty handle means that there > is no property name, then it should be true. However, if the empty handle means > that some exception was thrown, then it should be false. > > In other words, this V8 API breaks the invariant that an empty handle is > returned if and only if an exception was thrown. I guess we need to redesign the > API? > Yeah, I thought it was inconsistent, but kept the current behavior. Let's try to negate this. https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... Source/bindings/core/v8/Dictionary.cpp:256: bool Dictionary::toObject(v8::Local<v8::Object>* object) const On 2015/03/27 03:33:33, haraken wrote: > > Can we change v8::Local<v8::Object>* to v8::Local<v8::Object>? You don't need to > worry about the copying overhead since v8::Local<v8::Object> just holds a > pointer to the v8::Object. > > We basically don't want to use v8::Local<v8::Object>* in the binding layer to > avoid complexity. |object| is an out parameter, as ToLocal takes a pointer. We can use reference here (like other blink code), but I just followed what ToLocal does. https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... File Source/bindings/core/v8/DictionaryHelperForCore.cpp (right): https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... Source/bindings/core/v8/DictionaryHelperForCore.cpp:68: return getValueFromMaybe(v8Value->BooleanValue(dictionary.v8Context()), value); On 2015/03/27 03:33:33, haraken wrote: > > Can we rename getValueFromMaybe to something better? Other macros start with > "v8Call". renamed to v8Call().
https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... File Source/bindings/core/v8/Dictionary.cpp (right): https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... Source/bindings/core/v8/Dictionary.cpp:218: return true; On 2015/03/27 03:55:24, bashi1 wrote: > On 2015/03/27 03:33:33, haraken wrote: > > > > I'm not sure if this should always be true. If the empty handle means that > there > > is no property name, then it should be true. However, if the empty handle > means > > that some exception was thrown, then it should be false. > > > > In other words, this V8 API breaks the invariant that an empty handle is > > returned if and only if an exception was thrown. I guess we need to redesign > the > > API? > > > > Yeah, I thought it was inconsistent, but kept the current behavior. Let's try to > negate this. I'm not sure if this is right. If there is no property name in the object, I think you must return true here. I guess we cannot fix this issue without fixing the V8 API.
https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... File Source/bindings/core/v8/Dictionary.cpp (right): https://codereview.chromium.org/964553003/diff/120001/Source/bindings/core/v8... Source/bindings/core/v8/Dictionary.cpp:218: return true; On 2015/03/27 03:59:24, haraken wrote: > On 2015/03/27 03:55:24, bashi1 wrote: > > On 2015/03/27 03:33:33, haraken wrote: > > > > > > I'm not sure if this should always be true. If the empty handle means that > > there > > > is no property name, then it should be true. However, if the empty handle > > means > > > that some exception was thrown, then it should be false. > > > > > > In other words, this V8 API breaks the invariant that an empty handle is > > > returned if and only if an exception was thrown. I guess we need to redesign > > the > > > API? > > > > > > > Yeah, I thought it was inconsistent, but kept the current behavior. Let's try > to > > negate this. > > I'm not sure if this is right. If there is no property name in the object, I > think you must return true here. > > I guess we cannot fix this issue without fixing the V8 API. GetPropertyNames() calls NewJSArray() regardless there is no property name. I read this as "GetPropertyNames() returns an empty array (not an empty handle) when there is no property name". So I think this is fine. @dcanery, could you confirm?
lgtm from my perspective. https://codereview.chromium.org/964553003/diff/140001/Source/bindings/core/v8... File Source/bindings/core/v8/Dictionary.h (right): https://codereview.chromium.org/964553003/diff/140001/Source/bindings/core/v8... Source/bindings/core/v8/Dictionary.h:122: bool toObject(v8::Local<v8::Object>*) const; nit: Since you're using v8::Local<>& elsewhere, you might want to use non-const reference here, too.
https://codereview.chromium.org/964553003/diff/140001/Source/bindings/core/v8... File Source/bindings/core/v8/Dictionary.h (right): https://codereview.chromium.org/964553003/diff/140001/Source/bindings/core/v8... Source/bindings/core/v8/Dictionary.h:122: bool toObject(v8::Local<v8::Object>*) const; On 2015/03/27 06:48:33, Yuki wrote: > nit: Since you're using v8::Local<>& elsewhere, you might want to use non-const > reference here, too. Done.
LGTM (if the Local<Array>() concern is resolved).
> GetPropertyNames() calls NewJSArray() regardless there is no property name. I > read this as "GetPropertyNames() returns an empty array (not an empty handle) > when there is no property name". So I think this is fine. > > @dcanery, could you confirm? that's correct
> GetPropertyNames() calls NewJSArray() regardless there is no property name. I > read this as "GetPropertyNames() returns an empty array (not an empty handle) > when there is no property name". So I think this is fine. > > @dcanery, could you confirm? that's correct
On 2015/03/27 08:14:33, dcarney wrote: > > GetPropertyNames() calls NewJSArray() regardless there is no property name. I > > read this as "GetPropertyNames() returns an empty array (not an empty handle) > > when there is no property name". So I think this is fine. > > > > @dcanery, could you confirm? > > that's correct Thanks!
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/964553003/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/964553003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=192679 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
