950797 - Flickering in URLBar while entering URL bar that is the current top match

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Julian Viereck]

Assuming the Firefox profile contains history entries for "www.facebook.com" and other sites that match "faceb".

# Steps to reproduce:

1. delete the current URL shown in the URLBar
2. enter "fa"
3. (Firefox completes the URL based on the history entries and completes it to "fa|cebook.com", where "|cebook.com" is the completed part, which is selected"
4. type as next character a "c"
5. the text shows up as "fac" for a short time (depending on length the search query takes, the short time goes towards zero and won't be noticeable)
6. after a delay the urlBar gets autofilled to "fac|ebook.com"

# Expected behavior:

Step 5. should never be noticeable. This small delay causes the URL to flicker while typing in the URL for "facebook.com".


This is a regression introduced by bug 791776.

Comment 1

[Julian Viereck]

I took a look at how this bug could be fixed. At a high level, this is my proposal for a possible fix:

a) the urlbarBindings.xml (which is used for the URLBar in Firefox) inherits from autocomplete.xml. I guess this flickering/bug is worth fixing for all autocomplete elements in Firefox/Gecko, therefore the fix should be placed in autocomplete.xml

b) the autocomplete element should remember the last completion if one has happened. E.g. in the case above the completion for "fa" is "fa|cebook.com" and "facebook.com" should be stored as last completion

c) as the next character is typed it is checked if the new value of the autocomplete input field matches the last completed text and in such a case, the rest of the last completed match is put in the input field and is made selected. E.g. if after "fa" the user types "c" and the last completion is "facebook.com", then the autocomplete input field should show "fac|ebook.com" (even before the search result is available).


From the implementation perspective, I don't think there is a way from the autocomplete.xml to query for the last completed value. Therefore I would introduce a new SetCompleteTextValue(...) function on the nsIAutoCompleteInput.idl that the completion controller can call. In the case of the URLBar, the calls to |mInput->SetTextValue(...)| from (see [1])

  nsAutoCompleteController::CompleteValue(nsString &aValue)

should be replaced with |mInput->SetCompleteTextValue(...)|.

Once the autocomplete widget knows about the completed string, step (c) from the high-level idea can be implemented in the autocomplete.xml file.



[1]: http://dxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp?from=nsAutoCompleteController.cpp#1572

Comment 2

[Julian Viereck]

(In reply to Julian Viereck from comment #1)
> I took a look at how this bug could be fixed. At a high level, this is my
> proposal for a possible fix:
> 

Does this make sense and is worth getting implemented?

Comment 3

[Julian Viereck]

(In reply to Julian Viereck from comment #1)
>
> From the implementation perspective, I don't think there is a way from the
> autocomplete.xml to query for the last completed value. Therefore I would
> introduce a new SetCompleteTextValue(...) function on the
> nsIAutoCompleteInput.idl that the completion controller can call. In the
> case of the URLBar, the calls to |mInput->SetTextValue(...)| from (see [1])
> 
>   nsAutoCompleteController::CompleteValue(nsString &aValue)
> 
> should be replaced with |mInput->SetCompleteTextValue(...)|.

Thinking about this once more, instead of introducing a new API it might be enough to store the current text input value in the autocomplete widget once the |OnSearchComplete(...)| function is called and use it as the last completed value.

Comment 4

[Julian Viereck]

(In reply to Julian Viereck from comment #3)
>
> Thinking about this once more, instead of introducing a new API it might be
> enough to store the current text input value in the autocomplete widget once
> the |OnSearchComplete(...)| function is called and use it as the last
> completed value.

Looking into this more closely, using |OnSearchComplete(...)| is not useful as the function seems to be called before the new completed value gets put into the DOM. Therefore looking at the input field's value when |OnSearchComplete(...)| is called and using it for completion does not work.

Anyone has an idea to get hold of the completed value without introducing a new API function?

Comment 5

[Julian Viereck]

Attachment: bug-950797-flickering-fix

Implementation mostly done as outlined in the comments. Added a new method to |nsIAutoCompleteInput| (called |onInputComplete|), which is called as the input gets completed. The completed input is stored and used for follow up changes to complete the input field in case the user types in new characters that match the last completed value.

Comment 6

[Julian Viereck]

Try server submission is here: https://tbpl.mozilla.org/?tree=Try&rev=a781f56f7883

Comment 7

[Julian Viereck]

Attachment: bug-950797.diff

Fixed an edge case where the completion happened during a composition took place. The new iteration of the patch checks if the nsAutoCompleteController is in the composition state and if so the completion based of the old completion value does not happen.

New try-server run is here: https://tbpl.mozilla.org/?tree=Try&rev=f4b9e7f8caac

Comment 8

[Asaf Romano (gone)]

Any chance for unit test?

Comment 9

[Asaf Romano (gone)]

By the way, you need to rev the UUID of the interface changed.

Comment 10

[Julian Viereck]

Attachment: Updated version of patch addressing comment #8 & #9

> Any chance for unit test?

Done.

> By the way, you need to rev the UUID of the interface changed.

Good catch - thanks! Done.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Julian Viereck from comment #1)
> c) as the next character is typed it is checked if the new value of the
> autocomplete input field matches the last completed text and in such a case,
> the rest of the last completed match is put in the input field and is made
> selected. E.g. if after "fa" the user types "c" and the last completion is
> "facebook.com", then the autocomplete input field should show
> "fac|ebook.com" (even before the search result is available).

I don't think we can do this, for a simple reason: adaptive autocomplete.
Basically "fa" and "fac" may map to completely different results, due to adaptive autocomplete it's not given that when you add a "c" the result you will get is the same as before.
For clarity, when you type "fa" and select an entry (let's say "facebook"), we store that you selected that entry... then you may type "fac" and select something else (let's say "faces")... when you will type "fa" the first result will be facebook, once you add a "c" the first result will be faces.

Now, this may be re-evaluated but it's the primary reason we can't reuse results on typing.

Comment 12

[Julian Viereck]

Marco, thanks for your input on this. 

(In reply to Marco Bonardo [:mak] from comment #11)
> For clarity, when you type "fa" and select an entry (let's say "facebook"),
> we store that you selected that entry... then you may type "fac" and select
> something else (let's say "faces")... when you will type "fa" the first
> result will be facebook, once you add a "c" the first result will be faces.

You are right, that the search needs to be non-adaptive. I tried to get a non-adaptive result when typing in the URL, but I could not. Steps to reproduce:

1. create a new Firefox profile
2. clear the URL bar, type "facebook.com" and hit enter
3. wait for the page page to load
4. clear the URL bar, type "faces.com" and hit enter
5. wait for the page page to load
6. clear the URL bar, type "fa", click "facebook.com" from the popup menu which makes the site start to load
7. wait for the page page to load
8. clear the URL bar, type "fac", select "faces.com" from the popup menu which makes the site start to load

If I now type "fa" in the URL bar, it gets completed to "facebook.com". If I type an additional "c" (the URL text is then "fac"), the completion is still "facebook.com". However, reading your comment, "fac" should complete to "faces.com". 

Am I missing something here or is the URL search non-adaptive?

(In reply to Marco Bonardo [:mak] from comment #11)
> I don't think we can do this, for a simple reason: adaptive autocomplete.
> Basically "fa" and "fac" may map to completely different results, due to
> adaptive autocomplete it's not given that when you add a "c" the result you
> will get is the same as before.

Good point. Personally I don't think it is good UX to show a different completion text although the user typed a character that matches the previous completion. Nevertheless, developers might write this kind of adaptive autocomplete algorithms. Therefore, how about this:

i) The completion introduced in this bug only kicks in if the developer sets an attribute on the autocomplete box (e.g. "deterministic"?)
ii) Change the search for matching URLs to be non-adaptive

What do you think?

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Ok nevermind, I thought we were speaking about a popup flickering. Adaptive results only apply to the popup. Instead I just re-read everything and this is a flickering due to inline autocomplete, then my adaptive observations don't apply.

Thus yes, I think it makes sense for the autocomplete to not query again if the user only adds another letter that matches what is already completed. Actually I never noticed any flickering, but it's likely due to the fact my computers are quite fast.

I didn't look deeply at the patch, at first glance looks like there's some typo due to onInputComplete vs onInputCompleted (notice the final d), the approach must be evaluated with caution, since any autocomplete change is very prone to regressions, and limited as far as possible to only act in very specific cases. Firefox has very different autocomplete behavior to other consumers, we don't use the first popup entry, so for us this is fine, but other consumers may have different behaviors we can't predict. So, I'd probably limit the fix to our urlbar binding implementation.

Comment 14

[Julian Viereck]

(In reply to Marco Bonardo [:mak] from comment #13)
> [...] the
> approach must be evaluated with caution, since any autocomplete change is
> very prone to regressions, and limited as far as possible to only act in
> very specific cases.

Good point, thanks for raising it Marco.

To enable this feature only for selected autocomplete boxes, I have introduced a new property on the autocomplete box along these lines:

      <property name="useLastCompletion" onget="return this.getAttribute('uselastcompletion');">
        <setter><![CDATA[
          this.mUseLastCompletion = val == "true";
          this.setAttribute('uselastcompletion', val); return val;
          if (!this.mUseLastCompletion) {
            this._lastCompletion = null;
          }
        ]]></setter>
      </property>

      <method name="onInputCompleted">
        <body><![CDATA[
          // Only store the completed value if the autocomplete box completes
          // the user's input based on the last result.
          if (this.mUseLastCompletion) {
            this._lastCompletion = this.value;
            this._lastCompletionPrefixLength = this.mInputField.selectionStart;          
          }
        ]]></body>
      </method>

Does this looks good? Especially, I am very unsure if the way I have defined the new property makes sense at all.

Now that there is this new property on the autocompletebox, how can I turn it on for the Firefox urlbar --- can you give me a hint?

Comment 15

[Julian Viereck]

Attachment: WIP: 0.2

Use the last match to complete new typed in input only if the autocomplete box has 'autocompleteuselastcompletion' attribute set.

Try run at: https://tbpl.mozilla.org/?tree=Try&rev=4424bcd47a60

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Marco, is this something you could mentor? Could this be one of our [diamond] bugs maybe?

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #16)
> Marco, is this something you could mentor? Could this be one of our
> [diamond] bugs maybe?

Comment 18

[Julian Viereck]

(Just dumping some thoughts. I don't think I will have time to work on this anytime soon.)

In attachment 8369217 [details] [diff] [review] I tried to fix the problem in the autocomplete.xml widget. Maybe it would be better to implement the fix (aka. store the result of a search and reuse it if a new search-string matches the old search result) in the nsAutoCompleteController controller.

This might be better as:
- in the current patch I had to expose an internal state of the search controller (look for |nsAutoCompleteController::IsComposing|) to the view, which already indicates the attempt to tackle the problem might be wrong
- there is also the e10s version of autocomplete.xml that needs to be fixed as well. If we implement the fix in the controller, than we might fix the e10s code as well for free?

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Tim Taubert [:ttaubert] from comment #16)
> Marco, is this something you could mentor? Could this be one of our
> [diamond] bugs maybe?

The request here languished cause this code is scary, and reviewing any change like this one takes ages (days more realistically, and in the current process there's no "days" available to do a review, for me at least). So, it may surely be a mentored diamond bug, but I'm not sure I have enough time resources to follow it in a timely manner. If someone else wants to mentor it, sure thing!

(In reply to Julian Viereck from comment #18)
> In attachment 8369217 [details] [diff] [review] I tried to fix the problem
> in the autocomplete.xml widget. Maybe it would be better to implement the
> fix (aka. store the result of a search and reuse it if a new search-string
> matches the old search result) in the nsAutoCompleteController controller.

Surely a more centralized fix in controller would largely help, and I think for autoFill we should always try to avoid replacing a result value with the same value. So, it may be the right idea, but it's hard to tell without actually seeing which points in controller this is going to touch, and how consumers may react to those changes. autocomplete has LOTS of consumers.

Comment 20

[Julian Viereck]

Attachment: WIP: 0.3

This is based on Comment #18. As it turned out, the change is quite small. As Marco pointed out, changes to autocomplete can cause big problems. Therefore, the changes take only affect, if the property |useLastCompletion| is set to |true| on the |nsAutoCompleteController|. Whenever a new input element is set on the controller, this property is flipped to false. It is set to true only in the |autocomplete.xml| at the moment.

@mak: you mentioned you don't have much time for reviews. Can you still take a look at this or reassign it to someone else?

Comment 21

[Julian Viereck]

@mak, *PING*. Do you plan to take a look at the patch in attachment 8447747 [details] [diff] [review] or should I move the review to someone else?

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I'm really sorry, but I don't have a good answer off-hand :/ Due to the scary nature of this cude it requires quite some continuous hours to check it's not going to regress stuff and I couldn't find a large enough timeframe so far. I have also an hard time suggesting someone else, since the other persons with good knowledge here are Gavin and Enn, that unlikely have that time.
I'll try to look at it by end of the week, feel free to ping me again in case I miss that.

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8447747 [details] [diff] [review]
WIP: 0.3

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

paolo offered to do a first pass here, we discussed the approach briefly on vidyo

Comment 24

[:Paolo Amadini]

Comment on attachment 8447747 [details] [diff] [review]
WIP: 0.3

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

I did a first pass review, and I already have some comments that I hope may help in speeding up the rest. It will still take some time to be completed, and I'll not be able to do another pass until a week or two from now, but I think we are on the right track.

First, I'd separate the actual feature and its test from the addition of the attribute that controls its activation. If we test the implementation accurately, I'm not completely sure that the attribute will be required, but even if we don't land that part, having the patch ready could be valuable in case we identify the need for the attribute later.

In any case, having two patches on this bug will simplify the review. I think the attribute patch should apply on top of the implementation and test patch.

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +258,5 @@
> +  // is used for a synchronous completion below.
> +  bool doUseLastCompletion = mUseLastCompletion && !mBackspaced &&
> +    // The new input string needs to be compatible with the last completed string.
> +    // E.g. if the new value is "fob", but the last completion was "foobar",
> +    // then the last completion is incompatible.

style note: please avoid comments in-between statements.

@@ +1628,4 @@
>      // aValue is empty (we were asked to clear mInput), or mSearchString
>      // matches the beginning of aValue.  In either case we can simply
>      // autocomplete to aValue.
> +    mLastSearchCompleteString = aValue;

I don't think the CompleteValue function is the place where mLastSearchCompleteString should be set. It should only place the string in the textbox. mLastSearchCompleteString should be set in the callers.

::: toolkit/content/tests/chrome/test_autocomplete_complete_previous_value.xul
@@ +14,5 @@
> +          src="chrome://global/content/globalOverlay.js"/>
> +
> +<textbox id="autocomplete"
> +         type="autocomplete"
> +         autocompleteuselastcompletion="true"

The attribute name could use some work, though I don't have a suggestion that stands out. Maybe "autocompletekeepresultwhiletyping". Marco may have better suggestions.

@@ +173,5 @@
> +  // Typing a character that is not like the previous match. In this case, the
> +  // completion cannot happen directly and therefore the value will be completed
> +  // only after the search has finished.
> +  synthesizeKey("t", {});
> +  is(gAutoComplete.value, "ret", "Value should not be completion immediately");

completion => autocompleted

@@ +183,5 @@
> +  is(gAutoComplete.value, "retire", "Value should be completed immediately");
> +
> +  yield undefined;
> +
> +  is(gAutoComplete.value, "retire", "Value should be autocompleted to the same value");

We should do two different tests here in place of this one. The first:
 - Tell the search result object to return "retirement" instead of "retire".
 - Type the "i".
 - Check that the value is synchronously completed to "retire".
 - Check that, after the result arrived, the value is now completed to "retirement".

The second:
 - Tell the search result object to return "retire" again.
 - Type "rem".
 - Check that the value is synchronously completed to "retirement".
 - Check that, after the result arrived, the value is now "retirem".

I'm not sure if these tests would work out of the box because we do reuse results sometimes, but making them work is valuable.

The other tests are really spot on.

@@ +205,5 @@
> +
> +  yield undefined;
> +
> +  synthesizeKey("t", {}); // (3.2)
> +  is(gAutoComplete.value, "reti", "Inserting a character in the middle should not complete the alue");

alue => value

Comment 25

[Julian Viereck]

Thanks for your review :Paolo! I will have a look at it more closely during next week.

Comment 26

[Julian Viereck]

Here my feedback based on :Paolo's review.

(In reply to :Paolo Amadini [Away Aug 2 - Aug 10] from comment #24)
>
> First, I'd separate the actual feature and its test from the addition of the
> attribute that controls its activation. If we test the implementation
> accurately, I'm not completely sure that the attribute will be required, but
> even if we don't land that part, having the patch ready could be valuable in
> case we identify the need for the attribute later.
>
> In any case, having two patches on this bug will simplify the review. I
> think the attribute patch should apply on top of the implementation and test
> patch.

Agreed. I will move the tests under

  toolkit/completions/autocomplete/tests/unit/...


While I was reading over the code a little bit more, I think it makes more sense to store the "completeLastCompletionSync" flag on the nsIAutoCompleteInput (as defined in nsIAutoCompleteInput.idl) and not as a new flag on the nsAutoCompleteController. This should be more consistent as nsIAutoCompleteInput already has fields like |forceComplete|, which |completeLastCompletionSync| is very similar to.

> I'm not completely sure that the attribute will be required

With the flag enabled the behavior is changed slightly. As Macro pointed out, the completion widget is used in many places and therefore I think it is better to add an explicit switch.

> @@ +1628,4 @@
> >      // aValue is empty (we were asked to clear mInput), or mSearchString
> >      // matches the beginning of aValue.  In either case we can simply
> >      // autocomplete to aValue.
> > +    mLastSearchCompleteString = aValue;
> 
> I don't think the CompleteValue function is the place where
> mLastSearchCompleteString should be set. It should only place the string in
> the textbox. mLastSearchCompleteString should be set in the callers.

The |CompleteValue| function does not only place the string in the textbox, but also has some business logic to compute the final string that is set in the input element. Therefore, I think it is better to set the |mLastSearchCompleteString| in this function as well. Otherwise, all call paths need to read the completed value from the |input| element again, which feels wrong to me as well.

@Paolo: do you see my point? How do you feel about it and/or do you have a better idea?

> @@ +183,5 @@
> > +  is(gAutoComplete.value, "retire", "Value should be completed immediately");
> > +
> > +  yield undefined;
> > +
> > +  is(gAutoComplete.value, "retire", "Value should be autocompleted to the same value");
> 
> We should do two different tests here in place of this one. The first:
>  - Tell the search result object to return "retirement" instead of "retire".
>  - Type the "i".
>  - Check that the value is synchronously completed to "retire".
>  - Check that, after the result arrived, the value is now completed to
> "retirement".
> 
> The second:
>  - Tell the search result object to return "retire" again.
>  - Type "rem".
>  - Check that the value is synchronously completed to "retirement".
>  - Check that, after the result arrived, the value is now "retirem".
> 
> I'm not sure if these tests would work out of the box because we do reuse
> results sometimes, but making them work is valuable.
> 
> The other tests are really spot on.

Good catch! Will add them - thanks a lot!

Comment 27

[:Paolo Amadini]

(In reply to Julian Viereck from comment #26)
> I think it makes more sense to store the "completeLastCompletionSync" flag on the
> nsIAutoCompleteInput

It does indeed look similar to other flags we have there, my only concern is whether changing the interface would put some more burden on callers.

> With the flag enabled the behavior is changed slightly. As Macro pointed
> out, the completion widget is used in many places and therefore I think it
> is better to add an explicit switch.

Well, my point is exactly that, maybe, we are overly concerned with the behavior change, and if implemented properly the new behavior may be able to work for most consumers. If we keep the switch in a separate patch, applied on top of the feature patch, we can decide whether to land it or not later.

> The |CompleteValue| function does not only place the string in the textbox,
> but also has some business logic to compute the final string that is set in
> the input element.

Yes, unfortunately...

> Therefore, I think it is better to set the
> |mLastSearchCompleteString| in this function as well.

...but this is not a consequence of the former :-)

> Otherwise, all call
> paths need to read the completed value from the |input| element again, which
> feels wrong to me as well.

Not necessarily, if mLastSearchCompleteString just contains the argument that is passed to CompleteValue, before any processing by the function itself. I see the role of CompleteValue as "mix the wanted result with the current contents of the textbox". We just need to cache the wanted result to be able to preserve it while typing.

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to :Paolo Amadini from comment #27)
> my only concern is
> whether changing the interface would put some more burden on callers.

As a side note, we can create dedicated interfaces that inherit from the existing ones, and make the code use the new interface calls only if QI succeeds. We should have done this other times when instead we broke consumers :(

Comment 29

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #28)
> As a side note, we can create dedicated interfaces that inherit from the
> existing ones, and make the code use the new interface calls only if QI
> succeeds. We should have done this other times when instead we broke
> consumers :(

And this will be much less of a concern once we've separated from Thunderbird code, as per our previous discussion, following the proposal I've posted on mozilla.dev.platform today.

Comment 30

[Julian Viereck]

(In reply to :Paolo Amadini from comment #27)
> (In reply to Julian Viereck from comment #26)
> >
> > Otherwise, all call
> > paths need to read the completed value from the |input| element again, which
> > feels wrong to me as well.
> 
> Not necessarily, if mLastSearchCompleteString just contains the argument
> that is passed to CompleteValue, before any processing by the function
> itself. I see the role of CompleteValue as "mix the wanted result with the
> current contents of the textbox". We just need to cache the wanted result to
> be able to preserve it while typing.

Not sure I get you right here. If I understand you correctly, the change in |CompleteValue| should look more like this:

  nsAutoCompleteController::CompleteValue(nsString &aValue)
  {
    // Store the passed in value to complete for later temporary completion
    // until the search result comes back.
    mLastSearchCompleteString = aValue;
    [...]
  }

Following the above change, adjust the heuristic when to use the stored last completion in |SetInput| to:

  nsAutoCompleteController::SetInput(nsIAutoCompleteInput *aInput)
  {
    [...]
    mSearchString = newValue;

    // NOTE: this does NO LONGER check if |mLastSearchCompleteString|
    // matches the current value of the input field as in the last patch:
    //   StringBeginsWith(mLastSearchCompleteString, newValue, nsCaseInsensitiveStringComparator());
    // this is now the task of the |CompleteValue| function.
    bool doUseLastCompletion = mUseLastCompletion && !mBackspaced;
    
    if (doUseLastCompletion) {
      CompleteValue(mLastSearchCompleteString);
    } else {
      mLastSearchCompleteString = nsString();
    }
  }

Now, this looks cleaner as before :), BUT I don't think it does the right thing.

Let's assume the user typed "Hel", the completion was to "Hello" and |mLastSearchCompleteString = "Hello"|. The user types a "w", the |SetInput(...)| method from above is called, which causes

  1) Set: mSearchString = "Helw"
  2) Call: CompleteValue("Hello")

Now, from inside the |CompleteValue("Hello")| method, I think we end up in this |else| case at the very bottom of the function:

    } else {
      // Autocompleting something other than a URI from the middle.
      // Use the format "searchstring >> full string" to indicate to the user
      // what we are going to replace their search string with.
      input->SetTextValue(mSearchString + NS_LITERAL_STRING(" >> ") + aValue);

      endSelect = mSearchString.Length() + 4 + aValue.Length();

      // The current version of the patch does the following here:
      // // Reset the last search completion.
      // mLastSearchCompleteString = nsString();
    }

which causes us to display "Helw >> Hello" to the user, which I doubt is what is expected :/

The current version of the patch works around this by resetting the |mLastSearchCompleteString| string in the case we hit the last |else| case of the |CompleteValue(...)| function.

@Paolo, I hope you were able to follow what I try to explain here. Let me know if this makes sense and if not I guess it's better to try and talk over this via IRC.

(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #28)
> (In reply to :Paolo Amadini from comment #27)
> > my only concern is
> > whether changing the interface would put some more burden on callers.
> 
> As a side note, we can create dedicated interfaces that inherit from the
> existing ones, and make the code use the new interface calls only if QI
> succeeds. We should have done this other times when instead we broke
> consumers :(

I do not understand what you two mean here by "would put some more burden on callers." and the idea about "QI succeeds". 

My understanding is: a new boolean flag is added to the interface, let its default value be |false|. If the value of the boolean flag is |false|, then the new code path is ignored and things behave as they are right now. What is the additional burden we then put on the consumer? That they have to opt-in to get the new (hopefully better) behavior?

Comment 31

[:Paolo Amadini]

(In reply to Julian Viereck from comment #30)
>     // NOTE: this does NO LONGER check if |mLastSearchCompleteString|
>     // matches the current value of the input field as in the last patch:
>     //   StringBeginsWith(mLastSearchCompleteString, newValue,
> nsCaseInsensitiveStringComparator());

What if we keep this check instead? Are there cases that will not be handled correctly?

Feel free to ping me in IRC (:paolo) if you want!

> I do not understand what you two mean here by "would put some more burden on
> callers." and the idea about "QI succeeds". 

Since this is an XPCOM interface, it means that all objects implementing it would have to add a getter returning the boolean value, regardless of whether it returns true or false.

A derived interface would not require this, because the implementation of the QueryInterface method will return null when asked for the new interface in objects that have not been updated.

Comment 32

[Julian Viereck]

(In reply to :Paolo Amadini from comment #31)
> (In reply to Julian Viereck from comment #30)
> >     // NOTE: this does NO LONGER check if |mLastSearchCompleteString|
> >     // matches the current value of the input field as in the last patch:
> >     //   StringBeginsWith(mLastSearchCompleteString, newValue,
> > nsCaseInsensitiveStringComparator());
> 
> What if we keep this check instead? Are there cases that will not be handled
> correctly?

No, that doesn't work either. In |CompleteValue(...)| there is a code path, that matches a passed in completion value like "http://facebook.com" to a so far typed input "face" by trimming of the "http://"

    nsresult rv;
    nsCOMPtr<nsIIOService> ios = do_GetService(NS_IOSERVICE_CONTRACTID, &rv);
    NS_ENSURE_SUCCESS(rv, rv);
    nsAutoCString scheme;
    if (NS_SUCCEEDED(ios->ExtractScheme(NS_ConvertUTF16toUTF8(aValue), scheme))) {
      [...]
    }

With your proposed change, we end up with |mLastSearchCompleteString = "http://facebook"| and then checking, if the last completion matches the new value (e.g. assume "faceb") with

   StringBeginsWith(mLastSearchCompleteString, newValue, nsCaseInsensitiveStringComparator())

will not work:

   StringBeginsWith("http://facebook", "faceb", nsCaseInsensitiveStringComparator()) == False

I am sorry to say this, but after playing through all possible call paths for this, I think the only way to get the behavior right is to store the result of the completed value in |mLastSearchCompleteString| and NOT the value passed to |CompleteValue(...)|. This is what the patch is doing at the moment.
 
> > I do not understand what you two mean here by "would put some more burden on
> > callers." and the idea about "QI succeeds". 
> 
> Since this is an XPCOM interface, it means that all objects implementing it
> would have to add a getter returning the boolean value, regardless of
> whether it returns true or false.
> 
> A derived interface would not require this, because the implementation of
> the QueryInterface method will return null when asked for the new interface
> in objects that have not been updated.

Ahh, nice, now I have a rough idea what you are talking about - thanks for the explanation :)

How complicated is it to change the interface to a derived one? Can you maybe point me to an example/existing code where this is done already so I can learn from there?

Comment 33

[:Paolo Amadini]

(In reply to Julian Viereck from comment #32)
> With your proposed change, we end up with |mLastSearchCompleteString =
> "http://facebook"| and then checking, if the last completion matches the new
> value (e.g. assume "faceb") with
> 
>    StringBeginsWith(mLastSearchCompleteString, newValue,
> nsCaseInsensitiveStringComparator())
> 
> will not work:
> 
>    StringBeginsWith("http://facebook", "faceb",
> nsCaseInsensitiveStringComparator()) == False

Yeah, we'd not preserve the value in this case. I was initially thinking this wasn't a big deal, but I guess this is common enough in the location bar to want its own handling.

As a side note, now that we have the concept of "finalCompleteValue", we'll probably be able to move this special case for the "http://" prefix elsewhere, and simplify the code (see also the related bug 1054814).

> How complicated is it to change the interface to a derived one? Can you
> maybe point me to an example/existing code where this is done already so I
> can learn from there?

I'm not sure this approach is worth the cost, in a few weeks we'll effectively start making major changes to these interfaces (there is a related mozilla.dev.platform newsgroup thread) so we won't need a new interface.

So, the next steps are just updating and splitting the current patch, so that I can start reviewing and testing the functional part.

Comment 34

[Julian Viereck]

Thinking about a better name for |useLastCompletion| - how do you feel about |placeholdLastCompletion|? That's more accurate, as the last completion is really only used as a placeholder until the actual results come in. What do you think?

PS: I am working on moving the tests over to xpcshell-test under toolkit/components/autocomplete/tests/unit/ but it takes longer as expected. I keep you posted.

Comment 35

[:Paolo Amadini]

(In reply to Julian Viereck from comment #34)
> Thinking about a better name for |useLastCompletion| - how do you feel about
> |placeholdLastCompletion|? That's more accurate, as the last completion is
> really only used as a placeholder until the actual results come in. What do
> you think?

I've no great suggestion, unfortunately... but this is an improvement anyways. Marco may have other suggestions. This will be in the attribute-switch patch anyways, which is the second patch applied on top of the functional patch.

> PS: I am working on moving the tests over to xpcshell-test under
> toolkit/components/autocomplete/tests/unit/ but it takes longer as expected.

Well, I believe the mochitest-chrome test for keyboard interaction, that I already saw, should be enough for the functional patch. Or do you think you need an xpcshell type test for a reason?

Comment 36

[Julian Viereck]

(In reply to :Paolo Amadini from comment #35)
> I've no great suggestion, unfortunately... but this is an improvement
> anyways. Marco may have other suggestions. This will be in the
> attribute-switch patch anyways, which is the second patch applied on top of
> the functional patch.

Not really. The functional patch defines a new property on the |nsIAutoCompleteController.idl| file, which is currently:

    attribute boolean useLastCompletion;

 
> Well, I believe the mochitest-chrome test for keyboard interaction, that I
> already saw, should be enough for the functional patch. Or do you think you
> need an xpcshell type test for a reason?

Okay, seems like we have other ideas what needs to be done here ;) As I understood it, the current patch should be splitted in two parts:

  1. the functional bits: 
    1.1 changes to nsIAutoCompleteController.idl, nsAutoCompleteController.cpp etc
    1.2 include the unit tests for this feature
  2. the activation changes:
    2.1 changes to the autocomplete.xul binding and
    2.2 changes to the browser.xul to turn this feature for the URL bar on

The tests depend on the changes to the |autocomplete.xul| file. Therefore with the above splitting, I thought it's best to move the tests to toolkit/completions/autocomplete/tests/unit/... and therefore require to change them to xpcshell-test?

@paolo: when I interpret your reply correctly, you want to split only the 2.2 part from above in a separate patch? This way the test can stay as mochitest-chrome tests :)

Comment 37

[:Paolo Amadini]

Uh, yeah we're saying different things here, sorry for the misunderstanding!

I'm asking you to create two patches. The first one will be relatively simple, with no new attribute to control the behavior, just the minimal set of changes to fix the bug, along with its new test. The test obviously will be slightly different from the current patch because it will not need to activate the feature explicitly, but for the rest it will be the same.

This patch, ideally, should require no changes to existing tests. If it does, we know we have a potential regression to investigate if we later decide for opt-out rather than opt-in behavior.

The second patch will adds the attribute to control the behavior (we'll see if opt-in or opt-out). If the behavior is made opt-in, obviously it will contain a modification on top of the same test included in the first patch, to activate the feature, as well as a modification to the URL bar to opt-in to the feature.

Comment 38

[Julian Viereck]

(In reply to :Paolo Amadini from comment #37)
> Uh, yeah we're saying different things here, sorry for the misunderstanding!

No worries - good enough we get it straight now :)

> I'm asking you to create two patches. The first one will be relatively
> simple, with no new attribute to control the behavior, just the minimal set
> of changes to fix the bug, along with its new test.

Okay. To make sure, I will create a test under

  /toolkit/content/tests/chrome/test_autocomplete_placehold_last_complete.xul

which is a mochitest-chrome test using a <textbox type="autocomplete">. Does that sound right?
  
> The second patch will adds the attribute to control the behavior (we'll see
> if opt-in or opt-out). If the behavior is made opt-in, obviously it will
> contain a modification on top of the same test included in the first patch,
> to activate the feature, as well as a modification to the URL bar to opt-in
> to the feature.

Guess I get the first patch done and then check if we need opt-in or opt-out.

Comment 39

[:Paolo Amadini]

(In reply to Julian Viereck from comment #38)
> Okay. To make sure, I will create a test under
> 
>   /toolkit/content/tests/chrome/test_autocomplete_placehold_last_complete.xul
> 
> which is a mochitest-chrome test using a <textbox type="autocomplete">. Does
> that sound right?

Yes, it does. Thanks!

Comment 40

[Julian Viereck]

Attachment: wip-04.diff

This is a updated version of the patch following the review feedback in Comment 24.

Most noteable changes:
- I renamed |mLastSearchCompleteString| to |mPlaceholderCompletionString| - this makes reading the code much easier to understand and reflects more what this field is actually storing

- before the placeholder-fill-in logic was placed in nsAutoCompleteController::HandleText(). I move this over to nsAutoCompleteController::StartSearches(). This is, as StartSearches() is the central spot all searches go through and therefore it makes more sense to place the placeholder logic in there.

- added the two test cases requested by Paolo

- turned out, to get the two extra test cases working, I had to add a piece of new code in nsAutoCompleteController::CompleteDefaultIndex. Before, the input was not completed if the selection wasn't at the very end. Now with the placeholder added, it should be also legitim to complete the input with the search result if the selected text is from the placeholder string:

  bool isPlaceholderSelected =
      selectionEnd == (int32_t)mPlaceholderCompletionString.Length() &&
      selectionStart == (int32_t)mSearchString.Length() &&
      StringBeginsWith(mPlaceholderCompletionString,
        mSearchString, nsCaseInsensitiveStringComparator());

  // Don't try to automatically complete to the first result if there's already
  // a selection or the cursor isn't at the end of the input. In case the
  // selection is from the current placeholder completion value, then still
  // automatically complete.
  if (!isPlaceholderSelected && (selectionEnd != selectionStart ||
      selectionEnd != (int32_t)mSearchString.Length()))
    return NS_OK;

---

A try run with this patch is available here: https://tbpl.mozilla.org/?tree=Try&rev=e0f342db61a7 (which is still building while writing this comment).

@Paolo: Based on the results from the try run, I will request review for you on this bug.

Comment 41

[Julian Viereck]

The previous try run failed. Seemed like my copy of mozilla-central was to old. I updated my local tree, rebased the patch and just started a new try-run here:

  https://tbpl.mozilla.org/?tree=Try&rev=79778b0843b1

Comment 42

[Julian Viereck]

The try run indicates outfall from this patch. Will take a closer look at it later and then decide if a switch to turn the new behavior on or off is required.

Comment 43

[Julian Viereck]

Attachment: wip-05.diff

This is an improved version of the wip-04 patch. The broken tests from last try run seem to work now on linux. I don't have access to a OSX machine at the moment, but I hope things should work there as well.

Overall, the quality of the patch looks pretty promising to me. In case try run comes back good, this should be ready to go to review directly.

@mak: as discussed on IRC - can you please make a try run of this patch and share the link back? Working from a borrowed laptop at the moment and I don't have my ssh keys with me.

Comment 44

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8487295 [details] [diff] [review]
wip-05.diff

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

pushed!

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=09320494fafe

Comment 45

[Julian Viereck]

(In reply to Marco Bonardo [:mak] (needinfo? me) from comment #44)
> Comment on attachment 8487295 [details] [diff] [review]
> wip-05.diff
> 
> Review of attachment 8487295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> pushed!
> 
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=09320494fafe

Thanks Marco! The results look very promising :) On OSX two tests are failing. I will take a closer look once I am home again next week and have access to my MacBook. Otherwise, this patch seems to get into the right direction.

Comment 46

[Julian Viereck]

Attachment: wip06.diff

This is based on wip-05.diff. The failing mac-test-case was as the text was completed on cursor movements and replacing the text in the input element during the cursor movement screwed up a few assumptions. To prevent this, I added a new requirement for the "use-placeholder" logic to kick in: if the current search string equals case-insensitive the placeholder string, then don't do a completion on them. This is basically saying: if what you want to complete is already what you have, there is no need to call the completion function - which seams a reasonable thing to do.

The two mochitest-chrome tests
- toolkit/content/tests/chrome/test_autocomplete_mac_caret.xul
- toolkit/content/tests/chrome/test_autocomplete_placehold_last_complete.xul

work now fine for me locally. I don't have a linux setup at the moment, so I cannot say if the current patch will work there. But that's why I spawned a new try run ;)

Ongoing try run is here:

  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=24f1120ee76e

Comment 47

[Julian Viereck]

Comment on attachment 8494040 [details] [diff] [review]
wip06.diff

The results from try run look good. There are tests failing with messages like "There was an uncaught Promise rejection". To check if these errors are related to this bug I ran another try run without this patch applied and this try-run yields the same errors. Therefore I guess the breakage in the last try-run is unrelated to this patch.

Given that this patch passes all tests I think this patch is ready to go towards landing :)

@Paolo, do you mind doing a feedback round on the patch again before handing it over to Marko for review?

Comment 48

[:Paolo Amadini]

Comment on attachment 8494040 [details] [diff] [review]
wip06.diff

This looks good to me and the test may in fact help us with the autocomplete refactorings by giving us more test coverage.

Comment 49

[Julian Viereck]

Comment on attachment 8494040 [details] [diff] [review]
wip06.diff

Thanks Paolo for the fast feedback :)

Marco, based on Paolo's positive feedback I hand this patch over to you for review.

Comment 50

[:Paolo Amadini]

(In reply to Julian Viereck from comment #49)
> Thanks Paolo for the fast feedback :)

There was a good timing :-) I can't really guarantee fast feedback cycles these days!

Comment 51

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8494040 [details] [diff] [review]
wip06.diff

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

I think overall it's good, the functionality looks good as well.

I'm still a little bit scared cause AC code is very race-y, but we still have 2 weeks before the merge to test in Nightly, and it's also an easy backout in case of issues.

That said, still nerds some minor tweaks before r+, and a proper commit message (PS: I suggest you to install the hg trychooser extension)

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +113,5 @@
>    // Clear out this reference in case the new input's popup has no tree
>    mTree = nullptr;
>    // Reset all search state members to default values
>    mSearchString = newValue;
> +  mPlaceholderCompletionString = nsString();

you should not create a new string object everytime, rather call mPlaceholderCompletionString.Truncate();
This is valid also for the following code.

@@ +229,5 @@
>    {
>      // We need to throw away previous results so we don't try to search through them again
>      ClearResults();
>      mBackspaced = true;
> +    mPlaceholderCompletionString = nsString();

Here the text has been cropped (by backspacing we assume), you can do better than clearing the string, you could indeed Truncate it to newValue.Length()

@@ +1115,5 @@
>  }
>  nsresult
>  nsAutoCompleteController::StartSearches()
>  {
> +  nsCOMPtr<nsIAutoCompleteInput> input(mInput);

Before your change, we where null checking mInput before doing this assignment, and we should clearly keep doing that, cause race conditions can cause us enter here with null mInput and now we'd crash.

Actually, do you really have to run this code before the original (mTimer || !mInput) check? Unless there's a compelling reason for not doing so, I'd prefer to keep that check before anything else.

To improve readability, could you please move all of this placeholder support code into an helper method (something like maybeCompletePlaceholder()) and just call that here?

@@ +1136,5 @@
> +  bool usePlaceholderCompletion = StringBeginsWith(mPlaceholderCompletionString,
> +    mSearchString, nsCaseInsensitiveStringComparator()) &&
> +    !mPlaceholderCompletionString.Equals(mSearchString,
> +      nsCaseInsensitiveStringComparator()) &&
> +    selectionEnd == selectionStart && selectionEnd == (int32_t)mSearchString.Length();

so, first thing you should check here is !mPlaceholderCompletionString.IsEmpty(), otherwise we're just going to spend time for nothing.
And instead of checking equality I'd rather check length, if they are the same length they are either equal or different, in both cases we don't want to complete, right?
Since string comparison is the most expensive operation here, it should be the last.
Finally, this needs to be reindented.

So in the end we'd have something like:

bool usePlaceholderCompletion =
  !mPlaceholderCompletionString.IsEmpty() &&
  mPlacesholderCompletionString.Length() > mSearchString.Length() &&
  selectionEnd == selectionStart &&
  selectionEnd == (int32_t)mSearchString.Length() &&
  StringBeginsWith(mPlaceholderCompletionString, mSearchString,
                   nsCaseInsensitiveStringComparator());

@@ +1139,5 @@
> +      nsCaseInsensitiveStringComparator()) &&
> +    selectionEnd == selectionStart && selectionEnd == (int32_t)mSearchString.Length();
> +
> +  if (usePlaceholderCompletion) {
> +    CompleteValue(mPlaceholderCompletionString);

I suggest you an alternative here, since CompleteValue already does the StringBeginsWith check, but then it may fallback to something else we don't want, you could add an optional argument to it, "bool aOnlyMatchBeginning=false"

if CompleteValue doesn't match at the beginning and the optional argument is true, it should return NS_ERROR_FAILURE;

then you can avoid the StringBeginsWith in usePlaceholderCompletion (otherwise we'd do it twice) and here just do:

if (!usePlacesholderCompletion ||
    NS_FAILED(CompleteValue(mPlacesholderCompletionString)) {
  mPlaceholderCompletionString.Truncate()
}

::: toolkit/content/tests/chrome/test_autocomplete_placehold_last_complete.xul
@@ +36,5 @@
> +  this._searchId = searchId;
> +}
> +autoCompleteSimpleResult.retireCompletion = "Retire";
> +autoCompleteSimpleResult.prototype = {
> + _param: "",

indentation in this object is wrong, should be 2 spaces.

@@ +146,5 @@
> +
> +  runAsyncTest();
> +}
> +
> +function asyncTestGenerator() {

function*

Comment 52

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #51)
> @@ +229,5 @@
> >    {
> >      // We need to throw away previous results so we don't try to search through them again
> >      ClearResults();
> >      mBackspaced = true;
> > +    mPlaceholderCompletionString = nsString();
> 
> Here the text has been cropped (by backspacing we assume), you can do better
> than clearing the string, you could indeed Truncate it to newValue.Length()

Now that I better think about this, we're not going to win anything that way, so it should be fine to Truncate it. Sorry for confusion.

Comment 53

[Julian Viereck]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #51)
> Comment on attachment 8494040 [details] [diff] [review]
> wip06.diff
> 
> Review of attachment 8494040 [details] [diff] [review]:

Wow, thanks a lot for the bunch of detail comment and the detailed explanations along the way :)

While skimming over the comments I think they look reasonable. I will try to get the necessary changes done on Sunday or otherwise early the coming week. Thanks once again for this Marco!

Comment 54

[Julian Viereck]

Attachment: wip07.diff

This addresses the review comments from Marco in comment #51. All comments have been addressed except the following one which I will explain further below.

As for the commit message I went with:

  "Use result from last completion in nsAutoCompleteController to reduce UI flickering until search results come back."

@Marco: Can you please have a look at the updated patch?

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #51)
> @@ +1139,5 @@
> > +      nsCaseInsensitiveStringComparator()) &&
> > +    selectionEnd == selectionStart && selectionEnd == (int32_t)mSearchString.Length();
> > +
> > +  if (usePlaceholderCompletion) {
> > +    CompleteValue(mPlaceholderCompletionString);
>
> I suggest you an alternative here, since CompleteValue already does the
> StringBeginsWith check, but then it may fallback to something else we don't
> want, you could add an optional argument to it, "bool
> aOnlyMatchBeginning=false"
>
> if CompleteValue doesn't match at the beginning and the optional argument is
> true, it should return NS_ERROR_FAILURE;
>
> then you can avoid the StringBeginsWith in usePlaceholderCompletion
> (otherwise we'd do it twice) and here just do:
>
> if (!usePlacesholderCompletion ||
>     NS_FAILED(CompleteValue(mPlacesholderCompletionString)) {
>   mPlaceholderCompletionString.Truncate()
> }

Here is my reasoning why I have not done this review request and kept the code the way it is:

- I want the developer to understand all the requirement for |usePlaceholderCompletion| computation in |MaybeCompletePlaceholder()|
- this especially requires to compute the |StringBeginsWith(mPlaceholderCompletionString, mSearchString, nsCaseInsensitiveStringComparator());| but in the |MaybeCompletePlaceholder()| method
- to avoid the |StringBeginsWith(...)| bit to be computed a second time in |CompleteValue(...)| a new argument to |CompleteValue(...)| like the proposed |aOnlyMatchBeginning| is necessary. If this argument is set, the |CompleteValue(...)| should just perform the "complete from the beginning" action. But then the argument completely controls the behavior of the |CompleteValue(...)| function, which looks wrong to me (this might shallow bugs in case the implementation of |CompleteValue(...)| value changes).
- therefore, I haven't done the above proposed change.

Comment 55

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Julian Viereck from comment #54)
> As for the commit message I went with:
> 
>   "Use result from last completion in nsAutoCompleteController to reduce UI
> flickering until search results come back."

Since this won't solve popup flickering, we should state in the checkin comment that this is only about autoFill flickering.

> (In reply to Marco Bonardo [::mak] (needinfo? me) from comment #51)
> - I want the developer to understand all the requirement for
> |usePlaceholderCompletion| computation in |MaybeCompletePlaceholder()|
> - this especially requires to compute the
> |StringBeginsWith(mPlaceholderCompletionString, mSearchString,
> nsCaseInsensitiveStringComparator());| but in the
> |MaybeCompletePlaceholder()| method
> - to avoid the |StringBeginsWith(...)| bit to be computed a second time in
> |CompleteValue(...)| a new argument to |CompleteValue(...)| like the
> proposed |aOnlyMatchBeginning| is necessary. If this argument is set, the
> |CompleteValue(...)| should just perform the "complete from the beginning"
> action. But then the argument completely controls the behavior of the
> |CompleteValue(...)| function, which looks wrong to me (this might shallow

I think the first point is valid (though it may be addressed with a simple comment)
I don't feel like the risk to hide bugs if CompleteValue changes is very high cause it's a very short and dedicated method.

Regardless, it's not that we are saving much from a perf point of view, so I'm fine either way, if you prefer to keep that explicit, it's ok.

Comment 56

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8496729 [details] [diff] [review]
wip07.diff

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

Thanks.

r=me, with a green Try run on the final version.

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +1112,5 @@
>      PostSearchCleanup();
>    }
>    return NS_OK;
>  }
> +void

please add a newline above this

@@ +1115,5 @@
>  }
> +void
> +nsAutoCompleteController::MaybeCompletePlaceholder()
> +{
> +  nsCOMPtr<nsIAutoCompleteInput> input(mInput);

there should be no need to do this. The points where we do this is where there's risk some subsequent calls will make mInput null, and later we'll need it.  But in this method you don't have anything like that.  This doesn't even protect us in case the caller causes mInput to become null before invoking MaybeCompletePlaceholder.

You have 2 possibilities here:
1. MOZ_ASSERT(mInput); then just use it directly. This should protect from most development mistakes
2. pass input as an argument, and still MOZ_ASSERT on it being non null. This will enforce the caller to pay more attention to what he's passing, so should be a little bit more explicit about the responsibility of the caller

in both cases it may be worth, after asserting, to early return in case input is null (since assert only crashes in debug mode). so here you could do:

if (!aInput) { // or mInput depending on what you choose
  MOZ_ASSERT_UNREACHABLE("Input should always be valid at this point");
  return;
}

@@ +1151,4 @@
>  nsresult
>  nsAutoCompleteController::StartSearches()
>  {
> +  nsCOMPtr<nsIAutoCompleteInput> input(mInput);

what's the point of moving this assignment? please restore it where it was before and just call MaybeCompletePlaceholder after it.

@@ +1159,5 @@
>    if (mTimer || !mInput)
>      return NS_OK;
> +  // Check if the current input should be completed with the placeholder string
> +  // from the last completion until the actual search results come back.
> +  MaybeCompletePlaceholder();

please add a newline before and after this call, to increase its visibility.

@@ +1506,5 @@
> +  // a selection or the cursor isn't at the end of the input. In case the
> +  // selection is from the current placeholder completion value, then still
> +  // automatically complete.
> +  if (!isPlaceholderSelected && (selectionEnd != selectionStart ||
> +      selectionEnd != (int32_t)mSearchString.Length()))

indent the second row once more, since it's not at the same level as !isPlaceholderSelected

@@ +1676,2 @@
>                            Substring(aValue, mSearchStringLength + findIndex,
> +                                    endSelect);

please reindent as

      mPlaceholderCompletionString = mSearchString +
        Substring(aValue, mSearchStringLength + findIndex, endSelect);

Comment 57

[Julian Viereck]

Thanks for the comments again! I don't understand the following comment from you - when applying the patch locally and looking at the attachment in the browser, things look correct. Am I missing something here?

Just going over the other changes and will push to try this evening.

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #56)
> Comment on attachment 8496729 [details] [diff] [review]
> 
> ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
> @@ +1112,5 @@
> >      PostSearchCleanup();
> >    }
> >    return NS_OK;
> >  }
> > +void
> 
> please add a newline above this

There is a newline in the local file & patch file?
 
> @@ +1159,5 @@
> >    if (mTimer || !mInput)
> >      return NS_OK;
> > +  // Check if the current input should be completed with the placeholder string
> > +  // from the last completion until the actual search results come back.
> > +  MaybeCompletePlaceholder();
> 
> please add a newline before and after this call, to increase its visibility.

The newlines are in place already?

> @@ +1506,5 @@
> > +  // a selection or the cursor isn't at the end of the input. In case the
> > +  // selection is from the current placeholder completion value, then still
> > +  // automatically complete.
> > +  if (!isPlaceholderSelected && (selectionEnd != selectionStart ||
> > +      selectionEnd != (int32_t)mSearchString.Length()))
> 
> indent the second row once more, since it's not at the same level as
> !isPlaceholderSelected

I am not sure what your expected indention is here. Do you want it to be like this?

  if (!isPlaceholderSelected && (selectionEnd != selectionStart ||
        selectionEnd != (int32_t)mSearchString.Length()))

or like this:

  if (!isPlaceholderSelected && (selectionEnd != selectionStart ||
      selectionEnd != (int32_t)mSearchString.Length()))

The later is what I think you want me to end up with when reading your comment. But the later is also the currents status of the patch?

Comment 58

[Julian Viereck]

Attachment: wip08.diff

Updated version based on Marco's comment #56. Carrying over the r+ from last wip07.

A try run was just started here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=913b44e0307e

@Marco: Setting "need more information" for my questions in comment #57.

Comment 59

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Julian Viereck from comment #57)
> (In reply to Marco Bonardo [::mak] (needinfo? me) from comment #56)
> > Comment on attachment 8496729 [details] [diff] [review]
> > 
> > ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
> > @@ +1112,5 @@
> > >      PostSearchCleanup();
> > >    }
> > >    return NS_OK;
> > >  }
> > > +void
> > 
> > please add a newline above this
> 
> There is a newline in the local file & patch file?

I'm sorry, Splinter doesn't show that, as you can see from my quoting, if it's fine locally that's ok.

> > @@ +1159,5 @@
> > >    if (mTimer || !mInput)
> > >      return NS_OK;
> > > +  // Check if the current input should be completed with the placeholder string
> > > +  // from the last completion until the actual search results come back.
> > > +  MaybeCompletePlaceholder();
> > 
> > please add a newline before and after this call, to increase its visibility.
> 
> The newlines are in place already?

ditto, sounds like something wrong with Splinter review tool...


> > @@ +1506,5 @@
> > > +  // a selection or the cursor isn't at the end of the input. In case the
> > > +  // selection is from the current placeholder completion value, then still
> > > +  // automatically complete.
> > > +  if (!isPlaceholderSelected && (selectionEnd != selectionStart ||
> > > +      selectionEnd != (int32_t)mSearchString.Length()))
> > 
> > indent the second row once more, since it's not at the same level as
> > !isPlaceholderSelected
> 
> I am not sure what your expected indention is here. Do you want it to be
> like this?

if (!isPlaceholderSelected && (selectionEnd != selectionStart ||
      selectionEnd != (int32_t)mSearchString.Length()))

Comment 60

[Julian Viereck]

Attachment: wip09.diff

Same as previous attachment 8497054 [details] [diff] [review] (wip08.diff) but with an indention bug fixed.

Comment 61

[Julian Viereck]

Attachment: wip09.diff

Added author and proper commit message. Otherwise same as attachment 8497065 [details] [diff] [review]: wip09.diff.

Comment 62

[Julian Viereck]

"Green" try run is here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=913b44e0307e

There are a few tests failing but I don't think they are related to the changes here.

Therefore, setting keyword checkin-needed to get this landed :)

Comment 63

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yes, those failures are unrelated. thank you!

Comment 64

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/a98aca20c7be

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a98aca20c7be

Comment 66

[Ada [:adalucinet]]

I'm not able to reproduce this issue with Nightly 2013-12-16 nor Nightly 2014-09-23 on Windows 7 64-bit with STR from comment 0. What am I missing here? Thanks in advance!

Comment 67

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I guess you might need a slower system with a large places.sqlite. so maybe not that easy to reproduce off-hand. Btw let's wait for Julian hints first.

Comment 68

[Julian Viereck]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #66)
> I'm not able to reproduce this issue with Nightly 2013-12-16 nor Nightly
> 2014-09-23 on Windows 7 64-bit with STR from comment 0. What am I missing
> here? Thanks in advance!

I agree with Marco before. This "bug" is only visible if the time required to finish the search for the url to autoFill takes too long, such that a repaint of the UI happens in the meantime.

@Alexandra, is there a way you can test this bug with a profile containing a lot of visited URLs? E.g. make a copy of your normal Firefox profile you use all day and use it for testing? With a fresh profile this is hard to reproduce.

Comment 69

[Ada [:adalucinet]]

Unfortunately, I'm unable to reproduce with a large places.sqlite file (>40.000 items) on Windows 7 x64 and Mac OS X 10.9.5 (slower system).
Julian, since you are both the reporter and assignee, could you please verify this fix on latest 35.0a1?

Comment 70

[Julian Viereck]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #69)
> Unfortunately, I'm unable to reproduce with a large places.sqlite file
> (>40.000 items) on Windows 7 x64 and Mac OS X 10.9.5 (slower system).
> Julian, since you are both the reporter and assignee, could you please
> verify this fix on latest 35.0a1?

Yes, I can verify this is fixed for me on the latest Nightly 35.0a1 (2014-10-03).

Comment 71

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Marking as Verified based on comment 70. Thank you Julian!