Closed Bug 1156878 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Pocket, defect)

defect
Not set
normal
Points:
13

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.2 - 27 Apr
Tracking Status
firefox38.0.5 --- verified
firefox39 --- fixed
firefox40 --- verified

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch WIP (obsolete) — Splinter Review
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.
Attached patch WIP2 (obsolete) — Splinter Review
- 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)
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)
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+
Attached patch WIP3 (obsolete) — Splinter Review
- 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.
(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.
Attached patch Patch v4Splinter Review
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+
(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
Closed: 5 years ago
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
Flags: needinfo?(florian)
(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+
Comment on attachment 8597355 [details] [diff] [review]
Patch v4

[Triage Comment]
Fix the branch
Attachment #8597355 - Flags: approval-mozilla-beta+ → approval-mozilla-release+
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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.