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

RESOLVED FIXED in Firefox 4.0b10

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: ivanmelwise, Assigned: mmm)

Tracking

({regression})

unspecified
Firefox 4.0b10
x86
Windows XP
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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

7 years ago
Confirmed.

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

Probably something from the Places landing.
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
Keywords: regression

Updated

7 years ago
blocking2.0: ? → final+
Blocks: 56019
Blocks: 560198
No longer blocks: 56019

Comment 2

7 years ago
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
Blocks: 606460
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee: mak77 → mars.martian+bugmail
Created attachment 501237 [details] [diff] [review]
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).
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++) {

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.
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.
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?
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?
Created attachment 501723 [details] [diff] [review]
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.
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), example.com, example.net in the tree view. Looking into resolving it in a clean manner.
Created attachment 502251 [details] [diff] [review]
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.
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 = ("@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.
Attachment #502251 - Flags: review?(mak77) → review+
http://hg.mozilla.org/mozilla-central/rev/3ce1c7e22e85
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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.