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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.2a1
fixed1.9.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
Created attachment 357712 [details] [diff] [review]
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.
Isn't the test for this covered by adw's work in bug 370197?
(Assignee)

Comment 2

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

Comment 3

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

Updated

10 years ago
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
(Assignee)

Updated

10 years ago
Attachment #357712 - Flags: review?(dietrich)
(Assignee)

Comment 4

10 years ago
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+
Comment on attachment 357712 [details] [diff] [review]
patch v1.0

r=me
(Assignee)

Comment 6

10 years ago
http://hg.mozilla.org/mozilla-central/rev/7e3266e01563
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

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

asking approval, low risk
Attachment #357712 - Flags: approval1.9.1?
(Assignee)

Updated

10 years ago
Target Milestone: --- → mozilla1.9.2a1
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.