Closed Bug 474334 Opened 12 years ago Closed 12 years ago
"place:" is a valid query to deserialize and should not cause an "ignoring unknown key:" warning
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.
Isn't the test for this covered by adw's work in bug 370197?
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.
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.
Summary: "place:" shoud be a valid query to deserialize → "place:" is a valid query to deserialize and should not cause an "ignoring unknown key:" warning
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.
Attachment #357712 - Flags: review?(dietrich) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 357712 [details] [diff] [review] patch v1.0 asking approval, low risk
Attachment #357712 - Flags: approval1.9.1?
Attachment #357712 - Flags: approval1.9.1? → approval1.9.1+
You need to log in before you can comment on or make changes to this bug.