Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2441943002:
Retrieve prototype during custom element construction. (Closed)
|
Created:
4 years, 2 months ago by dominicc (has gone to gerrit) Modified:
4 years, 1 month ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-w3ctests_chromium.org, chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRetrieve prototype during custom element construction.
BUG=649158, 658066
Committed: https://crrev.com/8e2ff264b2d6eaddcf5546b83bb382dd91ae833e
Cr-Commit-Position: refs/heads/master@{#427333}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Feedback. #
Total comments: 3
Patch Set 3 : Update after https://codereview.chromium.org/2443543002 #Depends on Patchset: Messages
Total messages: 29 (13 generated)
The CQ bit was checked by dominicc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dominicc@chromium.org changed reviewers: + bashi@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp (right): https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:28: ScriptState* scriptState = ScriptState::current(isolate); Add: if (!scriptState->contextIsValid()) return; https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:46: } else { And remove this check. The liveness of the context should be checked at the beginning of this method. https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:47: V8ThrowException::throwTypeError(isolate, "The context has been destroyed"); Do you really want to throw TypeError when the context is detached? https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:71: v8::TryCatch tryCatch(isolate); Why do you need the TryCatch block? https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:83: if (!prototype->IsObject()) { Is this behavior really speced? It looks a bit weird that we fallback to the default prototype only when Get("prototype") didn't return an object... https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:84: if (V8PerContextData* realm = V8PerContextData::from( realm => perContextData BTW, why can't you use scriptState->perContextData() here? https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:89: "The context has been destroyed"); Ditto. I'm not sure if you want to throw TypeError.
The CQ bit was checked by dominicc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review. All done, PTAL. https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp (right): https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:71: v8::TryCatch tryCatch(isolate); On 2016/10/21 at 04:13:06, haraken wrote: > Why do you need the TryCatch block? I guess you mean we should just use the return value of v8Call? https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:83: if (!prototype->IsObject()) { On 2016/10/21 at 04:13:06, haraken wrote: > Is this behavior really speced? It looks a bit weird that we fallback to the default prototype only when Get("prototype") didn't return an object... Yes. Open the link at the top of the function and read step 7 if you are still unsure. https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:84: if (V8PerContextData* realm = V8PerContextData::from( On 2016/10/21 at 04:13:06, haraken wrote: > realm => perContextData > > BTW, why can't you use scriptState->perContextData() here? Because the scriptState->perContextData() and the context data being used here can be different, and the spec says to use this one.
https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp (right): https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:71: v8::TryCatch tryCatch(isolate); On 2016/10/21 05:18:14, dominicc wrote: > On 2016/10/21 at 04:13:06, haraken wrote: > > Why do you need the TryCatch block? > > I guess you mean we should just use the return value of v8Call? IIUC, when Get() fails, an exception is already thrown. Probably what haraken-san is suggesting is that: if (!v8Call(...)) return; ?
On 2016/10/21 at 05:25:13, bashi wrote: > https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... > File third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp (right): > > https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:71: v8::TryCatch tryCatch(isolate); > On 2016/10/21 05:18:14, dominicc wrote: > > On 2016/10/21 at 04:13:06, haraken wrote: > > > Why do you need the TryCatch block? > > > > I guess you mean we should just use the return value of v8Call? > > IIUC, when Get() fails, an exception is already thrown. Probably what haraken-san is suggesting is that: > > if (!v8Call(...)) > return; > > ? Yes as you can see I think I wrote that in patch set 2?
https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp (right): https://codereview.chromium.org/2441943002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:83: if (!prototype->IsObject()) { On 2016/10/21 05:18:14, dominicc wrote: > On 2016/10/21 at 04:13:06, haraken wrote: > > Is this behavior really speced? It looks a bit weird that we fallback to the > default prototype only when Get("prototype") didn't return an object... > > Yes. Open the link at the top of the function and read step 7 if you are still > unsure. Yes. I haven't checked PS#2. Sorry for the noise.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp (right): https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:31: V8ThrowException::throwError(isolate, "The context has been destroyed"); Just to confirm: You really want to throw an error when the context is detached, right? c.f., We have a lot of binding code that don't throw error in this case. https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:45: &V8HTMLElement::wrapperTypeInfo); I'm just curious but is it enough to check only V8HTMLElement::wrapperTypeInfo? e.g., don't we need to check other V8HTMLXXXElement::wrapperTypeInfo? https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:85: V8ThrowException::throwError(isolate, "The context has been destroyed"); Ditto.
On 2016/10/21 at 08:36:06, haraken wrote: > LGTM > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:45: &V8HTMLElement::wrapperTypeInfo); > > I'm just curious but is it enough to check only V8HTMLElement::wrapperTypeInfo? e.g., don't we need to check other V8HTMLXXXElement::wrapperTypeInfo? I agree, I think we have to use the same type of html element as what is being created (if you want it to work for customized built-in elements).
On 2016/10/21 at 09:34:50, yurak wrote: > On 2016/10/21 at 08:36:06, haraken wrote: > > LGTM > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:45: &V8HTMLElement::wrapperTypeInfo); > > > > I'm just curious but is it enough to check only V8HTMLElement::wrapperTypeInfo? e.g., don't we need to check other V8HTMLXXXElement::wrapperTypeInfo? > > I agree, I think we have to use the same type of html element as what is being created (if you want it to work for customized built-in elements). Yes, of course--that's what the patch yurak is working on adds, it passes the specific wrapperTypeInfo. Until that patch lands there's only HTMLElement!
On 2016/10/21 at 08:36:06, haraken wrote: > LGTM > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp (right): > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:31: V8ThrowException::throwError(isolate, "The context has been destroyed"); > > Just to confirm: You really want to throw an error when the context is detached, right? > > c.f., We have a lot of binding code that don't throw error in this case. > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:45: &V8HTMLElement::wrapperTypeInfo); > > I'm just curious but is it enough to check only V8HTMLElement::wrapperTypeInfo? e.g., don't we need to check other V8HTMLXXXElement::wrapperTypeInfo? > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:85: V8ThrowException::throwError(isolate, "The context has been destroyed"); > > Ditto. What is the best thing to do here? I just want it to not crash :) I think per the spec we should keep running, but Blink can't do that because we can't access PerContextData at that point. Is there any update on when Blink can keep running in detached contexts? (Won't that create the opportunity for a lot of leaks, BTW?)
On 2016/10/24 01:19:40, dominicc wrote: > On 2016/10/21 at 08:36:06, haraken wrote: > > LGTM > > > > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp (right): > > > > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:31: > V8ThrowException::throwError(isolate, "The context has been destroyed"); > > > > Just to confirm: You really want to throw an error when the context is > detached, right? > > > > c.f., We have a lot of binding code that don't throw error in this case. > > > > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:45: > &V8HTMLElement::wrapperTypeInfo); > > > > I'm just curious but is it enough to check only > V8HTMLElement::wrapperTypeInfo? e.g., don't we need to check other > V8HTMLXXXElement::wrapperTypeInfo? > > > > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:85: > V8ThrowException::throwError(isolate, "The context has been destroyed"); > > > > Ditto. > > What is the best thing to do here? I just want it to not crash :) > > I think per the spec we should keep running, but Blink can't do that because we > can't access PerContextData at that point. Is there any update on when Blink can > keep running in detached contexts? (Won't that create the opportunity for a lot > of leaks, BTW?) We normally just return without throwing any error.
On 2016/10/24 at 05:22:45, haraken wrote: > On 2016/10/24 01:19:40, dominicc wrote: > > On 2016/10/21 at 08:36:06, haraken wrote: > > > LGTM > > > > > > > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp (right): > > > > > > > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:31: > > V8ThrowException::throwError(isolate, "The context has been destroyed"); > > > > > > Just to confirm: You really want to throw an error when the context is > > detached, right? > > > > > > c.f., We have a lot of binding code that don't throw error in this case. > > > > > > > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:45: > > &V8HTMLElement::wrapperTypeInfo); > > > > > > I'm just curious but is it enough to check only > > V8HTMLElement::wrapperTypeInfo? e.g., don't we need to check other > > V8HTMLXXXElement::wrapperTypeInfo? > > > > > > > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:85: > > V8ThrowException::throwError(isolate, "The context has been destroyed"); > > > > > > Ditto. > > > > What is the best thing to do here? I just want it to not crash :) > > > > I think per the spec we should keep running, but Blink can't do that because we > > can't access PerContextData at that point. Is there any update on when Blink can > > keep running in detached contexts? (Won't that create the opportunity for a lot > > of leaks, BTW?) > > We normally just return without throwing any error. I think we need an exceptional return here. Because this is a constructor, returning null or undefined means V8 will return its allocated object wrapping a null pointer. We could return another object, but then which object should we return?
The CQ bit was checked by dominicc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2441943002/#ps40001 (title: "Update after https://codereview.chromium.org/2443543002")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/25 08:25:56, dominicc wrote: > On 2016/10/24 at 05:22:45, haraken wrote: > > On 2016/10/24 01:19:40, dominicc wrote: > > > On 2016/10/21 at 08:36:06, haraken wrote: > > > > LGTM > > > > > > > > > > > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp > (right): > > > > > > > > > > > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:31: > > > V8ThrowException::throwError(isolate, "The context has been destroyed"); > > > > > > > > Just to confirm: You really want to throw an error when the context is > > > detached, right? > > > > > > > > c.f., We have a lot of binding code that don't throw error in this case. > > > > > > > > > > > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:45: > > > &V8HTMLElement::wrapperTypeInfo); > > > > > > > > I'm just curious but is it enough to check only > > > V8HTMLElement::wrapperTypeInfo? e.g., don't we need to check other > > > V8HTMLXXXElement::wrapperTypeInfo? > > > > > > > > > > > > https://codereview.chromium.org/2441943002/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/bindings/core/v8/V8HTMLConstructor.cpp:85: > > > V8ThrowException::throwError(isolate, "The context has been destroyed"); > > > > > > > > Ditto. > > > > > > What is the best thing to do here? I just want it to not crash :) > > > > > > I think per the spec we should keep running, but Blink can't do that because > we > > > can't access PerContextData at that point. Is there any update on when Blink > can > > > keep running in detached contexts? (Won't that create the opportunity for a > lot > > > of leaks, BTW?) > > > > We normally just return without throwing any error. > > I think we need an exceptional return here. Because this is a constructor, > returning null or undefined means V8 will return its allocated object wrapping a > null pointer. We could return another object, but then which object should we > return? Makes sense.
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Retrieve prototype during custom element construction. BUG=649158,658066 ========== to ========== Retrieve prototype during custom element construction. BUG=649158,658066 Committed: https://crrev.com/8e2ff264b2d6eaddcf5546b83bb382dd91ae833e Cr-Commit-Position: refs/heads/master@{#427333} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8e2ff264b2d6eaddcf5546b83bb382dd91ae833e Cr-Commit-Position: refs/heads/master@{#427333} |
