Move BrowserWindow and sub UI classes into their own sub folder

RESOLVED FIXED in Firefox 42

Status

Testing
Firefox UI Tests
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: whimboo, Assigned: goma, Mentored)

Tracking

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

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

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

52 bytes, text/x-github-pull-request
whimboo
: review+
Details | Review | Splinter Review
(Reporter)

Description

3 years ago
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.
(Assignee)

Comment 1

2 years ago
Hey whimboo :)
Do you think that this bug would be a good bug to work on ??
Flags: needinfo?(hskupin)
(Reporter)

Comment 2

2 years ago
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)
(Assignee)

Comment 3

2 years ago
I'd like to work on this bug  :)
Thanks
(Reporter)

Comment 4

2 years ago
Great. If you are stuck with something please let me know.
Assignee: nobody → gbrmachado
Status: NEW → ASSIGNED
(Assignee)

Comment 5

2 years ago
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)
(Reporter)

Comment 6

2 years ago
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)
(Reporter)

Comment 7

2 years ago
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.
(Assignee)

Comment 8

2 years ago
Created attachment 8667061 [details] [review]
Link to Github Pull Request
Attachment #8667061 - Flags: review?(hskupin)
(Reporter)

Comment 9

2 years ago
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)
(Assignee)

Comment 10

2 years ago
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)
(Assignee)

Comment 11

2 years ago
Hey whimboo. I've already done it :)
(Assignee)

Comment 12

2 years ago
Created attachment 8668212 [details] [review]
Link to Github Pull Request
Attachment #8667061 - Attachment is obsolete: true
Attachment #8667061 - Flags: review?(hskupin)
Attachment #8668212 - Flags: review?(hskupin)
(Reporter)

Comment 13

2 years ago
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+
(Assignee)

Comment 14

2 years ago
I talked with you on IRC, the nits are ok(I think) :P
Flags: needinfo?(gbrmachado)
(Reporter)

Comment 15

2 years ago
I made a mistake in my last review. Instead of browser/windows.py it should be browser/window.py. Sorry for that.
(Reporter)

Comment 16

2 years ago
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: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
(Reporter)

Comment 17

2 years ago
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
Product: Mozilla QA → Testing
You need to log in before you can comment on or make changes to this bug.