Closed
Bug 1100294
Opened 10 years ago
Closed 9 years ago
Deprecate PlacesUtils.getURLAndPostDataForKeyword
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: mak, Assigned: ttaubert)
References
Details
Attachments
(3 files, 2 obsolete files)
12.42 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
9.29 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
2.09 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
getURIAndPostDataForKeyword should be deprecated in favor of promiseUrlAndPostDataForKeyword, but first we must fix the consumers.
Reporter | ||
Updated•10 years ago
|
Points: --- → 1
Flags: qe-verify-
Flags: firefox-backlog+
Comment 1•10 years ago
|
||
I bet this would make a fine mentored bug, if it had a mentor to hand.
Reporter | ||
Comment 2•10 years ago
|
||
this is not actionable and the dependency is hairy.
Reporter | ||
Updated•9 years ago
|
No longer blocks: placesAsyncBookmarks
Reporter | ||
Comment 3•9 years ago
|
||
it's not promiseUrlAndPostDataForKeyword anymore, it's instead PlacesUtils.keywords.fetch
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 39.3 - 30 Mar
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8583180 -
Flags: review?(mak77)
Assignee | ||
Comment 5•9 years ago
|
||
Accepting a callback again as well as returning a Promise. Passing a callback will yield a deprecation warning.
Attachment #8583180 -
Attachment is obsolete: true
Attachment #8583180 -
Flags: review?(mak77)
Attachment #8583186 -
Flags: review?(mak77)
Assignee | ||
Comment 6•9 years ago
|
||
Same patch without white space changes.
Attachment #8583187 -
Flags: review?(mak77)
Assignee | ||
Comment 7•9 years ago
|
||
Looking at the remaining usages of getURLAndPostDataForKeyword() I have a question: Can toolkit/components/places/tests/unit/test_398914.js be removed? It looks like it's testing a situation that can't occur with the new DB scheme anymore.
Flags: needinfo?(mak77)
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #7) > Can toolkit/components/places/tests/unit/test_398914.js be removed? It looks > like it's testing a situation that can't occur with the new DB scheme > anymore. Meant to say that I'm sure the new keywords API tests cover multiple keywords for a single URI.
Reporter | ||
Comment 9•9 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #7) > Looking at the remaining usages of getURLAndPostDataForKeyword() I have a > question: > > Can toolkit/components/places/tests/unit/test_398914.js be removed? It looks > like it's testing a situation that can't occur with the new DB scheme > anymore. yes, we can remove it.
Flags: needinfo?(mak77)
Reporter | ||
Comment 10•9 years ago
|
||
Comment on attachment 8583186 [details] [diff] [review] 0001-Bug-1100294-Deprecate-PlacesUtils.getURLAndPostDataF.patch, v2 Review of attachment 8583186 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +2097,5 @@ > allowThirdPartyFixup: allowThirdPartyFixup }); > } catch (e) {} > } > > +function getShortcutOrURIAndPostData(aURL, aCallback = null) { while here, would you mind adding a minimal javadoc to this? @@ +2122,5 @@ > + if (engine) { > + let submission = engine.getSubmission(param, null, "keyword"); > + postData = submission.postData; > + return { postData: submission.postData, url: submission.uri.spec, > + mayInheritPrincipal: mayInheritPrincipal }; nit: just mayInheritPrincipal @@ +2133,5 @@ > + } > + > + if (!shortcutURL) { > + return { postData: postData, url: aURL, > + mayInheritPrincipal: mayInheritPrincipal }; ditto, also for postData additionally if you change argument names to be url and callback, you can also do the same for url, so here we get return { postdata, url, mayInheritPrincipal }; @@ +2184,5 @@ > // document's principal. > mayInheritPrincipal = true; > > + return { postData: postData, url: shortcutURL, > + mayInheritPrincipal: mayInheritPrincipal }; ditto @@ +2193,5 @@ > + // the original URL. > + postData = null; > + > + return { postData: postData, url: aURL, > + mayInheritPrincipal: mayInheritPrincipal }; ditto @@ +2201,5 @@ > // document's principal. > mayInheritPrincipal = true; > > + return { postData: postData, url: shortcutURL, > + mayInheritPrincipal: mayInheritPrincipal }; ditto
Attachment #8583186 -
Flags: review?(mak77) → review+
Reporter | ||
Comment 11•9 years ago
|
||
Clearly before landing this, we need to fix all internal usage, included tests. once test_398914.js is removed, there isn't much, a couple migration tests, test_placesTxn and metro (that I'm not why it's still in-tree and thus I have no idea what we are supposed to do)
Reporter | ||
Updated•9 years ago
|
Attachment #8583187 -
Attachment is obsolete: true
Attachment #8583187 -
Flags: review?(mak77)
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8583841 -
Flags: review?(mak77)
Reporter | ||
Updated•9 years ago
|
Attachment #8583841 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/6306a0c0be65 https://hg.mozilla.org/integration/fx-team/rev/a4cc519364b5
Backed out for bc1 orange: remote: https://hg.mozilla.org/integration/fx-team/rev/280e27c77777 remote: https://hg.mozilla.org/integration/fx-team/rev/ea2a95df5a62 https://treeherder.mozilla.org/logviewer.html#?job_id=2420874&repo=fx-team
Flags: needinfo?(ttaubert)
Reporter | ||
Comment 15•9 years ago
|
||
The problem is here var newTabButtonObserver = { ... onDrop: function (aEvent) { let url = browserDragAndDrop.drop(aEvent, { }); getShortcutOrURIAndPostData(url, data => { if (data.url) { // allow third-party services to fixup this URL openNewTabWith(data.url, null, data.postData, aEvent, true); } }); } sounds like we are passing an empty string to getShortcutOrURIAndPostData?
Reporter | ||
Comment 16•9 years ago
|
||
ehr, and it's not the only place (for example newWindowButtonObserver), middleMousePaste.... We could make getSOUAP return immediately for an empty string, or make fetch do the same. At this point I don't think it's worth fixing the consumers.
Assignee | ||
Comment 17•9 years ago
|
||
Flags: needinfo?(ttaubert)
Attachment #8584438 -
Flags: review?(mak77)
Reporter | ||
Updated•9 years ago
|
Attachment #8584438 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5541383f6c41 https://hg.mozilla.org/integration/fx-team/rev/5dce13dd431c https://hg.mozilla.org/integration/fx-team/rev/1ba56f4da590
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5541383f6c41 https://hg.mozilla.org/mozilla-central/rev/5dce13dd431c https://hg.mozilla.org/mozilla-central/rev/1ba56f4da590
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•