Closed
Bug 1083469
Opened 10 years ago
Closed 9 years ago
Breakdown: allow keywords APIs to work in parallel with Bookmarks.jsm
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
People
(Reporter: mak, Assigned: mak)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(2 obsolete files)
once we have sync and async APIs, the old keywords API might be broken due to the internal caching, we should fix that and provide new keyword APIs if they are missing.
Flags: qe-verify-
Flags: in-testsuite+
Flags: firefox-backlog+
Assignee | ||
Comment 1•10 years ago
|
||
I don't think there's any better way to do this than removing the cache (So both APIs access the same underlying data). unfortunately that means consumers will hit the disk every time. The consumer I'm mostly worried about though, is autocomplete, through its usage of getURIForKeyword. We might need to introduce a cache in PlacesUtils that keeps a keyword->uri map updated based on notifications. Then autocomplete could use that cache.
Assignee | ||
Comment 2•10 years ago
|
||
also getShortcutOrURIAndPostData would benefit
Updated•10 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Assignee | ||
Comment 3•10 years ago
|
||
So, the problem here is getShortcutOrURIAndPostData uses a synchronous callback. That was done on purpose to be able to convert Task.jsm to resolve-on-next-tick promises without having to fix too many tests that are relying on same-tick resolution. Unfortunately we are again hitting the same issue, where getShortcutOrURIAndPostData should be async but it's not, and we'll hit it again in future. Another interesting thing is that when we use getCharsetForURI, the callback is effectively async already... So currently that method is half-and-half. I will see on Try how many tests are involved here, and decide based on that if it's worth to finally fix this properly, or delay again to another bug.
Assignee | ||
Comment 4•10 years ago
|
||
here is all of the tests failing due to async getShortcutOrURIAndPostData INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_getshortcutoruri.js INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_locationBarCommand.js INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_urlbarEnter.js INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_urlbarStop.js INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug1064280_changeUrlInPinnedTab.js INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/docshell/test/browser/browser_search_notification.js INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/docshell/test/browser/browser_uriFixupIntegration.js
Assignee | ||
Comment 5•10 years ago
|
||
I think I will split the getShortcutOrURL part to a separate bug, since it is a less hot path than autocomplete that should be fine and soon or later we'll convert all consumers regardless.
Assignee | ||
Comment 6•10 years ago
|
||
Filed bug 1100291 and made it block bug 1100294
Assignee | ||
Updated•10 years ago
|
Summary: Fix keywords APIs → Allow keywords APIs to work in parallel with Bookmarks.jsm
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8523913 -
Flags: review?(mano)
Assignee | ||
Comment 8•10 years ago
|
||
/r/719 - Bug 1083469 - Allow to use old keywords APIs along with Bookmarks.jsm r=mano Pull down this commit: hg pull review -r b4667a5cf865922d89d46b546661ce35b5be4f84
Comment 9•10 years ago
|
||
https://reviewboard.mozilla.org/r/717/#review437 ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 1) > + let observer = { Since KeywordsCache is an internal object, wouldn't it be better to make it implement the observer? ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 1) > + __noSuchMethod__() {}, // Catch all remaining onItem* methods. BTW, now that we've Object.assign, I think I'm going to file a bug on intorducing PlacesUtils.ImplementBookmarksObserver({ just the functions you want }) ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 1) > + yield this._reloadPromise missing ; ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 1) > + this._reloadCache().catch(Cu.reportError); Add a comment here starting that the cache should be readily avaiable as fast as possible. Otherwise someone may optimize this to just unset _initialized. ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 1) > + dump(`\nitemchanged reload\n\n`); remove that dump. ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 1) > + , Ci.nsIAnnotationObserver ]), Remove nsIAnnotationObserver. ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 1) > + _reloadCache: Task.async(function* () { Just _reloadCache(), since you use Task.spawn. ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 1) > + * Gets the href and its associated post data for a given keyword, if any. s/its associated/the/ ::: toolkit/components/places/nsPlacesAutoComplete.js (Diff revision 1) > - PlacesUtils.bookmarks.getURIForKeyword(this._currentSearchString)) { > + (yield PlacesUtils.bookmarks.getURIForKeyword(this._currentSearchString)).href) { I guess you meant to use the new async method here.
Updated•10 years ago
|
Attachment #8523913 -
Flags: review?(mano) → review+
Comment 10•10 years ago
|
||
https://reviewboard.mozilla.org/r/717/#review439 Ship It!
Updated•9 years ago
|
Attachment #8523913 -
Flags: review+
Comment 11•9 years ago
|
||
https://reviewboard.mozilla.org/r/717/#review503 Ship It!
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ce637f8721d3
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/ce637f8721d3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•9 years ago
|
||
I backed out this because tests were failing when disabling unified complete https://hg.mozilla.org/mozilla-central/rev/6467bdc11662 failures in browser/base/content/test/general/browser_bug304198.js browser/base/content/test/general/browser_bug559991.js browser/base/content/test/general/browser_bug817947.js browser/base/content/test/general/browser_canonizeURL.js looks like old autocomplete doesn't love a Task.spawn...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•9 years ago
|
Status: REOPENED → ASSIGNED
Iteration: 36.3 → 37.1
Assignee | ||
Comment 15•9 years ago
|
||
I did a few changes: nsPlacesAutocomplete is now yielding, that means a search can be canceled during the yield. Thus I introduced a _pendingSearch to check if after the yield the query is still valid. note we cannot use _pendingQuery cause that's tracking the sqlite query instead. the new code in Unified is better organized... UnifiedComplete is missing a couple pending checks, it should check pending after every yield. finally, there was a race condition in the KeywordsCache initialization, since _initialized was set at the end, we were calling _initialized multiple times, and addObserver was throwing (new test added for this). These 2 are all the changes I did, I hope this helps simplifying the review. Tests are now passing locally, still I'm pushing to try.
Attachment #8523913 -
Attachment is obsolete: true
Attachment #8530450 -
Flags: review?(mano)
Assignee | ||
Comment 16•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=0445874d34e2
Assignee | ||
Comment 17•9 years ago
|
||
hm, e10s BC1 is unhappy yet chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug559991.js | uncaught exception - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIAutoCompleteController.getImageAt] at chrome://global/content/bindings/autocomplete.xml:1144
Comment 18•9 years ago
|
||
Comment on attachment 8530450 [details] [diff] [review] patch v2 Cancelling review request both because the disturbing test failure and because Marco and I figured we need some sort of public access to the keywords cache, so that future Bookmarks.fetchTree/current PU.promiseBookmarksTree can make use of it. For this reason I also think it makes sense to have the cache object in Bookmarks.jsm rather than in PlacesUtils.jsm.
Attachment #8530450 -
Flags: review?(mano)
Updated•9 years ago
|
Iteration: 37.1 → 37.2
Updated•9 years ago
|
Iteration: 37.2 → 37.3
Updated•9 years ago
|
Iteration: 37.3 - 12 Jan → 38.1 - 26 Jan
Assignee | ||
Comment 19•9 years ago
|
||
This is becoming more complex than it was, so I'm going to convert it to a breakdown and file new bugs for the single pieces.
Summary: Allow keywords APIs to work in parallel with Bookmarks.jsm → Breakdown: allow keywords APIs to work in parallel with Bookmarks.jsm
Assignee | ||
Comment 20•9 years ago
|
||
All the dependancies are filed, will add the filed bugs to the backlog generation sheet.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•9 years ago
|
Attachment #8530450 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Target Milestone: mozilla36 → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•