Closed Bug 1361220 Opened 7 years ago Closed 7 years ago

Remove browser.formfill.saveHttpsForms support

Categories

(Toolkit :: Form Manager, enhancement)

Product:
Toolkit
Component:
Form Manager
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: MattN, Assigned: jonathanGB, Mentored)

References

(
URL
)

Details

Attachments

(1 file)

Bug 1361220 - Remove browser.formfill.saveHttpsForms support.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
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 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).
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+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/cfea7128182d
Remove browser.formfill.saveHttpsForms support. r=MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cfea7128182d
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: