Closed Bug 1161654 Opened 9 years ago Closed 9 years ago

Update Pocket code to latest version

Categories

(Firefox :: Pocket, defect)

defect
Not set
normal
Points:
5

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

(2 files)

      No description provided.
Flags: qe-verify+
Flags: firefox-backlog+
Attached patch PatchSplinter Review
Attachment #8601638 - Flags: review?(dolske)
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/pocketmultidevices@2x.png);
>          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/pocketsignup_hero@2x.png);
background-size: 255px 125px;
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.
(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 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.
Attachment #8601638 - Flags: review?(dolske) → review+
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
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.
Attached patch Follow-up patchSplinter Review
Attachment #8601800 - Flags: review?(dolske)
Attachment #8601800 - Flags: review?(dolske) → review+
(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.)
QA Contact: andrei.vaida
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.)
Attachment #8601638 - Flags: approval-mozilla-beta+
Attachment #8601638 - Flags: approval-mozilla-aurora+
Comment on attachment 8601638 [details] [diff] [review]
Patch

[Triage Comment]
Fix the branch
Attachment #8601638 - Flags: approval-mozilla-beta+ → approval-mozilla-release+
Depends on: 1162735
Blocks: Pocket
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.