Send a request to the server when clicking the Pocket toolbar button

VERIFIED FIXED in Firefox 38.0.5

Status

()

Firefox
Pocket
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

Trunk
Firefox 40
Points:
13
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox38.0.5 verified, firefox39 fixed, firefox40 verified)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8595463 [details] [diff] [review]
WIP

This applies after the current patches from bug 1155523.

Notes:
- During testing I've noticed that the Pocket server rejects any non http/https url, so we'll likely need to disable the pocket button for ftp, data, chrome, about, ... urls.
- I tried to keep the code flow as straight forward as possible for this first patch, but we'll want to abstract some of this away to a JS module as soon as we implement support for removing an item or tagging.
(Assignee)

Comment 1

3 years ago
Created attachment 8596068 [details] [diff] [review]
WIP2

- Moved all the code dealing with cookies and HTTP requests to a new Pocket.jsm module.
- The "remove link" button now works.
- about:reader URLs are handled correctly.
Attachment #8595463 - Attachment is obsolete: true
Attachment #8596068 - Flags: feedback?(jaws)

Updated

3 years ago
Status: NEW → ASSIGNED
Iteration: --- → 40.2 - 27 Apr
Flags: qe-verify?
Flags: firefox-backlog+
Hi Florian, can you provide a point value.
Flags: needinfo?(florian)
(Assignee)

Updated

3 years ago
Points: --- → 13
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(florian)
QA Contact: andrei.vaida
Comment on attachment 8596068 [details] [diff] [review]
WIP2

Review of attachment 8596068 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1198,5 @@
>          let doc = event.target.ownerDocument;
>  
>          let loginView = doc.getElementById("pocket-login-required");
>          let pageSavedView = doc.getElementById("pocket-page-saved");
> +        let showPageSaved = !!Pocket.isLoggedIn;

Does Pocket.isLoggedIn not return a Boolean type? It looks like it should.

@@ +1218,5 @@
> +        Pocket.save(uri, gBrowser.contentTitle).then(
> +          item => {
> +            doc.getElementById("pocket-remove-page").itemId = item.item_id; //TODO hiding the panel should clear this
> +          },
> +          error => {dump(error + "\n");}

We'll want to put an error notice somewhere on the panel.
Attachment #8596068 - Flags: feedback?(jaws) → feedback+
(Assignee)

Comment 4

3 years ago
Created attachment 8597309 [details] [diff] [review]
WIP3

- rebased on the next patch from bug 1155523
- made the hostname a pref
- added support for tagging
- changed the flag from feedback to review because if we want to land in chunks and then iterate in new small bugs, this may be a good point to land (assuming bug 1155523 lands very soon).
Attachment #8596068 - Attachment is obsolete: true
Attachment #8597309 - Flags: review?(jaws)
Comment on attachment 8597309 [details] [diff] [review]
WIP3

Review of attachment 8597309 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/pocket/Pocket.jsm
@@ +56,5 @@
> +      this._send("firefox/save", data,
> +        data => {
> +          // Update premium status, tags and since
> +          if (data.tags && Array.isArray(data.tags))
> +            this.prefBranch.setCharPref("tags", JSON.stringify(data.tags));

Are these the tags for the current saved page or tags that the user has used before? I think it's weird to save the current page's tags to the preferences. We could probably just store them in memory to get passed back to the panel.
(Assignee)

Comment 6

3 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Comment on attachment 8597309 [details] [diff] [review]
> WIP3
> 
> Review of attachment 8597309 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/pocket/Pocket.jsm
> @@ +56,5 @@
> > +      this._send("firefox/save", data,
> > +        data => {
> > +          // Update premium status, tags and since
> > +          if (data.tags && Array.isArray(data.tags))
> > +            this.prefBranch.setCharPref("tags", JSON.stringify(data.tags));
> 
> Are these the tags for the current saved page or tags that the user has used
> before?

They are the tags that the user has used before. I implemented this while porting the code from pktApi.js.

I don't see any use for that pref so I'll just remove it.
(Assignee)

Comment 7

3 years ago
Created attachment 8597355 [details] [diff] [review]
Patch v4
Attachment #8597309 - Attachment is obsolete: true
Attachment #8597309 - Flags: review?(jaws)
Attachment #8597355 - Flags: review?(jaws)
Comment on attachment 8597355 [details] [diff] [review]
Patch v4

Review of attachment 8597355 [details] [diff] [review]:
-----------------------------------------------------------------

r=me to land on top of bug 1155523 but please fix the onViewHiding before landing.

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1166,5 @@
> +          uri = ReaderMode.getOriginalUrl(uri.spec);
> +        else
> +          uri = uri.spec;
> +        if (!uri)
> +          return; //TODO should prevent the panel from showing

In a follow-up bug, we should change the panel to say that "This page cannot be saved to Pocket".

@@ +1171,5 @@
> +
> +        Pocket.save(uri, gBrowser.contentTitle).then(
> +          item => {
> +            //TODO hiding the panel should clear this
> +            doc.getElementById("pocket-remove-page").itemId = item.item_id;

Please add an onViewHiding function in CustomizableWidgets.jsm (next to the onViewShowing function already there) to remove this property.
Attachment #8597355 - Flags: review?(jaws) → review+

Comment 9

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/63495b340c64
(Assignee)

Comment 10

3 years ago
(In reply to Pulsebot from comment #9)
> https://hg.mozilla.org/integration/fx-team/rev/63495b340c64

Landed with comment 8 addressed, and the hostname changed to localhost in firefox.js to avoid requests to the dev server.
https://hg.mozilla.org/mozilla-central/rev/63495b340c64
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Confirmed fixed as of Nightly 40.0a1 (2015-04-29), using Ubuntu 14.04 (x64), Windows 8.1 (x64) and Mac OS X 10.9.5. Used the toolbar button to save various types of pages, including regular, in-content and about:reader URLs.

I noticed a weird scenario though, in which example.net is saved in Pocket with the following URL instead: example.iana.org, pointing to a "server not found" error. Any idea why this happens? I didn't manage to find another page showing the same behavior.
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified
Flags: needinfo?(florian)
(Assignee)

Comment 13

3 years ago
(In reply to Andrei Vaida, QA [:avaida] from comment #12)

> I noticed a weird scenario though, in which example.net is saved in Pocket
> with the following URL instead: example.iana.org, pointing to a "server not
> found" error. Any idea why this happens? I didn't manage to find another
> page showing the same behavior.

I think this is an issue on the Pocket side.
Flags: needinfo?(florian)
Comment on attachment 8597355 [details] [diff] [review]
Patch v4

a+ for uplift in service of 38.0.5 spring launch campaign.
Attachment #8597355 - Flags: approval-mozilla-beta+
Attachment #8597355 - Flags: approval-mozilla-aurora+
status-firefox38.0.5: --- → affected
status-firefox39: --- → affected
Comment on attachment 8597355 [details] [diff] [review]
Patch v4

[Triage Comment]
Fix the branch
Attachment #8597355 - Flags: approval-mozilla-beta+ → approval-mozilla-release+
https://hg.mozilla.org/releases/mozilla-aurora/rev/931867ea7d04
status-firefox39: affected → fixed
https://hg.mozilla.org/releases/mozilla-release/rev/16e406d46c18
status-firefox38.0.5: affected → fixed
Verified as fixed using the following environment:

FF 38.0.5 
BUild Id: 20150511143336
OS: Win 7 x64, Ubuntu 14.04 x86, Mac Os X 10.9.5
status-firefox38.0.5: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.