Implement RESULTS_AS_DATE_HOST_SITE_QUERY

NEW
Unassigned

Status

()

Firefox
Bookmarks & History
--
enhancement
10 years ago
7 years ago

People

(Reporter: Ronny Perinke, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

10 years ago
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
(Reporter)

Comment 2

10 years ago
Created attachment 312350 [details] [diff] [review]
implementation

- allows Domain- and Date-Domain-Site-Queries
- code is similar to the already existing Date-Site- and Site-Queries
(Reporter)

Comment 3

10 years ago
Created attachment 319427 [details] [diff] [review]
updated patch (diff against latest trunk)

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-
(Reporter)

Comment 5

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

Comment 6

10 years ago
(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.

Comment 7

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

(Reporter)

Comment 8

10 years ago
(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. ^^

Updated

10 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 9

10 years ago
Created attachment 321981 [details] [diff] [review]
unit test (updated history-sidebar-test)

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

Comment 10

10 years ago
Created attachment 328016 [details] [diff] [review]
updated patch

- 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)
You need to log in before you can comment on or make changes to this bug.