Closed Bug 621843 Opened 14 years ago Closed 13 years ago

By Date and Site sidebar view does not group entries by date


(Firefox :: Bookmarks & History, defect)

Windows XP
Not set



Firefox 4.0b10
Tracking Status
blocking2.0 --- final+


(Reporter: ivanmelwise, Assigned: mmm)



(Keywords: regression, Whiteboard: [softblocker])


(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b9pre) Gecko/20101228 Firefox/4.0b9pre
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b9pre) Gecko/20101228 Firefox/4.0b9pre

Mentioned view should group history entries by date and site. It does not group entries by date.

Reproducible: Always

Steps to Reproduce:
1. Open History sidebar.
2. Expand Today (for instance).
3. Go some previously (yesterday, last week, not today) visited page.
4. See site group appeared.
5. Expand site group.
Actual Results:  
*All* site entries from history under site group.

Expected Results:  
Only expanded date (or period) entries under site group.

Regression range:

Probably something from the Places landing.
blocking2.0: --- → ?
Ever confirmed: true
Keywords: regression
blocking2.0: ? → final+
Blocks: 56019
Blocks: 560198
No longer blocks: 56019
In local build(windows),
build from 4626f19fa27e : fails
build from b836e83fe061 : fails
build from 45655fe19c7f : works
build from debf742b7265 : works
build from 630a7a751079 : works
build from fd7058ddd38b : works
build from 68616dfb2bc9 : works
build from 5c484855cda4 : works

836e83fe061	Marco Bonardo — Bug 606460 - Queries enhancements after temp tables removal. r=sdwilsh a=blocking
Blocks: 606460
Assignee: nobody → mak77
Assignee: mak77 → mars.martian+bugmail
Attached patch Patch v1 along with test (obsolete) — Splinter Review
Fixed an omission in the SQL query.

The test for this basically ensures that a sort by date and site query works correctly, and even tests the live-updating part (albeit, in a slightly weird manner).
Attachment #501237 - Flags: review?(sdwilsh)
Comment on attachment 501237 [details] [diff] [review]
Patch v1 along with test

>diff --git a/toolkit/components/places/tests/queries/test_sort-date-site-grouping.js b/toolkit/components/places/tests/queries/test_sort-date-site-grouping.js

>+let olderThanSixMonths = today - (DAY_MICROSEC * 31 * 7);

this should go with other const in head_queries.js

>+let testData = [
>+  {

>+  }];

please move "];" alone to a new line for readability

>+function run_test() {
>+  populateDB(testData);
>+  // In this test, there are three levels of results:

nit: newline after populateDB

>+  let query = { };
>+  let options = { };
>+  PlacesUtils.history.queryStringToQueries("place:type=5", query, { }, options);
>+  query = (query.value)[0];
>+  options = options.value;

Actually I prefer if you create query and options objects through PlacesUtils.history.getNewQuery() and let query = PlacesUtils.history.getNewQueryOptions(); rather than exploding a place:

>+  let result = PlacesUtils.history.executeQuery(query, options);
>+  let root = result.root;

you don't need the result, so you can just let root = PU.h.executeQ().root;

>+  root.containerOpen = true;
>+  // This corresponds to the number of date ranges.
>+  do_check_eq(root.childCount, leveledTestData.length);
>+  // Here we check the first level of results.
>+  [0,1].forEach(function(index) {

this is probably simpler as a for loop since it's going through a ordered list of ints

>+    let node = root.getChild(index);
>+    node.containerOpen = true;
>+    do_check_true(PlacesUtils.nodeIsDay(node));
>+    node.QueryInterface(Ci.nsINavHistoryQueryResultNode);

iirc you can use PlacesUtils.asQuery(node) to QI

>+    let queries = node.getQueries();
>+    let options = node.queryOptions;
>+    do_check_eq(queries.length, 1);
>+    let query = queries[0];
>+    do_check_true(query.hasBeginTime && query.hasEndTime);
>+    // Here we check the second level of results.

could you please split some of this code in separate helper functions? having all the code inline makes hard to follow the logic.
For example the second level check looks like a good candidate for being moved out and you would not have naming conflicts (root|newRoot, index|secondIndex and so on).
Looks like you are complicating names just to avoid a nice helper :)

>+    let result = PlacesUtils.history.executeQuery(query, options);
>+    let newRoot = result.root;

oneline this

>+    roots.push([]);
>+    newRoot.containerOpen = true;
>+    do_check_eq(newRoot.childCount, leveledTestData[index].length);
>+    for (var secondIndex = 0; secondIndex < newRoot.childCount; secondIndex++) {


>+      let secondResult = PlacesUtils.history.
>+                                     executeQuery(secondQuery, secondOptions);
>+      let secondRoot = secondResult.root;

oneline this

the fix is fine, the test can be made cleaner but that's not blocking a positive review.
Attachment #501237 - Flags: review?(sdwilsh) → review+
I've tried this patch on my tree... works fine except for the "(local files)" folder, which still contains every local file I've ever looked at.
Attached patch Patch v2 (obsolete) — Splinter Review
Fixes nits from Marco and also handles localhost case.

Geoff: perhaps you want to give this patch a go?
Attachment #501237 - Attachment is obsolete: true
(In reply to comment #6)
> Created attachment 501485 [details] [diff] [review]
> Patch v2
> Fixes nits from Marco and also handles localhost case.
> Geoff: perhaps you want to give this patch a go?

Yup, that's better.
Mehdi, could you please update the test to cover localhost too?
Mak: my mistake, should've added those the first time.

As this has been two revisions, does it still need a quick review? Flagged it just in case.
Attachment #501485 - Attachment is obsolete: true
Attachment #501723 - Flags: review?(mak77)
Attachment #501723 - Flags: review?(mak77) → review+
Turns out the patch fails on Linux as it relies on the ordering of (local files),, in the tree view. Looking into resolving it in a clean manner.
Filed bug 624024 for Linux issue. I couldn't think of any clean way to get around it, so I just tested for Linux.

Was thinking of changing the test so that it allowed any order of domains but I figured that it would also be useful to check that sites are ordered.
Attachment #501723 - Attachment is obsolete: true
Attachment #502251 - Flags: review?(mak77)
I'd suggest to just skip the test on linux for now (early return) and add a note to bug 624024 to re-enable it once the underlying bug is fixed.
Comment on attachment 502251 [details] [diff] [review]
Patch v4, gets around linux failure.

>diff --git a/toolkit/components/places/tests/queries/test_sort-date-site-grouping.js b/toolkit/components/places/tests/queries/test_sort-date-site-grouping.js

>+  // On Linux, the (local files) folder is shown after sites unlike Mac/Windows.
>+  // We renumber the levels to handle this. (Bug 624024)
>+  let isLinux = (";1" in Components.classes);
>+  if (isLinux) {
>+  }

As I said, I think it makes no sense to test a behavior that we consider a bug.
You should just early return with a comment that points to bug 624024 and add a note in that bug to remove the early return once the bug is fixed.
Testing the opposite order would be fine if the inverted behavior would be the expected one.
You don't need a further review to do so.
Attachment #502251 - Flags: review?(mak77) → review+
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b10
Whiteboard: [softblocker]
You need to log in before you can comment on or make changes to this bug.