Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
140653013:
Have DOMImplementation::document() return reference. (Closed)
|
Created:
6 years, 10 months ago by deepak.sa Modified:
6 years, 10 months ago CC:
blink-reviews, dglazkov+blink, apavlov+blink_chromium.org, ed+blinkwatch_opera.com, darktears Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionHave DOMImplementation::document() return reference.
Have DOMImplementaion::document() return reference
as m_document is a stored as a reference.
As this function is used in V8, new IDL attribute
[SetWrapperReferenceFromReference] has been added, which supports document() returning reference.
New binding test has been added for this attribute.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167755
Patch Set 1 #Patch Set 2 : Added SetWrapperReferenceFromReference IDL attribute #
Total comments: 2
Patch Set 3 : Binding test added for [SetWrapperReferenceFrom] extended attribute. #
Total comments: 2
Patch Set 4 : Removed StyleSheetList::document changes #
Messages
Total messages: 28 (0 generated)
Please have a loook.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/140653013/1
Retried try job too often on mac_layout for step(s) webkit_lint http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
CQ bit was unchecked on CL. Ignoring.
On 2014/01/30 14:32:13, Chris Dumez wrote: > lgtm While compiling code on my system i got the error in file V8StyleSheetList.cpp (method: visitDOMWrapper), which is generated by code_generator_v8.pm script. I tried to modify the script but it inturn effects other V8* classes' methods which return pointer intead of reference like impl->element(). How should i change the script to incorporate this change. Thanx.
not lgtm then. +haraken to get his opinion. My proposal would be to introduce a new SetWrapperReferenceFromReference IDL extended attribute which would be the same as SetWrapperReferenceFrom but would expect the method to return a reference instead of a pointer. The generated code for visitDOMWrapper() would then be simpler as no null check would be needed. I am pretty sure SetWrapperReferenceFromReference would be useful for other types like NodeList where I seem to remember ownerNode cannot be null. This would help us do further refactoring and use more references in the code. Then again, please wait for haraken's feedback as he likely has an opinion and this may be impacted by the oilpan work.
> +haraken to get his opinion. > > My proposal would be to introduce a new SetWrapperReferenceFromReference IDL > extended attribute which would be the same as SetWrapperReferenceFrom but would > expect the method to return a reference instead of a pointer. The generated code > for visitDOMWrapper() would then be simpler as no null check would be needed. > > I am pretty sure SetWrapperReferenceFromReference would be useful for other > types like NodeList where I seem to remember ownerNode cannot be null. > > This would help us do further refactoring and use more references in the code. > Then again, please wait for haraken's feedback as he likely has an opinion and > this may be impacted by the oilpan work. - [SetWrapperReferenceFromReference] IDL attribute sounds good to me if it will clean up the code base. - If we can completely replace [SetWrapperReferenceFrom] with [SetWrapperReferenceFromReference], it would be the best. We don't want to have confusing two IDL attributes. - Regarding oilpan, it's still unclear how to interact Blink GC and V8 GC. So we don't need to care about it at the moment.
On 2014/01/31 15:39:43, haraken wrote: > > +haraken to get his opinion. > > > > My proposal would be to introduce a new SetWrapperReferenceFromReference IDL > > extended attribute which would be the same as SetWrapperReferenceFrom but > would > > expect the method to return a reference instead of a pointer. The generated > code > > for visitDOMWrapper() would then be simpler as no null check would be needed. > > > > I am pretty sure SetWrapperReferenceFromReference would be useful for other > > types like NodeList where I seem to remember ownerNode cannot be null. > > > > This would help us do further refactoring and use more references in the code. > > Then again, please wait for haraken's feedback as he likely has an opinion and > > this may be impacted by the oilpan work. > > - [SetWrapperReferenceFromReference] IDL attribute sounds good to me if it will > clean up the code base. > > - If we can completely replace [SetWrapperReferenceFrom] with > [SetWrapperReferenceFromReference], it would be the best. We don't want to have > confusing two IDL attributes. > > - Regarding oilpan, it's still unclear how to interact Blink GC and V8 GC. So we > don't need to care about it at the moment. @haraken @Chris Dumez Please have a look at the new patchset. Thanx.
https://codereview.chromium.org/140653013/diff/30001/Source/bindings/IDLExten... File Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/140653013/diff/30001/Source/bindings/IDLExten... Source/bindings/IDLExtendedAttributes.txt:87: SetWrapperReferenceFromReference=document You'll need bindings tests coverage for this new feature. Also, I would allow document|element|owner|ownerNode as for SetWrapperReferenceFrom so that we can more easily migrate to references later on. https://codereview.chromium.org/140653013/diff/30001/Source/bindings/scripts/... File Source/bindings/scripts/code_generator_v8.pm (right): https://codereview.chromium.org/140653013/diff/30001/Source/bindings/scripts/... Source/bindings/scripts/code_generator_v8.pm:579: my $isReachableReferenceMethod = $interface->extendedAttributes->{"SetWrapperReferenceFromReference"}; I would try to merge this as much as possible with the SetWrapperReferenceFrom code to avoid duplication.
On 2014/02/04 14:31:39, Chris Dumez wrote: > https://codereview.chromium.org/140653013/diff/30001/Source/bindings/IDLExten... > File Source/bindings/IDLExtendedAttributes.txt (right): > > https://codereview.chromium.org/140653013/diff/30001/Source/bindings/IDLExten... > Source/bindings/IDLExtendedAttributes.txt:87: > SetWrapperReferenceFromReference=document > You'll need bindings tests coverage for this new feature. > Also, I would allow document|element|owner|ownerNode as for > SetWrapperReferenceFrom so that we can more easily migrate to references later > on. > > https://codereview.chromium.org/140653013/diff/30001/Source/bindings/scripts/... > File Source/bindings/scripts/code_generator_v8.pm (right): > > https://codereview.chromium.org/140653013/diff/30001/Source/bindings/scripts/... > Source/bindings/scripts/code_generator_v8.pm:579: my $isReachableReferenceMethod > = $interface->extendedAttributes->{"SetWrapperReferenceFromReference"}; > I would try to merge this as much as possible with the SetWrapperReferenceFrom > code to avoid duplication. Deepak, any update on this?
On 2014/02/13 17:40:06, Chris Dumez wrote: > On 2014/02/04 14:31:39, Chris Dumez wrote: > > > https://codereview.chromium.org/140653013/diff/30001/Source/bindings/IDLExten... > > File Source/bindings/IDLExtendedAttributes.txt (right): > > > > > https://codereview.chromium.org/140653013/diff/30001/Source/bindings/IDLExten... > > Source/bindings/IDLExtendedAttributes.txt:87: > > SetWrapperReferenceFromReference=document > > You'll need bindings tests coverage for this new feature. > > Also, I would allow document|element|owner|ownerNode as for > > SetWrapperReferenceFrom so that we can more easily migrate to references later > > on. > > > > > https://codereview.chromium.org/140653013/diff/30001/Source/bindings/scripts/... > > File Source/bindings/scripts/code_generator_v8.pm (right): > > > > > https://codereview.chromium.org/140653013/diff/30001/Source/bindings/scripts/... > > Source/bindings/scripts/code_generator_v8.pm:579: my > $isReachableReferenceMethod > > = $interface->extendedAttributes->{"SetWrapperReferenceFromReference"}; > > I would try to merge this as much as possible with the SetWrapperReferenceFrom > > code to avoid duplication. > > Deepak, any update on this? Working on binding tests. Some pointers on binding tests will be very helpful. Like how to test it? Thanx
On 2014/02/16 14:33:53, deepak.sa wrote: > On 2014/02/13 17:40:06, Chris Dumez wrote: > > On 2014/02/04 14:31:39, Chris Dumez wrote: > > > > > > https://codereview.chromium.org/140653013/diff/30001/Source/bindings/IDLExten... > > > File Source/bindings/IDLExtendedAttributes.txt (right): > > > > > > > > > https://codereview.chromium.org/140653013/diff/30001/Source/bindings/IDLExten... > > > Source/bindings/IDLExtendedAttributes.txt:87: > > > SetWrapperReferenceFromReference=document > > > You'll need bindings tests coverage for this new feature. > > > Also, I would allow document|element|owner|ownerNode as for > > > SetWrapperReferenceFrom so that we can more easily migrate to references > later > > > on. > > > > > > > > > https://codereview.chromium.org/140653013/diff/30001/Source/bindings/scripts/... > > > File Source/bindings/scripts/code_generator_v8.pm (right): > > > > > > > > > https://codereview.chromium.org/140653013/diff/30001/Source/bindings/scripts/... > > > Source/bindings/scripts/code_generator_v8.pm:579: my > > $isReachableReferenceMethod > > > = $interface->extendedAttributes->{"SetWrapperReferenceFromReference"}; > > > I would try to merge this as much as possible with the > SetWrapperReferenceFrom > > > code to avoid duplication. > > > > Deepak, any update on this? > > Working on binding tests. Some pointers on binding tests will be very helpful. > Like how to test it? > Thanx Sure, the bindings test IDLs are located at: Source/bindings/tests/ The generated baselines are located at: Source/bindings/tests/results/ You can run the tests using: Tools/Scripts/run-bindings-tests You can update the generated baselines using: Tools/Scripts/run-bindings-tests --reset-results
Please have a look. Thanx.
https://codereview.chromium.org/140653013/diff/150001/Source/bindings/tests/i... File Source/bindings/tests/idls/TestInterfacePython3.idl (right): https://codereview.chromium.org/140653013/diff/150001/Source/bindings/tests/i... Source/bindings/tests/idls/TestInterfacePython3.idl:36: SetWrapperReferenceFromReference=document, // Conflicts with [SetWrapperReferenceFrom] Please create a new test IDL file instead of editing an existing one. We don't want to loose coverage for anything. Also, I don't think we should mess with the ones with "Python" in the name. Just add a new IDL such as "TestSetWrapperReferenceFromReference.idl". Also, don't forget to use this copyright header for the new idl: https://sites.google.com/a/chromium.org/dev/blink/coding-style#TOC-License
Note regarding IDL test case. https://codereview.chromium.org/140653013/diff/150001/Source/bindings/tests/i... File Source/bindings/tests/idls/TestInterfacePython3.idl (right): https://codereview.chromium.org/140653013/diff/150001/Source/bindings/tests/i... Source/bindings/tests/idls/TestInterfacePython3.idl:36: SetWrapperReferenceFromReference=document, // Conflicts with [SetWrapperReferenceFrom] To follow-up to what Chris said: On 2014/02/21 00:41:39, Chris Dumez wrote: > Please create a new test IDL file instead of editing an existing one. We don't > want to loose coverage for anything. Please do *not* delete existing tests: your change removes the test for [Custom=VisitDOMWrapper] Not LGTM. > Also, I don't think we should mess with the ones with "Python" > in the name. That's a fine guideline, but basically "Python" means "new, better organized test cases"; the Python IDL compiler now tests and passes everything except TestSVG.idl. (Just need to clean up the test files, as there's a lot of small ones and duplication.) > Just add a new IDL such as > "TestSetWrapperReferenceFromReference.idl". For consistency and simplicity, you can call it TestInterface2.idl (this means we can reuse it for other extended attributes) Given that this is "yet another wrapper extended attribute on interfaces", could you actually put this in TestInterfacePython4.idl with the comment: // Conflicts with [SetWrapperReferenceTo], [SetWrapperReferenceFrom], and [Custom=VisitDOMWrapper] Thanks!
On 2014/02/21 01:13:48, Nils Barth wrote: > Note regarding IDL test case. > > https://codereview.chromium.org/140653013/diff/150001/Source/bindings/tests/i... > File Source/bindings/tests/idls/TestInterfacePython3.idl (right): > > https://codereview.chromium.org/140653013/diff/150001/Source/bindings/tests/i... > Source/bindings/tests/idls/TestInterfacePython3.idl:36: > SetWrapperReferenceFromReference=document, // Conflicts with > [SetWrapperReferenceFrom] > To follow-up to what Chris said: > > On 2014/02/21 00:41:39, Chris Dumez wrote: > > Please create a new test IDL file instead of editing an existing one. We don't > > want to loose coverage for anything. > > Please do *not* delete existing tests: > your change removes the test for [Custom=VisitDOMWrapper] > Not LGTM. > > > Also, I don't think we should mess with the ones with "Python" > > in the name. > > That's a fine guideline, but basically "Python" means > "new, better organized test cases"; > the Python IDL compiler now tests and passes everything except TestSVG.idl. > (Just need to clean up the test files, as there's a lot of > small ones and duplication.) > > > Just add a new IDL such as > > "TestSetWrapperReferenceFromReference.idl". > > For consistency and simplicity, you can call it TestInterface2.idl > (this means we can reuse it for other extended attributes) > > Given that this is "yet another wrapper extended attribute on interfaces", could > you actually put this in > TestInterfacePython4.idl > with the comment: > // Conflicts with [SetWrapperReferenceTo], [SetWrapperReferenceFrom], and > [Custom=VisitDOMWrapper] > > Thanks! Thanks for your suggestion. But what about this new patch that have been uploaded recently? https://codereview.chromium.org/170403002/ Thanks.
On 2014/02/21 12:46:16, deepak.sa wrote: > But what about this new patch that have been uploaded recently? > https://codereview.chromium.org/170403002/ Note: CL is: StyleSheetList::document() should return null after detaching the StyleSheetList from document. https://codereview.chromium.org/170403002/ (Reply was not to me; I'm only commenting on IDL files.)
On 2014/02/21 12:46:16, deepak.sa wrote: > On 2014/02/21 01:13:48, Nils Barth wrote: > > Note regarding IDL test case. > > > > > https://codereview.chromium.org/140653013/diff/150001/Source/bindings/tests/i... > > File Source/bindings/tests/idls/TestInterfacePython3.idl (right): > > > > > https://codereview.chromium.org/140653013/diff/150001/Source/bindings/tests/i... > > Source/bindings/tests/idls/TestInterfacePython3.idl:36: > > SetWrapperReferenceFromReference=document, // Conflicts with > > [SetWrapperReferenceFrom] > > To follow-up to what Chris said: > > > > On 2014/02/21 00:41:39, Chris Dumez wrote: > > > Please create a new test IDL file instead of editing an existing one. We > don't > > > want to loose coverage for anything. > > > > Please do *not* delete existing tests: > > your change removes the test for [Custom=VisitDOMWrapper] > > Not LGTM. > > > > > Also, I don't think we should mess with the ones with "Python" > > > in the name. > > > > That's a fine guideline, but basically "Python" means > > "new, better organized test cases"; > > the Python IDL compiler now tests and passes everything except TestSVG.idl. > > (Just need to clean up the test files, as there's a lot of > > small ones and duplication.) > > > > > Just add a new IDL such as > > > "TestSetWrapperReferenceFromReference.idl". > > > > For consistency and simplicity, you can call it TestInterface2.idl > > (this means we can reuse it for other extended attributes) > > > > Given that this is "yet another wrapper extended attribute on interfaces", > could > > you actually put this in > > TestInterfacePython4.idl > > with the comment: > > // Conflicts with [SetWrapperReferenceTo], [SetWrapperReferenceFrom], and > > [Custom=VisitDOMWrapper] > > > > Thanks! > > Thanks for your suggestion. > But what about this new patch that have been uploaded recently? > https://codereview.chromium.org/170403002/ > > Thanks. Ok, then let's keep the change to StyleSheetList out of this CL as it is no longer valid after https://codereview.chromium.org/170403002/.
On 2014/02/21 16:29:52, Chris Dumez wrote: > On 2014/02/21 12:46:16, deepak.sa wrote: > > On 2014/02/21 01:13:48, Nils Barth wrote: > > > Note regarding IDL test case. > > > > > > > > > https://codereview.chromium.org/140653013/diff/150001/Source/bindings/tests/i... > > > File Source/bindings/tests/idls/TestInterfacePython3.idl (right): > > > > > > > > > https://codereview.chromium.org/140653013/diff/150001/Source/bindings/tests/i... > > > Source/bindings/tests/idls/TestInterfacePython3.idl:36: > > > SetWrapperReferenceFromReference=document, // Conflicts with > > > [SetWrapperReferenceFrom] > > > To follow-up to what Chris said: > > > > > > On 2014/02/21 00:41:39, Chris Dumez wrote: > > > > Please create a new test IDL file instead of editing an existing one. We > > don't > > > > want to loose coverage for anything. > > > > > > Please do *not* delete existing tests: > > > your change removes the test for [Custom=VisitDOMWrapper] > > > Not LGTM. > > > > > > > Also, I don't think we should mess with the ones with "Python" > > > > in the name. > > > > > > That's a fine guideline, but basically "Python" means > > > "new, better organized test cases"; > > > the Python IDL compiler now tests and passes everything except TestSVG.idl. > > > (Just need to clean up the test files, as there's a lot of > > > small ones and duplication.) > > > > > > > Just add a new IDL such as > > > > "TestSetWrapperReferenceFromReference.idl". > > > > > > For consistency and simplicity, you can call it TestInterface2.idl > > > (this means we can reuse it for other extended attributes) > > > > > > Given that this is "yet another wrapper extended attribute on interfaces", > > could > > > you actually put this in > > > TestInterfacePython4.idl > > > with the comment: > > > // Conflicts with [SetWrapperReferenceTo], [SetWrapperReferenceFrom], and > > > [Custom=VisitDOMWrapper] > > > > > > Thanks! > > > > Thanks for your suggestion. > > But what about this new patch that have been uploaded recently? > > https://codereview.chromium.org/170403002/ > > > > Thanks. > > Ok, then let's keep the change to StyleSheetList out of this CL as it is no > longer valid after https://codereview.chromium.org/170403002/. Please have a look. I have removed theStyleSheetList changes. Also added new TestInterfacePytho4.idl file.
LGTM but please let Nils take a look as well before landing as he previously had some objections.
On 2014/02/24 14:12:58, Chris Dumez wrote: > LGTM but please let Nils take a look as well before landing as he previously had > some objections. LGTM; thanks for fixing the test case!
The CQ bit was checked by deepak.sa@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/140653013/280001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/140653013/280001
Message was sent while issue was closed.
Change committed as 167755
Message was sent while issue was closed.
Deepak, could you please document this new [SetWrapperReferenceFromReference] extended attribute at the documentation page? I've set up a new section for it, if you could fill it in: http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-SetWra... Thanks!
Message was sent while issue was closed.
On 2014/02/26 06:21:37, Nils Barth wrote: > Deepak, could you please document this new [SetWrapperReferenceFromReference] > extended attribute at the documentation page? > > I've set up a new section for it, if you could fill it in: > http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-SetWra... > > Thanks! I am planning to get rid of this attribute today anyway and make the bindings code accept both a reference or a pointer. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
