1161654 - Update Pocket code to latest version

Status: VERIFIED FIXED

(Firefox :: Pocket, defect)

Comment 1

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

Attachment: Patch

Comment 3

[Nate Weiner]

Comment on attachment 8601638 [details] [diff] [review]
Patch

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

I had Nick take a took through a comparison of what is now in mozilla-central (with this patch) vs what is in the add-on repo and it surfaced a few differences I noted in the review.

::: browser/components/pocket/panels/css/saved.css
@@ +109,4 @@
>      transition: opacity 0.2s ease-out;
>      opacity: 0;
>  }
> +.pkt_ext_container_detailactive .pkt_ext_initload .pkt_ext_loadingspinner {

There was an extra line removed here on the m-c side that is in the repo.

Nick's feedback:
"They should keep the extra line to keep animation: none set. Otherwise you have a random CSS spinner running in the background (but not visible), eating GPU/CPU in any final state (tags added, page removed, error). In theory of course if the panel is blasted away, the performance hit goes away too. But worst of all, if somehow the panel sticks around users have this random spinner taking away from their performance for no reason."

Diff in repo here:
https://github.com/Pocket/Firefox/compare/develop...compare/patch-update-1?expand=1#diff-3930e43a6b06e5e881cf8b395934c5acL112

::: browser/components/pocket/panels/css/signup.css
@@ +99,3 @@
>      .pkt_ext_containersignup .pkt_ext_introimg {
>          background-image: url(../img/[e-mail]);
>          background-size: 171px 122px;

This image reference didn't get changed, see:
https://github.com/Pocket/Firefox/compare/develop...compare/patch-update-1?expand=1#diff-10017edc426773484a78bffceb6e4383L98

As a result, the "hero" version of the Logged Out panel is missing an image.

This should be:
background-image: url(../img/[e-mail]);
background-size: 255px 125px;

Comment 4

[Nate Weiner]

The other two issues I'm seeing (but haven't pinned down where the break-down is happening) that don't match the behavior of the add-on code in /develop:

1) Trying to save from a non-http(s) page like about:home is not showing the error message. It instead tries to open the panel (and fails to show anything). In the add-on this is working fine. In the code comparison I didn't see anything that immediately stuck out, so I have a hunch it has to do with something about how the panels are being loaded or the messaging between the panels and chrome.

2) The panel seems to be opening the wrong size as you go between different sizes. For example, save something, then log-out of Pocket and click the button again. You'll see the log-out panel load in the smaller saved-state-sized window. It seems like the panel height is using whatever the last value was, not actually updating for what it's intended to load at the time the button is clicked.

In looking at the diff, I no longer see any place that we adjust the actual panel size, this seems to only update the iframe now:
https://github.com/Pocket/Firefox/compare/develop...compare/patch-update-1?expand=1#diff-8b2d8aafc16d252181734b0fa6b7ef46L397

So I'm assuming that is happening somewhere else in native code, but it doesn't seem to be sticking.

Comment 5

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

(In reply to Nate Weiner from comment #4)
> 1) Trying to save from a non-http(s) page like about:home is not showing the
> error message. It instead tries to open the panel (and fails to show
> anything). In the add-on this is working fine. In the code comparison I
> didn't see anything that immediately stuck out, so I have a hunch it has to
> do with something about how the panels are being loaded or the messaging
> between the panels and chrome.

This is fixed by the patch in bug 1160578.

> 2) The panel seems to be opening the wrong size as you go between different
> sizes. For example, save something, then log-out of Pocket and click the
> button again. You'll see the log-out panel load in the smaller
> saved-state-sized window. It seems like the panel height is using whatever
> the last value was, not actually updating for what it's intended to load at
> the time the button is clicked.

I'm working on fixing the panel sizing in bug 1160396.

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 8601638 [details] [diff] [review]
Patch

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

I think the UA change is problematic, but marking r+ because I don't feel like I have a choice.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +231,5 @@
>  
>      <panelview id="PanelUI-pocketView" flex="1">
>        <vbox class="panel-subview-body">
>          <hbox id="pocket-panel-container" align="top" flex="1">
> +          <iframe id="pocket-panel-iframe" type="content" flex="1"/>

I don't think the flex="1" actually does anything here.

::: browser/components/pocket/pktApi.js
@@ +249,5 @@
>  		request.setRequestHeader('Content-Type', 'application/x-www-form-urlencoded; charset=UTF-8');
>  		request.setRequestHeader('X-Accept',' application/json');
>  
> +        // User Agent
> +        request.setRequestHeader("User-Agent" , getUserAgentString());

I think it's really bad practice to have browser code changing the UA (even on a per-request) basis, especially how much time Mozilla has spent advocating against doing this.

Unless there's there's a hard requirement to do this, I think we shouldn't.

Comment 7

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

I removed the user agent code from the patch after getting approval over IRC from Dolske. Fixed the issues that Nate brought up in comment #3 and landed,
https://hg.mozilla.org/integration/fx-team/rev/0f16abb82c08

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

Added bug 1161793 as blocking, with the race condition as described a good number of clicks are going to fail so we need to get that fixed before we pref this on.

Comment 9

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

Attachment: Follow-up patch

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2d0c64677380

Comment 11

[Justin Dolske [:Dolske]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Created attachment 8601800 [details] [diff] [review]
> Follow-up patch

Landed https://hg.mozilla.org/integration/fx-team/rev/2d0c64677380

(This basically caused context menus to be broken.)

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0f16abb82c08
https://hg.mozilla.org/mozilla-central/rev/2d0c64677380

Comment 13

[Justin Dolske [:Dolske]]

Comment on attachment 8601638 [details] [diff] [review]
Patch

a+ for uplift in service of 38.0.5 spring launch campaign. (Ditto for the followup patch here.)

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8601638 [details] [diff] [review]
Patch

[Triage Comment]
Fix the branch

Comment 15

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/b5c3ec189c68
https://hg.mozilla.org/releases/mozilla-aurora/rev/f32ce8fb1fd8

Comment 16

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

https://hg.mozilla.org/releases/mozilla-release/rev/125c7dbe7528
https://hg.mozilla.org/releases/mozilla-release/rev/3d9d572c9ec4

Comment 17

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