Closed
Bug 561450
(placesSessionID)
Opened 14 years ago
Closed 11 years ago
Stop supporting session ids in Places
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: stechz, Assigned: mak)
References
(Blocks 1 open bug)
Details
(Whiteboard: [snappy:p1])
Attachments
(1 file, 2 obsolete files)
86.30 KB,
patch
|
Details | Diff | Splinter Review |
Both History.cpp and nsNavHistory.cpp use the synchronous GetNewSessionId(). If session ID counter hasn't been initialized a sync SQL query is done to fetch the biggest ID, which is incremented by 1.
Assignee | ||
Updated•13 years ago
|
Blocks: StorageJank
Updated•13 years ago
|
Alias: placesSessionID
Updated•13 years ago
|
Assignee: nobody → adw
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•13 years ago
|
||
Hey Drew do you have ideas about this? since actually till we completely remove any sycnhronous addVisit api it's unlikely we can do this.
Comment 2•13 years ago
|
||
The only thing I thought of, other than making GetNewSessionId callers async, is making history service initialization async. The initial session ID counter could be cached then, along with whatever other I/O or blocking work that needs to happen on init, like, say, a database integrity check. I haven't really checked all the history service clients to see if they'd be ruined by an async init.
Assignee | ||
Comment 3•13 years ago
|
||
Till we kill addVisit, addPageWithDetails and such calls adding synchronous visits, it's unlikely we can make anything more async in the initialization process. That is bug 700250, Yoric took it but I don't think he plans to work on that anytime soon if you want to steal it, it likely requires to add an updatePlaces() wrapper in head_common.js that spins the async thread till the visit is added, to replace the thousands calls to addVisit in tests without having to rewrite them. It also requires fixing the IE migrator (bug 591884), that on the other side may be broken from some time and should be ported to JS (mimicking the Chrome migrator structure in bug 505192) to improve its maintainability and to use the updatePlaces() JS API.
Assignee | ||
Updated•12 years ago
|
Depends on: asyncHistory
Updated•12 years ago
|
Assignee: adw → nobody
Status: ASSIGNED → NEW
Assignee | ||
Updated•12 years ago
|
No longer depends on: asyncHistory
Updated•12 years ago
|
Whiteboard: [snappy]
Updated•12 years ago
|
Whiteboard: [snappy] → [snappy:p1]
Assignee | ||
Comment 4•12 years ago
|
||
just a small updated here, what is left to do is basically removing addVisit() from some toolkit and browser tests, then we can remove the AddVisit() API, at that point this should be trivial to fix.
Assignee | ||
Comment 5•11 years ago
|
||
At this point, based on the "recent" changes to Places, I think the simplest thing is to kill session ids support. Though, before doing that it's work to check if we need them for anything in-tree, and the use-cases in add-ons.
Summary: Make session ID generation asynchronous → Stop supporting session ids in Places
Assignee | ||
Updated•11 years ago
|
Blocks: asyncHistory
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•11 years ago
|
||
Assignee | ||
Comment 8•11 years ago
|
||
To sum up the add-ons check we did yesterday, this affects about 7 add-ons, out of these 3 are outdated and no more supported, a couple are affected but will keep working properly, one is affected but has few more than 100 users.
Assignee | ||
Comment 9•11 years ago
|
||
ehr, a couple is actually 3, btw the point in the end is that this change may affect a few more than 100 users
Assignee | ||
Comment 10•11 years ago
|
||
just some cleanup of things I noticed in the previous version
Attachment #728132 -
Attachment is obsolete: true
Attachment #728134 -
Flags: superreview?
Attachment #728134 -
Flags: review?(mano)
Assignee | ||
Updated•11 years ago
|
Attachment #728134 -
Flags: superreview? → superreview?(gavin.sharp)
Comment 11•11 years ago
|
||
Comment on attachment 728134 [details] [diff] [review] patch v1.1 Review of attachment 728134 [details] [diff] [review]: ----------------------------------------------------------------- It's pretty astonishing how much code we have for a feature which was never really used. ::: toolkit/components/places/PlacesUtils.jsm @@ +193,5 @@ > + "https://bugzilla.mozilla.org/show_bug.cgi?id=561450"); > + return this.nodeIsURI(aNode) && aNode.parent && > + this.nodeIsQuery(aNode.parent) && > + asQuery(aNode.parent).queryOptions.resultType == > + Ci.nsINavHistoryQueryOptions.RESULTS_AS_VISIT; What are the use cases for this method? If there are any use cases we may not want to deprecate it. @@ -189,5 @@ > * A result node > * @returns true if the node is a URL item, false otherwise > */ > - uriTypes: [Ci.nsINavHistoryResultNode.RESULT_TYPE_URI, > - Ci.nsINavHistoryResultNode.RESULT_TYPE_VISIT], How many addons rely on this one? I would consider replacing it with a deprecated getter. ::: toolkit/components/places/nsINavHistoryService.idl @@ +57,4 @@ > > // Full visit nodes are deprecated and unsupported. > // This line exists just to avoid reusing the value: > // const unsigned long RESULT_TYPE_FULL_VISIT = 2; // nsINavHistoryFullVisitResultNode I still think it's odd to remove keep FULL_VISIT in place while removing VISIT :) @@ +622,5 @@ > * (which will comprise the majority of visit notifications) to save work. > * > * @param aVisitID ID of the visit that was just created. > * @param aTime Time of the visit > + * @param aSessionID No longer supported, thus resolves to 0. ", thus resolves to 0" -> "(always set to 0)" would be better, I think. ::: toolkit/components/places/nsNavHistoryResult.cpp @@ -2490,5 @@ > addition->mTransitionType = aTransitionType; > if (!history->EvaluateQueryForNode(mQueries, mOptions, addition)) > return NS_OK; // don't need to include in our query > > - if (addition->IsVisit()) { So, like with the js helper (nodeIsVisit), I wonder if it's worth keeping it around. Your call.
Attachment #728134 -
Flags: review?(mano) → review+
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Mano from comment #11) > Comment on attachment 728134 [details] [diff] [review] > patch v1.1 > > Review of attachment 728134 [details] [diff] [review]: > ----------------------------------------------------------------- > > It's pretty astonishing how much code we have for a feature which was never > really used. > > ::: toolkit/components/places/PlacesUtils.jsm > @@ +193,5 @@ > > + "https://bugzilla.mozilla.org/show_bug.cgi?id=561450"); > > + return this.nodeIsURI(aNode) && aNode.parent && > > + this.nodeIsQuery(aNode.parent) && > > + asQuery(aNode.parent).queryOptions.resultType == > > + Ci.nsINavHistoryQueryOptions.RESULTS_AS_VISIT; > > What are the use cases for this method? If there are any use cases we may > not want to deprecate it. add-ons, more specifically an add-on with more than a thousands users. > @@ -189,5 @@ > > * A result node > > * @returns true if the node is a URL item, false otherwise > > */ > > - uriTypes: [Ci.nsINavHistoryResultNode.RESULT_TYPE_URI, > > - Ci.nsINavHistoryResultNode.RESULT_TYPE_VISIT], > > How many addons rely on this one? I would consider replacing it with a > deprecated getter. Snowl, that is no more maintained, and a thunderbird add-on that copied all of our code included PlacesUIUtils for whatever reason https://addons.mozilla.org/en-US/thunderbird/addon/wat-webapplicationtab/ > ::: toolkit/components/places/nsINavHistoryService.idl > @@ +57,4 @@ > > > > // Full visit nodes are deprecated and unsupported. > > // This line exists just to avoid reusing the value: > > // const unsigned long RESULT_TYPE_FULL_VISIT = 2; // nsINavHistoryFullVisitResultNode > > I still think it's odd to remove keep FULL_VISIT in place while removing > VISIT :) Didn't get this, we removed both... > @@ +622,5 @@ > > * (which will comprise the majority of visit notifications) to save work. > > * > > * @param aVisitID ID of the visit that was just created. > > * @param aTime Time of the visit > > + * @param aSessionID No longer supported, thus resolves to 0. > > ", thus resolves to 0" -> "(always set to 0)" would be better, I think. lol, I changed it before attaching :) > ::: toolkit/components/places/nsNavHistoryResult.cpp > @@ -2490,5 @@ > > addition->mTransitionType = aTransitionType; > > if (!history->EvaluateQueryForNode(mQueries, mOptions, addition)) > > return NS_OK; // don't need to include in our query > > > > - if (addition->IsVisit()) { > > So, like with the js helper (nodeIsVisit), I wonder if it's worth keeping it > around. Your call. nobody can call into this apart us, it's an internal helper.
Updated•11 years ago
|
Attachment #728134 -
Flags: superreview?(gavin.sharp) → superreview+
Assignee | ||
Comment 13•11 years ago
|
||
introduced a deprecated uriTypes getter, for now.
Attachment #728134 -
Attachment is obsolete: true
Assignee | ||
Comment 14•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/69f4d3ba11a5
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla22
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/69f4d3ba11a5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•