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.

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

Hi Florian, can you provide a point value.

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.

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

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.

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.

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

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.

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

Comment on attachment 8597355 [details] [diff] [review] Patch v4 a+ for uplift in service of 38.0.5 spring launch campaign.

Comment on attachment 8597355 [details] [diff] [review] Patch v4 [Triage Comment] Fix the branch

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