Last Comment Bug 546253 - moz_openpages_temp table needs to describe pages that may not be in moz_places
: moz_openpages_temp table needs to describe pages that may not be in moz_places
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla2.0b4
Assigned To: Marco Bonardo [::mak] (Away 6-20 Aug)
:
Mentors:
: 558368 562941 (view as bug list)
Depends on: 586506
Blocks: 546254 switch-to-tab 546255 558321 558330 558626 558841 563538 585966 607894
  Show dependency treegraph
 
Reported: 2010-02-14 23:48 PST by Blair McBride [:Unfocused] (UNAVAILABLE)
Modified: 2010-10-28 01:07 PDT (History)
13 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+


Attachments
patch v1.0 (10.71 KB, patch)
2010-08-10 17:27 PDT, Marco Bonardo [::mak] (Away 6-20 Aug)
sdwilsh: review+
Details | Diff | Splinter Review
patch v1.1 (11.48 KB, patch)
2010-08-13 12:54 PDT, Marco Bonardo [::mak] (Away 6-20 Aug)
no flags Details | Diff | Splinter Review
patch v1.2 (11.43 KB, patch)
2010-08-13 12:56 PDT, Marco Bonardo [::mak] (Away 6-20 Aug)
sdwilsh: review+
Details | Diff | Splinter Review

Description Blair McBride [:Unfocused] (UNAVAILABLE) 2010-02-14 23:48:13 PST
Followup for bug 480350.

The moz_openpages_temp currently joins with moz_places based on ID. However, if an open page has been removed from history, then it won't register as being open, since there will be no corresponding row in moz_places to join with. 

To solve this, moz_openpages_temp could instead hold URL and title, and join based on URL.
Comment 1 Blair McBride [:Unfocused] (UNAVAILABLE) 2010-04-09 14:25:16 PDT
*** Bug 558368 has been marked as a duplicate of this bug. ***
Comment 2 Marco Bonardo [::mak] (Away 6-20 Aug) 2010-04-30 10:35:45 PDT
*** Bug 562941 has been marked as a duplicate of this bug. ***
Comment 3 Johnathan Nightingale [:johnath] 2010-06-21 16:39:23 PDT
Definitely blocks, but we wouldn't hold beta1 for it
Comment 4 eyal gruss (eyaler) 2010-06-28 16:43:58 PDT
is this also relevant for switching to "about:" pages, or should i file a separate bug for that?
Comment 5 Marco Bonardo [::mak] (Away 6-20 Aug) 2010-06-28 16:44:39 PDT
this fix should work for about: pages afaict.
Comment 6 Marco Bonardo [::mak] (Away 6-20 Aug) 2010-08-10 11:38:27 PDT
stealing for now, I need this sooner than later, due to add page contentions with async visitURI
Comment 7 Shawn Wilsher :sdwilsh 2010-08-10 11:40:26 PDT
(In reply to comment #6)
> stealing for now, I need this sooner than later, due to add page contentions
> with async visitURI
Can you updated the dependencies?
Comment 8 Marco Bonardo [::mak] (Away 6-20 Aug) 2010-08-10 11:43:16 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > stealing for now, I need this sooner than later, due to add page contentions
> > with async visitURI
> Can you updated the dependencies?

bug 585966 is already in the blocks section of this bug, could also help bug 559217
Comment 9 Marco Bonardo [::mak] (Away 6-20 Aug) 2010-08-10 11:43:51 PDT
sorry I meant bug 563538
Comment 10 Marco Bonardo [::mak] (Away 6-20 Aug) 2010-08-10 17:27:52 PDT
Created attachment 464653 [details] [diff] [review]
patch v1.0
Comment 11 Marco Bonardo [::mak] (Away 6-20 Aug) 2010-08-11 07:03:50 PDT
Comment on attachment 464653 [details] [diff] [review]
patch v1.0

this also moves up results in the list, that was requested in another bug (but was easier to do it here).
The backend (register/unregister) part is something we want regardless, the ordering of the results changes here and would be nice to get users feedback.
Comment 12 Shawn Wilsher :sdwilsh 2010-08-11 11:12:24 PDT
Comment on attachment 464653 [details] [diff] [review]
patch v1.0

r=sdwilsh
Comment 13 Marco Bonardo [::mak] (Away 6-20 Aug) 2010-08-11 12:12:00 PDT
there is probably some glitch with about:blank registration since I see a related failure on tryserver:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1281549452.1281550402.29118.gz TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_tabMatchesInAwesomebar.js | tab is open (1 times) and should recorded in db: about:blank
Or it's just a timing issue of the test that should wait visits addition (bug 576605 has a wip patch for that).
Comment 14 Marco Bonardo [::mak] (Away 6-20 Aug) 2010-08-11 16:35:05 PDT
the test failure is caused by bug 586506 (browser_bug380960.js is the origin of the unmatching tab).
Comment 15 Marco Bonardo [::mak] (Away 6-20 Aug) 2010-08-13 12:54:11 PDT
Created attachment 465797 [details] [diff] [review]
patch v1.1

switch to tab was covering a missing change in docshell, will try to get a diff review
Comment 16 Marco Bonardo [::mak] (Away 6-20 Aug) 2010-08-13 12:56:28 PDT
Created attachment 465801 [details] [diff] [review]
patch v1.2

just a fix to a comment.
Comment 17 Marco Bonardo [::mak] (Away 6-20 Aug) 2010-08-13 13:02:11 PDT
Comment on attachment 465801 [details] [diff] [review]
patch v1.2

asking review on the small dochell change, the design of switch to tab was covering the fact docshell was still trying to set title synchronously before adding a page.
Comment 18 Shawn Wilsher :sdwilsh 2010-08-13 13:25:03 PDT
Comment on attachment 465801 [details] [diff] [review]
patch v1.2

r=sdwilsh on the docshell changes
Comment 19 Jeff Muizelaar [:jrmuizel] 2010-08-13 19:07:53 PDT
http://hg.mozilla.org/mozilla-central/rev/469328928474

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