Open Bug 816925 Opened 12 years ago Updated 2 years ago

Remove isDetails from the "queries" folder in Places tests

Categories

(Toolkit :: Places, defect, P5)

defect

Tracking

()

REOPENED

People

(Reporter: Paolo, Unassigned)

Details

The operation type "isDetails" may not be needed anymore in "head_queries.js", now that the updatePlaces API is available combining what was previously done by two different APIs.
Priority: -- → P5
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
From bug 778694 comment 3: > I think isDetails is now pointless... Previously addvisit was > unable to set the title, so isDetails was doing that... But now it looks > useless. I think we need a follow-up bug to replace isDetails with isVisit, > maybe you could try if changing if (qdata.isVisit) to if (qdata.isVisit || > qdata.isDetails) and see if tests pass. Then the follow-up may look into > removing it completely. Based on that I just tried changing the first if statement highlighted in the following link to `(qdata.isVisit || qdata.isDetails)` and in the second one I dropped the `!qdata.isDetails`: https://searchfox.org/mozilla-central/rev/6201a9e0067cf6af118c6a99ae9314b8ceb2c4d5/toolkit/components/places/tests/queries/head_queries.js#44,53 The tests still pass. However, I think the question is then what to do with: https://searchfox.org/mozilla-central/rev/6201a9e0067cf6af118c6a99ae9314b8ceb2c4d5/toolkit/components/places/tests/queries/head_queries.js#85-92 If I change that to qdata.isVisit then some of the tests fail. Marco, Paolo, any ideas?
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(mak77)
I'll defer to Marco, it's been a while since I looked at this code.
Flags: needinfo?(paolo.mozmail)
First, I must be clear this is an unclear framework we don't want to support long term, it was done by QA while working on these tests, but I don't think it is well designed. As such, any move towards removing this special framework is welcome. (In reply to Mark Banner (:standard8) from comment #2) > However, I think the question is then what to do with: > > https://searchfox.org/mozilla-central/rev/ > 6201a9e0067cf6af118c6a99ae9314b8ceb2c4d5/toolkit/components/places/tests/ > queries/head_queries.js#85-92 > > If I change that to qdata.isVisit then some of the tests fail. changing just this to isVisit is pointless, since there is a previous if (isVisit) check in the same scope. The right thing to do is probably to change the tests using isDetails to use isVisit, completely remove this if (isDetails). then ideally the first isVisit check will handle these calls (referrer can already be optional, Not sure if "transition: qdata.transType," would complain for undefined, but should be trivially fixable.
Flags: needinfo?(mak77)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.