Bug 1123401 - Implement handling of chrome windows #50
Conversation
|
|
||
| :returns: Platform name | ||
| """ | ||
| return self.marionette.session_capabilities['platformName'].lower() |
There was a problem hiding this comment.
I think this is the sort of thing that should go in FirefoxTestCase.
There was a problem hiding this comment.
I thought about this first too but abandoned this idea because it would not be available for sub modules in that package. Something I would like to have is that modules would be able to access API classes and properties like those. Therefor the puppeteer modules need to be refactored so it only holds API level stuff. I would like to do that later.
|
A commit for the review comments and fixes for functional tests have been pushed. The remaining todos will get their own commits. |
| # when the tabs module gets implemented | ||
| def find_by_url(win): | ||
| with win.marionette.using_context('content'): | ||
| return win.marionette.get_url() == url |
There was a problem hiding this comment.
Who is win.marionette? This is confusing, there's is only one marionette.
There was a problem hiding this comment.
The callbacks have a parameter, which is the actual BaseWindow instance to operate on. Keep in mind that switch_to() iterates over all the existent chrome windows. Once we have the tabs module it will most likely look like win.tabbar.location == url. But we might wanna find a better test here.
There was a problem hiding this comment.
I just mean that there's only one marionette, and it only relates to one browser state at a point in time. So we're saying win.marionette, but it's calling the same method as if we'd said self.marionette, but it sort of implies that we meaningfully have a marionette per window.
There was a problem hiding this comment.
Please see my example above how it will look like later. You won't see that we call something via marionette in the future here. This is just to ensure we keep the right context and do not switch between global and local scope. Exactly that would be confusing.
| "top level browsing contexts, but ended with %s." % | ||
| (self._start_handle_count, win_count)) | ||
| finally: | ||
| MarionetteTestCase.tearDown(self, *args, **kwargs) |
There was a problem hiding this comment.
I support this finally block.
|
@chmanchester I pushed a new commit with all the remaining fixes from your last review. Can you please have a look if that is fine? If yes, lets get it squashed and landed. |
|
Go ahead and merge it. We can leave anything else to a follow up. |
Just for a sneak peak. This PR is not ready yet and needs some more methods and cleanups. But if you have time lets give me feedback.