Bug 73162 – Implement [Supplemental] IDL and support it in run-bindings-tests

• Component: DOM
• Status: RESOLVED FIXED

[Kentaro Hara]

- Implement the [Supplemental] IDL in resolve-supplemental.pl and generate-bindings.pl.
- Support the [Supplemental] IDL in run-bindings-tests.
- Add TestSupplemental.idl as a bindings-test and confirm if it works.

After this patch is landed, I will support the [Supplemental] IDL in real build scripts for all platforms (e.g. WebCore.gyp, UseJSC.make, ...).

[Kentaro Hara]

Created attachment 116665 [details]
Patch

[WebKit Review Bot]

Attachment 116665 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:127:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/CPP/WebDOMTestInterface.cpp:31:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 116665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116665&action=review

> Source/WebCore/bindings/scripts/generate-bindings.pl:108
> +        exit 0;

Should we print a message in this case so someone looking at the build output can understand what happened?

> Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp:50
> +    return v8String(imp->str1());

I would have expected this line of code to be:

return v8String(TestSupplemental::str1(imp));

The idea being that in C++ we want to define the str1 function as a static member function of TestSupplemental rather than as a member function of TestInterface.  That way we don't need to mention str1 in the header or implementation file for TestInterface, only in TestSupplemental.

[Kentaro Hara]

(In reply to comment #3)
> (From update of attachment 116665 [details])
> > Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp:50
> > +    return v8String(imp->str1());
> 
> I would have expected this line of code to be:
> 
> return v8String(TestSupplemental::str1(imp));
> 
> The idea being that in C++ we want to define the str1 function as a static member function of TestSupplemental rather than as a member function of TestInterface.  That way we don't need to mention str1 in the header or implementation file for TestInterface, only in TestSupplemental.

Adam: Ah I got it, but let me confirm the reason just in case.

I first tried to do so (as you described above and as we discussed before) but I didn't, because I found that my current approach might be simpler. In my current approach, attributes in the TestSupplemental interface are treated as if they were described in the TestInterface interface, resulting in no header and no implementation for TestSupplemental. The advantage of this approach is that it requires no change in CodeGenerator{JS,V8,...}.pm. It just requires a change in generate-bindings.pl, since we just need to manipulate the contents of IDL files in generate-bindings.pl before CodeGenerator runs.

Is the reason why "we don't need to mention str1 in the header or implementation file for TestInterface, only in TestSupplemental" that str1 might be declared also in TestInterface? In other words, is the reason a name conflict?

[Adam Barth]

Consider the case of Navigator.webkitGamepads:

http://trac.webkit.org/browser/trunk/Source/WebCore/page/Navigator.idl#L66

We would like to implement the Gamepad feature without modifying any code outside of WebCore/Modules/gamepad.  With your patch, we would need to mention the word "gamepad" in Navigator.h because we're generating code that calls the webkitGamepads member function of Navigator.

In the approach in Comment #3, we call webkitGamepads as a static function of a class in the WebCore/Modules/gamepad directory.  That lets us not mention the word "gamepad" in WebCore/page/Navigator.h.

By moving the code for these features out of the "core" classes, we make it easier to implement new features because the new features are better contained within their modules and there's less overall complexity in WebCore.

[Adam Barth]

My guess is you'll want to add an attribute to these nodes when you move them onto the main interface that indicates which supplemental interface they came from so that when we call back into WebCore proper, we'll know which class to call.

[Kentaro Hara]

(In reply to comment #5)
> Consider the case of Navigator.webkitGamepads:
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/page/Navigator.idl#L66
> 
> We would like to implement the Gamepad feature without modifying any code outside of WebCore/Modules/gamepad.  With your patch, we would need to mention the word "gamepad" in Navigator.h because we're generating code that calls the webkitGamepads member function of Navigator.
> 
> In the approach in Comment #3, we call webkitGamepads as a static function of a class in the WebCore/Modules/gamepad directory.  That lets us not mention the word "gamepad" in WebCore/page/Navigator.h.
> 
> By moving the code for these features out of the "core" classes, we make it easier to implement new features because the new features are better contained within their modules and there's less overall complexity in WebCore.

Makes sense! Thanks.

[Kentaro Hara]

Adam: Now I am implementing a WIP patch to confirm that "the Gamepad feature without modifying any code outside of WebCore/Modules/gamepad" works.

(In reply to comment #6)
> My guess is you'll want to add an attribute to these nodes when you move them onto the main interface that indicates which supplemental interface they came from so that when we call back into WebCore proper, we'll know which class to call.

Your idea is something like this?

/* DOMWindowGamePad.idl */
interface [
    Conditional=GAMEPAD,
    Supplemental=DOMWindow
] DOMWindowGamePad {
    attribute [ImplementedBy=GamePad] DOMString supplementedAttr;  /* The getter and setter for supplementedAttr are redirected to class GamePad. */
}

Alternatively, how about allowing the [Supplemental] IDL to each attribute like this?

/* GamePad.idl */
interface [
    Conditional=GAMEPAD
] Gamepad {
    readonly attribute DOMString id; /* A regular attribute of GamePad */
    readonly attribute unsigned long index; /* Ditto */
    readonly attribute unsigned long long timestamp; /* Ditto */
    readonly attribute float[] axes; /* Ditto */
    readonly attribute float[] buttons; /* Ditto */
    attribute [Supplemental=DOMWindow] DOMString supplementedAttr; /* The getter and setter for supplementedAttr are redirected to this class, i.e. class GamePad. */
};

I don't know if this approach is acceptable or not because it is a bit far from the [Supplemental] spec of the Web IDL. The advantages of this approach are 

- that the [ImplementedBy] IDL is not necessary
- and that we do not need to prepare another IDL file just for describing [Supplemental] IDLs, i.e. we do not need to write the DOMWindowGamePad interface, which itself has no meaning.

WDYT?

[Adam Barth]

> Adam: Now I am implementing a WIP patch to confirm that "the Gamepad feature without modifying any code outside of WebCore/Modules/gamepad" works.

You're going to run into a problem because Gamepad requires some storage on Navigator, but that's something we can address after we get Supplemental working.

> > My guess is you'll want to add an attribute to these nodes when you move them onto the main interface that indicates which supplemental interface they came from so that when we call back into WebCore proper, we'll know which class to call.
> 
> Your idea is something like this?

Yes, but with one small difference.

> /* DOMWindowGamePad.idl */
> interface [
>     Conditional=GAMEPAD,
>     Supplemental=DOMWindow
> ] DOMWindowGamePad {
>     attribute [ImplementedBy=GamePad] DOMString supplementedAttr;  /* The getter and setter for supplementedAttr are redirected to class GamePad. */
> }

We don't need the ImplementedBy attribute explicitly in the IDL file.  We can add it implicitly when we merge the supplemental attributes into the main interface.  Specifically, we can imply ImplementedBy=DOMWindowGamePad, where we get the class name from the name of the interface with the supplemental attribute.

> Alternatively, how about allowing the [Supplemental] IDL to each attribute like this?

I'd prefer the first approach because it's the most similar to the WebIDL in the specifications.

> - that the [ImplementedBy] IDL is not necessary

We don't need the ImplementedBy attribute explicitly in the IDL file.  I think it's ok to generate it internally in the Perl.

> - and that we do not need to prepare another IDL file just for describing [Supplemental] IDLs, i.e. we do not need to write the DOMWindowGamePad interface, which itself has no meaning.

Yeah, this is one downside of this approach.  Let's look at the spec:

http://dvcs.w3.org/hg/webevents/raw-file/tip/gamepad.html#navigator-interface-extension

partial interface Navigator {
    readonly attribute Gamepad[] gamepads;
};

Maybe we could write something like:

partial interface [
    Conditional=GAMEPAD,
    ImplementedBy=Gamepad
] Navigator {
    readonly attribute Gamepad[] gamepads;
};

[Kentaro Hara]

(In reply to comment #9)
> > Adam: Now I am implementing a WIP patch to confirm that "the Gamepad feature without modifying any code outside of WebCore/Modules/gamepad" works.
> 
> You're going to run into a problem because Gamepad requires some storage on Navigator, but that's something we can address after we get Supplemental working.

OK. Then I'll do it later. (although, if possible, I want to confirm that my change on Perl scripts works with real IDL files before committing the change, since I cannot see if it really works or not just by watching the run-bindings-tests results:-)
 
> > > My guess is you'll want to add an attribute to these nodes when you move them onto the main interface that indicates which supplemental interface they came from so that when we call back into WebCore proper, we'll know which class to call.
> > 
> > Your idea is something like this?
> 
> Yes, but with one small difference.
> 
> > /* DOMWindowGamePad.idl */
> > interface [
> >     Conditional=GAMEPAD,
> >     Supplemental=DOMWindow
> > ] DOMWindowGamePad {
> >     attribute [ImplementedBy=GamePad] DOMString supplementedAttr;  /* The getter and setter for supplementedAttr are redirected to class GamePad. */
> > }
> 
> We don't need the ImplementedBy attribute explicitly in the IDL file.  We can add it implicitly when we merge the supplemental attributes into the main interface.  Specifically, we can imply ImplementedBy=DOMWindowGamePad, where we get the class name from the name of the interface with the supplemental attribute.

Yes, but my original concern was that in most cases we might want to implement supplementedAttr not in DOMWindowGamePad.cpp but in GamePad.cpp. (If we implement it in DOMWindowGamePad.cpp, we need to newly prepare DOMWindowGamePad.cpp.) It is possible to implicitly assume the class name from the interface name but it might not be so happy in most cases. If we want to implement it in GamePad.cpp, we need to explicitly write [ImplementedBy=GamePad] anyway. Rather than writing [ImplementedBy=GamePad] many times, I thought that the second approach would be better.

> > Alternatively, how about allowing the [Supplemental] IDL to each attribute like this?
> 
> I'd prefer the first approach because it's the most similar to the WebIDL in the specifications.

OK. Following the spec is more important. Let's take the first approach anyway.

> Let's look at the spec:
> 
> http://dvcs.w3.org/hg/webevents/raw-file/tip/gamepad.html#navigator-interface-extension
> 
> partial interface Navigator {
>     readonly attribute Gamepad[] gamepads;
> };
> 
> Maybe we could write something like:
> 
> partial interface [
>     Conditional=GAMEPAD,
>     ImplementedBy=Gamepad
> ] Navigator {
>     readonly attribute Gamepad[] gamepads;
> };

?? Where did [Supplemental] go away?

partial interface [
    Conditional=GAMEPAD,
    Supplemental=Navigator
] DOMWindowGamePad {
    readonly attribute Gamepad[] gamepads;
};

or if we want to implement gamepads not in DOMWindowGamePad.cpp but in GamePad.cpp, we can explicitly write 

partial interface [
    Conditional=GAMEPAD,
    Supplemental=Navigator
] DOMWindowGamePad {
    readonly attribute [ImplementedBy=GamePad] Gamepad[] gamepads;
};

Am I wrong?

[Adam Barth]

(In reply to comment #10)
> (In reply to comment #9)
> > > Adam: Now I am implementing a WIP patch to confirm that "the Gamepad feature without modifying any code outside of WebCore/Modules/gamepad" works.
> > 
> > You're going to run into a problem because Gamepad requires some storage on Navigator, but that's something we can address after we get Supplemental working.
> 
> OK. Then I'll do it later. (although, if possible, I want to confirm that my change on Perl scripts works with real IDL files before committing the change, since I cannot see if it really works or not just by watching the run-bindings-tests results:-)

We can move the function, and it's implementation.  We can just move the storage in a later patch.  WebAudio might be a better case to start with because it does not require any storage.

> Yes, but my original concern was that in most cases we might want to implement supplementedAttr not in DOMWindowGamePad.cpp but in GamePad.cpp. (If we implement it in DOMWindowGamePad.cpp, we need to newly prepare DOMWindowGamePad.cpp.) It is possible to implicitly assume the class name from the interface name but it might not be so happy in most cases. If we want to implement it in GamePad.cpp, we need to explicitly write [ImplementedBy=GamePad] anyway. Rather than writing [ImplementedBy=GamePad] many times, I thought that the second approach would be better.

I think it's ok to create a DOMWindowGamepad.cpp file for the Gamepad extensions to DOMWindow.  That will make the implementation easy to find if we follow that naming convention.

> ?? Where did [Supplemental] go away?

It became the "partial" keyword before interface.  IMHO, we should just use the Supplemental attribute.  We can change the syntax later if we want.

> or if we want to implement gamepads not in DOMWindowGamePad.cpp but in GamePad.cpp, we can explicitly write 

Either way is fine.  I have a slight preference for DOMWindowGamepad.cpp because that makes it clearer what's going on.

[Kentaro Hara]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> I think it's ok to create a DOMWindowGamepad.cpp file for the Gamepad extensions to DOMWindow.  That will make the implementation easy to find if we follow that naming convention.

Makes sense.

> > ?? Where did [Supplemental] go away?
> 
> It became the "partial" keyword before interface.  IMHO, we should just use the Supplemental attribute.  We can change the syntax later if we want.
> 
> > or if we want to implement gamepads not in DOMWindowGamePad.cpp but in GamePad.cpp, we can explicitly write 
> 
> Either way is fine.  I have a slight preference for DOMWindowGamepad.cpp because that makes it clearer what's going on.

I got it. In conclusion, for the time being, I will use [ImplementedBy] IDL only inside Perl.

[Kentaro Hara]

Created attachment 116738 [details]
Patch

[WebKit Review Bot]

Attachment 116738 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:146:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:166:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/CPP/WebDOMTestInterface.cpp:31:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
Total errors found: 3 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 116738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116738&action=review

> Source/WebCore/bindings/scripts/CodeGenerator.pm:575
>      if ($conditional =~ /&/) {
> -        return "ENABLE(" . join(") && ENABLE(", split(/&/, $conditional)) . ")";
> +        # Avoid duplicated conditions.
> +        my %conditions;
> +        map { $conditions{$_} = 1 } split(/&/, $conditional);
> +        return "ENABLE(" . join(") && ENABLE(", sort keys %conditions) . ")";
>      } elsif ($conditional =~ /\|/) {
> -        return "ENABLE(" . join(") || ENABLE(", split(/\|/, $conditional)) . ")";
> +        # Avoid duplicated conditions.
> +        my %conditions;
> +        map { $conditions{$_} = 1 } split(/\|/, $conditional);
> +        return "ENABLE(" . join(") || ENABLE(", sort keys %conditions) . ")";

This feels copy-pasted.  Should we break this out into a helper function?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1654
> +                        push(@implContent, "    return ${implementedBy}::$implGetterFunctionName(castedThis, exec);\n");

This doesn't seem quite right.  $implementedBy will be a WebCore-proper class, which means it shouldn't receive an ExecState.  Perhaps we should write "JS${implementedBy}::$implGetterFunctionName(" ?  (We can also make Custom and ImplementedBy an error to use together for now.)

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1948
>                  }

Looks like you're only handling getters and setters.  It seems like we should handle normal member functions as well.  We can do that in this patch or the next patch, whichever you prefer.

> Source/WebCore/bindings/scripts/generate-bindings.pl:141
> +                    # Record that this attribute is implemented by $interfaceName.
> +                    $attribute->signature->extendedAttributes->{"ImplementedBy"} = $interfaceName;

Should we move this outside the inner loop?  It seems like we'll end up make this assignment many times redundantly.

> Source/WebCore/bindings/scripts/test/ObjC/DOMTestInterface.mm:76
> +    WebCore::JSMainThreadNullState state;
> +    return IMPL->str1();

This won't compile, right?  We might need to modify the other code generators as well.

[Kentaro Hara]

Created attachment 116880 [details]
Patch

[Kentaro Hara]

Thanks for reviewing the big patch.

(In reply to comment #15)
> (From update of attachment 116738 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116738&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGenerator.pm:575
> >      if ($conditional =~ /&/) {
> > -        return "ENABLE(" . join(") && ENABLE(", split(/&/, $conditional)) . ")";
> > +        # Avoid duplicated conditions.
> > +        my %conditions;
> > +        map { $conditions{$_} = 1 } split(/&/, $conditional);
> > +        return "ENABLE(" . join(") && ENABLE(", sort keys %conditions) . ")";
> >      } elsif ($conditional =~ /\|/) {
> > -        return "ENABLE(" . join(") || ENABLE(", split(/\|/, $conditional)) . ")";
> > +        # Avoid duplicated conditions.
> > +        my %conditions;
> > +        map { $conditions{$_} = 1 } split(/\|/, $conditional);
> > +        return "ENABLE(" . join(") || ENABLE(", sort keys %conditions) . ")";
> 
> This feels copy-pasted.  Should we break this out into a helper function?

Fixed it.

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1654
> > +                        push(@implContent, "    return ${implementedBy}::$implGetterFunctionName(castedThis, exec);\n");
> 
> This doesn't seem quite right.  $implementedBy will be a WebCore-proper class, which means it shouldn't receive an ExecState.  Perhaps we should write "JS${implementedBy}::$implGetterFunctionName(" ?  (We can also make Custom and ImplementedBy an error to use together for now.)

Fixed it.
 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1948
> >                  }
> 
> Looks like you're only handling getters and setters.  It seems like we should handle normal member functions as well.  We can do that in this patch or the next patch, whichever you prefer.

Yeah, then I'd like to (1) support only getters and setters in this patch, (2) use the [Supplemental] IDL in webaudio and confirm that all platforms work (though webaudio does not require even getters and setters), (3) support normal member functions.

> > Source/WebCore/bindings/scripts/generate-bindings.pl:141
> > +                    # Record that this attribute is implemented by $interfaceName.
> > +                    $attribute->signature->extendedAttributes->{"ImplementedBy"} = $interfaceName;
> 
> Should we move this outside the inner loop?  It seems like we'll end up make this assignment many times redundantly.

Done.
 
> > Source/WebCore/bindings/scripts/test/ObjC/DOMTestInterface.mm:76
> > +    WebCore::JSMainThreadNullState state;
> > +    return IMPL->str1();
> 
> This won't compile, right?  We might need to modify the other code generators as well.

You're right. I modified CodeGenerator{GObject,CPP,ObjC}.pm as well.

[WebKit Review Bot]

Attachment 116880 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:147:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:167:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/CPP/WebDOMTestInterface.cpp:32:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 116880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116880&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1653
> +                        $implIncludes{"${implementedBy}.h"} = 1;

Do we need to include the header for JS${implementedBy} ?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1842
> +                                $implIncludes{"${implementedBy}.h"} = 1;

Same question.

[Kentaro Hara]

Created attachment 117072 [details]
rebased patch for commit

[Kentaro Hara]

(In reply to comment #19)
> (From update of attachment 116880 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116880&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1653
> > +                        $implIncludes{"${implementedBy}.h"} = 1;
> 
> Do we need to include the header for JS${implementedBy} ?
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1842
> > +                                $implIncludes{"${implementedBy}.h"} = 1;
> 
> Same question.

Fixed it.

After confirming that mac build succeeds (I guess that the mac build error we observed is not related our patch), I'll commit it manually to avoid style check errors.

[WebKit Review Bot]

Attachment 117072 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:147:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:167:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/CPP/WebDOMTestInterface.cpp:32:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kentaro Hara]

Created attachment 117090 [details]
to see if mac build passes

[WebKit Review Bot]

Attachment 117090 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:147:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:167:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/CPP/WebDOMTestInterface.cpp:32:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kentaro Hara]

Created attachment 117098 [details]
Test: Ignore this patch

[Kentaro Hara]

Created attachment 117101 [details]
rebased patch for commit

[WebKit Review Bot]

Attachment 117101 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:147:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:167:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/CPP/WebDOMTestInterface.cpp:32:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kentaro Hara]

Committed r101445: <http://trac.webkit.org/changeset/101445>

[Kentaro Hara]

Comment on attachment 117101 [details]
rebased patch for commit

View in context: https://bugs.webkit.org/attachment.cgi?id=117101&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm:1352
> +                $getterContent = $getterContentHead . ($getterContentHead =~ /\($|, $/ ? "ec" : ", ec") . $getterContentTail;

By the way, building an argument string in this way is dirty and error-prone. Instead, we should prepare a list @args, push all arguments into @args, and finally build an argument string by join(", ", @args).

As far as I see, this refactoring is not so trivial across many places in all CodeGenerators. I'll do it when I have time.

[Simon Fraser (smfr)]

This broke bindings generation tests:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Tests%29/builds/3265/steps/bindings-generation-tests/logs/stdio

[Csaba Osztrogonác]

Reopen because it broke run-bindings-tests on all bot: SL, GTK, Qt.
It strange for me that it fails on the Qt bot, but pass for me locally.

Could you fix it as soon as possible or roll out this patch?
I don't think if we should leave all bots red because of this ...

[Adam Barth]

I believe this was fixed in Bug 73558.