Closed
Bug 1156878
Opened 9 years ago
Closed 9 years ago
Send a request to the server when clicking the Pocket toolbar button
Categories
(Firefox :: Pocket, defect)
Firefox
Pocket
Tracking
()
People
(Reporter: florian, Assigned: florian)
References
Details
Attachments
(1 file, 3 obsolete files)
11.68 KB,
patch
|
jaws
:
review+
Dolske
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-release+
|
Details | Diff | 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.
Assignee | ||
Comment 1•9 years ago
|
||
- 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•9 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 40.2 - 27 Apr
Flags: qe-verify?
Flags: firefox-backlog+
Assignee | ||
Updated•9 years ago
|
Points: --- → 13
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(florian)
Updated•9 years ago
|
QA Contact: andrei.vaida
Comment 3•9 years ago
|
||
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•9 years ago
|
||
- 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 5•9 years ago
|
||
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•9 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•9 years ago
|
||
Attachment #8597309 -
Attachment is obsolete: true
Attachment #8597309 -
Flags: review?(jaws)
Attachment #8597355 -
Flags: review?(jaws)
Comment 8•9 years ago
|
||
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+
Assignee | ||
Comment 10•9 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.
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/63495b340c64
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment 12•9 years ago
|
||
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.
Assignee | ||
Comment 13•9 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 14•9 years ago
|
||
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+
Updated•9 years ago
|
status-firefox38.0.5:
--- → affected
status-firefox39:
--- → affected
Comment 15•9 years ago
|
||
Comment on attachment 8597355 [details] [diff] [review] Patch v4 [Triage Comment] Fix the branch
Attachment #8597355 -
Flags: approval-mozilla-beta+ → approval-mozilla-release+
Comment 18•9 years ago
|
||
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
Updated•9 years ago
|
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•