Closed Bug 1141294 Opened 9 years ago Closed 9 years ago

Move BrowserWindow and sub UI classes into their own sub folder

Categories

(Testing :: Firefox UI Tests, defect)

defect
Not set
normal

Tracking

(firefox42 fixed, firefox43 fixed, firefox44 fixed, firefox-esr38 wontfix)

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 --- fixed
firefox43 --- fixed
firefox44 --- fixed
firefox-esr38 --- wontfix

People

(Reporter: whimboo, Assigned: goma, Mentored)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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 ??
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(hskupin)
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?
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(hskupin)
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.
Attached file Link to Github Pull Request (obsolete) —
Attachment #8667061 - Flags: review?(hskupin)
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
Attachment #8667061 - Flags: review?(hskupin)
Comment on attachment 8667061 [details] [review]
Link to Github Pull Request

https://github.com/mozilla/firefox-ui-tests/pull/254
Attachment #8667061 - Flags: review?(hskupin)
Hey whimboo. I've already done it :)
Attachment #8667061 - Attachment is obsolete: true
Attachment #8667061 - Flags: review?(hskupin)
Attachment #8668212 - Flags: review?(hskupin)
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.
Flags: needinfo?(gbrmachado)
Attachment #8668212 - Flags: review?(hskupin) → review+
I talked with you on IRC, the nits are ok(I think) :P
Flags: needinfo?(gbrmachado)
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
Closed: 9 years ago
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)
Product: Mozilla QA → Testing
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: