Closed Bug 1163265 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Pocket, defect, P1)

defect
Points:
3

Tracking

()

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

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(1 file, 1 obsolete file)

+++ 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
Attached patch Patch v.1 WIP (obsolete) — Splinter Review
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
Assignee: nobody → dolske
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).
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.
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.
Attached patch Patch v.2Splinter Review
Implemented isInOverflowMenu(), otherwise unchanged from v.1
Attachment #8603692 - Attachment is obsolete: true
Attachment #8603778 - Flags: review?(jaws)
Attachment #8603778 - Attachment is patch: true
(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").
Blocks: 1163360
Flags: qe-verify?
Flags: firefox-backlog+
Attachment #8603778 - Flags: review?(jaws) → review+
Comment on attachment 8603778 [details] [diff] [review]
Patch v.2

[Triage Comment]

Required for Pocket / 38.0.5 release.
Attachment #8603778 - Flags: approval-mozilla-release+
Attachment #8603778 - Flags: approval-mozilla-aurora+
QA Contact: andrei.vaida
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 1164208
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify?
You need to log in before you can comment on or make changes to this bug.