Closed Bug 1478979 Opened Last year Closed Last year

Capability pageLoadStrategy should not accept null value

Categories

(Testing :: Marionette, enhancement)

enhancement
Not set

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: ato, Assigned: ato)

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → ato
Status: NEW → ASSIGNED
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.
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.
Attachment #8995496 - Attachment is obsolete: true
Attachment #8995515 - Flags: review?(hskupin)
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.
Attachment #8995515 - Flags: review?(hskupin) → review+
(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.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6069c381aa8e
Disallow pageLoadStrategy to be null. r=whimboo
https://hg.mozilla.org/mozilla-central/rev/6069c381aa8e
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(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.
You need to log in before you can comment on or make changes to this bug.