1100294 - Deprecate PlacesUtils.getURLAndPostDataForKeyword

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

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

Comment 1

[Mike Hoye [:mhoye]]

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

Comment 2

[Marco Bonardo [:mak]]

this is not actionable and the dependency is hairy.

Comment 3

[Marco Bonardo [:mak]]

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

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1100294-Deprecate-PlacesUtils.getURLAndPostDataF.patch

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1100294-Deprecate-PlacesUtils.getURLAndPostDataF.patch, v2

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

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001b-Bug-1100294-Deprecate-PlacesUtils.getURLAndPostDataF.patch

Same patch without white space changes.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 9

[Marco Bonardo [:mak]]

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

[Marco Bonardo [:mak]]

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

Comment 11

[Marco Bonardo [:mak]]

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)

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1100294-Remove-remaining-uses-of-PlacesUtils.get.patch

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/6306a0c0be65
https://hg.mozilla.org/integration/fx-team/rev/a4cc519364b5

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[Marco Bonardo [:mak]]

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?

Comment 16

[Marco Bonardo [:mak]]

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.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1100294-PlacesUtils.keywords.fetch-should-instea.patch

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

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

[Ryan VanderMeulen [:RyanVM]]

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