Merge lp://staging/~shakaran/userwebkit/class-refactor into lp://staging/userwebkit
| Status: | Needs review |
|---|---|
| Proposed branch: | lp://staging/~shakaran/userwebkit/class-refactor |
| Merge into: | lp://staging/userwebkit |
| Diff against target: |
705 lines (+334/-204) 8 files modified
couchview.py (+158/-0) debian/copyright (+1/-1) demo-app.py (+7/-7) demo-app2.py (+6/-6) hub.py (+81/-0) inspector.py (+54/-0) test_userwebkit.py (+12/-10) userwebkit.py (+15/-180) |
| To merge this branch: | bzr merge lp://staging/~shakaran/userwebkit/class-refactor |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Jason Gerard DeRose | Disapprove | ||
|
Review via email:
|
|||
Description of the change
Major changes:
- Split userwebkit classes (Hub, Inspector and CouchView) in different files for reuse components easily. Useful when you want use only a component from other project and don't import all module or only a part. Also it makes more easily to track changes with more reduced files.
- Update test according to last refactor.
Minor changes:
- Update copyright year (debian file), demo-app.py and demo-app2.py
- Remove blank lines and update shebang in demo-app.py and demo-app2.py
Problems to review:
- I cannot fix one test regarding to FactoryHub signals. I think that signals are not properly erased in teardown test. - Also check if setup.py include all refactored files in installation.
Unmerged revisions
- 70. By Angel Guzman Maeso <email address hidden>
-
Update test according to last refactor. Only I cannot fix one test. I think that signals are not properly erased in teardown test. Also check if setup.py include all refactored files in installation. Needs review
- 69. By Angel Guzman Maeso <email address hidden>
-
Split userwebkit classes (Hub, Inspector and CouchView) in different files for reuse components easily
- 68. By Angel Guzman Maeso <email address hidden>
-
Update copyright year, remove blank lines and update shebang
- 67. By Angel Guzman Maeso <email address hidden>
-
Update copyright year
Sorry Angel, I'm gonna say no on this one :)
As userwebkit.py is less than 500 lines, I don't think this split is yet needed for maintainability. Plus, I think we need more work on the existing API before we know how such a split should be done.
The startup time of a Python app also has a somewhat steep per-module overhead. So in terms of looking at performance and memory usage, it's generally more import to split modules based on what those modules themselves then import. And in this case, everything needs to import `gi.repository.Gtk` (which is where the biggest overhead is), so if you're using anything from userwebkit.py, there is little benefit to importing just a fragment of it.
One more note: once a split like this is done, the best way to do it is to bzr mv the current userwebkit.py into userwebkit/
I'm going to do the above when I bring back the JavaScript unit tester that used to be in Dmedia (it hasn't yet been ported to Python3 because of dependency issues). But this is something that will only be used for unit tests, not at run-time, so having it in a separate module makes sense.