The default bug view has changed. See this FAQ.

Deprecate PlacesUtils.getURLAndPostDataForKeyword

RESOLVED FIXED in Firefox 39

Status

()

Toolkit
Places
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mak, Assigned: ttaubert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla39
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
getURIAndPostDataForKeyword should be deprecated in favor of promiseUrlAndPostDataForKeyword, but first we must fix the consumers.
(Reporter)

Updated

2 years ago
Points: --- → 1
Flags: qe-verify-
Flags: firefox-backlog+

Comment 1

2 years ago
I bet this would make a fine mentored bug, if it had a mentor to hand.
(Reporter)

Comment 2

2 years ago
this is not actionable and the dependency is hairy.
(Reporter)

Updated

2 years ago
Blocks: 1140395
(Reporter)

Updated

2 years ago
No longer blocks: 519514
(Assignee)

Updated

2 years ago
Blocks: 1141547
(Reporter)

Comment 3

2 years ago
it's not promiseUrlAndPostDataForKeyword anymore, it's instead PlacesUtils.keywords.fetch
(Assignee)

Updated

2 years ago
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 39.3 - 30 Mar
(Assignee)

Comment 4

2 years ago
Created attachment 8583180 [details] [diff] [review]
0001-Bug-1100294-Deprecate-PlacesUtils.getURLAndPostDataF.patch
Attachment #8583180 - Flags: review?(mak77)
(Assignee)

Comment 5

2 years ago
Created attachment 8583186 [details] [diff] [review]
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.
Attachment #8583180 - Attachment is obsolete: true
Attachment #8583180 - Flags: review?(mak77)
Attachment #8583186 - Flags: review?(mak77)
(Assignee)

Comment 6

2 years ago
Created attachment 8583187 [details] [diff] [review]
0001b-Bug-1100294-Deprecate-PlacesUtils.getURLAndPostDataF.patch

Same patch without white space changes.
Attachment #8583187 - Flags: review?(mak77)
(Assignee)

Comment 7

2 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

2 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

2 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

2 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

2 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

2 years ago
Attachment #8583187 - Attachment is obsolete: true
Attachment #8583187 - Flags: review?(mak77)
(Assignee)

Comment 12

2 years ago
Created attachment 8583841 [details] [diff] [review]
0002-Bug-1100294-Remove-remaining-uses-of-PlacesUtils.get.patch
Attachment #8583841 - Flags: review?(mak77)
(Reporter)

Updated

2 years ago
Attachment #8583841 - Flags: review?(mak77) → review+
(Assignee)

Comment 13

2 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

2 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

2 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

2 years ago
Created attachment 8584438 [details] [diff] [review]
0003-Bug-1100294-PlacesUtils.keywords.fetch-should-instea.patch
Flags: needinfo?(ttaubert)
Attachment #8584438 - Flags: review?(mak77)
(Reporter)

Updated

2 years ago
Attachment #8584438 - Flags: review?(mak77) → review+
(Assignee)

Comment 18

2 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

Updated

2 years ago
Blocks: 1148415
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
Last Resolved: 2 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.