Closed Bug 824074 Opened 12 years ago Closed 11 years ago

Properly update frecency sorting in bookmarks queries. (was: Intermittent places/tests/unit/test_result_sort.js | 9 == 8)

Categories

(Toolkit :: Places, defect)

Product:
Toolkit
Component:
Places
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla21
Tracking Flags:
Tracking Status
firefox20 --- affected
firefox21 --- fixed

People

(Reporter: emorley, Assigned: mak)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

patch v1.0
10.92 KB, patch
asaf
: review+
Details | Diff | Splinter Review 
Rev3 WINNT 6.1 mozilla-inbound debug test xpcshell on 2012-12-21 03:30:11 PST for push b323378d9d27

slave: talos-r3-w7-053

https://tbpl.mozilla.org/php/getParsedLog.php?id=18161074&tree=Mozilla-Inbound

{
TEST-PASS | c:/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_result_sort.js | [checkOrder : 60] 8 == 8

TEST-PASS | c:/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_result_sort.js | [checkOrder : 58] 7 == 7

TEST-PASS | c:/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_result_sort.js | [checkOrder : 59] 9 == 9

TEST-PASS | c:/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_result_sort.js | [checkOrder : 60] 8 == 8

TEST-INFO | (xpcshell/head.js) | test 2 finished

TEST-INFO | (xpcshell/head.js) | running event loop

TEST-UNEXPECTED-FAIL | c:/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_result_sort.js | 9 == 8 - See following stack:
JS frame :: c:\talos-slave\test\build\xpcshell\head.js :: do_throw :: line 461
JS frame :: c:\talos-slave\test\build\xpcshell\head.js :: _do_check_eq :: line 555
JS frame :: c:\talos-slave\test\build\xpcshell\head.js :: do_check_eq :: line 576
JS frame :: c:/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_result_sort.js :: checkOrder :: line 58
JS frame :: c:/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_result_sort.js :: <TOP_LEVEL> :: line 117
JS frame :: resource://gre/modules/commonjs/promise/core.js :: effort :: line 53
JS frame :: resource://gre/modules/commonjs/promise/core.js :: resolveDeferred :: line 125
JS frame :: resource://gre/modules/commonjs/promise/core.js :: then :: line 34
JS frame :: resource://gre/modules/commonjs/promise/core.js :: resolve :: line 167
JS frame :: c:/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/head_bookmarks.js -> file:///c:/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/head_common.js :: .handleCompletion :: line 654
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
}
to be investigated
Assignee: nobody → mak77
There is a real bug here, we don't live update correctly the frecency in bookmarks queries. I have a patch that has a couple defects yet, though I'm not willing to handle those here, so I will file follow-ups:
1. onDeleteVisits we still don't do the right thing
2. if we'd pass frecency along with onItemVisited we may save a query, but then I wonder if we may have to do the same in onVisit and if this enforces us to do an additional query to fetch frecency after we set it.

There is an alternative that would be to consider a bookmarks query sorted by frecency a complex query, thus forcing a refresh(), but I'm less prone to whole refreshes nowadays.
Status: NEW → ASSIGNED
Attached patch patch v1.0 — — Splinter Review 

        Attachment #704220 -
        Flags: review?(mano)

    
  

        Attachment #704220 -
        Flags: review?(mano) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/f67a97dda10d
Flags: in-testsuite+
Summary: Intermittent places/tests/unit/test_result_sort.js | 9 == 8 → Properly update frecency sorting in bookmarks queries. (was: Intermittent places/tests/unit/test_result_sort.js | 9 == 8)
Target Milestone: --- → mozilla21

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/f67a97dda10d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Can we uplift this to Aurora too?

          status-firefox20:
          --- → affected

          status-firefox21:
          --- → fixed

    
    

    
  
(In reply to Ryan VanderMeulen from comment #44)
> Can we uplift this to Aurora too?

This also touches Places results code.  While the change is not risky, it doesn't bring specific wins to the users, thus I'm not sure there are the requirements for an uplift.  I think would be safer to just disable the test in Aurora if it's too noisy.

    
    

    
  
rs=me on test disable, if you want to do that.

    
    

    
      
  

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: