Closed Bug 566288 Opened 12 years ago Closed 12 years ago

[e10s] Move FormHelper code into its own file


(Firefox for Android Graveyard :: General, defect)

Not set


(Not tracked)



(Reporter: vingtetun, Assigned: vingtetun)




(2 files, 5 obsolete files)

Attached patch wip (obsolete) — Splinter Review
The above patch try to move as much as possible the FormHelper's code in its own file and to load it into the chrome process only when necessary.
We could probably also try to lazy load it too in the content process.
Blocks: 516521
Attached patch Patch (obsolete) — Splinter Review
Patch is updated o the electrolysis trunk, also I think this will help to merge with m-b
Assignee: nobody → 21
Attachment #445659 - Attachment is obsolete: true
Attachment #447314 - Flags: review?(webapps)
Attachment #447314 - Flags: review?(mark.finkle)
Comment on attachment 447314 [details] [diff] [review]

On vacation for a week.  Matt, would you mind taking over my review?
Attachment #447314 - Flags: review?(webapps) → review?(mbrubeck)
Comment on attachment 447314 [details] [diff] [review]

Is Forms.js missing from this patch?
Attached patch Patch (obsolete) — Splinter Review
Sorry for the missing file and thanks for have noticed it.
Attachment #447314 - Attachment is obsolete: true
Attachment #449732 - Flags: review?(mbrubeck)
Attachment #449732 - Flags: review?(mark.finkle)
Attachment #447314 - Flags: review?(mbrubeck)
Attachment #447314 - Flags: review?(mark.finkle)
Attachment #449732 - Flags: review?(mbrubeck) → review+
Comment on attachment 449732 [details] [diff] [review]

>+ * Contributor(s):

Should have some contributors.

r+ otherwise.
Comment on attachment 449732 [details] [diff] [review]

>diff --git a/chrome/content/Forms.js b/chrome/content/Forms.js

Most of our JS files are lowercase and some of the uppercase ones are going away soon. Let's just call it "forms.js"

>+function ContentFormManager() {

What is the common name for this feature now? :) We called it FormHelper in code previously, but maybe we should call it FormAssist now? Any thoughts?

Whatever we come up with should be used in a common way. Like here, "ContentFormManager" -> "FormAsist" ?

>+  addMessageListener("FennecClosedFormAssist", this);
>+  addMessageListener("FennecFormPrevious", this);
>+  addMessageListener("FennecFormNext", this);
>+  addMessageListener("FennecFormChoiceSelect", this);
>+  addMessageListener("FennecFormChoiceChange", this);

Switch over to new naming convention for messages. Maybe,  "FormAssist:Previous", "FormAssist:Closed", etc.

>+    let wrapper = new BasicWrapper(element);

BasicWrapper scares me a little, but I am willing to tweak it a little and experiment with it later.

>+  closeFormAssistant: function() {

>+  closedFormAssistant: function() {

These methods are named too much alike. Comments for what they do could be helpful. Maybe use "closing" and "closed" as names?

>+  receiveMessage: Util.receiveMessage,

Kill this

>+ * Responsible for iterating through form elements.
>+ * The navigable list is generated on instantiation, so construct new FormNavigators when
>+ * the current document is changed or the list of form elements needs to be regenerated.
>+ */

We fixed some bugs in FormHelper that exist because the list is never updated. Showing and hiding controls was a bug we fixed. Let's make sure we don't regress.

Also, if no other code uses FormNavigator, let's just move this code into FormAssist, above. No need for a separate class.

>+ * A form assistant wrapper for content elements.
>+ */

e10s is getting the concept of "handles" - opaque, references to objets that live in the other process. Think of "handles" as an integer ID reference to an object. You can pass it across processes and back agina to "reference" live objects.

We might be able to evolve this code into something nicer with "handles"

>+function BasicWrapper(element) {

>+  if (!this._fac)
>+    BasicWrapper.prototype._fac = Cc[";1"].getService(Ci.nsIFormAutoComplete);

I don't think we want to deal with the autocomplete suggestions in this process. We can pass the name + id of the element to chrome and let autocomplete happen there. Thoughts?

>+  /** Can the next and previous buttons get to this element? Does not factor in visibility. */
>+  canNavigateTo: function canNavigateTo() {

>+    if (element instanceof HTMLInputElement || element instanceof HTMLButtonElement) {
>+      return !(element.type == "hidden")

Just a nit: The comment says "Does not factor in visibility"

>+  /** Create a list of suggestions for input autocomplete. Returns array of strings. */
>+  getAutocompleteSuggestions: function() {
>+    if (!this.canAutocomplete())
>+      return [];
>+    return [];
>+    // XXX
>+    // Autocomplete fetch does not work with input fields in content, which leads me to believe
>+    // setting autocomplete entries won't work when text is inputted into a box either.

Does this comment mean this code doesn't work? Moving it to chrome would help.

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml
>+          try {
>+   BasicWrapper(this));
>+          }
>+          catch(e) {
>+            let loader = Cc[";1"].getService(Ci.mozIJSSubScriptLoader);
>+            loader.loadSubScript("chrome://browser/content/Forms.js", this);
>+   BasicWrapper(this));

Not exactly happy about this. We can work on a better way in a followup

r- for the the questions and nits, but this patch is certainly good enough to start porting to mobile-browser. We can discuss any changes we want to make in the short term.
Attachment #449732 - Flags: review?(mark.finkle) → review-
Attached patch Patch v0.2 (obsolete) — Splinter Review
This is not the final version. 
I currently building it with mobile e10s just to be sure to have everything working on separate process but once it is finished i will updated it to the mobile-browser repository.
Also I still want to think about the autocomplete code.

Mark, i see handles as window HWND in some ways, I just don't get how can I retrieve it?
Attached patch Patch v0.3Splinter Review
Autocomplete is finally done on the chrome side just because it works.
The patch needs a few cleanup again especially in the message names and because the caret repositionning will probably not work.
Attachment #449732 - Attachment is obsolete: true
Attachment #450373 - Attachment is obsolete: true
Attachment #450644 - Flags: feedback?(mark.finkle)
Comment on attachment 450644 [details] [diff] [review]
Patch v0.3

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+    messageManager.addMessageListener("FormAssist:Start", this);
>+    messageManager.addMessageListener("FormAssist:Previous:Return", this);
>+    messageManager.addMessageListener("FormAssist:Next:Return", this);

I am thinking that we can drop "FormAssist:Previous:Return" and "FormAssist:Next:Return" and change "FormAssist:Start" to "FennecAssist:Show"

>+    messageManager.addMessageListener("FormAssist:End", this);

"FormAssist:End" -> "FormAssist:Hide", if we change to ""FormAssist:Start" -> "FormAssist:Show"

>+    messageManager.addMessageListener("FormAssist:CaretRect", this);

"FormAssist:MoveCaret" or "FormAssist:Update"

I like to use fewer specific messages if possible

>+      case "FormAssist:Start":
>+      case "FormAssist:Previous:Return":
>+      case "FormAssist:Next:Return": {
>+        json.showNavigation ? FormNavigator(json))
>+                            : FormWrapper(json.current));

Just using "FormAssist:Show" should work here. The JSON is all that is needed to make it work, right?

>   _getRectForCaret: function formHelper_getRectForCaret() {
>+    return;

Kill the entire function?

>   endSession: function() {
>-    Browser.sendMessage(getBrowser(), "FennecClosedFormAssist");
>+    Browser.sendMessage(getBrowser(), "FormAssist:Close");

We need to stop using Browser.sendMessage. Use getBrowser().messageManager.sendAsyncMessage(...)

Overall, this is looking good

>diff --git a/chrome/content/forms.js b/chrome/content/forms.js

Still reviewing this part...
Attachment #450644 - Flags: feedback?(mark.finkle) → feedback+
Attached patch wip v0.1 - mobile-browser (obsolete) — Splinter Review
First non-really working draft to move form-helper into its own file in mobile-browser
Attached patch Patch v0.1Splinter Review
This patch depends on bugs 573041 and 573443.
It port the code from mobile-e10s to mobile-browser. I think there is a lot of places where the code needs to be cleaned up but having it landed would be a good first step in my opinion.
Attachment #452683 - Attachment is obsolete: true
Attachment #452697 - Flags: review?(mark.finkle)
Comment on attachment 452697 [details] [diff] [review]
Patch v0.1

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml

>       <handler event="click" button="0">

> ;

Shouldn't you remove this line?

>+          try {
>+   BasicWrapper(this));
>+          }
>+          catch(e) {
>+            let loader = Cc[";1"].getService(Ci.mozIJSSubScriptLoader);
>+            loader.loadSubScript("chrome://browser/content/forms.js", this);
>+   BasicWrapper(this));

I still don't like this. Especially since we load all of forms.js just for the BasicWrapper class. We could at least put the wrappers in a separate file. Followup bug for sure.

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+    FormMessageReceiver.start();

Can't we roll the message code into the FormHelper code? Followup bug!

>+ * Responsible for handling the interface for navigating forms and filling in information.
>+ *  * Navigating forms is handled by next and previous buttons.
>+ *  * When an elemented is highlighted, the browser view zooms in to the control.

"When an element is focused..."

> var FormHelper = {

>+  _navigator: null,

I think I'd like to see the navigator code pulled into FormHelper in a followup bug

>+  get _selectContainer() {
>+    delete this._selectContainer;
>+    return this._selectContainer = document.getElementById("select-container");
>+  },

Looks like you are removing changes that Matt added to fix bug 572716. See:

I see that popup changes, dock and undock have been removed too.

>+  updateAutocompleteFor: function updateAutocomplete(aElement) {

I notice that we have several methods that follow the "xxxxYyyyFor" pattern. I can't decide if I like it or not. Sounds like it should be "xxxxYyyyForElement", but I'll file a followup bug if I feel like we should change it.

>-  dock: function dock(aContainer) {
>-    aContainer.insertBefore(this._panel, aContainer.lastChild);
>- = (window.innerHeight / 1.8) + "px";
>-    this._docked = true;

>-  undock: function undock() {
>-    let rootNode = Elements.stack;
>-    rootNode.insertBefore(this._panel, rootNode.lastChild);
>- = "";
>-    this._docked = false;

Stuff that shouldn't be removed?

>diff --git a/chrome/content/content.js b/chrome/content/content.js

> function Content() {

>-  this._contentFormManager = new ContentFormManager();
>+  this._contentFormManager = new FormAssistant();

nit: _contentFormManager -> _formAssistant

>diff --git a/chrome/content/forms.js b/chrome/content/forms.js

>+ * Contributor(s):
>+ *   Mark Finkle <>
>+ *   Matt Brubeck <>

Add yourself!

With the nits and the bug 572716 changes fixed, this is OK to land. However, I am still not happy with the navigator classes, wrappers and JSON emitted from the wrappers.

We can work to improve that code in followup bugs. Please file any followup bugs you can think of for now. I can do the same.
Attachment #452697 - Flags: review?(mark.finkle) → review+

For cleaning up the form assistant part into browser-ui.js I've opened bug 573560 and for the forms.js part i've opened bug 573561.
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.