474334 - "place:" is a valid query to deserialize and should not cause an "ignoring unknown key:" warning

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Seen while looking at bug 370197.

When we have a query where we only defined default values (for example expandQueries = 1 and queryType=QUERY_TYPE_HISTORY) and we serialize it, we end up with a query like "place:".
When we deserialize this query we get a WARNING since we are trying to handle "place:" as a query option... instead we should simply return an array with a new default query in it, since that's a valid case.

i'll add a test for this.

Comment 1

[Shawn Wilsher :sdwilsh]

Isn't the test for this covered by adw's work in bug 370197?

Comment 2

[Marco Bonardo [:mak]]

could be, but it is not atm, i still have to look at the reason it's not failing, so if that can be fixed there i'll not create a new test, clearly.

Comment 3

[Marco Bonardo [:mak]]

mh instead this is currently working, even if the code is wrong, because when we get tokens we wrongly get a wrong token like "place:". Having at least one token we still try to deserialize, but we warn at
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistoryQuery.cpp#831
because "place:" is not a valid option.

So, the final result is the same, we get a valid default query, but through a wrong path. The test cannot fail due to that, and is indeed correctly testing this.

Moreover i cannot write a test to check an internal method when API is still returning a correct value.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 357712 [details] [diff] [review]
patch v1.0

test in bug 370197 is valid to cover deserialization of "place:", the wrong path can't be tested though.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 357712 [details] [diff] [review]
patch v1.0

r=me

Comment 6

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/7e3266e01563

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 357712 [details] [diff] [review]
patch v1.0

asking approval, low risk

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 357712 [details] [diff] [review]
patch v1.0

a191=beltzner

Comment 9

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c17c492fcb57