Bug 95937 – [Gtk] GObject DOM generator should consider conditionals for EventTarget interface

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKitGTK

[José Dapena Paz]

GObject DOM generator generates the implementation in GObject of the EventTarget interface in method GenerateEventTargetIface. The implementation dumps the hardcoded methods with no check about conditionals.

In the case we add support for BATTERY_STATUS on WebKit GTK, it will break in case BATTERY_STATUS is disabled, as it will generate anyway code depending on battery module.

So support for conditionals is required in this case.

[José Dapena Paz]

Created attachment 162437 [details]
Add support for conditional interface EventTarget on bindings

[WebKit Review Bot]

Attachment 162437 [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/ObjC/DOMTestConditionalEventTargetInternal.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/JS/JSTestConditionalEventTarget.h:62:  More than one command on the same line in if  [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/CPP/WebDOMTestConditionalEventTarget.h:44:  The parameter name "listener" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/CPP/WebDOMTestConditionalEventTarget.h:45:  The parameter name "listener" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/CPP/WebDOMTestConditionalEventTarget.cpp:34:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
Total errors found: 5 in 15 files


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

[José Dapena Paz]

(In reply to comment #2)
> Attachment 162437 [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/ObjC/DOMTestConditionalEventTargetInternal.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
> Source/WebCore/bindings/scripts/test/JS/JSTestConditionalEventTarget.h:62:  More than one command on the same line in if  [whitespace/parens] [4]
> Source/WebCore/bindings/scripts/test/CPP/WebDOMTestConditionalEventTarget.h:44:  The parameter name "listener" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Source/WebCore/bindings/scripts/test/CPP/WebDOMTestConditionalEventTarget.h:45:  The parameter name "listener" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Source/WebCore/bindings/scripts/test/CPP/WebDOMTestConditionalEventTarget.cpp:34:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
> Total errors found: 5 in 15 files

Unfortunately all these reports are caused by bugs in bindings generators. The generated code does not match our style guidelines.

[Xan Lopez]

The code changes look OK (although I really hate to have to change to so many 'push' calls, but don't see a better option).

Only comment is whether it wouldn't make more sense to just add a Conditional like in the aleady existing TestEventTarget.idl file.

[José Dapena Paz]

(In reply to comment #4)
> The code changes look OK (although I really hate to have to change to so many 'push' calls, but don't see a better option).
> 
> Only comment is whether it wouldn't make more sense to just add a Conditional like in the aleady existing TestEventTarget.idl file.

I kept the old test, as this way we are testing 2 different outputs: non conditional and conditional. So tests will be more accurate catching regressions.

[Xan Lopez]

(In reply to comment #5)
> (In reply to comment #4)
> > The code changes look OK (although I really hate to have to change to so many 'push' calls, but don't see a better option).
> > 
> > Only comment is whether it wouldn't make more sense to just add a Conditional like in the aleady existing TestEventTarget.idl file.
> 
> I kept the old test, as this way we are testing 2 different outputs: non conditional and conditional. So tests will be more accurate catching regressions.

OK. I went through the different tests and it seemed some were already including Conditional clauses as part of them, so it could make sense to just make the EventTarget test include that. Anyway maybe you can just try to ask people whether they want a new test or not, and then we'll move forward with whatever they prefer. No strong opinion here.

[José Dapena Paz]

(In reply to comment #6)
> OK. I went through the different tests and it seemed some were already including Conditional clauses as part of them, so it could make sense to just make the EventTarget test include that. Anyway maybe you can just try to ask people whether they want a new test or not, and then we'll move forward with whatever they prefer. No strong opinion here.

Yeah, the main difference is that the conditional applies to an EventTarget. Another approach would be just adding the conditional clause to the TestEventTarget.idl. Anyway, I'll check with committers of the other idl files.

[Xan Lopez]

(In reply to comment #7)
> Yeah, the main difference is that the conditional applies to an EventTarget. Another approach would be just adding the conditional clause to the TestEventTarget.idl. Anyway, I'll check with committers of the other idl files.

Adding the conditional clause to TestEventTarget.idl is exactly what I'm proposing.

From comment #4:

> Only comment is whether it wouldn't make more sense to just add a Conditional like in the aleady existing TestEventTarget.idl file.

[Philippe Normand]

Comment on attachment 162437 [details]
Add support for conditional interface EventTarget on bindings

r- as per Xan's comments and style issues, I suppose some of them, at least, should be fixed :)

[Zan Dobersek]

Fixed in bug #105743.

*** This bug has been marked as a duplicate of bug 105743 ***