Bug 95937 - [Gtk] GObject DOM generator should consider conditionals for EventTarget interface
Summary: [Gtk] GObject DOM generator should consider conditionals for EventTarget inte...
Status: RESOLVED DUPLICATE of bug 105743
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: José Dapena Paz
URL:
Keywords:
Depends on:
Blocks: 95582
  Show dependency treegraph
 
Reported: 2012-09-05 23:46 PDT by José Dapena Paz
Modified: 2013-05-07 01:45 PDT (History)
8 users (show)

See Also:


Attachments
Add support for conditional interface EventTarget on bindings (76.59 KB, patch)
2012-09-06 00:42 PDT, José Dapena Paz
pnormand: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description José Dapena Paz 2012-09-05 23:46:50 PDT
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.
Comment 1 José Dapena Paz 2012-09-06 00:42:44 PDT
Created attachment 162437 [details]
Add support for conditional interface EventTarget on bindings
Comment 2 WebKit Review Bot 2012-09-06 00:45:59 PDT
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.
Comment 3 José Dapena Paz 2012-09-06 01:22:30 PDT
(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.
Comment 4 Xan Lopez 2012-10-02 10:39:00 PDT
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.
Comment 5 José Dapena Paz 2012-10-03 08:08:56 PDT
(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.
Comment 6 Xan Lopez 2012-10-03 09:31:04 PDT
(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.
Comment 7 José Dapena Paz 2012-10-04 01:50:42 PDT
(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.
Comment 8 Xan Lopez 2012-10-04 04:01:44 PDT
(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.
Comment 9 Philippe Normand 2013-04-12 12:18:36 PDT
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 :)
Comment 10 Zan Dobersek 2013-05-07 01:45:52 PDT
Fixed in bug #105743.

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