Bug 84863 – REGRESSION:Bindings sequence<T> in Console.idl, Internals.idl and ScriptProfileNode.idl should be T[]

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vineet Chaudhary (vineetc)]

As per discussion on bug Bug 80696 and with reference to http://www.w3.org/TR/WebIDL/#idl-sequence

"Sequences must not be used as the type of an attribute, constant or exception field."
This caused REGRESSION in files Console.idl, Internals.idl and ScriptProfileNode.idl.
We should change these bindings as

From:
page/Console.idl:        readonly attribute sequence<ScriptProfile> profiles;
    testing/Internals.idl:        attribute sequence<String> userPreferredLanguages;
    inspector/ScriptProfileNode.idl:        readonly attribute sequence<ScriptProfileNode> children;

To:

    page/Console.idl:        readonly attribute ScriptProfile[] profiles;
    testing/Internals.idl:        attribute String[] userPreferredLanguages;
    inspector/ScriptProfileNode.idl:        readonly attribute ScriptProfileNode[] children;

[Vineet Chaudhary (vineetc)]

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.

[Kentaro Hara]

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?

[Vineet Chaudhary (vineetc)]

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 :)

[Vineet Chaudhary (vineetc)]

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 :)

[Kentaro Hara]

(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.

[Vineet Chaudhary (vineetc)]

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]+)\[\]/;

[Kentaro Hara]

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.

[Vineet Chaudhary (vineetc)]

(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?

[Kentaro Hara]

(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?

[Vineet Chaudhary (vineetc)]

(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..

[Kentaro Hara]

(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

[Kentaro Hara]

And it is a good idea to add LayoutTests for the code:

   internals.userPreferredLanguages === internals.userPreferredLanguages
   console.profiles === console.profiles
   scriptProfileNode.children === scriptProfileNode.children

[Kentaro Hara]

Comment on attachment 138941 [details]
updatedPatch

Marking r- due to the recent comments.

[Vineet Chaudhary (vineetc)]

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

[Kentaro Hara]

(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?

[Erik Arvidsson]

(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

[Ojan Vafai]

(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>)

[Vineet Chaudhary (vineetc)]

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?

[Erik Arvidsson]

(In reply to comment #18)
> Is this is acceptable?

If the tests are update. Yes.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Vineet Chaudhary (vineetc)]

Created attachment 143242 [details]
updatedPatch

Updated patch with fixes to failing layout tests.

[Kentaro Hara]

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()?

[Vineet Chaudhary (vineetc)]

(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.

[Kentaro Hara]

(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.

[Kentaro Hara]

(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.

[Vineet Chaudhary (vineetc)]

Created attachment 143296 [details]
UpdatedPatch

[Erik Arvidsson]

Comment on attachment 143296 [details]
UpdatedPatch

Doesn't this need any changes to Source/WebCore/inspector/?

[Vineet Chaudhary (vineetc)]

(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.

[Erik Arvidsson]

(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

[Vineet Chaudhary (vineetc)]

(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..?

[Mikhail Naganov]

(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.

[Erik Arvidsson]

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.

[Mikhail Naganov]

(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.

[Vineet Chaudhary (vineetc)]

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.

[Vineet Chaudhary (vineetc)]

(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/*.

[Mikhail Naganov]

(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;
    };

[Vineet Chaudhary (vineetc)]

(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().

[Mikhail Naganov]

(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?

[Mikhail Naganov]

(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.

[Vineet Chaudhary (vineetc)]

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

[Vineet Chaudhary (vineetc)]

(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.

[Erik Arvidsson]

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

[Vineet Chaudhary (vineetc)]

Created attachment 147261 [details]
patch

Thanks Erik..
Updated patch renaming assert function and few nits as suggested.

[Kentaro Hara]

Comment on attachment 147261 [details]
patch

arv: thank you very much for the review!

Looks good to me too.

[WebKit Review Bot]

Comment on attachment 147261 [details]
patch

Clearing flags on attachment: 147261

Committed r120206: <http://trac.webkit.org/changeset/120206>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

This patch appears to have caused ~30 regressions on GTK and EFL ports. All of the new failures are fast/profiler test cases.