Code review comment for lp:~uriboni/oxide/find-in-page

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

Thanks for working on this. I have some general comments:

- I think I'd prefer WebView.findController and FindController for the corresponding class name in QML

- The implementation doesn't need to live in qt/quick/api - it should go in qt/core/api as OxideQFindController, as it doesn't really have any dependencies outside of the core library or QtCore and the implementation could be shared with a QWidget port - if someone ever decides it would be a good idea to write one :)

- As we want to open up the C++ API in the future, the data members on the public class should be in a private class.

I've also left some comments inline.

review: Needs Fixing

« Back to merge proposal