639870 - Implementation of a window handler

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

For the API refactor we need a way to handle multiple windows. This should be handled by the driver module, so we can simply replace it later once Mozmill also contains this feature.

The question now is how the API has to look like so we can make this transition as easy as possible. I would assume we want to have different methods/classes for modal and normal windows.

Clint and Andrew, have you already made some thoughts about it?

Right now we have the modal dialog class and the handleWindow method inside the utils module. The latter only supports the front-most window at the moment and has to be extended.

Comment 1

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Moving this to block the milestone. Anything listed as a primary milestone task on the wiki can go straight on the milestone. Refactors should parent to-do stuff that aren't in the main plan.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I have shortly spoken with Clint yesterday, and we hashed out a raw concept what would be wanted for Mozmill. For now I would like to see that we are using the same interface in our API refactoring project. Reason is that I do not want to have to update all the tests later again. The version which will be implemented by this bug is nearly only a transition over from the existing modalDialog class and the handleWindow function in our current shared modules.

Here some flashed out ideas:

1. For the tests it should be mostly transparent if a window/dialog is modal or non-modal. In both cases we should have the same interface to classes which handle the given window. Only via the constructor and/or a property you can select between a modal and a non-modal window handler.

2. The handling of multiple windows should probably be a separate method on the driver class. It will return a list of wrapped nsIDOMWindow instances, and the tests will have to figure out which window to use. We can't make Mozmill that smart to handle all cases. 

3. The wrapped window class is a base class which can be used for all windows. You can subclass it to match the window type you expect, i.e. BrowserWindow, PageInfoWindow, and others. Those sub-classed window objects are not part of Mozmill but our API around it and contain the static element map on which Geo is working on. 


In general this bug only takes care about 1). Everything else is provided by Mozmill or Geo's work on other bugs.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The work which involved here I have to cut down in different iterations which I will work on step by step:

1. Create methods to handle windows

2. Create the wrapper class for DOMWindows and update the code from 1) to make it use the new wrapper class

3. Port the existing code for handling non-modal and modal windows

At the moment I'm working on 1) and already have some really helpful methods which even work with multiple windows involved. I will have something to show off later today.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #3)
> 1. Create methods to handle windows

This should have been called "find" instead of "handle".

Everything we would need for this part should now be covered by:
https://github.com/geoelectric/mozmill-api-refactor/blob/window_handler/modules/windows.js

That means you can do the following:

// List of windows sorted by age (newest first):
windows.getLastOpenedWindows();

// List of windows sorted by z order (top most first)
windows.getMostRecentWindows();

// Last opened window
win = windows.getLastOpenedWindow();

// Last opened page info window
win = windows.getLastOpenedWindow(windows.filterByType, "Browser:page-info");

// Top most page info window
win = windows.getMostRecentWindow(windows.filterByType, "Browser:page-info");

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

First implementation of the window wrapper and handling of non-modal windows. It's works that's all. It will need more thinking and I will continue latest next week after some discussion how we can integrate the static map, which isn't that trivial currently.

https://github.com/geoelectric/mozmill-api-refactor/commit/6baba290054e633a4512bc034893bcf17e2b7186

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Window wrapper and handling v1 [checked-in]

This patch brings the basics for handling windows and includes:

* Methods to get a list of windows in a given order
* Methods to find a specific window
* The window wrapper class
* Mixin capabilities
* And various other stuff for our own API

Comment 7

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 529184 [details] [diff] [review]
Window wrapper and handling v1 [checked-in]

Looks good. Let's go ahead and get it in, and iron out any API usability issues once we've had a chance to exercise it a little. r+

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 529184 [details] [diff] [review]
Window wrapper and handling v1 [checked-in]

Landed on master (also I forgot to squash first):
https://github.com/geoelectric/mozmill-api-refactor/commit/d5da2ebbd4a1c66c7a103ffb5cafbd25b924c218
https://github.com/geoelectric/mozmill-api-refactor/commit/16e8e3bbaa65b0d33d40e07689e3b190e175f9fd
https://github.com/geoelectric/mozmill-api-refactor/commit/0af5b0629e5d3cefede48e6cae6260f2b5153a06
https://github.com/geoelectric/mozmill-api-refactor/commit/b223274a84ded549919381e789ec01a2e69faba9

With the next patch I will follow-up now with the handling of modal dialogs.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: WIP (modal dialog handler)

This WIP includes the following improvements:

* New error classes for invalid parameters and unexpected situations
* Reverts keyPress to keypress
* Adds classes for handling modal dialogs
* Improvements to the window wrapper
* Documentation

Still todo:
* Have to add a deinit module which safely resets a wrapped window
* Add tests for the new API

I hope to have a finalized patch ready by tomorrow.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

And an important bit I have missed in my last comment. I have also added a default handler for modal dialogs, which makes sure that we do not have hangs in our test runs because a modal dialog opened unexpectedly. Each type of window will have this handler set by default.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Modal dialog handler + general improvements v1 [checked-in]

Ok, so this is now the final version of the patch, which includes a way more improvements as the WIP. Mostly to also handle the teardown function correctly. I know that was part of Milestone 3 but it was necessary now to correctly clean-up the browser. Therefore I have moved the init.js module to head.js, which corresponds to the way how Mochitests include the general init/deinit module.

Here some overall details:
* We have two new error classes for unexpected behavior and invalid parameters

* Tests have to call head.setup(module) inside the setupModule function. I have removed the 'Test' suffix because I don't think that we will ever have a equivalent function for ui modules. But if that's the case those can use an init function like setupModule.

* Tests have to call head.teardown(module) inside the teardownModule function. It's necessary to clean-up the browser from the running modal dialog observer. In the future there will be more items we can include.

* New tests have been added for handling of windows and modal dialogs. Existing tests have been updated for the above mentioned changes.

Comment 12

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 533109 [details] [diff] [review]
Modal dialog handler + general improvements v1 [checked-in]

Only had a chance to skim over it so far, will complete review tomorrow.

I get why you changed from init, since you put teardown logic in. Why "head" though? That's not a meaningful term to me in context like init was. Does it mean something I'm not getting here?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #12)
> I get why you changed from init, since you put teardown logic in. Why "head"
> though? That's not a meaningful term to me in context like init was. Does it
> mean something I'm not getting here?

As said I have used it for now the same way as how it is used in Mochitest. If you have a better idea for a name, I'm not reluctant to change it. It's just a search and replace, which we can do at any time. 'head' as probably short-term for heading means to include some functionality at the top/beginning of a test. Same what you would likely do with a template engine when building up web pages. But anyway, I do not see a blocker in it right now. If we don't like it we can change it kinda easily.

Comment 14

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 533109 [details] [diff] [review]
Modal dialog handler + general improvements v1 [checked-in]

No, not a blocker at all, I was just curious. r+

Re: the name, if it's consistent with Mochitest for the same operations, that's good enough for me. If it's not, I'd like to eventually find something more descriptive.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 533109 [details] [diff] [review]
Modal dialog handler + general improvements v1 [checked-in]

Landed as:
https://github.com/geoelectric/mozmill-api-refactor/commit/aeaeeb0e32d5bb43f4bddba000e7eadc5969e605

I will now hop onto the last part of this bug and work on the in-content window handler.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Rename WindowWrapper to ChromeWindowWrapper [checked-in]

As talked with Geo earlier this week the current class name is not ideal. We should rename it to a more obvious name, which visualizes what it represents. Due to the fact that we will have a class like ContentWindowWrapper we should rename WindowWrapper to ChromeWindowWrapper. This patch makes this change.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 536753 [details] [diff] [review]
Rename WindowWrapper to ChromeWindowWrapper [checked-in]

As talked with Geo via email, he is ok with that change.

Landed as:
https://github.com/geoelectric/mozmill-api-refactor/commit/cac8ff9afa33970fa735efe9fc1dc1ed9d1bf58a

Comment 18

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Can this be closed out? I feel like we have the basics down.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We still need the in-content class and the basic window class. So I kinda would like to cover that by this bug.

Comment 20

[u415141]

Henrik Skupin deleted the linked story in Pivotal Tracker

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Base WindowWrapper class v1 [checked-in]

This patch implements a base class called WindowWrapper. The name is similar to what we had before but is now necessary so we can share code for chrome and in-content windows.

The following code has not been moved to the base class:

* The handling of modal dialogs always happens in the chrome window class at the moment. It could be that it should be handled by in-content windows but really, that's something I will have to investigate on bug 673399. So I don't want to move this code around for the time being.

* The title and type properties are only useful for chrome windows. They are not used for in-content windows

* findWindow/handleWindow is not needed for in-content windows, because their window is known given by the defaultView of the tab document or the frame.

All tests except the known broken ones are passing.

Comment 22

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 558554 [details] [diff] [review]
Base WindowWrapper class v1 [checked-in]

Looks good. Hard to review a refactor patch like this comprehensively, but I'm pretty confident we'll catch any problems as you do the in-content part. r+, let's land it.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 558554 [details] [diff] [review]
Base WindowWrapper class v1 [checked-in]

Landed as:
https://github.com/geoelectric/mozmill-api-refactor/commit/8248658bcc41294d5e10f7bf80ae56d2a27a7c4e

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Content window handler [checked-in]

Implementation for the content window wrapper class, which allows to also create a ui map for elements in content (tabs, frames, and other objects). Keep in mind that you must have installed the latest Mozmill 1.5.5 dev version to successfully run this test.

Comment 25

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 561126 [details] [diff] [review]
Content window handler [checked-in]

Minor:
	
> function WindowWrapper_getContentWindow(aWindow, aWindowClass) {
>   if (typeof(aWindowClass) === 'undefined')

I think I made this mistake elsewhere, but typeof is a unary operator and not a function:

  if (typeof aWindowClass) === 'undefined')

> function MozillaSiteWindow(aWindow) {
>   dump("**" + " constructor" + "\n");

Probably should remove the dump() statement, or at least comment it out as a debug-only thing. I don't really care much since this is a unit test though.

Otherwise, looks fine, r+. The typeof touchup would be nice before you land.

Comment 26

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Oops, typoed above and left unbalanced parentheses. Meant:

if (typeof aWindowClass === 'undefined')

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed as:
https://github.com/geoelectric/mozmill-api-refactor/commit/905e796f612d901360a1f027b51ccb58c2364272

Remaining will happen on other bugs which I will mark as dependencies now.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We never finished that project. Closing bug.