1205262 - We can't use more than one browser window if we have to switch between them

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

Description

[Martijn Wargers (dead)]

This is a follow-up from bug 1203060, see bug 1203060, comment 14.

We have this Search app and Browser app.
http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/apps/search/app.py
http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/apps/search/regions/browser.py

When we now have a test, what usually happens:
search = Search(self.marionette)
search.launch()
browser = search.go_to_url(some_url)

This doesn't make sense. Browser and Search are almost the same, except Browser has some browser extra browser UI and Search has this start page that Browser doesn't have.
Currently, you can't do browser.go_to_url(some_url) for example, because Browser doesn't have that method.

I propose to combine these 2 app objects and add 2 properties, is_browser and is_private_browser so that tests can distinguish between the 3 different states.
I could also make sure that methods that are specific to one/two specific state(s) give a nice failure when the app object is currently not in that state.
What do you think?

Btw, this Browser and Search app thing was written in:
https://github.com/mozilla-b2g/gaia/pull/23467

Comment 1

[Martijn Wargers (dead)]

Johan, what do you think about this? This subject came also more or less up in the pull request of bug 1208912.
For me, the browser, search and privatewindow have so much in common that I think it would be better to combine them in one Browser object. Then we could distinguish between them by a isBrower(), isSearch and isPrivateWindow property or something along that line.

We also need to make sure we can access all unique and individual browser instances that might be open (and we should write a test for that).

Comment 2

[Johan Lorenzo [:jlorenzo]]

After taking a closer look, Search and Browser share:
* The iFrame locator
* The url bar (and the possibility to change the URL)

I think it makes sense to factorize these parts.

But, the rest is specific to each page class and I would leave them in a different class, and not use the methods like isThisType(). For example:
* The browsing history in "Search"
* The reload button in "Browser"
* The menu where you can add a link to the home screen in "Browser"

Also, I think the naming of "Search" and "Browser" has to be improved. For example:
* "Search" can become "BrowserHomepage". That's the place where you can see your history and open new windows
* "Browser" can become "BrowserWindow". You only have it when you opened a window that browses the web.
* "PrivateWindow", "BrowserPrivateWindow".

As a consequence, I propose:
> class Browser(Base):
>     name = 'Browser'
>     _browser_app_locator = (By.CSS_SELECTOR, 'div[data-manifest-name="Browser"][transition-state="opened"]')
>
>     def __init__(self, marionette):
>          Base.__init__(self, marionette)
>          self.marionette.switch_to_frame()
>          self._root_element = self.marionette.find_element(*self._browser_app_locator)
> 
>     def go_to_url(self):
>         # To define
> 
> class BrowserHomepage(Browser):
>     # Define the methods related to history and opening new tabs
> 
> class BrowserWindow(Browser):
>     # define here all the methods about browsing, saving and sharing
> 
> class BrowserPrivateWindow(BrowserWindow):
>     # redefine the locator and go_to_url (that is specific)


What do you think?

Comment 3

[Martijn Wargers (dead)]

Ok, so BrowserWindow, BrowserHomepage and BrowserPrivateWindow are the iframe and the url bar, right?
I guess there also needs to be a BrowserPrivateHomePage that inherits from BrowserHomepage then.
Perhaps BrowserStartpage and BrowsePrivateStartPage is a better name?

I'm still not sure what the difference between Browser and BrowserWindow is. I guess in your model, Browser is everything that BrowserWindow, BrowserHomepage and BrowserPrivateWindow have in common? And BrowserWindow, BrowserHomepage, BrowserPrivateWindow only contain methods that they don't have in common with each other?
I guess I would still get confused with the name BrowserWindow then.

Comment 4

[Johan Lorenzo [:jlorenzo]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #3)
> Perhaps BrowserStartpage and BrowsePrivateStartPage is a better name?
Sounds fine to me.

> I'm still not sure what the difference between Browser and BrowserWindow is.
> I guess in your model, Browser is everything that BrowserWindow,
> BrowserHomepage and BrowserPrivateWindow have in common?
That's right. They share the same type of iframe and they have the same way to tap on the URL bar, IIRC.

> And BrowserWindow, BrowserHomepage, BrowserPrivateWindow only contain methods that they don't
> have in common with each other?
That's also right. For example BrowserStartPage is the only one to have the browsing history.

> I guess I would still get confused with the name BrowserWindow then.
BrowserWindow is a window in the sense "open this link in a new window". Now that you mention it, I agree this name is ambiguous. BrowserTab might be better?

Comment 5

[Martijn Wargers (dead)]

The problem is that the Browser app can have multiple instances.
We want to be able to access every instance from Python, which is currently not the case with our api.
luanch() on a browser app can launch a Browser with the homepage or it can go back to the last opened browser instance.
Currently instantiating Search(self.marionette) points to the first browser instance.
That makes sense for normal app objects, where there's only 1 instance active every time.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] mwargers:1205262 > mozilla-b2g:master

Comment 7

[Martijn Wargers (dead)]

Comment on attachment 8673667 [details] [review]
[gaia] mwargers:1205262 > mozilla-b2g:master

Sorry, this actually totally not doing what the summary of this bug says, so far.
This is about having the ability to target every single browser window separately in Python, which currently doesn't work at all (it targets the first CSS selector).

- I had added wait_to_be_displayed/wait_to_not_be_displayed for the Browser app, because that doesn't work currently at all.
- I changed some things in cards_view.py for use in the test I added. It seems to me that a lot of methods should be part of the Card object, not of the CardsView object. Perhaps I should file a new bug on that and work on it from there?
- go_to_url on a Search object (Browser with startpage) returns a Browser object. go_to_url on a PrivateWindow returns a PrivateWindow object.
- open_new_window opens a Search window (Browser with startpage) or makes it visible again. In general, there can be only one Search window (Browser with startpage).

Some things that need to be done too:
- The browser menu shouldn't be part of the Browser object, it should be a BrowserMenu object or something and we should put the related stuff in there. Should I file a new bug on this?

Comment 8

[Martijn Wargers (dead)]

Regarding comment 2, I guess that's the way to go here.
In that case, I think we want something like this:
- BrowserBase class that contains everything that the various browser objects have in common. 
- Search(Base) -> BrowserHomePage(BrowserBase)
- Browser(Base) -> BrowserWindow(BrowserBase)
- PrivateWindow(Browser) -> PrivateBrowserWindowHomePage(BrowserBase)
- PrivateWindow(Browser) -> PrivateBrowserWindow(Browser)

We talked about this in yesterday's meeting, Johan. You mentioned something of keeping backwards compatibility for a while for other consumers, but I don't recally very well on how to do that. What did you have in mind?

Comment 9

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8673667 [details] [review]
[gaia] mwargers:1205262 > mozilla-b2g:master

These changes are coherent, modulo the items called in the PR. I think we can handle the renaming of the classes in another bug, then.


> - I changed some things in cards_view.py for use in the test I added. It
> seems to me that a lot of methods should be part of the Card object, not
> of the CardsView object. Perhaps I should file a new bug on that and work
> on it from there?
I think it makes sense to have them in Card. If we don't rename the classes in this bug, it'd be okay to delete the unecessary functions in CardsView


> - The browser menu shouldn't be part of the Browser object, it should be
> a BrowserMenu object or something and we should put the related stuff in
> there. Should I file a new bug on this?
We can handle it in the class renaming bug.

Comment 10

[Martijn Wargers (dead)]

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #9)
> > - I changed some things in cards_view.py for use in the test I added. It
> > seems to me that a lot of methods should be part of the Card object, not
> > of the CardsView object. Perhaps I should file a new bug on that and work
> > on it from there?
> I think it makes sense to have them in Card. If we don't rename the classes
> in this bug, it'd be okay to delete the unecessary functions in CardsView

I've done this work now in bug 1217485.

Comment 11

[GitHub Autolander Bot]

Attachment: [gaia] mwargers:1205262_v2 > mozilla-b2g:master

Comment 12

[Martijn Wargers (dead)]

Comment on attachment 8681556 [details] [review]
[gaia] mwargers:1205262_v2 > mozilla-b2g:master

Johan, what do you think of this? Combining Search, Browser, PrivateWindow in one file and having one common BrowserBase class?

Comment 13

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8681556 [details] [review]
[gaia] mwargers:1205262_v2 > mozilla-b2g:master

The layout of the classes is good. We need to make their naming a bit more explicit. In the current state, it's hard to guess what's the difference between:
* BrowserBase
* Search
* Browser
* PrivateWindow
* SearchPanel

Comment 14

[Martijn Wargers (dead)]

Yes, that is what bug 1215351 is about. The problem is that it will cause problems for other consumers of Gaia UI test. They will have to update their code as well.
The solution for that is mentioned in bug 1215351, comment 1.

Comment 15

[Martijn Wargers (dead)]

Comment on attachment 8681556 [details] [review]
[gaia] mwargers:1205262_v2 > mozilla-b2g:master

Ok, let me know what you think of this.
This should still be backwards compatible with what we currently have.
Or in the spirit of what you mentioned in bug 1215351, comment 1, I should already start renaming the stuff and copy and leave the old things as we have?

Reminder to myself:
- The browser menu dialog could have its own PageRegion.
- The first pull request in this bug, https://bugzilla.mozilla.org/attachment.cgi?id=8673667 , has some useful review comments from Johan about cards_view, that I still could address.

Comment 16

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8681556 [details] [review]
[gaia] mwargers:1205262_v2 > mozilla-b2g:master

The changes look good to me. Thanks for reminding me bug 1215351. I proposed some ways reduce the length of the test, so it'll be easier to read. I won't block on them, though.

Comment 17

[Martijn Wargers (dead)]

Gaia UI tests are becoming obsolete, so marking WONTFIX.