Closed Bug 1159744 Opened 9 years ago Closed 9 years ago

Use the panel implementations from the Pocket add-on for the Pocket feature

Categories

(Firefox :: Pocket, defect)

defect
Not set
normal
Points:
13

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.3 - 11 May
Tracking Status
firefox38.0.5 --- verified
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(1 file, 5 obsolete files)

      No description provided.
Attachment #8600145 - Flags: review?(dolske)
Probably should avoid adding the test.html file.

We should get license headers (or a LICENSE file) added to all of the files that don't already have them.

The "pocketAPIhost"/"pocketCookieHost" values need to be removed/preffed before landing. There are also some references in tmpl.js that probably need to be cleaned up?

I think we will want to remove all of the "install the button", "if user has legacy pkt add-on installed", and "If user has social add-on installed" code in main.js in favor of Drew's code from bug 1155521?

Similarly with the context menu code (replace with Florian's work in bug 1155518).

Is browser/components/pocket/panel.xhtml still used?

We should make double-extra sure that the stuff in browser/components/pocket/panels/ only gets loaded on content docshells.

<iframe id="pocket-panel-iframe" type="content">

 looks good but let's also test it.

Does the addMessageListener really work? Seems to add a message listener on the chrome document, do the relevant PKT_ events really bubble up there from the content iframe?
Comment on attachment 8600134 [details] [diff]
Patch v2 (fixed icon in menu panel and palette for OSX Yosemite)

Review of attachment 8600134 [details] [diff]:
-----------------------------------------------------------------

General: there are a whole bunch of hardcoded references to "firefox.dev.readitlater.com". It's all in the code from Pocket, though, so this is something they should fix for an upcoming drop.

Submitting comments so far, I still need to look at main.js and pktApi.js.

::: browser/components/pocket/Pocket.jsm
@@ +19,5 @@
> +  "resource:///modules/CustomizableUI.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "ReaderMode",
> +  "resource://gre/modules/ReaderMode.jsm");
> +
> +const kUserRemovedPref = "browser.pocket.removedByUser";

This constant seems unused?

::: browser/components/pocket/jar.mn
@@ +9,4 @@
>    content/browser/pocket/img/signup_logo.png     (img/signup_logo.png)
>    content/browser/pocket/img/signup_logo@2x.png  (img/signup_logo@2x.png)
>    content/browser/pocket/img/signup_or.png       (img/signup_or.png)
>    content/browser/pocket/img/signup_or@2x.png    (img/signup_or@2x.png)

Just checked -- these 4 images can be removed from jar.mn now, and hg rm'd as well. The CSS referring to them is removed in this patch.

@@ +18,5 @@
> +  content/browser/pocket/panels/css/saved.css           (panels/css/saved.css)
> +  content/browser/pocket/panels/css/signup.css           (panels/css/signup.css)
> +# TODO: nuke the other font format files? I think FF only needs .woff
> +  content/browser/pocket/panels/fonts/FiraSans-Regular.woff (panels/fonts/FiraSans-Regular.woff)
> +# TODO: I'm just blindly adding all the files in img/

Can remove the 3 TODOs comments here. All handled elsewhere or are things we can raise separately with Pocket.

::: browser/components/pocket/panel.xhtml
@@ +1,1 @@
> +<html:style scoped="scoped">

I think this entire file is no longer used? hg rm.

::: browser/components/pocket/panels/css/firasans.css
@@ +5,5 @@
> +         url('../fonts/FiraSans-Regular.woff') format('woff'),
> +         url('../fonts/FiraSans-Regular.ttf') format('truetype');
> +    font-weight: normal;
> +    font-style: normal;
> +}
\ No newline at end of file

As noted in jar.mn, I think Firefox is happy with just the WOFF format, and so we don't need the others.
 
Let's |hg rm| them in this patch, ask Pocket to do the same, and also have them update this file (firasans.css) so in the next addon drop it just has the woff src specified.

::: browser/components/pocket/panels/js/saved.js
@@ +29,5 @@
> +    this.fillTagContainer = function(tags,container,tagclass) {
> +        var newtagleft = 0;
> +        container.children().remove();
> +        for (var i = 0; i < tags.length; i++) {
> +            var newtag = $('<li><a href="#" class="token_tag ' + tagclass + '">' + tags[i] + '</a></li>');

Scary.

@@ +423,5 @@
> +        else {
> +            myself.fillSuggestedTags();
> +        }
> +    };
> +    this.sanitizeText = function(s) {

TODO

::: browser/components/pocket/panels/js/tmpl.js
@@ +1,2 @@
> +(function() {
> +  var template = Handlebars.template, templates = Handlebars.templates = Handlebars.templates || {};

This whole file is potentially scary, lots of semimanual HTML construction.

::: browser/themes/linux/jar.mn
@@ +32,5 @@
>  * skin/classic/browser/engineManager.css
>    skin/classic/browser/fullscreen-darknoise.png
>    skin/classic/browser/Geolocation-16.png
>    skin/classic/browser/Geolocation-64.png
> +  skin/classic/browser/pocket-panel.css                     (../shared/pocket-panel.css)

Dead now.

::: browser/themes/osx/jar.mn
@@ +34,5 @@
>    skin/classic/browser/Geolocation-16.png
>    skin/classic/browser/Geolocation-16@2x.png
>    skin/classic/browser/Geolocation-64.png
>    skin/classic/browser/Geolocation-64@2x.png
> +  skin/classic/browser/pocket-panel.css                     (../shared/pocket-panel.css)

Dead now.

::: browser/themes/shared/pocket-panel.css
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

This whole file is also unused now (with removal of panel.xhtml too).

::: browser/themes/windows/jar.mn
@@ +76,5 @@
>          skin/classic/browser/notification-64.png
>          skin/classic/browser/pageInfo.css
>          skin/classic/browser/pageInfo.png
>          skin/classic/browser/pageInfo-XP.png
> +        skin/classic/browser/pocket-panel.css                       (../shared/pocket-panel.css)

Dead now.
Attached patch Patch v3, feedback addressed (obsolete) — Splinter Review
Attachment #8600145 - Attachment is obsolete: true
Attachment #8600145 - Flags: review?(dolske)
Attachment #8600159 - Flags: review?(dolske)
Comment on attachment 8600159 [details] [diff] [review]
Patch v3, feedback addressed

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

Just looking at main.js and pktApi.js...

(Might be nice to make these proper .jsms, but we can help them do that later.)

r- to fix the addEventListener/perf and .prototype issues.

::: browser/components/pocket/main.js
@@ +696,5 @@
> +    };
> +}());
> +
> +window.addEventListener("load", function() {pktUI.onLoad();}, false);
> +window.addEventListener("unload", function() {pktUI.onUnload();}, false);

The onUnload code is empty, so not needed.

Worst, running onLoad code here is pretty much a guaranteed startup/Twinopen regression. :(

But the Pocket code is just handling addon/socialAPI migration, and we know we want to do that differently anyway.

So, let's just remove the addEventListener calls, and let Pocket remove the onLoad/onUnload deadwood code in the next drop.

::: browser/components/pocket/pktApi.js
@@ +24,5 @@
> +
> +    // Base url for all api calls
> +    // TODO: This is a dev server and will be changed before launch
> +    var pocketAPIhost = Services.prefs.getCharPref("browser.pocket.hostname");
> +    var pocketCookieHost = Services.prefs.getCharPref("browser.pocket.hostname");

Nit: pocketCookieHost seems to be unused?

@@ +33,5 @@
> +
> +    /**
> +     * Auth keys for the API requests
> +     */
> +    var oAuthConsumerKey = "40249-e88c401e1b1f2242d9e441c4";

Does this need to be configurable? EG, should a 3rd party build derived from Firefox code be using something different?

@@ +48,5 @@
> +    // Helper method for is array check
> +    if (typeof Array.isArray === 'undefined') {
> +        Array.isArray = function(obj) {
> +            return Object.prototype.toString.call(obj) === '[object Array]';
> +        }

This code shouldn't ever run (because .isArray has been in our JS engine since Firefox 4), but it should still be removed. Can't be redefining the prototypes of JS builtins in the global scope, since this effects basically _all_ Firefox and addon code.

@@ +54,5 @@
> +
> +    if (typeof String.prototype.startsWith === 'undefined') {
> +        String.prototype.startsWith = function(str){
> +            return this.slice(0, str.length) == str;
> +        };

Ditto. (.startsWith has existed since Firefox 17)

@@ +124,5 @@
> +     *  All cookies from the Pocket domain
> +     *  The return format: { cookieName:cookieValue, cookieName:cookieValue, ... }
> +    */
> +    function getCookiesFromPocket() {
> +    

Random note: might be good to look the code Florian wrote, and see if we want to have Pocket switch to using it. Not critical for 38, though.

@@ +200,5 @@
> +        data.locale_lang = window.navigator.language;
> +        data.consumer_key = oAuthConsumerKey;
> +
> +        var request = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Components.interfaces.nsIXMLHttpRequest);
> +        request.open("POST", url, true);

Random thought: I wonder if we could improve responsiveness of the save operation by preopening a connection upon onmouseover of the button... Hmm.

@@ +478,5 @@
> +                // Sort usedTagsObjectArray based on timestamp
> +                usedTagsObjectArray.sort(function(a, b) {
> +                    a = new Date(a.timestamp);
> +                    b = new Date(b.timestamp);
> +                    return a < b ? -1 : a > b ? 1 : 0;

Super inefficient to create Date objects instead of just using (what I hope are?) the numerical .timestamps...
Attachment #8600159 - Flags: review?(dolske) → review-
(In reply to Justin Dolske [:Dolske] from comment #10)

> > +    var oAuthConsumerKey = "40249-e88c401e1b1f2242d9e441c4";
> 
> Does this need to be configurable? EG, should a 3rd party build derived from
> Firefox code be using something different?

From IRC:

<nate> Yea, that would be ideal. We use that to understand for some analytics to understand which app the save came from.

So I think it's fine for now, but we'll want to help Pocket make this configurable in the client prefs/build.
Attached patch Patch v4 (obsolete) — Splinter Review
I added a LICENSE file within the /panels/ subdirectory and put the license note on the two files outside of that directory.

Pushed to tryserver,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=789a0b73c269
Attachment #8600159 - Attachment is obsolete: true
Attachment #8600406 - Flags: review?(dolske)
Points: 8 → 13
Flags: qe-verify?
Flags: firefox-backlog+
Attached patch Patch v4 (with LICENSE file) (obsolete) — Splinter Review
Attachment #8600406 - Attachment is obsolete: true
Attachment #8600406 - Flags: review?(dolske)
Attachment #8600428 - Flags: review?(dolske)
Attachment #8600428 - Attachment is obsolete: true
Attachment #8600428 - Flags: review?(dolske)
Attachment #8600433 - Flags: review?(dolske)
Depends on: 1160629
Comment on attachment 8600452 [details] [diff] [review]
Patch v6

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

::: browser/components/pocket/panels/css/firasans.css
@@ +2,5 @@
> +    font-family: 'FiraSans';
> +    src: url('../fonts/FiraSans-Regular.eot');
> +    src: url('../fonts/FiraSans-Regular.eot?iefix') format('eot'),
> +         url('../fonts/FiraSans-Regular.woff') format('woff'),
> +         url('../fonts/FiraSans-Regular.ttf') format('truetype');

I think you reverted a bit too far here, only need the woff rule.
Attachment #8600452 - Flags: review?(dolske) → review+
https://hg.mozilla.org/mozilla-central/rev/1e8d30cb367e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Blocks: 1161593
Depends on: 1161704
Comment on attachment 8600452 [details] [diff] [review]
Patch v6

a+ for uplift in service of 38.0.5 spring launch campaign.
Attachment #8600452 - Flags: approval-mozilla-beta+
Attachment #8600452 - Flags: approval-mozilla-aurora+
Comment on attachment 8600452 [details] [diff] [review]
Patch v6

[Triage Comment]
Fix the branch
Attachment #8600452 - Flags: approval-mozilla-beta+ → approval-mozilla-release+
Jared, is there anything manual QA should be looking at here? If so, do we have a UI spec to compare this patch to (or any other guideline)?
Flags: needinfo?(jaws)
You can compare the functionality to the Pocket add-on that can be installed. It should match the same user experience roughly, although desktop Firefox is likely to get some new user interface within the panel. Unfortunately I don't know of a spec.
Flags: needinfo?(jaws)
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
Pocket's toolbar button —and the feature as a whole— has been exposed to in-depth Regression testing on 38.0.5b1 (build2: 20150511143336), with no issues found affecting it. 

Although there aren't many similarities between the overall UI/UX of the add-on and the built-in Pocket, the button works as expected across the following platforms: Windows 7 (x64), Windows 8.1 (x86), Mac OS X 10.9.5 and Ubuntu 14.04 (x64).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.