Closed Bug 1461247 Opened 5 years ago Closed 5 years ago

Only load FormSubmitObserver when an invalidformsubmit notification happens

Categories

(Firefox :: General, enhancement)

Product:
Firefox
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: Felipe, Assigned: Felipe)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p1])

Attachments

(3 files)

Bug 1461247 - Only load FormSubmitObserver.jsm when an invalidformsubmit notification happens.
59 bytes, text/x-review-board-request
MattN
: review+
Details
form-submit-alternate.diff
4.01 KB, patch
Details | Diff | Splinter Review
allow-trapping-of-QueryInterface-setter.diff
2.43 KB, patch
Details | Diff | Splinter Review 
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?
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.
(cleaned up the main patch a bit.. organizing all the XPCOMUtils.defineLazyProxy stuff together makes it easier to read)
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)
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
https://hg.mozilla.org/mozilla-central/rev/0398ab28d537
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.