Closed Bug 1436187 Opened 6 years ago Closed 6 years ago

Clarify that frame-script-utils.js is only for tests

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1436187 - Move existing shared head files to devtools/client/shared.
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1436187 - Centralize frame-script-utils loading via shared-head.
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1436187 - Move frame-script-utils.js to shared/test.
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1436187 - Fix linting errors in shared-head.js.
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
We should move / rename devtools/client/shared/frame-script-utils.js so that it is clear that it is only for tests.

Maybe moving it into devtools/client/shared/test is enough?

Some related notes:

* bug 1356037 mentions we ship this file to our users
* allowed-dupes.mn[1] suggests we might be shipping two copies (not clear why...)

[1]: https://searchfox.org/mozilla-central/search?q=frame-script-utils.js&case=true&path=
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #0)
> We should move / rename devtools/client/shared/frame-script-utils.js so that
> it is clear that it is only for tests.
> 
> Maybe moving it into devtools/client/shared/test is enough?

Seems reasonable. And then to fix bug 1356037 it could be added to support-files in each browser.ini that references it as `!/devtools/client/shared/test/frame-script-utils.js` and then referenced as "chrome://mochitests/content/browser/devtools/client/shared/test/frame-script-utils.js".

We should also consider either moving shared-head.js into client/shared/test or moving this file into client/framework/test so that our shared test files will be in one place.
Alex, maybe you are interested...?
Flags: needinfo?(poirot.alex)
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Flags: needinfo?(poirot.alex)
Priority: -- → P3
Comment on attachment 8956591 [details]
Bug 1436187 - Move existing shared head files to devtools/client/shared.

https://reviewboard.mozilla.org/r/225506/#review231470
Attachment #8956591 - Flags: review?(bgrinstead) → review+
Comment on attachment 8956592 [details]
Bug 1436187 - Centralize frame-script-utils loading via shared-head.

https://reviewboard.mozilla.org/r/225508/#review231472

Nice cleanup

::: commit-message-2bfb6:7
(Diff revision 4)
> +
> +Use `loadFrameScriptUtils` from shared-head as central utility for load the
> +frame script utils helper.  This means less stray references to the utils file's
> +path across our tests.
> +
> +As part of this, I went ahead and converted Cavans Debugger, Shader Editor, and

Typo: "Canvas"

::: devtools/client/framework/test/head.js:159
(Diff revision 4)
>   *        the url
>   * @return {Promise} a promise that resolves when the element has been created
>   */
>  function createScript(url) {
>    info(`Creating script: ${url}`);
> -  let mm = getFrameScript();
> +  // This is not ideal if called multiple times, as it loads the frame script

Is this something we could fix in a follow up (i.e. check if the frame script is already loaded before loading it again in `loadFrameScriptUtils`)?
Attachment #8956592 - Flags: review?(bgrinstead) → review+
Comment on attachment 8956593 [details]
Bug 1436187 - Move frame-script-utils.js to shared/test.

https://reviewboard.mozilla.org/r/225510/#review231476
Attachment #8956593 - Flags: review?(bgrinstead) → review+
Comment on attachment 8956618 [details]
Bug 1436187 - Fix linting errors in shared-head.js.

https://reviewboard.mozilla.org/r/225558/#review231478

::: commit-message-40d6d:3
(Diff revision 2)
> +Bug 1436187 - Fix linting errors in shared-head.js. r=bgrins
> +
> +After moving shared-head.js, linting activated because the new path is not

Nit: "linting" -> "linting is"
Attachment #8956618 - Flags: review?(bgrinstead) → review+
Comment on attachment 8956592 [details]
Bug 1436187 - Centralize frame-script-utils loading via shared-head.

https://reviewboard.mozilla.org/r/225508/#review231472

> Typo: "Canvas"

Thanks, fixed.

> Is this something we could fix in a follow up (i.e. check if the frame script is already loaded before loading it again in `loadFrameScriptUtils`)?

Yes, we could do something better here.  Filed bug 1443680.  I don't know of an nice way to explicitly check if a script has already been loaded, but we could use a boolean or something to manually track if we've been called before.
Comment on attachment 8956618 [details]
Bug 1436187 - Fix linting errors in shared-head.js.

https://reviewboard.mozilla.org/r/225558/#review231478

> Nit: "linting" -> "linting is"

Thanks, fixed.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e747480f3cd2
Move existing shared head files to devtools/client/shared. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/b2d808aa8c2e
Centralize frame-script-utils loading via shared-head. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/9875771c9967
Move frame-script-utils.js to shared/test. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/253f74e28723
Fix linting errors in shared-head.js. r=bgrins
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/670b7ca7a72f
Backed out 4 changesets for mochitest devtools failures at browser_se_editors-error-gutter.js on a CLOSED TREE
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/da463ccea0f5
Move existing shared head files to devtools/client/shared. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/4a9f92bb52f7
Centralize frame-script-utils loading via shared-head. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/6cc542dc0950
Move frame-script-utils.js to shared/test. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/0e820a5e59df
Fix linting errors in shared-head.js. r=bgrins
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: