1497150 - Stop calling TargetFront.attach from options panel to know if JS is enabled

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement)

Description

[Alexandre Poirot [:ochameau]]

Followup to bug 1485660.
While refactoring TabClient to a front, we saw that the options panel is calling BrowsingContextTargetActor's attach request to know if JS is enabled on the backend side. This looks unecessary as we should be able to read this state directly from the TargetFront.

https://searchfox.org/mozilla-central/rev/5786b9be9f887ed371c804e786081b476a403104/devtools/client/framework/toolbox-options.js#463-464

Comment 1

[Alexandre Poirot [:ochameau]]

Here is an handy test document:
  data:text/html,<script>document.write("JS");</script><br/>xxxxx

Note that this code looks antique!
You would expect the checkbox in options panel to always be updated to the right state, but it is not with/without this patch.
Here is the controvertial STR:
* open the test doc
* open toolbox on the options panel
* toggle JS to *disabled*
* close the toolbox  ===> the BrowsingContextTargetActor will reset docShell flag on destroy, but not reload the document
* reopen the toolbox

=> options panel says the JS is *enabled*. Well that's kind of true, docShell's flag allows JS to run, but you have to reload to see that.

At the end, we might as well just consider the JS always enabled from the options panel, it would behave the same as today and the code would be simplier...

But here is a patch to stop calling attach and rely on the state stored on the front.
I added a preliminary revision, to switch a test for JS disabling to async test.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1497150 - Convert browser_toolbox_options_disable_js.js to async test.

MozReview-Commit-ID: 132swUYwGZw

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1497150 - Use BrowsingContextFront's javascriptEnabled cached value instead of calling attach.

For now, the options panel was calling `attach` to know if the javascript was disabled
on the debugged document. But this property is already cached during the `attach`
request done by the toolbox.

MozReview-Commit-ID: JcDT6vxCUzN

Depends on D8847

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1497150 - Convert browser_toolbox_options_disable_js.js to async test.

MozReview-Commit-ID: 132swUYwGZw

Comment 5

[Alexandre Poirot [:ochameau]]

Note that I removed `cacheDisabled` as it doesn't seem to be used at first sight, but try might say otherwise:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=495e6609d129b191ed4d71cb429d8ec407df8034

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1497150 - Use BrowsingContextFront's javascriptEnabled cached value instead of calling attach.

For now, the options panel was calling `attach` to know if the javascript was disabled
on the debugged document. But this property is already cached during the `attach`
request done by the toolbox.

MozReview-Commit-ID: JcDT6vxCUzN

Depends on D8851

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/965bcae7a111
Convert browser_toolbox_options_disable_js.js to async test. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/8ea8fc9bc95b
Use BrowsingContextFront's javascriptEnabled cached value instead of calling attach. r=jdescottes

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/965bcae7a111
https://hg.mozilla.org/mozilla-central/rev/8ea8fc9bc95b