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

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[ivanmelwise]

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.

Comment 1

[u60234]

Confirmed.

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f11f7ed625ba&tochange=a5413c3c1013

Probably something from the Places landing.

Comment 2

[Alice0775 White]

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

Candidate:
836e83fe061	Marco Bonardo — Bug 606460 - Queries enhancements after temp tables removal. r=sdwilsh a=blocking

Comment 3

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch v1 along with test

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).

Comment 4

[Marco Bonardo [:mak]]

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++) {

s/var/let/

>+      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.

Comment 5

[Geoff Lankow (:darktrojan)]

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.

Comment 6

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch v2

Fixes nits from Marco and also handles localhost case.

Geoff: perhaps you want to give this patch a go?

Comment 7

[Geoff Lankow (:darktrojan)]

(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.

Comment 8

[Marco Bonardo [:mak]]

Mehdi, could you please update the test to cover localhost too?

Comment 9

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch v3, adds tests for localhost.

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.

Comment 10

[Mehdi Mulani [:mmm] (I don't check this)]

Turns out the patch fails on Linux as it relies on the ordering of (local files), example.com, example.net in the tree view. Looking into resolving it in a clean manner.

Comment 11

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch v4, gets around linux failure.

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.

Comment 12

[Marco Bonardo [:mak]]

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 13

[Marco Bonardo [:mak]]

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 = ("@mozilla.org/gnome-gconf-service;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.

Comment 14

[Mehdi Mulani [:mmm] (I don't check this)]

http://hg.mozilla.org/mozilla-central/rev/3ce1c7e22e85