Remove browser.formfill.saveHttpsForms support

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Form Manager
--
enhancement
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: MattN, Assigned: jonathanGB, Mentored)

Tracking

53 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Since everything is moving to HTTPS, it no longer makes sense to not save form history for HTTPS.

Taking this as a good intern bug.
Assignee: MattN+bmo → jonathan.guillotte.blouin
See the URL field for code and tests to change.

Use `./mach test` to run tests
Comment hidden (mozreview-request)
Comment on attachment 8866534 [details]
Bug 1361220 - Remove browser.formfill.saveHttpsForms support.

https://reviewboard.mozilla.org/r/138150/#review141344

Hi Jonathan, I think things could be simplified a bit more.

::: toolkit/components/satchel/test/test_form_submission.html:145
(Diff revision 1)
> -  <!-- form data submitted through HTTPS, when browser.formfill.saveHttpsForms is false -->
> +  <!-- form 20 (removed): data submitted through HTTPS, when browser.formfill.saveHttpsForms is false -->
> -  <form id="form20" action="https://www.example.com/" onsubmit="return checkSubmit(20)">
> -    <input type="text" name="test1">
> -    <button type="submit">Submit</button>
> -  </form>
>  
> -  <!-- Form 21 is submitted into an iframe, not declared here. -->
> +  <!-- Form 21 (removed) is submitted into an iframe, not declared here. -->

I don't think the comments will be useful in the future. Can you renumber forms 22-24 by subtracting 2 to get rid of the gap and simplify the test please?

::: toolkit/components/satchel/test/test_form_submission.html:245
(Diff revision 1)
>    <form id="form109" action="https://www.example.com/" onsubmit="return checkSubmit(109)">
>      <input type="text" name="test9">
>      <button type="submit">Submit</button>
>    </form>
>  
> -  <!-- regular form data, when browser.formfill.saveHttpsForms is false -->
> +  <!-- form 110 (removed): regular form data, when browser.formfill.saveHttpsForms is false -->

You probably don't need this comment since it's the last test being removed.

::: toolkit/components/satchel/test/test_form_submission.html:441
(Diff revision 1)
> -    SpecialPowers.clearUserPref("browser.formfill.saveHttpsForms");
> -
>    // End the test now on SeaMonkey.
> -  if (formNum == 21 && navigator.userAgent.match(/ SeaMonkey\//)) {
> +  if (formNum == 22 && navigator.userAgent.match(/ SeaMonkey\//)) {
>      checkObserver.uninit();
> -    is(numSubmittedForms, 21, "Ensuring all forms were submitted.");
> +    is(numSubmittedForms, 20, "Ensuring all forms were submitted.");

Shouldn't this be 19 if you removed 20 and 21? Your mach test with Firefox won't run this check so you may not have noticed this failing but Seamonkey infrastructure would catch it.

::: toolkit/components/satchel/test/test_form_submission.html:454
(Diff revision 1)
> -
>    // End the test at the last form.
> -  if (formNum == 110) {
> -    is(numSubmittedForms, 35, "Ensuring all forms were submitted.");
> +  if (formNum == 109) {
> +    is(numSubmittedForms, 32, "Ensuring all forms were submitted.");
>      checkObserver.uninit();
> -    SimpleTest.finish();
> +    SimpleTest.executeSoon(SimpleTest.finish);

Is this the fix for the issue you had yesterday? What is the reason for this approach?

::: toolkit/components/satchel/test/test_form_submission.html:468
(Diff revision 1)
>    // This in itself is fine, but if there are errors in the code, mochitests
>    // will in some cases give you "server too busy", which is hard to debug!
>    //
>    setTimeout(function() {
>      checkObserver.waitForChecks(function() {
> -      var nextFormNum = formNum == 24 ? 100 : (formNum + 1);
> +      var nextFormNum = formNum == 19 ? 22 : formNum == 24 ? 100 : (formNum + 1);

If you were to need this in a future bug, I would recommend using brackets to make the precedence order clear.
Attachment #8866534 - Flags: review?(MattN+bmo)
(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #3)

> 
> ::: toolkit/components/satchel/test/test_form_submission.html:145
> (Diff revision 1)
> > -  <!-- form data submitted through HTTPS, when browser.formfill.saveHttpsForms is false -->
> > +  <!-- form 20 (removed): data submitted through HTTPS, when browser.formfill.saveHttpsForms is false -->
> > -  <form id="form20" action="https://www.example.com/" onsubmit="return checkSubmit(20)">
> > -    <input type="text" name="test1">
> > -    <button type="submit">Submit</button>
> > -  </form>
> >  
> > -  <!-- Form 21 is submitted into an iframe, not declared here. -->
> > +  <!-- Form 21 (removed) is submitted into an iframe, not declared here. -->
> 
> I don't think the comments will be useful in the future. Can you renumber
> forms 22-24 by subtracting 2 to get rid of the gap and simplify the test
> please?
> 
> ::: toolkit/components/satchel/test/test_form_submission.html:245
> (Diff revision 1)
> >    <form id="form109" action="https://www.example.com/" onsubmit="return checkSubmit(109)">
> >      <input type="text" name="test9">
> >      <button type="submit">Submit</button>
> >    </form>
> >  
> > -  <!-- regular form data, when browser.formfill.saveHttpsForms is false -->
> > +  <!-- form 110 (removed): regular form data, when browser.formfill.saveHttpsForms is false -->
> 
> You probably don't need this comment since it's the last test being removed.

Will do in next commit


> ::: toolkit/components/satchel/test/test_form_submission.html:441
> (Diff revision 1)
> > -    SpecialPowers.clearUserPref("browser.formfill.saveHttpsForms");
> > -
> >    // End the test now on SeaMonkey.
> > -  if (formNum == 21 && navigator.userAgent.match(/ SeaMonkey\//)) {
> > +  if (formNum == 22 && navigator.userAgent.match(/ SeaMonkey\//)) {
> >      checkObserver.uninit();
> > -    is(numSubmittedForms, 21, "Ensuring all forms were submitted.");
> > +    is(numSubmittedForms, 20, "Ensuring all forms were submitted.");
> 
> Shouldn't this be 19 if you removed 20 and 21? Your mach test with Firefox
> won't run this check so you may not have noticed this failing but Seamonkey
> infrastructure would catch it.

I put 22 because I thought that at 21 we wanted to stop the tests on SeaMonkey. Considering test 21 was removed, I switched it to 22 to stop the tests at an "equivalent" place (makes sense?). Otherwise, changing it to 19 would not run the test on 19, which is not equivalent to the original behaviour, which ran the test at form 19.

> ::: toolkit/components/satchel/test/test_form_submission.html:454
> (Diff revision 1)
> > -
> >    // End the test at the last form.
> > -  if (formNum == 110) {
> > -    is(numSubmittedForms, 35, "Ensuring all forms were submitted.");
> > +  if (formNum == 109) {
> > +    is(numSubmittedForms, 32, "Ensuring all forms were submitted.");
> >      checkObserver.uninit();
> > -    SimpleTest.finish();
> > +    SimpleTest.executeSoon(SimpleTest.finish);
> 
> Is this the fix for the issue you had yesterday? What is the reason for this
> approach?

Putting `finish` inside `executeSoon` solved the problem of "log after finish" when running the whole suite of tests; it is not necessary when running `test_form_submission.html` only. This approach was already used in the "SeaMonkey conditional block" a few lines before, as explained here:

// finish(), yet let the test actually end first, to be safe.
SimpleTest.executeSoon(SimpleTest.finish);

> 
> ::: toolkit/components/satchel/test/test_form_submission.html:468
> (Diff revision 1)
> >    // This in itself is fine, but if there are errors in the code, mochitests
> >    // will in some cases give you "server too busy", which is hard to debug!
> >    //
> >    setTimeout(function() {
> >      checkObserver.waitForChecks(function() {
> > -      var nextFormNum = formNum == 24 ? 100 : (formNum + 1);
> > +      var nextFormNum = formNum == 19 ? 22 : formNum == 24 ? 100 : (formNum + 1);
> 
> If you were to need this in a future bug, I would recommend using brackets
> to make the precedence order clear.

With the shift of 2 forms in the next commit, the ternary will come back to its original format (3 terms rather than 5).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
The 3rd revision removed the `saveHttpsForms` preference in `all.js`
Comment on attachment 8866534 [details]
Bug 1361220 - Remove browser.formfill.saveHttpsForms support.

https://reviewboard.mozilla.org/r/138150/#review141812

::: toolkit/components/satchel/test/subtst_form_submission_1.html:9
(Diff revision 3)
> -<form id="subform1" onsubmit="return checkSubmit(21)">
> -  <input id="subtest1" type="text" name="subtest1">
> +<!-- subform1 removed, depended on savehttpsform -->
> +

Nit: please remove this since it's not going to be useful
Attachment #8866534 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Keywords: checkin-needed

Comment 10

5 months ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/cfea7128182d
Remove browser.formfill.saveHttpsForms support. r=MattN
Keywords: checkin-needed

Comment 11

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cfea7128182d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.