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)
Toolkit
Places
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.
Updated•8 years ago
|
Priority: -- → P5
Comment 1•7 years ago
|
||
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
Updated•6 years ago
|
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
Comment 2•6 years ago
|
||
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)
Reporter | ||
Comment 3•6 years ago
|
||
I'll defer to Marco, it's been a while since I looked at this code.
Flags: needinfo?(paolo.mozmail)
Comment 4•6 years ago
|
||
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)
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•