Only load FormSubmitObserver when an invalidformsubmit notification happens

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

(Blocks 1 bug)

Trunk
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [fxperf:p1])

Attachments

(3 attachments)

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 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 hidden (mozreview-request)
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.
And this is another alternative to use defineLazyProxy but improve the readability a bit by allowing QueryInterface to be defined in the proxy.
Comment hidden (mozreview-request)
(cleaned up the main patch a bit.. organizing all the XPCOMUtils.defineLazyProxy stuff together makes it easier to read)
Comment hidden (mozreview-request)
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.
Attachment #8975390 - Flags: review?(MattN+bmo) → review+
(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

a year ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0398ab28d537
Only load FormSubmitObserver.jsm when an invalidformsubmit notification happens. r=MattN

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0398ab28d537
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.