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

The WebDriver:TakeScreenshot command currently takes an "id" field
with a web element reference UUID, as well as an array of UUIDs for
the "highlights" field.  If these were both given actual web element
JSON Objects they could be automatically deserialised when they
reach the content frame script and would simplify our code quite a
bit.

It is however important to preserve backwards compatibility with
earlier Marionette clients.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 2 • 7 years ago

I’d appreciate if this patch could get some attention soon.

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

Comment on attachment 8920819 [details]
Bug 1410649 - Let WebDriver:TakeScreenshot accept web elements.

https://reviewboard.mozilla.org/r/191804/#review197236

Sorry, but the review request slipped through. In such a case please make sure to set ni? a couple of days later.

::: testing/marionette/driver.js:2957
(Diff revision 1)
>   */
>  GeckoDriver.prototype.takeScreenshot = function(cmd) {
> -  let win = assert.window(this.getCurrentWindow());
> +  const win = assert.window(this.getCurrentWindow());
>  
> -  let {id, highlights, full, hash} = cmd.parameters;
> -  highlights = highlights || [];
> +  // TODO(ato): id is deprecated and can be removed with Firefox 60
> +  let area = cmd.parameters.element || cmd.parameters.id;

Could we use a variable name which is a bit closer to an `element`? `area` doesn't really say something to me what is actually involved. I would more expect that it contains coordinates but not an element when reading this code.

::: testing/marionette/driver.js:2967
(Diff revision 1)
> +  }
> +
> +  // TODO(ato): with Firefox 60 highlights becomes an array of
> +  // web elements instead of an array of string UUIDs
> +  let highlights = cmd.parameters.highlights || [];
> +  highlights.forEach((highlight, i, arr) => {

map() should still be better suited here.

::: testing/marionette/driver.js:2970
(Diff revision 1)
> +  // web elements instead of an array of string UUIDs
> +  let highlights = cmd.parameters.highlights || [];
> +  highlights.forEach((highlight, i, arr) => {
> +    if (typeof highlight == "string") {
> +      arr[i] = WebElement.fromUUID(highlight, this.context);
> +    } else if (typeof highlight != "undefined") {

Just do a `if (highlight)` here.

::: testing/marionette/listener.js:1646
(Diff revision 1)
>  }
>  
>  /**
>   * Perform a screen capture in content context.
>   *
> - * Accepted values for |opts|:
> + * @param {WebElement=} area

Similar to driver.js, I'm not happy with the naming of the argument.

::: testing/marionette/listener.js:1647
(Diff revision 1)
>  
>  /**
>   * Perform a screen capture in content context.
>   *
> - * Accepted values for |opts|:
> - *
> + * @param {WebElement=} area
> + *     Optional web element to take a screenshot of.  By default

`By default...` what?

::: testing/marionette/listener.js:1649
(Diff revision 1)
>   * Perform a screen capture in content context.
>   *
> - * Accepted values for |opts|:
> - *
> - *     @param {UUID=} id
> - *         Optional web element reference of an element to take a screenshot
> + * @param {WebElement=} area
> + *     Optional web element to take a screenshot of.  By default
> + * @param {Array.<WebElement>=} highlights
> + *     Draw a border around the elements.

The description sounds more like a boolean. So maybe `Elements to draw a border around`.

::: testing/marionette/listener.js:1652
(Diff revision 1)
> - *
> - *     @param {UUID=} id
> - *         Optional web element reference of an element to take a screenshot
> - *         of.
> - *     @param {boolean=} full
> - *         True to take a screenshot of the entire document element.  Is not
> + *     Optional web element to take a screenshot of.  By default
> + * @param {Array.<WebElement>=} highlights
> + *     Draw a border around the elements.
> + * @param {boolean=} [full=true] full
> + *     True to take a screenshot of the entire document element.
> + *     Is not considered <var>captureElement</var> is not defined.

Missing `if`.

::: testing/marionette/listener.js
(Diff revision 1)
> - *         references.
> + *     before taking the screenshot.  Defaults to true.
> - *     @param {boolean=} scroll
> - *         When |id| is given, scroll it into view before taking the
> - *         screenshot.  Defaults to true.
> - *
> - * @param {capture.Format} format

`hash` still has to be part of the documentation.

::: testing/marionette/listener.js:1668
(Diff revision 1)
> -  let full = !!opts.full;
> -  let highlights = opts.highlights || [];
> -  let scroll = !!opts.scroll;
> -
> -  let win = curContainer.frame;
> +      area = null,
> +      highlights = [],
> +      full = true,
> +      scroll = true,
> +      hash = true,
> +    } = {}) {

This declaration looks ugly, sorry. We should really keep the `opts = {}` in the paramter list, and do the destructuring afterward in the function body.

::: testing/marionette/listener.js
(Diff revision 1)
>    let canvas;
> -  let highlightEls = highlights
> -      .map(ref => WebElement.fromUUID(ref, "content"))
> -      .map(webEl => seenEls.get(webEl, win));
> +  if (!area && !full) {
> +    canvas = capture.viewport(win, highlights);
> +  } else if (area) {
> -
> -  // viewport

Please keep the comments which makes it easier to follow which case screenshots what exactly.

::: testing/marionette/listener.js:1680
(Diff revision 1)
> -      if (scroll) {
> +    if (scroll) {
> -        element.scrollIntoView(el);
> +      element.scrollIntoView(area);
> -      }
> +    }
> +    canvas = capture.element(area, highlights);
> -    } else {
> +  } else {
> -      el = win.document.documentElement;
> +    let {documentElement} = win.document;

nit: I don't see the need for this extra variable. We can directly pass it into capture.element().

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 4 • 7 years ago
mozreview-review-reply

Comment on attachment 8920819 [details]
Bug 1410649 - Let WebDriver:TakeScreenshot accept web elements.

https://reviewboard.mozilla.org/r/191804/#review197236

> Just do a `if (highlight)` here.

You don’t want to map null items. [null] will be passed to
WebElement.fromJSON if we use that if-condition.

> This declaration looks ugly, sorry. We should really keep the `opts = {}` in the paramter list, and do the destructuring afterward in the function body.

I would argue this destructuring is safer and clearer.

> nit: I don't see the need for this extra variable. We can directly pass it into capture.element().

This is so to not have to break up the line, so I think this is fine as it is.

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

Comment on attachment 8920819 [details]
Bug 1410649 - Let WebDriver:TakeScreenshot accept web elements.

https://reviewboard.mozilla.org/r/191804/#review197236

> You don’t want to map null items. [null] will be passed to
> WebElement.fromJSON if we use that if-condition.

`typeof null` is object, and would run the code of this if block when using your condition. As you just said, we don't want that. So `if highlight` would exclude those items correctly. You could also use `if !!hightlight` if that feels better to you.

> I would argue this destructuring is safer and clearer.

What should be safer? Just moving it into the function body doesn't change anything in how we destructure the associative array. It's totally equal in behavior. 

Also when I have a look at function arguments I want to immediately see and being able to understand what has to be passed in. This multi-line and complex declaration doesn't make it possible at all.

> This is so to not have to break up the line, so I think this is fine as it is.

Even `canvas = capture.element(win.document.documentElement, highlights);` has only 72 chars which fits nicely in the 78 char line limit.

	David Burns :automatedtester
	Comment 6 • 6 years ago

[Mass update] Setting P1 as currently being worked on.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 7 • 6 years ago

Not being actively worked on anymore.