1163265 - Update Pocket code to latest version (May 8th code drop)

Status: VERIFIED FIXED

(Firefox :: Pocket, defect, P1)

Description

[Justin Dolske [:Dolske]]

+++ This bug was initially created as a clone of Bug #1163111 +++

Update to today's small code drop, mainly for https://github.com/Pocket/Firefox/issues/20 (menupanel subview sizing) and localized strings.

https://github.com/Pocket/Firefox/pull/63/files

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 WIP

Copied over files from Github PR and cleaned up.

With the exception of main.js, the others had zero-to-trivial fixup (comparing the diff against m-c with the diff GitHub showed for the PR). Usually just some whitespace that differed between m-c and the github base.

main.js was more complicated, but I basically cleaned it up so that the diff in the patch reflects the diff GitHub shows. The other parts that are out-of-sync (between m-c and the github base) are immaterial to the the changes here. (N.B.: for savePanelHeights, Jared says we're adding 4 to the values in the Pocket code to avoid scrollbars, so I left the m-c values unchanged with this patch.)

It's unfortunate that bugzilla can't just ignore whitespace in diffs, because that's what most of the second half of the main.js changes are. I just left it, to sync us up with github, rather than try to fix it.

One TODO:

* We need to help implement the isInOverflowMenu() stub in main.js

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8603692 [details] [diff] [review]
Patch v.1 WIP

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

::: browser/components/pocket/main.js
@@ +642,5 @@
>      }
>      
> +    function isInOverflowMenu() {
> +    	// TODO : STUB
> +    	// Firefox needs to complete this method based on where the Pocket button is

This should be:
let widgetGroupWrapper = CustomizableUI.getWidget("pocket-button");
let widget = widgetGroupWrapper && widgetGroupWrapper.forWindow(window);
let overflowed = widget && widget.overflowed;
return !!overflowed;

::: browser/components/pocket/panels/js/saved.js
@@ +7,5 @@
>      var myself = this;
>      this.inited = false;
>      this.active = false;
>      this.wrapper = null;
> +    this.pockethost = "getpocket.com";

This should be pulled from the preferences (browser.pocket.site).

Comment 3

[Justin Dolske [:Dolske]]

From a quick test: (In reply to Justin Dolske [:Dolske] from comment #1)
> Created attachment 8603692 [details] [diff] [review]
> Patch v.1 WIP

From a quick test the Pocket button is still generall functional (can sign in and save things with it), but when placed in the menupanel it's pretty glitchy -- empty subviews, and the menu itself sometimes ends up with weird sizes. But I get the same without this patch.

Comment 4

[Justin Dolske [:Dolske]]

Since the subviews were already behaving without this patch, I did some checking to see what broke it (since I thought it was "working" before, in that the contents scrolled as they were too big for the subview, but the menupanel itself didn't get broken)... It was broken before the previous uplift (bug 1163111), and was in fact regressed by bug 1161793.

I see Stuart's already filed bug 1163319 on the issue, so let's continue this topic over there. Since it's an existing issue, I think we should go ahead and land the uplift here in this bug, as it doesn't make things any worse.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Implemented isInOverflowMenu(), otherwise unchanged from v.1

Comment 6

[Justin Dolske [:Dolske]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)

> > +    function isInOverflowMenu() {
> > +    	// TODO : STUB
> 
> This should be:

Heh, actually the function name is a misnomer, it should actually be isInMenuSubview(). I implemented this using the same thing bug 1074498 did.


> ::: browser/components/pocket/panels/js/saved.js
>
> > +    this.pockethost = "getpocket.com";
> 
> This should be pulled from the preferences (browser.pocket.site).

I'm going to leave this as-is for the current code drop, but this should be fixed on Pocket side.

It's content code so it can't check the pref directly, but there's already other code in this file doing the same thing (look for "myself.overlay.pockethost").

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 8603778 [details] [diff] [review]
Patch v.2

[Triage Comment]

Required for Pocket / 38.0.5 release.

Comment 8

[Pulsebot]

https://hg.mozilla.org/mozilla-central/rev/50d6fcc42b55

Comment 9

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f50fcff1fce0
https://hg.mozilla.org/releases/mozilla-release/rev/86e98ffc152b

Comment 10

[Andrei Vaida [:avaida]]

Confirmed fixed through the Regression testing performed on 38.0.5b1-build2 (20150511143336), using Windows 7 (x64), Windows 8.1 (x86), Mac OS X 10.9.5 and Ubuntu 14.04 (x64).