Closed Bug 754265 Opened 9 years ago Closed 7 years ago

Add a dedicated API to provide a final complete value different from the matching one

Categories

(Toolkit :: Autocomplete, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla31

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: addon-compat, Whiteboard: p=3 s=it-31c-30a-29b.3 [qa-])

Attachments

(1 file, 3 obsolete files)

in bug 720081 I'm using a temporary hack that we can backport to Aurora, though we should modify the result API to provide a way to do this properly.
The idea is that the defaultComplete value can differ from the final value we push to textValue when the user confirms a match.
Blocks: 959573
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attached patch patch v1 (obsolete) — Splinter Review
I tried to write tests covering the various code points I touched, existing tests in autocomplete and Places are passing (I'm going to make a Try run though, just in case).

This allows to enter a different value when the user confirms a selection, so a result can suggest that a different final value is used. This is useful for inline completion (we were already supporting this through the comment hack) but also in future for searches, since we want to "mask" all of the pointless affilate code in autocomplete.

Neil, any chance you may review this along this week? I need it for bug 959573, eventually if that's problematic I may try to ask gavin to find a different reviewer.
Attachment #8403899 - Flags: review?(enndeakin)
Flags: firefox-backlog+
Comment on attachment 8403899 [details] [diff] [review]
patch v1

>+  nsresult GetResultLabelAt(int32_t aIndex, nsAString & _retval);
> private:
>   nsresult GetResultValueLabelAt(int32_t aIndex, bool aValueOnly,
>                                  bool aGetValue, nsAString & _retval);

You changed the argument names in the C++ file but not here in the header.



>diff --git a/toolkit/components/autocomplete/nsIAutoCompleteController.idl b/toolkit/components/autocomplete/nsIAutoCompleteController.idl
>--- a/toolkit/components/autocomplete/nsIAutoCompleteController.idl
>+++ b/toolkit/components/autocomplete/nsIAutoCompleteController.idl
>@@ -1,17 +1,17 @@
>+   * Get the final value that should be completed when the user confirms
>+   * the match at the given index.
>+   */
>+  AString getFinalCompleteValueAt(in long index);
>+
>+  /*
>    * Get / set the current search string.  Note, setting will not start searching
>    */
>   attribute AString searchString;
> };
>diff --git a/toolkit/components/autocomplete/nsIAutoCompleteResult.idl b/toolkit/components/autocomplete/nsIAutoCompleteResult.idl
>--- a/toolkit/components/autocomplete/nsIAutoCompleteResult.idl
>+++ b/toolkit/components/autocomplete/nsIAutoCompleteResult.idl
>@@ -77,14 +77,20 @@ interface nsIAutoCompleteResult : nsISup
> 
>   /**
>+   * Get the final value that should be completed when the user confirms
>+   * the match at the given index.
>+   */
>+  AString getFinalCompleteValueAt(in long index);
>+

A little confusing to have the same comment on both getFinalCompleteValueAt methods even though the index means something different I think.
Attachment #8403899 - Flags: review?(enndeakin) → review+
Summary: Add a dedicated API to provide a final defaultComplete value different from the matching one → Add a dedicated API to provide a final complete value different from the matching one
Attached patch patch v2 (obsolete) — Splinter Review
addressed comments, proceeding towards SR
Attachment #8403899 - Attachment is obsolete: true
Attachment #8407486 - Flags: superreview?(gavin.sharp)
looks like I should fix
class nsFileResult MOZ_FINAL : public nsIAutoCompleteResult
and do another try run, there is some failure in a recent try run I did that seem related to this
Attached patch patch v3 (obsolete) — Splinter Review
I indeed had forgotten to update some nsIAutocompleteResult instances in satchel and passwordmgr and this takes care of those. The changes are pretty much mechanical (adding the method and forwarding it to getValueAt) so not going to re-ask for review. Though if you have any comments feel free to post them.
Now I'll push this to Try.
Attachment #8407486 - Attachment is obsolete: true
Attachment #8407486 - Flags: superreview?(gavin.sharp)
Attachment #8408125 - Flags: superreview?(gavin.sharp)
Attached patch patch v4Splinter Review
fix a typo, the tests I re-ran locally that were failing are now passing. Doing another try run.
Attachment #8408125 - Attachment is obsolete: true
Attachment #8408125 - Flags: superreview?(gavin.sharp)
Attachment #8408216 - Flags: superreview?(gavin.sharp)
Comment on attachment 8408216 [details] [diff] [review]
patch v4

(I only reviewed the interface changes)
Attachment #8408216 - Flags: superreview?(gavin.sharp) → superreview+
https://hg.mozilla.org/integration/fx-team/rev/669dff46362f
Flags: in-testsuite+
Target Milestone: --- → mozilla31
https://hg.mozilla.org/mozilla-central/rev/669dff46362f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Hi Marco, can you provide a point estimate.
Flags: needinfo?(mak77)
Whiteboard: p=0 s=it-31c-30a-29b.3 [qa?]
Hi Liz, can you review this bug to determine if it requires further QA verification [qa+] or not [qa-].
Flags: needinfo?(lhenry)
Flags: needinfo?(lhenry)
Whiteboard: p=0 s=it-31c-30a-29b.3 [qa?] → p=0 s=it-31c-30a-29b.3 [qa-]
Status: RESOLVED → VERIFIED
Whiteboard: p=0 s=it-31c-30a-29b.3 [qa-] → p=3 s=it-31c-30a-29b.3 [qa-]
Flags: needinfo?(mak77)
Depends on: 1003856
Keywords: addon-compat
Depends on: 1003461
Depends on: 1009469
Depends on: 1043584
Any thoughts about bug 1043584, it is the expected behavior?
(In reply to YF (Yang) from comment #13)
> Any thoughts about bug 1043584, it is the expected behavior?

Any behavioral change is not expected here, so I don't think it can be the expected behavior. We should fix that bug.
Depends on: 1108186
You need to log in before you can comment on or make changes to this bug.