1083983 - marionette findElement and findElements shouldn't duplicate logic

Status: RESOLVED WONTFIX

(Remote Protocol :: Marionette, defect)

Description

[Andrew Halberstadt [:ahal]]

In marionette-elements.js there are two separate methods, findElement and findElements that do almost the exact same thing:
http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-elements.js#294

We should be able to remove findElement() and change the above to:

found = findElements(...);
if (!all) {
  found = found[0];
}

I'll test this out in case there's something tricky going on.

Aside: I'd make the argument that marionette server shouldn't even expose 'findElement' as clients can easily implement that using 'findElements'. Though I may be missing historical context.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: remove_find_element_duplication

This patch passed all marionette unittests locally. Here's a try run to be safe:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f574c3f92a5f

Comment 2

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8506385 [details] [diff] [review]
remove_find_element_duplication

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

(In reply to Andrew Halberstadt [:ahal] from comment #0)
> Aside: I'd make the argument that marionette server shouldn't even expose
> 'findElement' as clients can easily implement that using 'findElements'.
> Though I may be missing historical context.

The mechanisms for element lookup of single and multiple elements differ.  WebDriver should generally map down to the closest equivalent for each selector in the browser.  So document.querySelector for a single element by CSS, document.querySelectorAll for a sequence of elements, &c.

Your patch always uses multiple path and then extracts the first element in the sequence.  This has dramatic performance effects on moderately large documents.  Calling console.time("all"); document.querySelectorAll("p")[0]; console.timeEnd("all"); on the HTML5 spec document is ~200x slower than using document.querySelector("p").

Comment 3

[Andrew Halberstadt [:ahal]]

Alright, I guess I was thrown off by some of the methods like CLASS_NAME and TAG that do query all elements and return the first item already. Probably not worth updating just those, so WONTFIX'ing this.