1519099 - removeFromDb parameter of nsIAutoCompleteResult::RemoveValueAt is always true

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, enhancement, P5)

Description

[Mark Banner (:standard8)]

The removeFromDb parameter of nsIAutoCompleteResult::RemoveValueAt is always set to true:

https://searchfox.org/mozilla-central/search?q=RemoveValueAt&redirect=false

We could probably tidy that up a bit and clean up some code.

Comment 1

[SresthaSrivastava [:srestha]]

Hey, can I take this up? Let me know how to fix it.

Comment 2

[Aashna]

Hey,can you direct me to the codebase for this bug?

Comment 3

[Paarmita Bhargava]

Hey Mark,
Can I work on this issue?
I guess we need to put some condition on RemoveValueAt to be true. Let me know what needs to be fixed in it.

Comment 4

[Marco Bonardo [:mak]]

The parameter must be removed from the definition in nsIAutoCompleteResult.idl, from the implementation in nsAutoCompleteSimpleResult.cpp and then from all the call points, the link in comment 0 shows all of them.

Comment 5

[Paarmita Bhargava]

Thanks, @Marco Got it! Working on this.

Comment 6

[Paarmita Bhargava]

So it should be something like this in nsIAutoCompleteResult.idl
void removeValueAt();

and in nsAutoCompleteSimpleResult.cpp file, parameters must also be removed?

nsAutoCompleteSimpleResult::RemoveValueAt(int32_t aRowIndex,
                                          bool aRemoveFromDb) {
  CHECK_MATCH_INDEX(aRowIndex, false);

  nsString value = mMatches[aRowIndex].mValue;
  mMatches.RemoveElementAt(aRowIndex);

  if (mListener) {
    mListener->OnValueRemoved(this, value, aRemoveFromDb);
  }

  return NS_OK;
}

Comment 7

[Marco Bonardo [:mak]]

(In reply to paarmita1998 from comment #6)

So it should be something like this in nsIAutoCompleteResult.idl
`
void removeValueAt();

no, the method has 2 arguments, only removeFromDb should be removed, per the bug description.

Comment 8

[Marco Bonardo [:mak]]

I'd suggest starting with getting a Firefox build working, in optimized mode (because it's faster to build). Since this requires to work with cpp, it's important to be able to verify things compile properly. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Comment 9

[Paarmita Bhargava]

Hey, will this project run if I install Firefox Artifact as it is a C++ project?

Comment 10

[Mark Banner (:standard8)]

(In reply to paarmita1998 from comment #9)

Hey, will this project run if I install Firefox Artifact as it is a C++ project?

No, an artifact build does not pick up changes to C++. You'll need a full (optimized) build.

Comment 11

[Paarmita Bhargava]

I couldn't provide a patch for this as I have artifacts version installed. So you can assign it to others.
Thanks for your help.

Comment 12

[Shivam Singhal [ :championshuttler ]]

I would work on it. I have complete build on my laptop already. :)

Thanks

Comment 13

[Karina G]

Hello, I have a complete build installed as well. If no one else takes this up let me know if it's okay to work on it.

Comment 14

[Avneesh Singhal]

Hi Mark,
If anyone is not working on it currently, can I take this up, to get hands dirty on some artifacts for the first time.

Thanks
Avneesh Singhal

Comment 15

[Mark Banner (:standard8)]

Well in this case, it is Marco (:mak) that is the mentor, though I'm quite happy to help as well!

Comment 16

[Avneesh Singhal]

Yeah, but you are the reporter :-) . I will ask Marco if I get stuck somewhere. Thanks for assigning.

Comment 17

[Avneesh Singhal]

Hi Marco,
I got the change and have done the code changes after running the full optimized build(this step took some time), and the new build is also running fine as it seems. Can you guide me to any automated tests I can do to before raising the patch?

Thanks

Comment 18

[Marco Bonardo [:mak]]

you can run some tests using ./mach test toolkit/components/autocomplete

Comment 19

[Avneesh Singhal]

(In reply to Marco Bonardo [::mak] from comment #18)

you can run some tests using ./mach test toolkit/components/autocomplete

https://searchfox.org/mozilla-central/source/toolkit/components/autocomplete/tests/unit/test_330578.js

Thanks for waiting!
There is this test which uses false attribute in removeValueAt instead of as quoted true in all cases, please go through it and let me know what to do with it.
Thanks
Avneesh Singhal

Comment 20

[Marco Bonardo [:mak]]

Because the removeFRomDB argument is going away, you will remove the aRemoveFromDb from the test and as a consequence also this._lastRemoveFromDb. Then you should remove all the test lines involving _lastRemoveFromDb.
all the calls will just be result.removeValueAt(0);

Comment 21

[Avneesh Singhal]

(In reply to Marco Bonardo [::mak] from comment #20)

Because the removeFRomDB argument is going away, you will remove the aRemoveFromDb from the test and as a consequence also this._lastRemoveFromDb. Then you should remove all the test lines involving _lastRemoveFromDb.
all the calls will just be result.removeValueAt(0);

Thanks, I will do as stated.

Comment 22

[aarushivij]

Hello, Can I take up this issue ?
Thanks :)
Aarushi

Comment 23

[Marco Bonardo [:mak]]

Feel free to start working on it, read the previous comments, ensure you have a working build. Feel free to ask questions.
We assign the bug once a patch is attached.

Comment 24

[aarushivij]

Attachment: Bug 1519099 - removeFromDb parameter of nsIAutoCompleteResult::RemoveValueAt is always true

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f97ae8ad0dd
removeFromDb parameter of nsIAutoCompleteResult::RemoveValueAt is always true r=mak

Comment 26

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/0f97ae8ad0dd