Closed Bug 421953 Opened 12 years ago Closed 2 years ago

Implement RESULTS_AS_DATE_HOST_SITE_QUERY

Categories

(Firefox :: Bookmarks & History, enhancement)

x86
Windows XP
enhancement
Not set

Tracking

()

RESOLVED INACTIVE

People

(Reporter: ronny.perinke, Unassigned)

Details

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031004 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031004 Minefield/3.0b5pre

Bug 385245 landed a while ago and many things which we where able to do with places are not working anymore. Hence, I'm requesting a DateHostSiteQuery type (smiliar to DateQuery  and DateSiteQuery), which sort/order the resulst in the following manner:
1st level: date
2nd level: host
3rd level: site

e.g.
today > mozillazine.org > forums.mozillazine.org

this was equal to places query place:group=0&group=2&group=1&sort=1


p.s.
quote from Ondrej Brablc:
"This should be easy to implement and fast to execute. Please file a bug and try to get votes for it."

Reproducible: Always
DateSiteQuery stands for RESULTS_AS_DATE_SITE_QUERY and DateQuery for RESULTS_AS_DATE_QUERY
Summary: Implement DateHostSiteQuery → Implement RESULTS_AS_DATE_HOST_SITE_QUERY
Attached patch implementation (obsolete) — Splinter Review
- allows Domain- and Date-Domain-Site-Queries
- code is similar to the already existing Date-Site- and Site-Queries
updated version of the patch diff'ed against todays trunk
Attachment #312350 - Attachment is obsolete: true
Attachment #319427 - Flags: review?(dietrich)
Comment on attachment 319427 [details] [diff] [review]
updated patch (diff against latest trunk)

great, thanks for doing this! however, there's no chance of getting this in at this late stage w/o unit tests. can you please add tests for this result type, and confirm that your change does not regress any of the existing tests?
Attachment #319427 - Flags: review?(dietrich) → review-
Whew!
You mean like one of those http://lxr.mozilla.org/mozilla/source/browser/components/places/tests/unit/?

Do you have some hints for me about what/how to test? :>


best,
Ronny
(In reply to comment #5)
> Whew!
> You mean like one of those
> http://lxr.mozilla.org/mozilla/source/browser/components/places/tests/unit/?
> 
> Do you have some hints for me about what/how to test? :>

I think you should extend this test:

http://mxr.mozilla.org/mozilla/source/toolkit/components/places/tests/unit/test_history_sidebar.js

Do you really need to use mozStorageFunctionGetDomainFromUrl, cannot you just extend existing code which works with rev_host? I did not analyze what should be faster, but you have probably done that.

(In reply to comment #5)
> Whew!
> You mean like one of those
> http://lxr.mozilla.org/mozilla/source/browser/components/places/tests/unit/?
> 
> Do you have some hints for me about what/how to test? :>
It's also good to be sure to test that LiveUpdate of a result set from your query type work properly.  You can see the specific query API tests that I've done [1] (which are probably more complicated than what you want to do in a unit test) and I've got a stub test [2] to illustrate the approach for Live Update testing.
 
[1]: http://wiki.mozilla.org/QA/TDAI/Projects/Places_Tests
[2]: http://mxr.mozilla.org/mozilla/source/toolkit/components/places/tests/queries/stub-test.js

(In reply to comment #6)
> Do you really need to use mozStorageFunctionGetDomainFromUrl, cannot you just
> extend existing code which works with rev_host? I did not analyze what should
> be faster, but you have probably done that.
> 

I wanted a reliable existing method to get the host out of the url (e.g. mozilla.org from bugzilla.mozilla.org). I chose the nsIEffectiveTLDService and because it required a valid nsIURI, I took the url instead of rev_host (the full url can be transfered easily into a nsIURI).

Thanks for the hints about the tests. I'll try to create one. ^^
Status: UNCONFIRMED → NEW
Ever confirmed: true
I followed Ondrej's suggestion and extended the test for the history sidebar at http://mxr.mozilla.org/mozilla/source/toolkit/components/places/tests/unit/test_history_sidebar.js

The test is similar to the RESULTS_AS_DATE_SITE_QUERY test. I expected the following structure of the history when using RESULTS_AS_DATE_DOMAIN_SITE_QUERY:

Today
  |--google.com
    |--mirror0.google.com
      |--http://mirror0.google.com/c
      |--http://mirror0.google.com/d
  |--mozilla.com
    |--mirror0.mozilla.com
      |--http://mirror0.mozilla.com/c
      |--http://mirror0.mozilla.com/d
Yesterday
  |--google.com
    |--mirror1.google.com
      |--http://mirror1.google.com/c
      |--http://mirror1.google.com/d
  |--mozilla.com
    |--mirror1.mozilla.com
      |--http://mirror1.mozilla.com/c
      |--http://mirror1.mozilla.com/d
2 days ago
  |--google.com
    |--mirror2.google.com
      |--http://mirror2.google.com/c
      |--http://mirror2.google.com/d
  |--mozilla.com
    |--mirror2.mozilla.com
      |--http://mirror2.mozilla.com/c
      |--http://mirror2.mozilla.com/d
3 days ago
  |--google.com
    |--mirror3.google.com
      |--http://mirror3.google.com/c
      |--http://mirror3.google.com/d
  |--mozilla.com
    |--mirror3.mozilla.com
      |--http://mirror3.mozilla.com/c
      |--http://mirror3.mozilla.com/d
4 days ago
  |--google.com
    |--mirror4.google.com
      |--http://mirror4.google.com/a
      |--http://mirror4.google.com/b
  |--mozilla.com
    |--mirror4.mozilla.com
      |--http://mirror4.mozilla.com/a
      |--http://mirror4.mozilla.com/b
5 days ago
  |--google.com
    |--mirror5.google.com
      |--http://mirror5.google.com/a
      |--http://mirror5.google.com/b
  |--mozilla.com
    |--mirror5.mozilla.com
      |--http://mirror5.mozilla.com/a
      |--http://mirror5.mozilla.com/b
6 days ago
  |--google.com
    |--mirror6.google.com
      |--http://mirror6.google.com/a
      |--http://mirror6.google.com/b
  |--mozilla.com
    |--mirror6.mozilla.com
      |--http://mirror6.mozilla.com/a
      |--http://mirror6.mozilla.com/b
Older than 6 days
  |--google.com
    |--mirror7.google.com
      |--http://mirror7.google.com/a
      |--http://mirror7.google.com/b
  |--mozilla.com
    |--mirror7.mozilla.com
      |--http://mirror7.mozilla.com/a
      |--http://mirror7.mozilla.com/b

The leafs of each node are visits.
I tested "4 days ago" for the expected structure and it passed the test.

Here is the result:
*** test pending
*** test finished
*** exiting
*** PASS ***
Exception: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_places/unit/head_bookmarks.js :: clearDB :: line 99"  data: no]

Unfortunately I do not have an explanation for the exception, because "everyone" has all permissions and it's not write-protected.
Attached patch updated patchSplinter Review
- includes sidebar unit test

- additions to browser/components/places/content/controller.js in _buildSelectionMetadata() used by buildContextMenu()

- adds nodeIsDomain() to PlacesUtils and updates nodeIsDay() as well as nodeIsHistoryContainer()


Would be nice to have it in Firefox 3.1 ;-)
Attachment #319427 - Attachment is obsolete: true
Attachment #328016 - Flags: review?(dietrich)
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Comment on attachment 328016 [details] [diff] [review]
updated patch

Cleaning out my review queue. Wow, I am sad to have let this sit so long. I'm very sorry Ronny. This is likely rotted quite far at this point.
Attachment #328016 - Flags: review?(dietrich)
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: 2 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.