| Summary: | REGRESSION:Bindings sequence<T> in Console.idl, Internals.idl and ScriptProfileNode.idl should be T[] | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Vineet Chaudhary (vineetc) <code.vineet> | ||||||||||||||||||||
| Component: | New Bugs | Assignee: | Vineet Chaudhary (vineetc) <code.vineet> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, ap, arv, cdumez, dglazkov, eric.carlson, feature-media-reviews, haraken, japhet, jochen, mnaganov, morrita, ojan, pfeldman, sam, webkit.review.bot, xuewen.ok | ||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||
| Bug Blocks: | 87503 | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Vineet Chaudhary (vineetc)
2012-04-25 07:48:21 PDT
Created attachment 138826 [details] wip_patch In this wip_patch we are just changing representation of attributes in the IDL. From the discussion of the bug Bug 80696 what I understood is that T[] should return shared Array and sequence<T> returns a new Array every time, I think now we should check how we achieve this. Comment on attachment 138826 [details] wip_patch View in context: https://bugs.webkit.org/attachment.cgi?id=138826&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2967 > + my $seqType = $codeGenerator->GetSequenceType($type); Nit: $seqType => $sequenceType, for consistency in the following $sequenceType. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3585 > + my $seqType = $codeGenerator->GetSequenceType($type); Nit: $seqType => $sequenceType, for consistency in the following $sequenceType. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3826 > + return "v8Array($value, $getIsolate)"; Thanks for passing Isolate:) > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1185 > - impl->setUnsignedLongSequenceAttr(toNativeArray<unsigned long>(exec, value)); > + impl->setUnsignedLongSequenceAttr(jsUnsignedLongArrayToVector(exec, value)); Is this expected? I mean, why is the generated code changed for unsignedLong[] only? Comment on attachment 138826 [details] wip_patch View in context: https://bugs.webkit.org/attachment.cgi?id=138826&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2967 >> + my $seqType = $codeGenerator->GetSequenceType($type); > > Nit: $seqType => $sequenceType, for consistency in the following $sequenceType. Oops. Will do. >> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1185 >> + impl->setUnsignedLongSequenceAttr(jsUnsignedLongArrayToVector(exec, value)); > > Is this expected? I mean, why is the generated code changed for unsignedLong[] only? The reason for different code is http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm?rev=115112#L2953 This was introduced in http://trac.webkit.org/changeset/107926. I am not sure about these changes as I could not find usage of this. But If these are resonable changes then may be we can generalized these changes for other numeric types using traits too. > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:242 > + return v8Array(imp->intSequenceAttr(), info.GetIsolate()); rebaseline :) Comment on attachment 138826 [details] wip_patch View in context: https://bugs.webkit.org/attachment.cgi?id=138826&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2967 >> + my $seqType = $codeGenerator->GetSequenceType($type); > > Nit: $seqType => $sequenceType, for consistency in the following $sequenceType. Oops. Will do. >> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1185 >> + impl->setUnsignedLongSequenceAttr(jsUnsignedLongArrayToVector(exec, value)); > > Is this expected? I mean, why is the generated code changed for unsignedLong[] only? The reason for different code is http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm?rev=115112#L2953 This was introduced in http://trac.webkit.org/changeset/107926. I am not sure about these changes as I could not find usage of this. But If these are resonable changes then may be we can generalized these changes for other numeric types using traits too. > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:242 > + return v8Array(imp->intSequenceAttr(), info.GetIsolate()); rebaseline :) (In reply to comment #4) > >> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1185 > >> + impl->setUnsignedLongSequenceAttr(jsUnsignedLongArrayToVector(exec, value)); > > > > Is this expected? I mean, why is the generated code changed for unsignedLong[] only? > > The reason for different code is http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm?rev=115112#L2953 > This was introduced in http://trac.webkit.org/changeset/107926. I am not sure about these changes as I could not find usage of this. > But If these are resonable changes then may be we can generalized these changes for other numeric types using traits too. Makes sense. Maybe we should generalize the changes to other numeric types using traits, but anyway let's do it in another patch. Thanks for the clarification. Created attachment 138941 [details] updatedPatch Updated patch. I am not very good at perl-script so just wonder if below regExp is correct For parsing T[]. > Source/WebCore/bindings/scripts/CodeGenerator.pm > return $1 if $type =~ /^([\w\d_\s]+)\[\]/; Comment on attachment 138941 [details] updatedPatch View in context: https://bugs.webkit.org/attachment.cgi?id=138941&action=review Looks OK in terms of supporting T[]. There are a couple of "FIXME: support T[] ..." comments and "if (xxxx[])" conditions in CodeGenerator*.pm. We might want to clean them up in a follow-up patch. > Source/WebCore/bindings/scripts/CodeGenerator.pm:466 > + return $1 if $type =~ /^([\w\d_\s]+)\[\]/; Looks good. (In reply to comment #7) > (From update of attachment 138941 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138941&action=review > > Looks OK in terms of supporting T[]. There are a couple of "FIXME: support T[] ..." comments and "if (xxxx[])" conditions in CodeGenerator*.pm. We might want to clean them up in a follow-up patch. > > > Source/WebCore/bindings/scripts/CodeGenerator.pm:466 > > + return $1 if $type =~ /^([\w\d_\s]+)\[\]/; > > Looks good. Thanks haraken for review. Should we consider this patch for cq? (In reply to comment #8) > Thanks haraken for review. Should we consider this patch for cq? I think it's OK to commit it. And then let's fix the original bug https://bugs.webkit.org/show_bug.cgi?id=84540 Any concern to commit it? (In reply to comment #9) > (In reply to comment #8) > > Thanks haraken for review. Should we consider this patch for cq? > > I think it's OK to commit it. And then let's fix the original bug https://bugs.webkit.org/show_bug.cgi?id=84540 Any concern to commit it? As we have just done changes for representation of attributes in the IDL here. I don't see any concerns for landing this. But I am bit confused for the statement "T[] should return shared Array and sequence<T> returns a new Array every time" are we archiving this? Or we need some changes in JSDOM/V9Bindings.Can you please clarify on this.. (In reply to comment #10) > As we have just done changes for representation of attributes in the IDL here. I don't see any concerns for landing this. > > But I am bit confused for the statement "T[] should return shared Array and sequence<T> returns a new Array every time" are we archiving this? Or we need some changes in JSDOM/V9Bindings.Can you please clarify on this.. arv: In my understanding, 'shared' or 'non-shared' should be handled (not in bindings but) in WebCore. Right? Or does 'shared' mean that JSC/V8 needs to return the same wrapper object? vineetc: In the generated code with your patch, what is the result for the following code? It should be true, per the spec of T[]. internals.userPreferredLanguages === internals.userPreferredLanguages And it is a good idea to add LayoutTests for the code: internals.userPreferredLanguages === internals.userPreferredLanguages console.profiles === console.profiles scriptProfileNode.children === scriptProfileNode.children Comment on attachment 138941 [details]
updatedPatch
Marking r- due to the recent comments.
Created attachment 138969 [details] test_case_for_internals (In reply to comment #11) > (In reply to comment #10) > > As we have just done changes for representation of attributes in the IDL here. I don't see any concerns for landing this. > arv: In my understanding, 'shared' or 'non-shared' should be handled (not in bindings but) in WebCore. Right? Or does 'shared' mean that JSC/V8 needs to return the same wrapper object? > > vineetc: In the generated code with your patch, what is the result for the following code? It should be true, per the spec of T[]. > > internals.userPreferredLanguages === internals.userPreferredLanguages We have test case http://trac.webkit.org/browser/trunk/LayoutTests/fast/harness/user-preferred-language.html?rev=105315 which test if arrays are mutable or not. I have modified this test to test "internals.userPreferredLanguages == internals.userPreferredLanguages" which is "false" with patch for both V8 and JS. I think this is bug original custom binding code because we have just removed custom bindings changing representation the IDLs. We should actually align this behaviour first but as you said not sure where to fix in WebCore or Bindings. BTW below is the output of the attached test case. CONSOLE MESSAGE: line 22: Original Array en-us CONSOLE MESSAGE: line 34: Is shared array false CONSOLE MESSAGE: line 40: New first-language CONSOLE MESSAGE: line 41: Old first-language CONSOLE MESSAGE: line 40: New en-us CONSOLE MESSAGE: line 41: Old en-us CONSOLE MESSAGE: line 40: New last-language CONSOLE MESSAGE: line 41: Old last-language This test verifies that internals.userPreferredLanguages returns a mutable Array of the user's preferred languages. internals.userPreferredLanguages returns a non-empty array: PASS internals.userPreferredLanguages is mutable, and returns the same value passed to it: PASS (In reply to comment #14) > We have test case http://trac.webkit.org/browser/trunk/LayoutTests/fast/harness/user-preferred-language.html?rev=105315 which test if arrays are mutable or not. > I have modified this test to test "internals.userPreferredLanguages == internals.userPreferredLanguages" which is "false" with patch for both V8 and JS. > I think this is bug original custom binding code because we have just removed custom bindings changing representation the IDLs. > We should actually align this behaviour first but as you said not sure where to fix in WebCore or Bindings. Sorry for the confusing previous comments. It seems to me that - To make "internals.userPreferredLanguages == internals.userPreferredLanguages" true, the bindings must return the same wrapper object. (This implies that WebCore must return the same C++ object.) I would guess that WebCore is already returning the same C++ object, but the bindings does not yet return the same wrapper object. - I am not sure if we should fix the current behavior, in terms of compatibility. Could you confirm the behavior in other browsers? Nit: Please use === instead of ==. arv, sam, ap: Any comments? (In reply to comment #15) > (In reply to comment #14) > > We have test case http://trac.webkit.org/browser/trunk/LayoutTests/fast/harness/user-preferred-language.html?rev=105315 which test if arrays are mutable or not. > > I have modified this test to test "internals.userPreferredLanguages == internals.userPreferredLanguages" which is "false" with patch for both V8 and JS. > > I think this is bug original custom binding code because we have just removed custom bindings changing representation the IDLs. > > We should actually align this behaviour first but as you said not sure where to fix in WebCore or Bindings. > > Sorry for the confusing previous comments. It seems to me that > > - To make "internals.userPreferredLanguages == internals.userPreferredLanguages" true, the bindings must return the same wrapper object. (This implies that WebCore must return the same C++ object.) I would guess that WebCore is already returning the same C++ object, but the bindings does not yet return the same wrapper object. > > - I am not sure if we should fix the current behavior, in terms of compatibility. Could you confirm the behavior in other browsers? All of these are WebKit proprietary extensions. I think the confusing thing is that T[] is supposed to be a live, shared array so that changes done on either side (JS or C++) may be reflected. Think of it as a live NodeList or DOMTokenList but instead of having a unique interface for every TList we have T[]. I'm not really sure we care about liveness in these 3 cases. Implementing the liveness seems like a lot of work for very little gain. I see 3 possible "solutions": 1. Change the APIs to use methods that return sequence<T>. Since these are all proprietary APIs the risk is very low. 2. Make the objects alien and disallow changes from JS but allow changes from C++ 3. Live with the bug that console.profiles !== console.profiles (In reply to comment #16) > 1. Change the APIs to use methods that return sequence<T>. Since these are all proprietary APIs the risk is very low. > 2. Make the objects alien and disallow changes from JS but allow changes from C++ > 3. Live with the bug that console.profiles !== console.profiles For internals.userPreferredLanguages and scriptProfileNode.children, the only callers are in WebCore. So we should just change the APIs to methods that return sequence<T>. For console.profiles, I'm OK with living with console.profiles !== console.profiles. I'd like a comment in the IDL though explaining that other attributes should not use sequence<T> in order to ward off other people cargo-culting console.profiles (i.e. use a method, not an attribute if you want a sequence<T>) Created attachment 139978 [details] Patch sorry for late reply. (In reply to comment #17) > (In reply to comment #16) > > 1. Change the APIs to use methods that return sequence<T>. Since these are all proprietary APIs the risk is very low. > > 2. Make the objects alien and disallow changes from JS but allow changes from C++ > > 3. Live with the bug that console.profiles !== console.profiles > > For internals.userPreferredLanguages and scriptProfileNode.children, the only callers are in WebCore. So we should just change the APIs to methods that return sequence<T>. > > For console.profiles, I'm OK with living with console.profiles !== console.profiles. I'd like a comment in the IDL though explaining that other attributes should not use sequence<T> in order to ward off other people cargo-culting console.profiles (i.e. use a method, not an attribute if you want a sequence<T>) With this patch I have changed APIs to Methods. Changing APIs to methods will break cases like var languages = internals.userPreferredLanguages; should be now var languages = internals.userPreferredLanguages(); internals.userPreferredLanguages = languages; should be now internals.setUserPreferredLanguages(languages); Is this is acceptable? (In reply to comment #18) > Is this is acceptable? If the tests are update. Yes. Comment on attachment 139978 [details] Patch Attachment 139978 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12634036 New failing tests: inspector/profiler/cpu-profiler-profiling-without-inspector.html media/track/track-language-preference.html Created attachment 140162 [details]
Archive of layout-test-results from ec2-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 143242 [details]
updatedPatch
Updated patch with fixes to failing layout tests.
Comment on attachment 143242 [details] updatedPatch View in context: https://bugs.webkit.org/attachment.cgi?id=143242&action=review The implementation looks good to me. Waiting for arv's review. > Source/WebCore/ChangeLog:12 > + Tests:bindings/scripts/test/TestObj.idl You need to update run-bindings-tests results. > Source/WebCore/bindings/scripts/CodeGeneratorCPP.pm:226 > + die "Sequences must not be used as the type of an attribute, constant or exception field." if $codeGenerator->GetSequenceType($attribute->signature->type); It would be dirty to write this string here and there. Shall we make CodeGenerator::AssertSequenceType()? (In reply to comment #23) > (From update of attachment 143242 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143242&action=review > > The implementation looks good to me. Waiting for arv's review. > > > Source/WebCore/ChangeLog:12 > > + Tests:bindings/scripts/test/TestObj.idl > > You need to update run-bindings-tests results. Sorry I missed that.. I see inclusion of "ContextEnabledFeatures.h" headers in some of the file can you please help me in rebaseline those. > > Source/WebCore/bindings/scripts/CodeGeneratorCPP.pm:226 > > + die "Sequences must not be used as the type of an attribute, constant or exception field." if $codeGenerator->GetSequenceType($attribute->signature->type); > > It would be dirty to write this string here and there. Shall we make CodeGenerator::AssertSequenceType()? Sure I will do that. (In reply to comment #24) > Sorry I missed that.. > I see inclusion of "ContextEnabledFeatures.h" headers in some of the file can you please help me in rebaseline those. Does the inclusion cause any problem? morrita@ would know about that well. (In reply to comment #24) > I see inclusion of "ContextEnabledFeatures.h" headers in some of the file can you please help me in rebaseline those. Ah, I got it. It is already rebaselined at r117989. Created attachment 143296 [details]
UpdatedPatch
Comment on attachment 143296 [details]
UpdatedPatch
Doesn't this need any changes to Source/WebCore/inspector/?
(In reply to comment #28) > (From update of attachment 143296 [details]) > Doesn't this need any changes to Source/WebCore/inspector/? In JavaScripts whereever it is using node.children needs to be replaced with node.children() like LayoutTests/inspector/profiler/cpu-profiler-profiling-without-inspector.html I don't think we need changes in Source/WebCore/inspector/ *.cpp/*.h As we have changed JS interface only. (In reply to comment #29) > (In reply to comment #28) > > (From update of attachment 143296 [details] [details]) > > Doesn't this need any changes to Source/WebCore/inspector/? > > In JavaScripts whereever it is using node.children needs to be replaced with node.children() like LayoutTests/inspector/profiler/cpu-profiler-profiling-without-inspector.html > > I don't think we need changes in Source/WebCore/inspector/ *.cpp/*.h As we have changed JS interface only. There is a bunch of js files under Source/WebCore/inspector http://code.google.com/p/chromium/source/search?q=file%3Asource%2Fwebcore%2Finspector+file%3A%5C.js+%5C.children%5Cb&origq=file%3Asource%2Fwebcore%2Finspector+file%3A%5C.js+%5C.children%5Cb&btnG=Search+Trunk (In reply to comment #30) > (In reply to comment #29) > > (In reply to comment #28) > > > (From update of attachment 143296 [details] [details] [details]) > > > Doesn't this need any changes to Source/WebCore/inspector/? > > > > In JavaScripts whereever it is using node.children needs to be replaced with node.children() like LayoutTests/inspector/profiler/cpu-profiler-profiling-without-inspector.html > > > > I don't think we need changes in Source/WebCore/inspector/ *.cpp/*.h As we have changed JS interface only. > > There is a bunch of js files under Source/WebCore/inspector > > http://code.google.com/p/chromium/source/search?q=file%3Asource%2Fwebcore%2Finspector+file%3A%5C.js+%5C.children%5Cb&origq=file%3Asource%2Fwebcore%2Finspector+file%3A%5C.js+%5C.children%5Cb&btnG=Search+Trunk Ahh right I need to change that too. I just grep thro' found 430 occurrences of "children" in Source/WebCore/inspector. It seems big patch to upload and review any alternatives..? (In reply to comment #31) > (In reply to comment #30) > > (In reply to comment #29) > > > (In reply to comment #28) > > > > (From update of attachment 143296 [details] [details] [details] [details]) > > > > Doesn't this need any changes to Source/WebCore/inspector/? > > > > > > In JavaScripts whereever it is using node.children needs to be replaced with node.children() like LayoutTests/inspector/profiler/cpu-profiler-profiling-without-inspector.html > > > > > > I don't think we need changes in Source/WebCore/inspector/ *.cpp/*.h As we have changed JS interface only. > > > > There is a bunch of js files under Source/WebCore/inspector > > > > http://code.google.com/p/chromium/source/search?q=file%3Asource%2Fwebcore%2Finspector+file%3A%5C.js+%5C.children%5Cb&origq=file%3Asource%2Fwebcore%2Finspector+file%3A%5C.js+%5C.children%5Cb&btnG=Search+Trunk > > Ahh right I need to change that too. I just grep thro' found 430 occurrences of "children" in Source/WebCore/inspector. It seems big patch to upload and review any alternatives..? A lot of "children" entries actually come from WebInspector.DataGridNode and derived classes usages. They don't have any relation to the ScriptProfileNode interface. ScriptProfileNode instances are mostly referenced as 'profileNode' in Inspector's code, though I can't guarantee that this is in 100% of cases. A lot of the children occurrences may also come from HTMLElement.prototype.children getter. This seems like a hard thing to change without input from the inspector people. (In reply to comment #33) > A lot of the children occurrences may also come from HTMLElement.prototype.children getter. > > This seems like a hard thing to change without input from the inspector people. We are discussing this issue with Vineet by mail. Yes you are right. Actually I discussed this with Mikhail over mail. We won't need to change all occurrence of children as not all related to ScriptProfileNode. So we have spotted few places where we need changes like 1) http://trac.webkit.org/browser/trunk/Source/WebCore/inspector/front-end/TopDownProfileDataGridTree.js?rev=113971#L36 2) http://trac.webkit.org/browser/trunk/Source/WebCore/inspector/front-end/TopDownProfileDataGridTree.js?rev=113971#L81 3) http://trac.webkit.org/browser/trunk/Source/WebCore/inspector/front-end/BottomUpProfileDataGridTree.js?rev=113971#L202 4) http://trac.webkit.org/browser/trunk/Source/WebCore/inspector/front-end/CPUProfileView.js?rev=118503#L518 5) http://trac.webkit.org/browser/trunk/Source/WebCore/inspector/front-end/CPUProfileView.js?rev=118503#L522 But before submitting the patch as Mikhail suggested I would play with inspector to see if JavaScript errors in console wrt. to these changes. (In reply to comment #35) As per below discussion I tried changing above mentioned files/places, but after building code code profiling tool was not working. While, reverting only below JS changes it works. It indicates to me that instances of "children" under inspector code might be of HTMLElement.prototype.children. Even after that commenting "children" interface from ScriptProfileNode.idl inspector tool was working without any issues. So I think we need not any changes to Source/WebCore/inspector/*. (In reply to comment #36) > (In reply to comment #35) > > As per below discussion I tried changing above mentioned files/places, > but after building code code profiling tool was not working. > > While, reverting only below JS changes it works. > > It indicates to me that instances of "children" under inspector code might be of HTMLElement.prototype.children. > > Even after that commenting "children" interface from ScriptProfileNode.idl inspector tool was working without any issues. > > So I think we need not any changes to Source/WebCore/inspector/*. No, it's definitely not HTMLElement. If you look into ScriptProfile.idl, its 'head' property returns a ScriptProfileNode: ] ScriptProfile { readonly attribute DOMString title; readonly attribute unsigned long uid; readonly attribute ScriptProfileNode head; }; (In reply to comment #37) > (In reply to comment #36) > > (In reply to comment #35) > > > > As per below discussion I tried changing above mentioned files/places, > > but after building code code profiling tool was not working. > > > > While, reverting only below JS changes it works. > > > > It indicates to me that instances of "children" under inspector code might be of HTMLElement.prototype.children. > > > > Even after that commenting "children" interface from ScriptProfileNode.idl inspector tool was working without any issues. > > > > So I think we need not any changes to Source/WebCore/inspector/*. > > No, it's definitely not HTMLElement. If you look into ScriptProfile.idl, its 'head' property returns a ScriptProfileNode: > > ] ScriptProfile { > readonly attribute DOMString title; > readonly attribute unsigned long uid; > readonly attribute ScriptProfileNode head; > }; I Mikhail, I debugged the code, but whenever in scripts it calls profile.head it doesn't go to ScriptProfile::head(). Also I observed there is one more Document::head() which gets called when we use profiling. Also ScriptProfileNode::children() wasn't called single time while playing with profiling tool but calls HTMLElement::children(). (In reply to comment #38) > (In reply to comment #37) > > (In reply to comment #36) > > > (In reply to comment #35) > > > > > > As per below discussion I tried changing above mentioned files/places, > > > but after building code code profiling tool was not working. > > > > > > While, reverting only below JS changes it works. > > > > > > It indicates to me that instances of "children" under inspector code might be of HTMLElement.prototype.children. > > > > > > Even after that commenting "children" interface from ScriptProfileNode.idl inspector tool was working without any issues. > > > > > > So I think we need not any changes to Source/WebCore/inspector/*. > > > > No, it's definitely not HTMLElement. If you look into ScriptProfile.idl, its 'head' property returns a ScriptProfileNode: > > > > ] ScriptProfile { > > readonly attribute DOMString title; > > readonly attribute unsigned long uid; > > readonly attribute ScriptProfileNode head; > > }; > > I Mikhail, I debugged the code, but whenever in scripts it calls profile.head it doesn't go to ScriptProfile::head(). Also I observed there is one more Document::head() which gets called when we use profiling. > > Also ScriptProfileNode::children() wasn't called single time while playing with profiling tool but calls HTMLElement::children(). That's weird. I'll check that. Which port (Chromium, Mac, Qt) are you testing? (In reply to comment #39) > (In reply to comment #38) > > (In reply to comment #37) > > > (In reply to comment #36) > > > > (In reply to comment #35) > > > > > > > > As per below discussion I tried changing above mentioned files/places, > > > > but after building code code profiling tool was not working. > > > > > > > > While, reverting only below JS changes it works. > > > > > > > > It indicates to me that instances of "children" under inspector code might be of HTMLElement.prototype.children. > > > > > > > > Even after that commenting "children" interface from ScriptProfileNode.idl inspector tool was working without any issues. > > > > > > > > So I think we need not any changes to Source/WebCore/inspector/*. > > > > > > No, it's definitely not HTMLElement. If you look into ScriptProfile.idl, its 'head' property returns a ScriptProfileNode: > > > > > > ] ScriptProfile { > > > readonly attribute DOMString title; > > > readonly attribute unsigned long uid; > > > readonly attribute ScriptProfileNode head; > > > }; > > > > I Mikhail, I debugged the code, but whenever in scripts it calls profile.head it doesn't go to ScriptProfile::head(). Also I observed there is one more Document::head() which gets called when we use profiling. > > > > Also ScriptProfileNode::children() wasn't called single time while playing with profiling tool but calls HTMLElement::children(). > > That's weird. I'll check that. Which port (Chromium, Mac, Qt) are you testing? (In reply to comment #38) > (In reply to comment #37) > > (In reply to comment #36) > > > (In reply to comment #35) > > > > > > As per below discussion I tried changing above mentioned files/places, > > > but after building code code profiling tool was not working. > > > > > > While, reverting only below JS changes it works. > > > > > > It indicates to me that instances of "children" under inspector code might be of HTMLElement.prototype.children. > > > > > > Even after that commenting "children" interface from ScriptProfileNode.idl inspector tool was working without any issues. > > > > > > So I think we need not any changes to Source/WebCore/inspector/*. > > > > No, it's definitely not HTMLElement. If you look into ScriptProfile.idl, its 'head' property returns a ScriptProfileNode: > > > > ] ScriptProfile { > > readonly attribute DOMString title; > > readonly attribute unsigned long uid; > > readonly attribute ScriptProfileNode head; > > }; > > I Mikhail, I debugged the code, but whenever in scripts it calls profile.head it doesn't go to ScriptProfile::head(). Also I observed there is one more Document::head() which gets called when we use profiling. > > Also ScriptProfileNode::children() wasn't called single time while playing with profiling tool but calls HTMLElement::children(). Vineet, I've tried your patch and while playing with it, I've recalled an important thing. Usage of console.profiles isn't the same as Inspector internal access to profiles. console.profiles indeed uses those .idl files. That's why your change affects the cpu-profiler-profiling-without-inspector.html test -- it deals with the Console API exposed to the page. Inspector itself uses its own JSON-based protocol, so your changes to .idl files doesn't affect it. In other words, you may safely ignore all those ".children" entries in Inspector's code. I apologize I didn't recall that the first time you've asked. Created attachment 146239 [details]
Patch
Thanks Mikhail for clarifying.
Reposting patch for review with small correction to previous patch in LayoutTests/inspector/profiler/cpu-profiler-profiling-without-inspector.html
(In reply to comment #41) > Created an attachment (id=146239) [details] > Patch Erik as Mikhail clarified abvove we don't need to change Source/WebCore/inspector/*. Can you please review new patch once again. Thanks. Comment on attachment 146239 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146239&action=review I'm not a reviewer but once the assert function is renamed I would recommend a reviewer to r+ this :-) > Source/WebCore/bindings/scripts/CodeGenerator.pm:470 > +sub AssertSequenceType Shouldn't this be called AssertNotSequenceType? > Source/WebCore/page/Console.idl:49 > + // As per spec : http://www.w3.org/TR/WebIDL/#idl-sequence // As per spec: http://www.w3.org/TR/WebIDL/#idl-sequence Created attachment 147261 [details]
patch
Thanks Erik..
Updated patch renaming assert function and few nits as suggested.
Comment on attachment 147261 [details]
patch
arv: thank you very much for the review!
Looks good to me too.
Comment on attachment 147261 [details] patch Clearing flags on attachment: 147261 Committed r120206: <http://trac.webkit.org/changeset/120206> All reviewed patches have been landed. Closing bug. This patch appears to have caused ~30 regressions on GTK and EFL ports. All of the new failures are fast/profiler test cases. |