742947 - Integrate selenium atoms with marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Malini Das [:mdas] - Away, not checking bugmail]

Selenium maintains a set of code called "atoms" which are optimized, compressed bits of JS code designed for specific Selenium calls (click, sendKeys, etc.). We should use these atoms where possible since the Selenium team maintains this code and there would be unnecessary overlap if we implement the exact same code on our end.

Comment 1

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: Integrate the selenium atoms and add related functionality

This implements the remaining methods from https://wiki.mozilla.org/Auto-tools/Projects/Marionette/Marionette_Client_API#HTMLElement_Methods

Comment 2

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: Test cases

Here are the related test cases. I had to add a xul document that we can load in order to test some of the chrome methods.

Also getElementAttribute changed to getAttributeValue in the WebDriver docs, so I updated the client.

Comment 3

[Jonathan Griffin (:jgriffin)]

Comment on attachment 612705 [details] [diff] [review]
Integrate the selenium atoms and add related functionality

Review of attachment 612705 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a few minor notes:

::: testing/marionette/marionette-actors.js
@@ +301,5 @@
> +      lines.push(el.value);
> +    }
> +    else if (el["label"]) {
> +      lines.push(el.label);
> +    }

Should also probably include the value of 'label' elements, as well as the text content of any text nodes (you can have text nodes inside either <label> or <description> elements).

@@ +940,5 @@
> +    if (this.context == "chrome") {
> +      try {
> +        //Selenium atom doesn't quite work here
> +        let el = this.curBrowser.elementManager.getKnownElement(aRequest.element, this.getCurrentWindow());
> +        if (el.checked != undefined) {

We should probably be checking either the selected or checked attributes, as appropriate.

@@ +969,5 @@
> +    if (this.context == "chrome") {
> +      try {
> +        let el = this.curBrowser.elementManager.getKnownElement(aRequest.element, this.getCurrentWindow());
> +        el.focus();
> +        utils.synthesizeKey(aRequest.value, { type: "keypress" });

EventUtils.synthesizeKey is used for sending a single key, but this API, "sendKeysToElement", requires potentially sending multiple keys.  I think we probably want to call sendString here instead.

Comment 4

[Jonathan Griffin (:jgriffin)]

Comment on attachment 612706 [details] [diff] [review]
Test cases

Review of attachment 612706 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the new tests!  

There are a couple of minor issues here.  One is that a few tests use this line of code:

>        self.marionette.execute_script("window.open('file:///Users/mdas/code/malini_marionette_client/marionette/www/test.xul', '_blank', 'chrome,centerscreen');")

Can we use get_absolute_url to construct these url's?  If that doesn't work for these xul files, then we'll need to construct a path using Python.

Second issue is that a few chrome tests verify things about the current XUL structure of Firefox, like:

>	def test_child_elements(self):
>		el = self.marionette.find_element("id", "bundle_shell")
>		parent = self.marionette.find_element("id", "stringbundleset")
>		found_els = parent.find_elements("tag name", "stringbundle")
>		self.assertTrue(el.id in [found_el.id for found_el in found_els])

This seems potentially fragile; it would probably be better to use your test.xul file to do these tests.

Comment 5

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: Integrate the selenium atoms and add related functionality v0.2

Updated with your comments

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: Test cases v0.2

Accessing the .xul file using http didn't quite work, so I had to get the file:// based url, so I ended up having to use os.path.

Also updated the dependencies on browser.xul, thanks for finding these issues!

Comment 7

[Jonathan Griffin (:jgriffin)]

I have one more request.  For the sake of future maintenance, can we get a list of atoms that we're bundling put into some text file, e.g., atoms/HOWTO?  I found it was actually difficult to figure this out, since the minified version of the atoms is so unreadable.

Comment 8

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: Integrate the selenium atoms and add related functionality v0.3

Sure, good point, I have them as comments in the atoms file, but adding them to the HOWTO would be a very helpful addition :)

I also noticed that last patch had leaked a change in browser/confvars.sh so I removed that.

Comment 9

[Jonathan Griffin (:jgriffin)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d384f54707cc and 
http://hg.mozilla.org/integration/mozilla-inbound/rev/65f02c832a0e

Comment 10

[Jonathan Griffin (:jgriffin)]

Backed out 65f02c832a0e due to b2g build bustage:

http://hg.mozilla.org/integration/mozilla-inbound/rev/acb53b7241c5

Comment 11

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: Integrate the selenium atoms and add related functionality v0.4

Added the modified ChromeUtils.js and EventUtils.js files to testing/marionette.

Tested on a purged m-c repository with no problems.

Comment 12

[Jonathan Griffin (:jgriffin)]

Re-landed as http://hg.mozilla.org/integration/mozilla-inbound/rev/731f7b016723

Comment 13

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/d384f54707cc
https://hg.mozilla.org/mozilla-central/rev/731f7b016723