Similar to the page info window class, which can be found under firefox_puppeteer/ui/page_info, we should move out the BrowserWindow class from the windows.py module into firefox_puppeteer/ui/browser. Along that also all sub ui modules have to be moved too into the same folder. This belongs to tabbar.py, toolbars.py.
Hey whimboo :) Do you think that this bug would be a good bug to work on ??
Totally! It's actually something which I wanted to do for a long time to get a better module/class structure. If you could really pick this up I would really appreciate. Please let me know if you want to take it. Thanks a lot in advance.
I'd like to work on this bug :) Thanks
Great. If you are stuck with something please let me know.
Assignee: nobody → gbrmachado
Status: NEW → ASSIGNED
So, basically, I have to move the content from: https://github.com/gbrmachado/firefox-ui-tests/blob/master/firefox_puppeteer/ui/windows.py#L418-L621 into a new file firefox_puppeter/ui/browser/browser_window.py After that, move the submodules inside browser_window.py into 2 new files: firefox_puppeter/ui/browser/tabbar.py firefox_puppeter/ui/browser/toolbars.py And then, change all the files that were depending of ui/windows.py before?
That's correct. It should only be ui libs which actually depend on windows.py. So it's a very limited set to watch out. Once done with the move please do not forget to also update the documentation for the ui libraries under firefox_puppeteer/docs.
Oh, it would be good if you can do the work in separate commits, like: 1. Move tabbar and toolbar modules to ui/browser/ + update docs 2. Move BrowserWindow class to ui/browser/ + update docs We can do an interim review after step 1 and could squash everything before you continue with step 2. That would make it easier for me to review.
Created attachment 8667061 [details] [review] Link to Github Pull Request
Comment on attachment 8667061 [details] [review] Link to Github Pull Request We have removed the master branch yesterday in favor of the new mozilla-central branch. Because of this your PR has been closed and you will have to create a new one. Sorry for that. So here what you should do: * run 'git fetch upstream' to get the new branch from upstream * checkout your developer branch * run 'git rebase mozilla-central' to rebase against the new branch * Push to your fork and create the PR as usual
Comment on attachment 8667061 [details] [review] Link to Github Pull Request https://github.com/mozilla/firefox-ui-tests/pull/254
Hey whimboo. I've already done it :)
Created attachment 8668212 [details] [review] Link to Github Pull Request
Comment on attachment 8668212 [details] [review] Link to Github Pull Request Looks perfect and I can see great improvements in your work! Thank you for the updated patch. Please make sure to fix the nits which I pointed out and let me know when the new commit is up so I can merge it.
Attachment #8668212 - Flags: review?(hskupin) → review+
I talked with you on IRC, the nits are ok(I think) :P
I made a mistake in my last review. Instead of browser/windows.py it should be browser/window.py. Sorry for that.
Everything has been updated and the PR got merged to mozilla-central as: https://github.com/mozilla/firefox-ui-tests/commit/99715a8fcdfc109d6d992f47e9d80298947b2076 Thank you for the patch, Gabriel. If you want to have another challenge please let me know. We can even find something harder for you if you want.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
The changes on this bug as landed on mozilla-central only and which are also now on mozilla-aurora due to the branch merged for Firefox 45.0a1 are blocking the backport of bug 1217046 because those are not present yet on beta, and release. esr38 we cannot easily update so I will skip that. Means I'm backporting the patch to all other branches now: https://github.com/mozilla/firefox-ui-tests/commit/e93cdbff656a32cdcc8d3e864fbd9434d13930e2 (beta) https://github.com/mozilla/firefox-ui-tests/commit/fecb66745633c0ae337e9bf431ed150a8766221a (release)
status-firefox42: --- → fixed
status-firefox43: --- → fixed
status-firefox-esr38: --- → wontfix
You need to log in before you can comment on or make changes to this bug.