Bug 30504 – wxPython bindings missing 'WebViewDOMElementInfo'.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit wx

[Pedro Romano]

The 'wxPython' SWIG input file is missing the necessary includes to generate the bindings for the C++ 'WebViewDOMElementInfo' class. This prevents the usage of, among others, the 'WebFrame.HitTest' method whose return value is an instance of 'WebViewDOMElementInfo'.

[Pedro Romano]

Created attachment 41402 [details]
'WebFrame.h' include in SWIG wxPython input file.

Including 'WebFrame.h' in the 'webview.i' wxPython bindings input file seems to correct the reported shortcoming by generating bindings for 'WebViewDOMElementInfo'.

[Eric Seidel (no email)]

Comment on attachment 41402 [details]
'WebFrame.h' include in SWIG wxPython input file.

Every change requires a ChangeLog, see http://webkit.org/coding/contributing.html

How do we test this?

[Eric Seidel (no email)]

It seems it would be easy to write some python unit tests for this sort of thing, no?

[Pedro Romano]

I'll try to get some help from the 'wxWebKit' maintainers on how best to proceed. Thanks!

[Pedro Romano]

Created attachment 41568 [details]
Revised with changelog: [wxPython bindings missing 'WebViewDOMElementInfo']

Hi Eric,

Corrected patch to include 'Changelog'.

This is a change to the SWIG input file for automatic generation of the Python bindings source files, and it isn't suited to having tests written for it. Already consulted with Kevin Ollivier who advised on proceeding this way.

Thanks!

[Eric Seidel (no email)]

Why can't we write a python unit test for this?  I disagree with this being r+'d.

[Kevin Ollivier]

Landed in r49908, thanks!

[Eric Seidel (no email)]

Kinda lame to land with my disagreement.

[Kevin Ollivier]

(In reply to comment #8)
> Kinda lame to land with my disagreement.

Sorry, I've been busy prepping a release and so I forgot to write back to you myself, but Pedro did reply with an answer to you already (which I agree with).

Moreover, I did not see any reason to continue discussing this particular issue because, well, I do frankly know more about the wx port than you do having actually written the file being reviewed, and even I have no idea what relevance questions about unit tests have to this particular change. The only way I could possibly see to unit test this particular change is to actively test that every SWIG generated Python class does indeed expose every method we intend to make available to users, and that clearly sounds like overkill to me. Even if the issue were considered relevant, the concern should have been raised when the Python bindings were first submitted (or now in a separate ticket named "wxPython bindings should test that their methods exist"), and not as part of this particular patch review. In other words, no matter how I look at it, I can't see how the unit testing question is appropriate for this particular patch review.

I appreciate that sometimes r? patches stagnate for a couple weeks or more, and at that time they do deserve some sort of response even if not from someone explicitly knowledgeable in that area, but I rarely let a wx port r? patch sit in the tree for weeks; in fact, I bet most are handled within 24 hours, so I don't really understand the urge to get wx patches reviewed and out of the queue when they are going to be taken care of promptly anyway by someone more knowledgeable in that area.

I think the wx(Python) port maintainer is probably the best person to take the first crack at a completely wx(Python)-specific patch, don't you? It seems almost a waste of time and effort not to, actually.

[Eric Seidel (no email)]

(In reply to comment #9)
> In other words, no matter how I look at
> it, I can't see how the unit testing question is appropriate for this
> particular patch review.

Testing is always an appropriate question for a patch review.

> I appreciate that sometimes r? patches stagnate for a couple weeks or more, and
> at that time they do deserve some sort of response even if not from someone
> explicitly knowledgeable in that area, but I rarely let a wx port r? patch sit
> in the tree for weeks; in fact, I bet most are handled within 24 hours, so I
> don't really understand the urge to get wx patches reviewed and out of the
> queue when they are going to be taken care of promptly anyway by someone more
> knowledgeable in that area.

WebKit reviewers should review any patch they feel comfortable reviewing. We don't have corded off areas off the code.  I felt comfortable reviewing this in that I speak python.  Python is very easy to unit test.  The lack of any attempt at testing is against normal webkit process.  http://webkit.org/coding/contributing.html talks about our strong belief in testing.

> I think the wx(Python) port maintainer is probably the best person to take the
> first crack at a completely wx(Python)-specific patch, don't you? It seems
> almost a waste of time and effort not to, actually.

Yes, I'm glad that you review the wx patches.  You're certainly better at it than I am.  I still think that all patches to WebKit should have testing.  It's easy to test a change like this.  Creating a python unit test is dead simple: http://docs.python.org/library/unittest.html

I also don't think that just because we didn't add testing when the swig bindings landed doesn't mean they shouldn't have it now.

My last statement of disagreement was that you landed this without personally addressing my feedback.  Not a huge deal on a patch like this, but also isn't the most polite thing to do.

I would strongly advise future changes to python bindings like this to have some form of unit testing.  We don't have complete coverage of every piece of JS binding code, but every time we change the JS bindings we add a test for them.  Likewise, whenever the python bindings are changed, we should add tests for them.

[Kevin Ollivier]

(In reply to comment #10)
> (In reply to comment #9)
> > In other words, no matter how I look at
> > it, I can't see how the unit testing question is appropriate for this
> > particular patch review.
> 
> Testing is always an appropriate question for a patch review.
>
> > I appreciate that sometimes r? patches stagnate for a couple weeks or more, and
> > at that time they do deserve some sort of response even if not from someone
> > explicitly knowledgeable in that area, but I rarely let a wx port r? patch sit
> > in the tree for weeks; in fact, I bet most are handled within 24 hours, so I
> > don't really understand the urge to get wx patches reviewed and out of the
> > queue when they are going to be taken care of promptly anyway by someone more
> > knowledgeable in that area.
> 
> WebKit reviewers should review any patch they feel comfortable reviewing. We
> don't have corded off areas off the code.  I felt comfortable reviewing this in
> that I speak python.  Python is very easy to unit test.  The lack of any
> attempt at testing is against normal webkit process. 
> http://webkit.org/coding/contributing.html talks about our strong belief in
> testing.

Here is what it says: "For any feature that affects the layout engine, a new regression test must be constructed." Everything subsequent written on that page is pretty explicitly geared towards writing layout tests. This patch does not come anywhere near the layout engine, and thus it is not violating current WebKit process requirements.
 
> > I think the wx(Python) port maintainer is probably the best person to take the
> > first crack at a completely wx(Python)-specific patch, don't you? It seems
> > almost a waste of time and effort not to, actually.
> 
> Yes, I'm glad that you review the wx patches.  You're certainly better at it
> than I am.  I still think that all patches to WebKit should have testing.  It's
> easy to test a change like this.  Creating a python unit test is dead simple:
> http://docs.python.org/library/unittest.html

I've been using Python for nearly 10 years now and I also pushed for and mentored a SoC project to develop a PyUnit test suite for wxPython that now runs quite a few tests (1000+ counting tests of subclass methods). I know about unit testing in Python. You should be careful about making assumptions and generalizations. 

My beef is that it makes little sense to argue that all changes must have unit tests when there is no unit testing infrastructure in place in this part of the codebase, and requesting one is not grounds for keeping this change out of the tree, *especially* since it's not required by written WebKit policy, it's grounds for a new ticket.

> I also don't think that just because we didn't add testing when the swig
> bindings landed doesn't mean they shouldn't have it now.

Again, file a ticket about it.

> My last statement of disagreement was that you landed this without personally
> addressing my feedback.  Not a huge deal on a patch like this, but also isn't
> the most polite thing to do.

No, but I really feel it wasn't necessary either given the situation. Moreover, I really felt the comments about unit testing were written hurriedly and with a lack of understanding of the wx port's current state, which I didn't find very polite either. If you're going to take on the responsibility of a review, take some time out to understand things first or leave it to someone who's more familiar with things.

> I would strongly advise future changes to python bindings like this to have
> some form of unit testing. We don't have complete coverage of every piece of
> JS binding code, but every time we change the JS bindings we add a test for
> them.  Likewise, whenever the python bindings are changed, we should add tests
> for them.

Yes, but it's really easy to make statements like this when you have no idea of the practical implications of what you're suggesting. What I need is not a pep talk about the importance of unit testing (you're preaching to the choir), I need people helping to get DRT running and passing all the relevant tests, I need someone to help build a CPPUnit (or something else, if better) test framework for the C++ API, and then when those are done, I'd love to have tests for the bindings too. Or, I could do those things myself if someone's willing to help fill the wx feature holes I need filled to do a release of my app for me. :)

If you've got some ideas for how to help me actually tackle those problems, that's something I'd really like to hear.