1461247 - Only load FormSubmitObserver when an invalidformsubmit notification happens

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[:Felipe Gomes (needinfo for replies!)]

An instance of the FormSubmitObserver object is created for every tab (in content.js) only to add its observer for the "invalidformsubmit" notification.

We can add this observer in content.js and only load the .jsm when that happens.

This patch makes use of the helper being added in bug 1457988

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1461247 - Only load FormSubmitObserver.jsm when an invalidformsubmit notification happens.

Review commit: https://reviewboard.mozilla.org/r/243694/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243694/

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8975390 [details]
Bug 1461247 - Only load FormSubmitObserver.jsm when an invalidformsubmit notification happens.

https://reviewboard.mozilla.org/r/243694/#review249494

::: browser/base/content/content.js:51
(Diff revision 1)
>  // Load the form validation popup handler
> -var formSubmitObserver = new FormSubmitObserver(content, this);
> +var formSubmitObserver = XPCOMUtils.defineLazyProxy(null, null, () => {
> +  let tmp = {};
> +  ChromeUtils.import("resource:///modules/FormSubmitObserver.jsm", tmp);
> +  return new tmp.FormSubmitObserver(content, this);
> +}, { QueryInterface: XPCOMUtils.generateQI([Ci.nsIFormSubmitObserver, Ci.nsISupportsWeakReference])});

Nit: I think this should be ChromeUtils.generateQI?

::: browser/modules/FormSubmitObserver.jsm:5
(Diff revision 1)
>  /*
>   * Handles the validation callback from nsIFormFillController and
>   * the display of the help panel on invalid elements.
>   */
>  
>  "use strict";
>  
>  var EXPORTED_SYMBOLS = [ "FormSubmitObserver" ];
>  
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  ChromeUtils.import("resource://gre/modules/BrowserUtils.jsm");
>  
>  function FormSubmitObserver(aWindow, aTabChildGlobal) {
>    this.init(aWindow, aTabChildGlobal);
>  }
>  
> +// nsIFormSubmitObserver callback about invalid forms. See HTMLFormElement
> +// for details.

Maybe this comment should be merged with the one at the top of the file?

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8975390 [details]
Bug 1461247 - Only load FormSubmitObserver.jsm when an invalidformsubmit notification happens.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243694/diff/1-2/

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: form-submit-alternate.diff

I updated MozReview with the cleaner version that we talked about, and I'm attaching here the alternate approach that doesn't use defineLazyProxy for you to choose.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: allow-trapping-of-QueryInterface-setter.diff

And this is another alternative to use defineLazyProxy but improve the readability a bit by allowing QueryInterface to be defined in the proxy.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8975390 [details]
Bug 1461247 - Only load FormSubmitObserver.jsm when an invalidformsubmit notification happens.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243694/diff/2-3/

Comment 7

[:Felipe Gomes (needinfo for replies!)]

(cleaned up the main patch a bit.. organizing all the XPCOMUtils.defineLazyProxy stuff together makes it easier to read)

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8975390 [details]
Bug 1461247 - Only load FormSubmitObserver.jsm when an invalidformsubmit notification happens.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243694/diff/3-4/

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8975390 [details]
Bug 1461247 - Only load FormSubmitObserver.jsm when an invalidformsubmit notification happens.

https://reviewboard.mozilla.org/r/243694/#review250938

::: browser/base/content/content.js:51
(Diff revision 4)
>    return new ContextMenu(global);
>  });
>  
> +XPCOMUtils.defineLazyProxy(this, "formSubmitObserver", () => {
> +  return new FormSubmitObserver(content, this);
> +}, { QueryInterface: ChromeUtils.generateQI([Ci.nsIFormSubmitObserver, Ci.nsISupportsWeakReference])});

Nit: I think it would be easier to read with the property on its own line:
```js
…
}, {
  QueryInterface: ChromeUtils.generateQI([Ci.nsIFormSubmitObserver, Ci.nsISupportsWeakReference])
});
```

::: browser/base/content/content.js:56
(Diff revision 4)
> -// Load the form validation popup handler
> +Services.obs.addObserver(formSubmitObserver, "invalidformsubmit", true);
> -var formSubmitObserver = new FormSubmitObserver(content, this);

I have no idea what the test coverage of this is like (and whether they are enabled) so please do a manual test and/or check that automated tests run in automation.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #9)x
> …
> }, {
>   QueryInterface: ChromeUtils.generateQI([Ci.nsIFormSubmitObserver,
> Ci.nsISupportsWeakReference])
> });
> ```
> 
> ::: browser/base/content/content.js:56
> (Diff revision 4)
> > -// Load the form validation popup handler
> > +Services.obs.addObserver(formSubmitObserver, "invalidformsubmit", true);
> > -var formSubmitObserver = new FormSubmitObserver(content, this);
> 
> I have no idea what the test coverage of this is like (and whether they are
> enabled) so please do a manual test and/or check that automated tests run in
> automation.

They are disabled due to being intermittent, but I ran them locally and they pass:
  browser_form_validation.js, browser_validation_iframe.js, browser_validation_invisible.js

(The first one has a testcase that was never fixed for e10s, so it fails with e10s, but it's unrelated to this change)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0398ab28d537
Only load FormSubmitObserver.jsm when an invalidformsubmit notification happens. r=MattN

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/0398ab28d537