897061 - [e10s] Basic support for form autocomplete

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Felipe said that we collect the data correctly with a content script (formSubmitListener.js), but the autocompletion code isn't e10s-ready (autocomplete.xml ..).

Comment 2

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

I'm surprised this wasn't duped to 516762 instead of the other way around.

Comment 3

[Tom S [:evilpie]]

(In reply to Mark Hammond (:markh) from comment #2)
> I'm surprised this wasn't duped to 516762 instead of the other way around.
Both bugs had about zero content, so what gives?

So I started looking into this, kinda scratching at the surface of course. It looks like in normal mode we have a node with autcomplete.xml binding per browser. When we switch tabs we call "attachFormFill" and attach nsIFormFillController to the browser's docShell and passing a reference to the global autocomplete node. This obviously won't work, we would need an instance of that autcomplete box per content process. Or maybe we want to proxy this stuff to chrome? I am not sure what the best solution here is. I guess we don't really want to have xul elements in the content at all. Somebody knows what fennec/b2g did/do for this?

Comment 4

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Tom Schuster [:evilpie] from comment #3)
> (In reply to Mark Hammond (:markh) from comment #2)
> > I'm surprised this wasn't duped to 516762 instead of the other way around.
> Both bugs had about zero content, so what gives?

It just surprised me :)  Generally when neither have content the earliest bug is used.  No big deal though.

> So I started looking into this, kinda scratching at the surface of course.
> It looks like in normal mode we have a node with autcomplete.xml binding per
> browser. When we switch tabs we call "attachFormFill" and attach
> nsIFormFillController to the browser's docShell and passing a reference to
> the global autocomplete node. This obviously won't work, we would need an
> instance of that autcomplete box per content process. Or maybe we want to
> proxy this stuff to chrome? I am not sure what the best solution here is. I
> guess we don't really want to have xul elements in the content at all.
> Somebody knows what fennec/b2g did/do for this?

Bug 691601 comment 1 talks a little about this.  I'm not sure about fennec/b2g, but metro did some e10s related work (even though they don't use e10s now IIUC) and I refer to that in 691601 - the code should be easy to find in the browser/metro directory.  In short, they seem to have reimplemented some things in relatively incomplete ways, whereas I think we should take a different approach (ie, re-implement the form controller itself, upgrade panels so they can be anchored against an element in a different process, etc).  Sadly not trivial tasks, so it might be necessary to take a short-term pragmatic approach in the meantime.

Comment 5

[Tom S [:evilpie]]

Attachment: debugging work

Felipe was interested in what I got done here. I think the change to nsFormAutoComplete.js was because we couldn't read actually results from the databse. For some reason I had to initialize the formfillcontroller later otherwise it wouldn't work.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Attachment: formautocomplete - checkpoint 1

Hi, here's an update on the ongoing work here:

I've had a hacky-working version of form autocomplete, and I've now began moving code around to better/proper places for a good implementation. This patch is the start of that.

Instead of relying on the framescripts this is now working mostly through the nsIFormAutocomplete component, which gets registered with the proper component in the parent (that will do the calls to FormHistory et al), and a stub component in the child process which will forward the requests to the parent through the process message manager.

I'm still trying to see if I can do without having to implement a fake nsIAutoCompletePopup in the child, because all of the popup code should be in the parent, but i'm still not sure if it will be possible without having to refactor a bunch of code in formFillController.. so it might be it's just not worth it.

Also, I might use a cpow to get info about the positioning of the dropdown, but I'm trying to avoid that.

FTR the form submission part, i.e. adding submitted entries to the formhistory database, already works properly with no changes.

I expect to have patches for feedback and review by next week. This patch is a bunch of the main patches on my queue merged together, so there's debugging and it's not cleaned up.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Oh, another thing to mention (reminder to self): with the split of nsFormAutocomplete.js I need to make sure to not break android/metro which might be using the module differently

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: autocomplete - wip 2

Here's an updated patch! So far I had been using a popup code based off bug 897060, but ultimately that had some problems with being a very different bind & styling, and to make all the necessary features work with it would be complicated.  So I've scrapped all of the code :P and replaced it with something better that works similarly to how autocomplete works now. The code now creates a treeview implementation with that data results from the autocomplete search, which is then assigned to the existing autocomplete popup and it draws the data automatically from the treeview.

With the new structure it was also easier to properly position and size the popup, and it just works now.

I still think it's possible to couple things more tightly with the autocomplete controller code, but I'm not gonna do it in this bug because what we've got now is good enough to use the feature, and I can iterate on it with the patches in the tree afterwards.

What's left now is to implement the proper key hooks to support up/down/enter keys to work in the popup. I think I'll need to implement a stub controller in the parent for that.

There are two remaining issues. There's some invalidation issue with the popup tree (the items are filled and you can click on them, but they are not displayed), but I'm ignoring it for now because I think it will go away after the controller is implemented (if I press keydown everything appears so I think it's related to the scroll info of the popup).

And a minor one is that none of the code now goes through the front-end anymore, except one usage of RecentWindow.jsm. I think for that I'll just replace it with the direct Services.wm call.


Bill, a review request should come soon, but if you want to already take a look at the changes, especially nsFormAutoComplete.js and AutoCompleteE10S.jsm, feel free. There's still debugging/logging code around but I didn't want to remove them just yet

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8334887 [details] [diff] [review]
autocomplete - wip 2

Review of attachment 8334887 [details] [diff] [review]:
-----------------------------------------------------------------

I mostly just looked at the message manager aspects.

::: toolkit/components/satchel/AutoCompleteE10S.jsm
@@ +61,5 @@
> +
> +this.AutoCompleteE10S = {
> +  search: function(message) {
> +    let browserWindow = RecentWindow.getMostRecentBrowserWindow();
> +    this.browser = browserWindow.gBrowser.selectedBrowser;

As we discussed, I think that it would be better to use globalmessagemanager. Then you can get the browser element as message.target.

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +45,5 @@
> +      this._pendingListener = aListener;
> +
> +      let rect = aField.getBoundingClientRect();
> +
> +      this.cpmm.sendAsyncMessage("FormAutoComplete:AutoCompleteSearchAsync", {

I asked Olli, and you should be able to get a regular message manager here as follows. First, you need a docshell. I'm pretty sure you should be able to get one through aField or something. Then you can do docShell.getInterface(Ci.nsIContentFrameMessageManager).

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: nsFormAutoComplete and tree view

Hi Mark, this implements the main code that makes autocomplete e10s-enabled, by creating stub implementations of some of the autocomplete interfaces in the content, which communicates with the related implementation in the parent.

Some glue code to make the event handling on the popup work is split into another patch

(In reply to Bill McCloskey (:billm) from comment #9)
> Comment on attachment 8334887 [details] [diff] [review]
>
> ::: toolkit/components/satchel/AutoCompleteE10S.jsm
> @@ +61,5 @@
> > +
> > +this.AutoCompleteE10S = {
> > +  search: function(message) {
> > +    let browserWindow = RecentWindow.getMostRecentBrowserWindow();
> > +    this.browser = browserWindow.gBrowser.selectedBrowser;
> 
> As we discussed, I think that it would be better to use
> globalmessagemanager. Then you can get the browser element as message.target.

Thanks Bill/Olli, I implemented this suggestion and it worked nicely; now we know exactly which window the request came from, and there are no dependencies front-end (browser/) code anymore.

Comment 11

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8341946 [details] [diff] [review]
nsFormAutoComplete and tree view

Review of attachment 8341946 [details] [diff] [review]:
-----------------------------------------------------------------

Just some nits and/or suggestions, but LGTM.  I'm also interested to know how this is being tested (but not expecting a good answer given e10s testing is still broken)?

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +22,5 @@
> +/**
> + * FormAutoComplete
> + *
> + * Implements the nsIFormAutoComplete interface in the main process
> + * and performs the autoComplete actions a child content process,

Did you mean "actions *in* a child content process"?  Also, this is a bit wordy - probably just a sentence here, then "It forwards the auto-complete..."

@@ +316,5 @@
> + * FormAutoCompleteChild
> + *
> + * Implements the nsIFormAutoComplete interface in a child content process,
> + * and forwards the auto-complete requests to the parent process which
> + * implements also implements a nsIFormAutoComplete interface and has

extra "implements" here.

@@ +330,2 @@
>  
> +    _debug: true,

this should probably be reset to false before checkin?

@@ +351,5 @@
> +    /*
> +     * log
> +     *
> +     * Internal function for logging debug messages to the Error Console
> +     * window

I don't think dumps go to the Error Console?  I wonder if Console.jsm (or even Log.jsm) would be a good choice (but to be honest, so long as this._debug is false, I don't really care)

@@ +481,5 @@
>      }
>  };
>  
> +
> +if (Services.prefs.getBoolPref("browser.tabs.remote") &&

do we really need to check the pref here?  Isn't the processType check good enough?

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Attachment: Autocomplete controller

And this is the controller code, used to properly handle keyboard usage for autocomplete (i.e., pressing up/down to navigate through results, Enter to select, Esc to close..)

The real controller is nsAutoCompleteController.cpp and runs in the child, and this patch implements the minimal supporting interface in the parent to make a bridge between the processes. The exising AutoCompleteE10SView object is used to implement nsIAutoCompleteController, because that makes it look the same as the current setup.


Implementation is straightforward, and there's a small hack to workaround .openPopup not triggering the popupshowing event, which I will follow-up separately.


The only sucky part is that we need to send the autocomplete results to the child process, which should in theory be avoidable. But it is necessary due to the following reason: 
ultimately the function that fills the textbox with the selection is nsAutoCompleteController::EnterMatch. That function could work by asking the selected value from the popup, but instead it asks only for the selectedIndex, as the controller is meant to have cached the ordered result list.
So there's a bit of back-and-forth to send the results back to the child and listen to it through the correct message manager.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

(In reply to Mark Hammond [:markh] from comment #11)
> Comment on attachment 8341946 [details] [diff] [review]
> nsFormAutoComplete and tree view
> 
> Review of attachment 8341946 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just some nits and/or suggestions, but LGTM.  I'm also interested to know
> how this is being tested (but not expecting a good answer given e10s testing
> is still broken)?

I honestly don't know yet what will the testing situation be for this feature, but I suspect the most painful part will be coordinating text input in the child while expecting the popup to open in the parent.. I'll look at how it should be tested later.

> 
> ::: toolkit/components/satchel/nsFormAutoComplete.js
> @@ +22,5 @@
> > +/**
> > + * FormAutoComplete
> > + *
> > + * Implements the nsIFormAutoComplete interface in the main process
> > + * and performs the autoComplete actions a child content process,
> 
> Did you mean "actions *in* a child content process"?  Also, this is a bit
> wordy - probably just a sentence here, then "It forwards the
> auto-complete..."

Yeah, will fix.

> 
> @@ +316,5 @@
> > + * FormAutoCompleteChild
> > + *
> > + * Implements the nsIFormAutoComplete interface in a child content process,
> > + * and forwards the auto-complete requests to the parent process which
> > + * implements also implements a nsIFormAutoComplete interface and has
> 
> extra "implements" here.
> 
> @@ +330,2 @@
> >  
> > +    _debug: true,
> 
> this should probably be reset to false before checkin?

yep

> 
> @@ +351,5 @@
> > +    /*
> > +     * log
> > +     *
> > +     * Internal function for logging debug messages to the Error Console
> > +     * window
> 
> I don't think dumps go to the Error Console?  I wonder if Console.jsm (or
> even Log.jsm) would be a good choice (but to be honest, so long as
> this._debug is false, I don't really care)

Ah I just copied this function from the other obj in this file.. I was meaning to look if Services.console.log works from the child process but the dump() was enough logging for me so I never got around to it.

> 
> @@ +481,5 @@
> >      }
> >  };
> >  
> > +
> > +if (Services.prefs.getBoolPref("browser.tabs.remote") &&
> 
> do we really need to check the pref here?  Isn't the processType check good
> enough?

It is for desktop, but I want to prevent this code change to affect the other apps. I know that at least Metro is using nsFormAutocomplete in a different way, and I suspect changing the object in the child will affect it and also possibly Android.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8343506 [details] [diff] [review]
Autocomplete controller

Review of attachment 8343506 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/satchel/AutoCompleteE10S.jsm
@@ +116,5 @@
>      this.popup.invalidate();
>  
>      if (count > 0) {
> +      this.popup.openPopup(this.browser, "overlap", this.x, this.y, true, true);
> +      this.popup.mPopupOpen = true;

Oh and I added this comment here:

>  // This openPopup call is not triggering the "popupshowing" event, which
>  // autocomplete.xml uses to track the openness of the popup by setting
>  // its mPopupOpen flag. This flag needs to be properly set for closePopup
>  // to work. For now, we set it manually.

Comment 15

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8343506 [details] [diff] [review]
Autocomplete controller

Review of attachment 8343506 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me.  r=me with the followups you mentioned filed and bug numbers added to comments around the affected areas.

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +396,5 @@
> +            null
> +          );
> +          if (aListener) {
> +            aListener.onSearchCompletion(result);
> +          } 

nit: trailing space

::: toolkit/content/browser-child.js
@@ +257,5 @@
>    },
>  
>    get input () { return this._input; },
>    get overrideValue () { return null; },
> +  set selectedIndex (index) { },

I'm surprised this is a noop.  If there is something todo here, then a reference to a followup bug number would be good - otherwise can it throw?

@@ +267,5 @@
> +    // selectedIndex is trivial to catch (e.g. moving the mouse over the
> +    // list).
> +    return sendSyncMessage("FormAutoComplete:GetSelectedIndex", {});
> +  },
> +  get popupOpen () { 

trailing space

Comment 16

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10edfe4c3208

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Updating the title to be a bit better, and I filed bug 947503 and referenced it in the comment.

The selectedIndex setter is really a no-op now because it doesn't need to be set from the content, at least for the feature set that works at the moment. If it needs to be implemented it will end up naturally as part of some other bug.

I have a list of things necessary to achieve feature completeness and I'll file them shortly.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10edfe4c3208