Remove browser.soft_reload.only_force_validate_top_level_document pref

It's been enabled by default for about 3 years (bug 1752558)

Hi There,
I am a new contributor and happy to pick up this bug.

Hi Gregory,
I just have a couple of questions on this bug.

I have removed the pref from the StaticPrefList.yaml file, and then went on to remove the references from the test files:
-> devtools/client/frameworks/test/browser_toolbox_window_reload_target_force.js
-> test/browser_toolbox_window_reload_target_force.js
-> image/test/crashtests/crashtests.list

But I was unsure of the extent to which I remove code that depends on the pref. For example, in the below would I completely remove all the lines that call expectedHeader?

 const expectedHeader = Services.prefs.getBoolPref(
 "browser.soft_reload.only_force_validate_top_level_document",
 false
 )
 ? ""
 : "max-age=0";
 await testReload("toolbox.reload.key", toolbox, expectedHeader);
 await testReload("toolbox.reload2.key", toolbox, expectedHeader);
 await testReload("toolbox.forceReload.key", toolbox, "no-cache");
 await testReload("toolbox.forceReload2.key", toolbox, "no-cache");

Hello Gregory,

Please see the link to my WIP patch in the previous comment.

modules/libpref/init/StaticPrefList.yaml

• Removed the pref

image/test/crashtests/crashtests.list

• Removed the pref from the list

devtools/client/framework/test/browser_toolbox_window_reload_target_force.js

• Removed the pref and set ExpectedHeader to the false outcome of the original ternary operator

dom/base/test/browser_refresh_content.js

• I did not make any changes to this file as I was unsure how to best proceed. If you could please advise and I will make the changes accordingly.

Once you advise on the point above and any other changes required I will action them for the official patch submission. Could you please also advise on which tests if any I need to run once the ammendments are complete.

Tx,

Ds

Answered in Phabricator

Hi Gregory,

Thanks for the feedback, I have made the changes and they are linked below (I believe I followed the right process to just amend the commit and re-upload with moz-phab, but let me know if I did something wrong)

as per your feedback I removed the forceRevalidate param from the run_test_browser_refresh function and removed any assertions that were conditioned on forceRevalidate being true. This meant that I just have the assertions that would have run on forceRevalidate being false previously.

I then removed the content from add_task except for a single invocation of run_test_browser_refresh since we don't need to run it for each case of forceRevalidate being true and false

if this is all good, please let me know any tests I need to run before submitting the official patch.

Tx,
Ds

Daniel,
it is better to make the patch available for review rather than go back and forth in bugzilla, as that splits the discussion and makes it harder for others to follow.

Your patch needs to remove the usage in C++ as well as tests, then it should be reviewable
https://searchfox.org/firefox-main/search?q=only_force_validate_top_level_document&path=&case=false&regexp=false

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:dansinger, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.