Andreas Tolfsen ❲:ato❳ Assignee
	Description • 7 years ago

In Marionette it would be useful to represent web elements in a WebElement abstraction so that specific functionality could be associated with it.

What prompted me to think about this is https://bugzilla.mozilla.org/show_bug.cgi?id=1400225 which is going to introduce a WebElementEventTarget type that would normally be an extension of a WebElement type to match Element and EventTarget in the web platform API.

Having a WebElement abstraction would allow adding event listeners directly on web element references in chrome:

> let webElement = await this.listener.findElement("window", …);
> await new Promise(resolve => {
>   webElement.addEventListener("visibilitychange", resolve, {once: true});
>   // calls WebElementEventTarget.prototype.addEventListener
> });

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8915663 [details]
Bug 1400256 - Add missing break statement to find_.

https://reviewboard.mozilla.org/r/186842/#review192140

I don't really see that as necessary because default is always the last case, and also our linter never complains about.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8915664 [details]
Bug 1400256 - Fix element.findByLinkText docs.

https://reviewboard.mozilla.org/r/186844/#review192142

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8915665 [details]
Bug 1400256 - Return single anon element from WebDriver:FindElement.

https://reviewboard.mozilla.org/r/186846/#review192144

::: commit-message-8187c:5
(Diff revision 1)
> +Bug 1400256 - Return single anon element from WebDriver:FindElements. r?whimboo
> +
> +The WebDriver:FindElements command returned an array of elements when
> +looking up anonymous elements.  This patch rectifies the behaviour
> +so that only a single element is returned.

I assume you mean `WebDriver::FindElement` here. FindElements should still return an array.

::: commit-message-8187c:12
(Diff revision 1)
> +It introduces a new helper function called element.findAnonymousNodes,
> +akin to similar helper functions for other strategies.  This function
> +returns an iterator of anonymous nodes so that WebDriver:FindElements
> +(plural) and WebDriver:FindElement (singular) can share the same
> +code path.
> +

So it means searching for anon nodes would behave differently then all the other strategies?

::: testing/marionette/element.js:392
(Diff revision 1)
>  /**
> + * Find anonymous nodes of <var>node</var>.
> + *
> + * @param {XULElement} rootNode
> + *     Root node of the document.
> + * @param {XULELement} node

nit: XULElement

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 23 • 7 years ago
mozreview-review

Comment on attachment 8915665 [details]
Bug 1400256 - Return single anon element from WebDriver:FindElement.

https://reviewboard.mozilla.org/r/186846/#review192148

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 24 • 7 years ago
mozreview-review

Comment on attachment 8915666 [details]
Bug 1400256 - Bind this.resetValues after performing legacy action chain.

https://reviewboard.mozilla.org/r/186848/#review192150

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 25 • 7 years ago
mozreview-review

Comment on attachment 8915667 [details]
Bug 1400256 - Document element.isCollection.

https://reviewboard.mozilla.org/r/186850/#review192152

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 26 • 7 years ago
mozreview-review

Comment on attachment 8915668 [details]
Bug 1400256 - Make element.isXULElement more resiliant.

https://reviewboard.mozilla.org/r/186852/#review192154

r=me+wc

::: testing/marionette/element.js:972
(Diff revision 1)
>  };
>  
> +/**
> + * Ascertains whether <var>el</var> is a XUL element.
> + *
> + * @param {XULElement} el

It's not required to only allow XULElements. In such a case this method would be superfluous.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 27 • 7 years ago
mozreview-review

Comment on attachment 8915672 [details]
Bug 1400256 - Add test for element.isXULElement.

https://reviewboard.mozilla.org/r/186860/#review192156

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 28 • 7 years ago
mozreview-review

Comment on attachment 8915671 [details]
Bug 1400256 - Add element.isDOMWindow.

https://reviewboard.mozilla.org/r/186858/#review192158

::: testing/marionette/element.js:991
(Diff revision 1)
>  };
>  
> +/**
> + * Ascertains whether <var>node</var> is a <code>WindowProxy</code>.
> + *
> + * @param {WindowProxy} node

This can be any kind of node. Otherwise this method would also be superfluous.

::: testing/marionette/element.js:998
(Diff revision 1)
> + *
> + * @return {boolean}
> + *     True if <var>node</var> is a DOM window.
> + */
> +element.isDOMWindow = function(node) {
> +  let t = Object.prototype.toString.call(node);

Doesn't that give a linter failure now given that t is not used? Can't you remove that line at all?

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 29 • 7 years ago
mozreview-review

Comment on attachment 8915670 [details]
Bug 1400256 - Add element.isDOMElement.

https://reviewboard.mozilla.org/r/186856/#review192160

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 30 • 7 years ago
mozreview-review

Comment on attachment 8915674 [details]
Bug 1400256 - Use WebElement.generateUUID to make session ID.

https://reviewboard.mozilla.org/r/186864/#review192162

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 31 • 7 years ago
mozreview-review

Comment on attachment 8915675 [details]
Bug 1400256 - Serialise IPC messages with evaluate.toJSON.

https://reviewboard.mozilla.org/r/186866/#review192164

::: testing/marionette/proxy.js:151
(Diff revision 1)
>          }
>        };
>  
>        // The currently selected tab or window has been closed. No clean-up
>        // is necessary to do because all loaded listeners are gone.
> -      this.closeHandler = event => {
> +      this.closeHandler = ({type, target}) => {

Personally I'm not such a fan of doing all that in the parameter list. It makes it harder to read, and you cannot easily search for listeners by using 'event'. But well...

::: testing/marionette/proxy.js
(Diff revision 1)
> -    let payload;
> -    if (data && typeof data.toJSON == "function") {
> +    let data = evaluate.toJSON(payload);
> +    const msg = {type, data};
> -      payload = data.toJSON();
> -    } else {
> -      payload = data;
> -    }

That clarifies the misuse of "payload", which was confusing me in the past multiple times. Thanks.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 32 • 7 years ago
mozreview-review

Comment on attachment 8915676 [details]
Bug 1400256 - Drop unused arguments to evaluate.toJSON/fromJSON.

https://reviewboard.mozilla.org/r/186868/#review192166

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 33 • 7 years ago
mozreview-review

Comment on attachment 8915679 [details]
Bug 1400256 - Recognise web element references in evaluate.toJSON.

https://reviewboard.mozilla.org/r/186874/#review192168

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 34 • 7 years ago
mozreview-review

Comment on attachment 8915669 [details]
Bug 1400256 - Add web element abstractions.

https://reviewboard.mozilla.org/r/186854/#review192170

I only skimmed the patch because the try build shows a lot of failures. Please make sure to get those fixed first, before requesting another review. Thanks.

::: testing/marionette/element.js:1077
(Diff revision 1)
> +   * @return {boolean}
> +   *     True if this and <var>other</var> are the same.  False
> +   *     otherwise.
> +   */
> +  is(other) {
> +    return this instanceof WebElement && this.uuid === other.uuid;

why do we need the instance check for this? Shouldn't this be done for other?

::: testing/marionette/element.js:1104
(Diff revision 1)
> +    const uuid = WebElement.generateUUID();
> +
> +    if (element.isDOMElement(node)) {
> +      return new ContentWebElement(uuid);
> +    } else if (element.isDOMWindow(node)) {
> +      if (node.opener === null) {

I'm skeptical that we can use the opener to make a decision if the element is a frame or window. Opener is a reference to a window, which opened the current window. So why do frames have an opener?

::: testing/marionette/element.js:1113
(Diff revision 1)
> +    } else if (element.isXULElement(node)) {
> +      return new ChromeWebElement(uuid);
> +    }
> +
> +    throw new TypeError("Expected DOM window/element " +
> +        pprint`or XUL element, got: ${node}`);

Which details of noe does pprint output here?

::: testing/marionette/element.js:1190
(Diff revision 1)
> +   */
> +  static fromUUID(uuid, context) {
> +    assert.string(uuid);
> +
> +    switch (context) {
> +      case "chrome":

Please use the name of the constant. Same for content below

::: testing/marionette/element.js:1308
(Diff revision 1)
> +    }
> +    let uuid = json[ContentWebFrame.Identifier];
> +    return new ContentWebFrame(uuid);
> +  }
> +}
> +ContentWebFrame.Identifier = "frame-075b-4da1-b6ba-e579c2d3230a";

I assume there is no legacy identifier as for elements? Is that something we would need in webdriver_rust too?

::: testing/marionette/element.js:1332
(Diff revision 1)
> +    }
> +    let uuid = json[ChromeWebElement.Identifier];
> +    return new ChromeWebElement(uuid);
> +  }
> +}
> +ChromeWebElement.Identifier = "xulelement-9fc5-4b51-a3c8-01716eedeb04";

Same question as above for webdriver_rust.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 35 • 7 years ago
mozreview-review

Comment on attachment 8915673 [details]
Bug 1400256 - Remove element.isWebElementReference.

https://reviewboard.mozilla.org/r/186862/#review192182

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 36 • 7 years ago
mozreview-review

Comment on attachment 8915677 [details]
Bug 1400256 - Make element.Store work with web elements.

https://reviewboard.mozilla.org/r/186870/#review192184

I will wait reviewing this until the WebElement implementation patch can be fully reviewed.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 37 • 7 years ago
mozreview-review

Comment on attachment 8915678 [details]
Bug 1400256 - Use WebElement for marshaling web elements in evaluate.fromJSON.

https://reviewboard.mozilla.org/r/186872/#review192188

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 38 • 7 years ago
mozreview-review

Comment on attachment 8915680 [details]
Bug 1400256 - Marshal IPC messages to and from frame script.

https://reviewboard.mozilla.org/r/186876/#review192192

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 39 • 7 years ago
mozreview-review

Comment on attachment 8915681 [details]
Bug 1400256 - Make IPC messages work with implicitly marshaled elements.

https://reviewboard.mozilla.org/r/186878/#review192194

::: testing/marionette/driver.js:1736
(Diff revision 1)
>  
>    let {id, element, focus} = cmd.parameters;
>  
> +  let webEl;
> +  if (typeof element != "undefined") {
> +    webEl = WebElement.fromUUID(element, Context.CHROME);

Why are we forcing always chrome here?

Also please move this whole block down to where it is actually used.

::: testing/marionette/driver.js:2185
(Diff revision 1)
>   */
>  GeckoDriver.prototype.clickElement = async function(cmd) {
>    assert.window(this.getCurrentWindow());
>    assert.noUserPrompt(this.dialog);
>  
> -  let id = cmd.parameters.id;
> +  let webEl = WebElement.fromUUID(cmd.parameters.id, this.context);

We don't check for a valid id. Can you please add an assertion and update the documentation?

::: testing/marionette/driver.js:2245
(Diff revision 1)
>  GeckoDriver.prototype.getElementAttribute = async function(cmd, resp) {
>    assert.window(this.getCurrentWindow());
>    assert.noUserPrompt(this.dialog);
>  
>    let {id, name} = cmd.parameters;
> +  let webEl = WebElement.fromUUID(id, this.context);

We don't assert for a valid id here too. This also applies to following methods in this file.

::: testing/marionette/driver.js:2382
(Diff revision 1)
>  
> -  let id = cmd.parameters.id;
> -
>    switch (this.context) {
>      case Context.CHROME:
> -      let el = this.curBrowser.seenEls.get(id);
> +      let el = this.curBrowser.seenEls.get(webEl);

I think you missed to paste the code for generating the webEl above. Instead it' only in content with a hard-coded context.

::: testing/marionette/driver.js:2549
(Diff revision 1)
>    assert.window(this.getCurrentWindow());
>    assert.noUserPrompt(this.dialog);
>  
>    let {id, text} = cmd.parameters;
> +  let webEl = WebElement.fromUUID(id, this.context);
>    assert.string(text);

We should assert first.

::: testing/marionette/driver.js:3383
(Diff revision 1)
>          // we don't arbitrarily save previousFrameElement, since
>          // we allow frame switching after modals appear, which would
>          // override this value and we'd lose our reference
>          if (message.json.storePrevious) {
> -          this.previousFrameElement = this.currentFrameElement;
> +          this.previousFrameElement =
> +              new ChromeWebElement(this.currentFrameElement);

Why a ChromeWebElement?

::: testing/marionette/driver.js:3390
(Diff revision 1)
> +        if (message.json.frameValue) {
> +          this.currentFrameElement =
> +              new ChromeWebElement(message.json.frameValue);
> +        } else {
> +          // null
> -        this.currentFrameElement = message.json.frameValue;
> +          this.currentFrameElement = message.json.frameValue;

Just assign null for clarity.

::: testing/marionette/legacyaction.js:209
(Diff revision 1)
>        event.sendKeyUp(pack[1], keyModifiers, this.container.frame);
>        this.actions(chain, touchId, i, keyModifiers, cb);
>        break;
>  
>      case "click":
> -      el = this.seenEls.get(pack[1]);
> +      webEl = WebElement.fromUUID(pack[1], "content");

I assume that for legacyActions we only want to support content?

::: testing/marionette/listener.js:783
(Diff revision 1)
>  
>  async function executeInSandbox(script, args, timeout, opts) {
>    opts.timeout = timeout;
> -
>    let sb = sandboxes.get(opts.sandboxName, opts.newSandbox);
> -  let wargs = evaluate.fromJSON(
> +  return await evaluate.sandbox(sb, script, args, opts);

You should remove await here similar to execute.

::: testing/marionette/listener.js
(Diff revision 1)
>    }
>  
>    let el = await element.find(curContainer, strategy, selector, opts);
> -  let elRef = seenEls.add(el);
> +  return seenEls.add(el);
> -  let webEl = element.makeWebElement(elRef);
> -  return webEl;

We should return a WebElement here too. Maybe there was a problem in rebasing the patch.

::: testing/marionette/listener.js:1333
(Diff revision 1)
>    } catch (e) {
>      sendError(e, commandID);
>    }
>  }
>  
> -function getElementAttribute(id, name) {
> +function getElementAttribute(el, name) {

If you find the time to add documentation for those two methods would be fine.

::: testing/marionette/listener.js
(Diff revision 1)
>   * web element.
> - *
> - * @param {String} id
> - *     Web element reference.
> - * @param {String} prop
> - *     The CSS property to get.

You don't want to keep the parameters anywhere?

::: testing/marionette/listener.js:1475
(Diff revision 1)
>      }
>      return;
>    }
>  
>    let foundShadowRoot;
> -  let hostEl = seenEls.get(id);
> +  foundShadowRoot = el.shadowRoot;

Combine both lines in case we need this extra variable at all.

::: testing/marionette/listener.js:1679
(Diff revision 1)
>  
> -  let highlightEls = highlights.map(ref => seenEls.get(ref));
> -
>    let canvas;
> +  let highlightEls = highlights
> +      .map(ref => WebElement.fromUUID(ref, "content"))

Please use Context.Content in both cases.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 40 • 7 years ago
mozreview-review-reply

Comment on attachment 8915665 [details]
Bug 1400256 - Return single anon element from WebDriver:FindElement.

https://reviewboard.mozilla.org/r/186846/#review192144

> I assume you mean `WebDriver::FindElement` here. FindElements should still return an array.

Fixed.

> So it means searching for anon nodes would behave differently then all the other strategies?

Yes, other helper functions return arrays which is a bit pointless.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 41 • 7 years ago
mozreview-review-reply

Comment on attachment 8915665 [details]
Bug 1400256 - Return single anon element from WebDriver:FindElement.

https://reviewboard.mozilla.org/r/186846/#review192144

> nit: XULElement

Fixed.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 42 • 7 years ago
mozreview-review-reply

Comment on attachment 8915668 [details]
Bug 1400256 - Make element.isXULElement more resiliant.

https://reviewboard.mozilla.org/r/186852/#review192154

> It's not required to only allow XULElements. In such a case this method would be superfluous.

Not sure what you’re saying here.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 43 • 7 years ago
mozreview-review-reply

Comment on attachment 8915669 [details]
Bug 1400256 - Add web element abstractions.

https://reviewboard.mozilla.org/r/186854/#review192170

> why do we need the instance check for this? Shouldn't this be done for other?

Keenly spotted!  This is meant to be "other".

> I'm skeptical that we can use the opener to make a decision if the element is a frame or window. Opener is a reference to a window, which opened the current window. So why do frames have an opener?

Right, this is meant to be window.parent !== window.

> Which details of noe does pprint output here?

You can check pprint in testing/marionette/error.js:

    https://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/testing/marionette/error.js#151-179

OTOH for DOM elements it will output a prettified HTML tag
with IDs and classes, and for objects it will output the
Object.prototype.toString name of node, unless it has a custom
toJSON function.

> Please use the name of the constant. Same for content below

We can’t do that because the Context constant is defined in
testing/marionette/driver.js which would cause a circular dependency
between the two files.

> I assume there is no legacy identifier as for elements? Is that something we would need in webdriver_rust too?

Actually, when serialised, I always include the LegacyIdentifier so
that this will be backwards compatible with existing clients:

> class ContentWebFrame extends WebElement {
>   toJSON() {
>     return {
>       [ContentWebFrame.Identifier]: this.uuid,
>       [ContentWebElement.LegacyIdentifier]: this.uuid,
>     };
> }

This will need a follow-up change for clients to pick up the right
identifiers.

> Same question as above for webdriver_rust.

And same answer as above.  Dropping this issue so we can track the
conversation in the other.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 44 • 7 years ago
mozreview-review-reply

Comment on attachment 8915671 [details]
Bug 1400256 - Add element.isDOMWindow.

https://reviewboard.mozilla.org/r/186858/#review192158

> This can be any kind of node. Otherwise this method would also be superfluous.

Do you mean you want it annotated with "*"?

> Doesn't that give a linter failure now given that t is not used? Can't you remove that line at all?

It does now when this changeset is rebased on m-c which contains the
lint patch for unused variables.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 45 • 7 years ago
mozreview-review-reply

Comment on attachment 8915681 [details]
Bug 1400256 - Make IPC messages work with implicitly marshaled elements.

https://reviewboard.mozilla.org/r/186878/#review192194

> Why are we forcing always chrome here?
> 
> Also please move this whole block down to where it is actually used.

Because WebElement.fromUUID is a workaround for when the provided
element ID doesn’t contain any information about what type of
reference it is.  If all was well we would receive a JSON Object
{"xulelement-xxx": "<uuid>"} which would tell us what type to
derive, but here we receive {element: <uuid>}.  Because we know
the web element passed to WebDriver:SwitchToFrame is always a XUL
element, we can force chrome.

This workaround will be rendered unnecessary if the client at some
point is changed from passing UUIDs by string to actual web element
reference objects.

> We don't check for a valid id. Can you please add an assertion and update the documentation?

Well we didn’t previously either, but I have rectified this.

> We don't assert for a valid id here too. This also applies to following methods in this file.

Fixed.

> I think you missed to paste the code for generating the webEl above. Instead it' only in content with a hard-coded context.

Fixed.

> We should assert first.

Fixed.

> Why a ChromeWebElement?

Because this.previousFrameElement in GeckoDriver#getActiveFrame
is passed back to the client, so it needs to be a web element
representation.

> Just assign null for clarity.

Fixed.

> I assume that for legacyActions we only want to support content?

Yes, it is only supported in content.

> You should remove await here similar to execute.

Fixed.

> We should return a WebElement here too. Maybe there was a problem in rebasing the patch.

We do return a web element. seenEls.add(el) returns a web element
reference.

> You don't want to keep the parameters anywhere?

For the content frame script this isn’t of equal value as in
driver.js, mostly because we control its input in driver.js.  The
listener.js API is not “formalised” like the one in driver.js,
where functions only receive a "cmd" object as input that can really
be anything.

> Combine both lines in case we need this extra variable at all.

Fixed.

> Please use Context.Content in both cases.

We don’t have access to the Context enum in listener.js.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 102 • 7 years ago

OK, in my latest update almost all tests are passing, except this
one:

>  LOG ERROR | InvalidArgumentError: Expected 'origin' to be undefined, "viewport", "pointer", or an element, got: [object Number] -1 at chrome://marionette/content/error.js:239 [log…] 

I fixed the mouse action WPT tests, but forgot to update the
xpcshell unit test.  The original problem was that action.js was
expecting web element references, but the content frame script now
deserialises these automatically to DOM elements, and I’m assuming
this xpcshell failure is now simply wrong.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 103 • 7 years ago

> and I’m assuming this xpcshell failure is now simply wrong.

s/failure/assertion/

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 104 • 7 years ago

Comment on attachment 8915665 [details]
Bug 1400256 - Return single anon element from WebDriver:FindElement.

Not sure why these got re-set.

	David Burns :automatedtester
	Comment 105 • 7 years ago
mozreview-review

Comment on attachment 8916732 [details]
Bug 1400256 - Adapt actions for implicitly unmarshaled elements.

https://reviewboard.mozilla.org/r/187802/#review193072

This has xpcshell test failures on try

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 106 • 7 years ago

(In reply to David Burns :automatedtester from comment #105)
> Comment on attachment 8916732 [details]
> Bug 1400256 - Adapt actions for implicitly unmarshaled elements.
> 
> https://reviewboard.mozilla.org/r/187802/#review193072
> 
> This has xpcshell test failures on try

Yes, see https://bugzilla.mozilla.org/show_bug.cgi?id=1400256#c102.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 107 • 7 years ago
mozreview-review-reply

Comment on attachment 8915668 [details]
Bug 1400256 - Make element.isXULElement more resiliant.

https://reviewboard.mozilla.org/r/186852/#review192154

> Not sure what you’re saying here.

I inferred what meant here from a later comment, so fixed!

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 130 • 7 years ago
mozreview-review-reply

Comment on attachment 8915665 [details]
Bug 1400256 - Return single anon element from WebDriver:FindElement.

https://reviewboard.mozilla.org/r/186846/#review192144

> Yes, other helper functions return arrays which is a bit pointless.

So why don't you also return an array here so that we have a single type for whatever strategy in use. Or convert others to iterators which we could land before? Is that so complicated? Having different types is far from ideal. When we have to quickly get the patch in, please file a new bug so we don't forget about to make them all iterators.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 131 • 7 years ago
mozreview-review-reply

Comment on attachment 8915669 [details]
Bug 1400256 - Add web element abstractions.

https://reviewboard.mozilla.org/r/186854/#review192170

> You can check pprint in testing/marionette/error.js:
> 
>     https://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/testing/marionette/error.js#151-179
> 
> OTOH for DOM elements it will output a prettified HTML tag
> with IDs and classes, and for objects it will output the
> Object.prototype.toString name of node, unless it has a custom
> toJSON function.

Fantastic. The `pprint` template should be moved to `format.js` now. Please file a separate bug for that.

> We can’t do that because the Context constant is defined in
> testing/marionette/driver.js which would cause a circular dependency
> between the two files.

I had this problem recently too for the Firefox application ID. Maybe we should add a new file which serves various constants.

> Actually, when serialised, I always include the LegacyIdentifier so
> that this will be backwards compatible with existing clients:
> 
> > class ContentWebFrame extends WebElement {
> >   toJSON() {
> >     return {
> >       [ContentWebFrame.Identifier]: this.uuid,
> >       [ContentWebElement.LegacyIdentifier]: this.uuid,
> >     };
> > }
> 
> This will need a follow-up change for clients to pick up the right
> identifiers.

Ok, please file the required bugs and mark them as dependencies. Once done mark it as fixed. Thanks.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 132 • 7 years ago
mozreview-review-reply

Comment on attachment 8915671 [details]
Bug 1400256 - Add element.isDOMWindow.

https://reviewboard.mozilla.org/r/186858/#review192158

> Do you mean you want it annotated with "*"?

I assume your question is a no-op now and you figured it out yourself. But yes, the input can be any kind of node, and not only `WindowProxy`.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 133 • 7 years ago
mozreview-review-reply

Comment on attachment 8915681 [details]
Bug 1400256 - Make IPC messages work with implicitly marshaled elements.

https://reviewboard.mozilla.org/r/186878/#review192194

> Because WebElement.fromUUID is a workaround for when the provided
> element ID doesn’t contain any information about what type of
> reference it is.  If all was well we would receive a JSON Object
> {"xulelement-xxx": "<uuid>"} which would tell us what type to
> derive, but here we receive {element: <uuid>}.  Because we know
> the web element passed to WebDriver:SwitchToFrame is always a XUL
> element, we can force chrome.
> 
> This workaround will be rendered unnecessary if the client at some
> point is changed from passing UUIDs by string to actual web element
> reference objects.

At this line of code the element can be chrome or content. The separtion will happen later in the if blocks. So if webEl is only used in the chrome block, move those lines in there, or let it pick up the current context.

> Because this.previousFrameElement in GeckoDriver#getActiveFrame
> is passed back to the client, so it needs to be a web element
> representation.

What I'm confused with is that this block of code handles the messages between the driver and listener. It means the frame we handle here lives in content. But ChromeWebElement is an element which lives in chrome scope.

> We do return a web element. seenEls.add(el) returns a web element
> reference.

In that case all fine. Thanks.

> For the content frame script this isn’t of equal value as in
> driver.js, mostly because we control its input in driver.js.  The
> listener.js API is not “formalised” like the one in driver.js,
> where functions only receive a "cmd" object as input that can really
> be anything.

It would still be helpful, but I'm ok with getting it removed.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 134 • 7 years ago
mozreview-review-reply

Comment on attachment 8915665 [details]
Bug 1400256 - Return single anon element from WebDriver:FindElement.

https://reviewboard.mozilla.org/r/186846/#review192144

> So why don't you also return an array here so that we have a single type for whatever strategy in use. Or convert others to iterators which we could land before? Is that so complicated? Having different types is far from ideal. When we have to quickly get the patch in, please file a new bug so we don't forget about to make them all iterators.

I don’t want to convert the other functions because I’m not
touching them in this patch.  Let’s remember that these are helper
functions and not published APIs.

The effect I want to achieve here is to not do unnecessary computation,
so I think you’ll agree that this is the right approach to take.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 135 • 7 years ago
mozreview-review-reply

Comment on attachment 8915669 [details]
Bug 1400256 - Add web element abstractions.

https://reviewboard.mozilla.org/r/186854/#review192170

> Fantastic. The `pprint` template should be moved to `format.js` now. Please file a separate bug for that.

Fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=1408454.

> I had this problem recently too for the Firefox application ID. Maybe we should add a new file which serves various constants.

Yes, I think ideally should clean up our dependency hierarchy
here.  Context.{CONTENT,CHROME} should probably be specifying in
testing/marionette/browser.js.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1408508.

> Ok, please file the required bugs and mark them as dependencies. Once done mark it as fixed. Thanks.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1408509.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 155 • 7 years ago
mozreview-review-reply

Comment on attachment 8915665 [details]
Bug 1400256 - Return single anon element from WebDriver:FindElement.

https://reviewboard.mozilla.org/r/186846/#review192144

> I don’t want to convert the other functions because I’m not
> touching them in this patch.  Let’s remember that these are helper
> functions and not published APIs.
> 
> The effect I want to achieve here is to not do unnecessary computation,
> so I think you’ll agree that this is the right approach to take.

Ok, sounds fine. Maybe at least add it to the description of the return value that it is clear? Btw. should the return value actually be `{Array.<DOMElement>}`?

	David Burns :automatedtester
	Comment 156 • 7 years ago
mozreview-review

Comment on attachment 8916732 [details]
Bug 1400256 - Adapt actions for implicitly unmarshaled elements.

https://reviewboard.mozilla.org/r/187802/#review194590