Closed Bug 1410652 Opened 7 years ago Closed 7 years ago

Allow WebDriver:SwitchToFrame to take a web element

Categories

(Remote Protocol :: Marionette, enhancement)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(4 files)

Bug 1410652 - Let WebDriver:SwitchToFrame take a web element.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1410652 - Fix API docs of assert functions.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1410652 - Fix various API documentation in element module.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1410652 - Use invalid argument error for web element deserialisation.
59 bytes, text/x-review-board-request
whimboo
: review+
Details 
The "element" field in the command body for the
WebDriver:SwitchToFrame command curently takes a string web element
reference UUID, whereas it could take a web element JSON Object.

This would somewhat simplify our code in the future because we would
not have to deduce what type of input we’re dealing with.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Attachment #8920822 - Flags: review?(hskupin)
Comment on attachment 8920822 [details]
Bug 1410652 - Let WebDriver:SwitchToFrame take a web element.

https://reviewboard.mozilla.org/r/191812/#review197588

::: testing/marionette/driver.js:1741
(Diff revision 1)
> +  // element JSON Object.  Can be removed with Firefox 60.
> +  let byFrame;
> +  if (typeof cmd.parameters.element == "string") {
> +    byFrame = WebElement.fromUUID(cmd.parameters.element, Context.Chrome);
> +  } else if (cmd.parameters.element) {
> +    byFrame = WebElement.fromJSON(cmd.parameters.element);

I don't see that we handle the internal `TypeError` as it can be thrown by `fromJSON` for eg. `undefined`. It has to throw the spec defined `InvalidArgumentError` instead.
Attachment #8920822 - Flags: review-
Comment on attachment 8920822 [details]
Bug 1410652 - Let WebDriver:SwitchToFrame take a web element.

https://reviewboard.mozilla.org/r/191812/#review197588

> I don't see that we handle the internal `TypeError` as it can be thrown by `fromJSON` for eg. `undefined`. It has to throw the spec defined `InvalidArgumentError` instead.

Sure yes, that is a good point.  I’ve submitted patches to remedy
this.
Comment on attachment 8921800 [details]
Bug 1410652 - Fix API docs of assert functions.

https://reviewboard.mozilla.org/r/192830/#review198298
Attachment #8921800 - Flags: review?(hskupin) → review+
Comment on attachment 8921801 [details]
Bug 1410652 - Fix various API documentation in element module.

https://reviewboard.mozilla.org/r/192832/#review198300
Attachment #8921801 - Flags: review?(hskupin) → review+
Comment on attachment 8921802 [details]
Bug 1410652 - Use invalid argument error for web element deserialisation.

https://reviewboard.mozilla.org/r/192834/#review198304

::: testing/marionette/test_element.js:249
(Diff revision 1)
>    ok(WebElement.from(domEl) instanceof ContentWebElement);
>    ok(WebElement.from(domWin) instanceof ContentWebWindow);
>    ok(WebElement.from(domFrame) instanceof ContentWebFrame);
>    ok(WebElement.from(xulEl) instanceof ChromeWebElement);
>  
> -  Assert.throws(() => WebElement.from({}), /Expected DOM window\/element/);
> +  Assert.throws(() => WebElement.from({}), InvalidArgumentError);

So not using a regex here for the error class is fine? I'm still unsure given that the other bug hasn't been fixed yet.
Attachment #8921802 - Flags: review?(hskupin) → review+
Comment on attachment 8920822 [details]
Bug 1410652 - Let WebDriver:SwitchToFrame take a web element.

https://reviewboard.mozilla.org/r/191812/#review198306

r=wc

::: testing/marionette/driver.js:1735
(Diff revision 2)
>    assert.window(this.getCurrentWindow());
>    assert.noUserPrompt(this.dialog);
>  
> -  let {id, element, focus} = cmd.parameters;
> +  let {id, focus} = cmd.parameters;
> +
> +  // TODO(ato): element can be either string (deprecated) or a web

Please remove your name here. It's not necessary to know who added that. All what we have to know is when it can be removed.

::: testing/marionette/driver.js:1770
(Diff revision 2)
>  
>    if (this.context == Context.Chrome) {
>      let foundFrame = null;
>  
>      // just focus
> -    if (typeof id == "undefined" && typeof element == "undefined") {
> +    if (typeof id == "undefined" && !byFrame) {

Why don't you use a `undefined` check for `byFrame` here as what you do for id? We shouldn't mix it, and in the past you always wanted to have the typeof usage.

::: testing/marionette/driver.js:1781
(Diff revision 2)
>            checkLoad.bind(this), 100, Ci.nsITimer.TYPE_ONE_SHOT);
>        return;
>      }
>  
>      // by element (HTMLIFrameElement)
> -    if (typeof element != "undefined") {
> +    if (byFrame) {

Same here regarding `typeof`.

::: testing/marionette/driver.js:1885
(Diff revision 2)
>      } else {
>        throw new NoSuchFrameError(`Unable to locate frame: ${id}`);
>      }
>  
>    } else if (this.context == Context.Content) {
> -    if (!id && !element &&
> +    if (!id && !byFrame &&

Funny that we use that many different methods of comparing in just a single method.
Attachment #8920822 - Flags: review?(hskupin) → review+
Comment on attachment 8920822 [details]
Bug 1410652 - Let WebDriver:SwitchToFrame take a web element.

https://reviewboard.mozilla.org/r/191812/#review198306

> Please remove your name here. It's not necessary to know who added that. All what we have to know is when it can be removed.

This is a pretty weird thing to raise an issue about.  I can find
over 5,800 instances in m-c alone of this practice.

> Why don't you use a `undefined` check for `byFrame` here as what you do for id? We shouldn't mix it, and in the past you always wanted to have the typeof usage.

Because my patch fixes it so that byFrame is always populated.
With id we still have to do the check because we don’t do any
sanitation of the input.  I think avoiding typeof check swhen we can
makes the code easier on the eye.

> Funny that we use that many different methods of comparing in just a single method.

GeckoDriver#switchToFrame is terrible, but I think this is a tiny
step in the right direction.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/265a168b4e37
Fix API docs of assert functions. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/48048929bb17
Fix various API documentation in element module. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/66754caa4c52
Use invalid argument error for web element deserialisation. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/97031d4ea2bd
Let WebDriver:SwitchToFrame take a web element. r=whimboo
I’ve fixed the lint failure.
Flags: needinfo?(ato)
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf841cb4ea9a
Fix API docs of assert functions. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/a6e574e19c3d
Fix various API documentation in element module. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/b48a22103f3f
Use invalid argument error for web element deserialisation. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/89c77901b957
Let WebDriver:SwitchToFrame take a web element. r=whimboo
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: