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

VERIFIED FIXED in mozilla31

Status

()

defect
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({addon-compat})

Trunk
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

7 years ago
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.
Assignee

Updated

5 years ago
Blocks: 959573
Assignee

Updated

5 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee

Comment 1

5 years ago
Posted 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 2

5 years ago
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+
Assignee

Updated

5 years ago
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
Assignee

Comment 3

5 years ago
Posted patch patch v2 (obsolete) — Splinter Review
addressed comments, proceeding towards SR
Attachment #8403899 - Attachment is obsolete: true
Attachment #8407486 - Flags: superreview?(gavin.sharp)
Assignee

Comment 4

5 years ago
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
Assignee

Comment 5

5 years ago
Posted 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)
Assignee

Comment 6

5 years ago
Posted 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+
Assignee

Comment 9

5 years ago
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: 5 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-]
Assignee

Updated

5 years ago
Flags: needinfo?(mak77)
Depends on: 1003856
Assignee

Updated

5 years ago
Keywords: addon-compat

Updated

5 years ago
Depends on: 1009469

Updated

5 years ago
Depends on: 1043584

Comment 13

5 years ago
Any thoughts about bug 1043584, it is the expected behavior?
Assignee

Comment 14

5 years ago
(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.

Updated

5 years ago
Depends on: 1108186
You need to log in before you can comment on or make changes to this bug.