Closed Bug 1414070 Opened 2 years ago Closed 2 years ago

Specify whether the URL is a test or reference when instructing reftest-content.js to load a new URL

Categories

(Testing :: Reftest, enhancement)

enhancement
Not set

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

Details

Attachments

(1 file)

The rendering fuzzer I've been working on needs to know whether the URL that is being loaded is a test or reference (it should never fuzz reference files).
Comment on attachment 8924744 [details]
Bug 1414070 - Specify whether the URL is a test or reference when instructing reftest-content.js to load a new URL.

https://reviewboard.mozilla.org/r/195976/#review201520

r- for the moment, due to the final comment (bool-vs-enum argument consistency) -- it seems like this would merit another quick review pass after that's addressed.

::: commit-message-f3f80:1
(Diff revision 2)
> +Bug 1414070 - Specify whether the URL is a test or reference when instructing reftest-content.js to load a new URL. r=dholbert

Please mention in the extended commit message that you're also replacing the global "state" variable.  

(I noticed the removal in globals.jsm and was confused by it -- since it seemed unrelated -- until I got to the very end of the patch where you replace its usages with this new functionality.)

::: layout/tools/reftest/reftest.jsm:1005
(Diff revision 2)
>  
>          FinishTestItem();
>          return;
>      }
>  
> -    if (g.urls[0]["prefSettings" + g.state].length == 0 &&
> +    const recordingRef = (g.currentURLTargetType == URL_TARGET_TYPE_REFERENCE);

Nit: could you add an "is" prefix here, "isRecordingRef", to make it more clearly boolean-flavored?

::: layout/tools/reftest/reftest.jsm:1007
(Diff revision 2)
>          return;
>      }
>  
> -    if (g.urls[0]["prefSettings" + g.state].length == 0 &&
> +    const recordingRef = (g.currentURLTargetType == URL_TARGET_TYPE_REFERENCE);
> +
> +    if (g.urls[0][recordingRef ? "prefSettings2" : "prefSettings1"].length == 0 &&

This line is longer than 80 chars -- needs to be wrapped (maybe after the question-mark).

Though really, maybe it'd be nice to pull out this expression...
g.urls[0][recordingRef ? "prefSettings2" : "prefSettings1"]
...into a clearly-named local variable, and then check that local variable's "length == 0" in the if expression.  That would make this much easier to read/reason about.

::: layout/tools/reftest/reftest.jsm:1546
(Diff revision 2)
> -function SendLoadTest(type, uri, timeout)
> +function SendLoadTest(type, uri, uriType, timeout)
>  {
>      g.browserMessageManager.sendAsyncMessage("reftest:LoadTest",
> -                                            { type: type, uri: uri, timeout: timeout }
> +                                            { type: type, uri: uri,
> +                                              isRef: uriType == URL_TARGET_TYPE_REFERENCE,
> +                                              timeout: timeout }

It feels a bit awkward that some APIs here (e.g. StartCurrentURI, SendLoadTest) take a two-value URL_TARGET_TYPE_{TEST|REF} enum, whereas other APIs (e.g. LoadTest,  RecvLoadTest, StartTestURI) take an equivalent-but-different "isRef" boolean value.

It feels like we're encoding the same information inconsistently, in two different ways.  Can we just standardize on passing this information around by using the enum value?

(That would mean RecvLoadScriptTest etc. would need to manually pass "URL_TARGET_TYPE_TEST" instead of "false" into startTestURI -- and that seems accurate & fine.)
Attachment #8924744 - Flags: review?(dholbert) → review-
(In reply to Daniel Holbert [:dholbert] from comment #3)
> Comment on attachment 8924744 [details]
> Please mention in the extended commit message that you're also replacing the
> global "state" variable.  
> 
> (I noticed the removal in globals.jsm and was confused by it -- since it
> seemed unrelated -- until I got to the very end of the patch where you
> replace its usages with this new functionality.)

Yeah, the whole "state" thing (basically, first or second url for a given test) was confusing to me, which is exactly why I'm replacing it. It wasn't at all obvious that this "state" was related to that, and the string concatenation with the 1 or 2 was hiding accesses to "url1" and "url2" etc.

> It feels a bit awkward that some APIs here (e.g. StartCurrentURI,
> SendLoadTest) take a two-value URL_TARGET_TYPE_{TEST|REF} enum, whereas
> other APIs (e.g. LoadTest,  RecvLoadTest, StartTestURI) take an
> equivalent-but-different "isRef" boolean value.

I preferred an "enum" in the chrome (.jsm) code because it makes what's being passed around there much clearer. In the reftest-content.js content script we don't have access to the enum values though. We could redefine them, requiring us to keep two sets of enums in sync, passing their underlying values through as a number in the JSON that is passed to the content script. But it seemed safer to have the JSON that's sent to the content script have a named "isRef" boolean value. Given that, it then just seemed cleaner on the content side to store that in a gCurrentURLIsRef boolean.

If you'd rather we define the enum values in two places I guess I can live with that, but the above is worth pointing out for your reconsideration. ;)
Comment on attachment 8924744 [details]
Bug 1414070 - Specify whether the URL is a test or reference when instructing reftest-content.js to load a new URL.

https://reviewboard.mozilla.org/r/195976/#review201650

Thanks! r=me with nits:

::: commit-message-f3f80:4
(Diff revisions 2 - 4)
>  Bug 1414070 - Specify whether the URL is a test or reference when instructing reftest-content.js to load a new URL. r=dholbert
>  
> +As part of this change, the confusingly named global variable 'state' is
> +renamed to'currentURLTargetType', and named "enum" values are assigned

add space after "to"

::: layout/tools/reftest/reftest.jsm:1550
(Diff revisions 2 - 4)
>  function SendLoadTest(type, uri, uriType, timeout)
>  {
>      g.browserMessageManager.sendAsyncMessage("reftest:LoadTest",
>                                              { type: type, uri: uri,
> -                                              isRef: uriType == URL_TARGET_TYPE_REFERENCE,
> +                                              uriTargetType: uriType,

s/uriType/uriTargetType/ (i.e. rename this function's own arg to uriTargetType) for consistency, probably?
Attachment #8924744 - Flags: review?(dholbert) → review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8d6110960ac
Expose whether the URL is a test or reference when telling reftest-content.js to load a URL. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/b8d6110960ac
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.