Closed Bug 1275078 Opened 3 years ago Closed 3 years ago

Merge toolbox-init.js from Positron branch

Categories

(DevTools :: General, enhancement)

47 Branch
enhancement
Not set

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: myk, Assigned: jryans)

References

Details

Attachments

(2 files)

After bug 1268134 is resolved, we should merge toolbox-init.js from Positron again to address these issues, per <https://github.com/mozilla/positron/pull/50>:

1. The script tests `if (iframe)` after calling `iframe.QueryInterface()`, but that call should throw an exception if *iframe* is falsy, so the conditional will never be false.

2. After fixing issue #1 by testing *iframe* before calling `iframe.QueryInterface()`, if *iframe* is falsy, then *toolboxOpened* is never defined, so the `toolboxOpened.catch()` call below will fail.

3. The promise chain will reach the `toolboxOpened.then()` function even if there was an error opening the toolbox, since it gets attached to the chain after `toolboxOpened.catch()`, which doesn't re-throw the error.
Severity: normal → enhancement
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Summary: merge toolbox-init.js from Positron branch → Merge toolbox-init.js from Positron branch
Comment on attachment 8840040 [details]
Bug 1275078 - Fix ESLint issues in toolbox and target files.

https://reviewboard.mozilla.org/r/114594/#review116344

::: devtools/client/framework/toolbox-options.js:415
(Diff revision 2)
> -          this._origJavascriptEnabled = !response.javascriptEnabled;
> +      this._origJavascriptEnabled = !response.javascriptEnabled;
> -          this.disableJSNode.checked = this._origJavascriptEnabled;
> +      this.disableJSNode.checked = this._origJavascriptEnabled;
> -          this.disableJSNode.addEventListener("click",
> -            this._disableJSClicked);
> -        });
> +      this.disableJSNode.addEventListener("click", this._disableJSClicked);
> +    } else {
> +      // Hide the checkbox and label
> +      this.disableJSNode.parentNode.style = "display: none;";

this.disabledJSNode.parentNode.style.display = "none"; ?
Attachment #8840040 - Flags: review?(poirot.alex) → review+
Comment on attachment 8840041 [details]
Bug 1275078 - toolbox-init.js error handling tweaks from Positron.

https://reviewboard.mozilla.org/r/114596/#review116346
Attachment #8840041 - Flags: review?(poirot.alex) → review+
Comment on attachment 8840040 [details]
Bug 1275078 - Fix ESLint issues in toolbox and target files.

https://reviewboard.mozilla.org/r/114594/#review116344

> this.disabledJSNode.parentNode.style.display = "none"; ?

Thanks, fixed.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f66f556b7893
Fix ESLint issues in toolbox and target files. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/22c5f7ea3629
toolbox-init.js error handling tweaks from Positron. r=ochameau
https://hg.mozilla.org/mozilla-central/rev/f66f556b7893
https://hg.mozilla.org/mozilla-central/rev/22c5f7ea3629
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.