Closed Bug 1497150 Opened 11 months ago Closed 11 months ago

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

Categories

(DevTools :: Framework, enhancement)

enhancement
Not set

Tracking

(firefox64 fixed)

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(2 files, 2 obsolete files)

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
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.
Assignee: nobody → poirot.alex
MozReview-Commit-ID: 132swUYwGZw
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
MozReview-Commit-ID: 132swUYwGZw
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
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
Attachment #9017507 - Attachment is obsolete: true
Attachment #9017506 - Attachment is obsolete: true
Pushed by apoirot@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/965bcae7a111
https://hg.mozilla.org/mozilla-central/rev/8ea8fc9bc95b
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Depends on: 1500141
You need to log in before you can comment on or make changes to this bug.