1478979 - Capability pageLoadStrategy should not accept null value

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement)

Description

[Andreas Tolfsen ❲:ato❳]

pageLoadStrategy can be null when given to geckodriver, where we
do capabilities matching, but not when we send the effective set
of capabilities to Marionette.

This was discovered when reviewing
https://bugzilla.mozilla.org/show_bug.cgi?id=1264259.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: Disallow pageLoadStrategy to be null. r=whimboo

Capabilities matching is now done in geckodriver and Marionette
receives the negotiated set of capabilities, so there is no need
to derive default values in case the value is null.

Comment 2

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4135c783db273f40e967cb2d8df7d6ff76ede62

Comment 3

[Andreas Tolfsen ❲:ato❳]

Attachment: Disallow pageLoadStrategy to be null.

Capabilities matching is now done in geckodriver and Marionette
receives the negotiated set of capabilities, so there is no need
to derive default values in case the value is null.

Comment 4

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=399c0ab1260c29a43012ba6362557117860e5fca

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8995515 [details] [diff] [review]
Disallow pageLoadStrategy to be null.

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

::: testing/marionette/capabilities.js
@@ +473,5 @@
>  
> +          if (Object.values(PageLoadStrategy).includes(v)) {
> +            matched.set("pageLoadStrategy", v);
> +          } else {
> +            throw new InvalidArgumentError("Unknown page load strategy: " + v);

Please change to use a template string.

Comment 6

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #5)

> ::: testing/marionette/capabilities.js
> @@ +473,5 @@
> >  
> > +          if (Object.values(PageLoadStrategy).includes(v)) {
> > +            matched.set("pageLoadStrategy", v);
> > +          } else {
> > +            throw new InvalidArgumentError("Unknown page load strategy: " + v);
> 
> Please change to use a template string.

This shouldn’t be necessary.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6069c381aa8e
Disallow pageLoadStrategy to be null. r=whimboo

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/6069c381aa8e

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andreas Tolfsen 「:ato」 from comment #6)
> > Please change to use a template string.
> 
> This shouldn’t be necessary.

Sure, but it would have made the code look identical to all the other error lines.