getURIAndPostDataForKeyword should be deprecated in favor of promiseUrlAndPostDataForKeyword, but first we must fix the consumers.

I bet this would make a fine mentored bug, if it had a mentor to hand.

this is not actionable and the dependency is hairy.

it's not promiseUrlAndPostDataForKeyword anymore, it's instead PlacesUtils.keywords.fetch

Accepting a callback again as well as returning a Promise. Passing a callback will yield a deprecation warning.

Same patch without white space changes.

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.

(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.

(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.

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

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)

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

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?

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.

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

https://hg.mozilla.org/mozilla-central/rev/5541383f6c41 https://hg.mozilla.org/mozilla-central/rev/5dce13dd431c https://hg.mozilla.org/mozilla-central/rev/1ba56f4da590