Closed
Bug 1461247
Opened 5 years ago
Closed 5 years ago
Only load FormSubmitObserver when an invalidformsubmit notification happens
Categories
(Firefox :: General, enhancement)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: Felipe, Assigned: Felipe)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fxperf:p1])
Attachments
(3 files)
59 bytes,
text/x-review-board-request
|
MattN
:
review+
|
Details |
4.01 KB,
patch
|
Details | Diff | Splinter Review | |
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 hidden (mozreview-request) |
Comment 2•5 years ago
|
||
mozreview-review |
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) |
Assignee | ||
Comment 4•5 years ago
|
||
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.
Assignee | ||
Comment 5•5 years ago
|
||
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) |
Assignee | ||
Comment 7•5 years ago
|
||
(cleaned up the main patch a bit.. organizing all the XPCOMUtils.defineLazyProxy stuff together makes it easier to read)
Comment hidden (mozreview-request) |
Comment 9•5 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 10•5 years ago
|
||
(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•5 years 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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0398ab28d537
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Assignee | ||
Updated•5 years ago
|
Blocks: memshrink-content
You need to log in
before you can comment on or make changes to this bug.
Description
•