1227743 - Remove snippet (id) from rotation after the user clicks on it

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jean Collings]

Right now we don't have any logic in place to remove the snippet from the rotation after the user clicks on it. Users shouldn't see that snippet if they've taken action on it.

Since we also re-use snippet IDs (swapping copy), please make sure we re-introduce that id after a given period of time (similar logic to: https://bugzilla.mozilla.org/show_bug.cgi?id=1221768)

Margaret, let me know if I filed this bug incorrectly!

Comment 1

[Jessica]

Hi Margaret,

Happy New Year!  I wanted to follow-up on this bug to see if there are any updates on progress thus far?

Thank you,
Jessica

Comment 2

[:Margaret Leibovic]

(In reply to Jessica from comment #1)
> Hi Margaret,
> 
> Happy New Year!  I wanted to follow-up on this bug to see if there are any
> updates on progress thus far?
> 
> Thank you,
> Jessica

Sorry for the slow response! I haven't had time to get around to working on this, but I'll try to write a patch some time this week (and bug 1221768).

Is there a certain Firefox release you're targeting for these changes? I could probably uplift these patches if they're not too big, but even with that the earliest we could have this is probably Fx 45 or 46.

Comment 3

[Jessica]

(In reply to :Margaret Leibovic from comment #2)
> (In reply to Jessica from comment #1)
> > Hi Margaret,
> > 
> > Happy New Year!  I wanted to follow-up on this bug to see if there are any
> > updates on progress thus far?
> > 
> > Thank you,
> > Jessica
> 
> Sorry for the slow response! I haven't had time to get around to working on
> this, but I'll try to write a patch some time this week (and bug 1221768).
> 
> Is there a certain Firefox release you're targeting for these changes? I
> could probably uplift these patches if they're not too big, but even with
> that the earliest we could have this is probably Fx 45 or 46.

Hi Margaret,
Let's plan to include these patches as a part of FX 45 if possible.  I am happy to jump on vidyo as well and explain or chat through this further.  Thanks so much!
Jessica

Comment 4

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1227743 - Remove snippet (id) from rotation after the user clicks on it. r=mfinkle

Review commit: https://reviewboard.mozilla.org/r/34057/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34057/

Comment 5

[:Margaret Leibovic]

I'm having trouble testing this locally, but this seems pretty straightforward... I want to work on writing some snippets tests, maybe I can get those done this week.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8717136 [details]
MozReview Request: Bug 1227743 - Remove snippet (id) from rotation after the user clicks on it. r=mfinkle

https://reviewboard.mozilla.org/r/34057/#review30821

Assuming this patch is only supposed to handle removing the snippet from the rotation after the user clicks on it. Looks like a separate bug was filed to unexpire IDs.

Comment 7

[:Margaret Leibovic]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51bf830bc6d5

Comment 8

[:Margaret Leibovic]

Comment on attachment 8717136 [details]
MozReview Request: Bug 1227743 - Remove snippet (id) from rotation after the user clicks on it. r=mfinkle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34057/diff/1-2/

Comment 9

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1227743 - Create test for snippets. r=mfinkle

Review commit: https://reviewboard.mozilla.org/r/34555/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34555/

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8718401 [details]
MozReview Request: Bug 1227743 - Create test for snippets. r=mfinkle

https://reviewboard.mozilla.org/r/34555/#review31237

Snippets.js is already an XPCOM component, so adding an IDL interface isn't a big deal.

LGTM

Comment 12

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #7)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=51bf830bc6d5

The test is timing out here... in the log I see this:

02-11 10:57:24.479  4851  4862 I GeckoDump: ⰲ겿{"action":"test_start","time":1455217044469,"thread":null,"pid":null,"source":"mochitest","test":"chrome://mochitests/content/chrome/mobile/android/tests/browser/chrome/test_snippets.html","js_source":"TestRunner.js"}ⰲ겿
02-11 10:57:27.225  4851  4862 I GeckoDump: ⰲ겿{"action":"log","time":1455217047226,"thread":null,"pid":null,"source":"mochitest","level":"INFO","message":"Now waiting for Snippets:UpdatedBanner notification from Gecko","js_source":"TestRunner.js"}ⰲ겿
02-11 10:57:28.130  4851  4862 I GeckoDump: ⰲ겿{"action":"log","time":1455217048120,"thread":null,"pid":null,"source":"mochitest","level":"INFO","message":"Received Snippets:UpdatedBanner notification from Gecko","js_source":"TestRunner.js"}ⰲ겿
02-11 10:57:28.524  4851  4862 I GeckoDump: ⰲ겿{"action":"test_status","time":1455217048512,"thread":null,"pid":null,"source":"mochitest","test":"chrome://mochitests/content/chrome/mobile/android/tests/browser/chrome/test_snippets.html","subtest":"Found message 1","status":"PASS","js_source":"TestRunner.js"}ⰲ겿
02-11 10:57:28.564  4851  4862 I GeckoDump: ⰲ겿{"action":"test_status","time":1455217048531,"thread":null,"pid":null,"source":"mochitest","test":"chrome://mochitests/content/chrome/mobile/android/tests/browser/chrome/test_snippets.html","subtest":"Found message 2","status":"PASS","js_source":"TestRunner.js"}ⰲ겿
02-11 10:57:28.604  4851  4862 D GeckoTabs: handleMessage: Content:StateChange
02-11 10:57:28.614  4851  4851 D GeckoToolbar: onTabChanged: START
02-11 10:57:28.614  4851  4851 I GeckoToolbarDisplayLayout: zerdatime 3374842 - Throbber start
02-11 10:57:28.614  4851  4851 D GeckoBrowserApp: BrowserApp.onTabChanged: 0: START
02-11 10:57:28.614  4851  4851 V GeckoDynamicToolbarAnimator: Requested toolbar animation to translation 0.0
02-11 10:57:28.614  4851  4851 V GeckoDynamicToolbarAnimator: Changing animation to immediate jump
02-11 10:57:28.714  4851  4862 I GeckoDump: ⰲ겿{"action":"test_status","time":1455217048715,"thread":null,"pid":null,"source":"mochitest","test":"chrome://mochitests/content/chrome/mobile/android/tests/browser/chrome/test_snippets.html","subtest":"Didn't find message 1 after clicking on it","status":"PASS","js_source":"TestRunner.js"}ⰲ겿
02-11 10:57:28.864  4851  4862 I GeckoDump: ⰲ겿{"action":"test_status","time":1455217048827,"thread":null,"pid":null,"source":"mochitest","test":"chrome://mochitests/content/chrome/mobile/android/tests/browser/chrome/test_snippets.html","subtest":"Didn't find message 1 after refreshing snippets","status":"PASS","js_source":"TestRunner.js"}ⰲ겿
02-11 10:57:29.784  4851  4862 I GeckoDump: MEMORY STAT | vsize 672MB | residentFast 218MB | heapAllocated 88MB
02-11 10:57:31.045  4851  4862 E GeckoConsole: [JavaScript Error: "Error parsing snippets responseText: Home.banner: Can't remove message that doesn't exist: id = {85b3e2b7-f5c2-4072-a81b-9e58ec3a0968}" {file: "jar:jar:file:///data/app/org.mozilla.fennec-1.apk!/assets/omni.ja!/components/Snippets.js" line: 128}]
02-11 10:57:31.045  4851  4862 E GeckoConsole: updateSnippets/<@jar:jar:file:///data/app/org.mozilla.fennec-1.apk!/assets/omni.ja!/components/Snippets.js:128:7
02-11 10:57:31.045  4851  4862 E GeckoConsole: onload@jar:jar:file:///data/app/org.mozilla.fennec-1.apk!/assets/omni.ja!/components/Snippets.js:341:7

So it looks like the test is passing, but then it's running into a JS error and dying. It's strange that this passes for me locally. I'll have to debug.

Comment 13

[:Margaret Leibovic]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12ed2fb3df3e

Comment 14

[:Margaret Leibovic]

Well, I fixed that JS error (which is actually a real error), but that wasn't the cause of these timeout problems.

gbrown, do you have any idea what could be causing this problem? The test passes fine for me locally on the emulator.

Comment 15

[Geoff Brown [:gbrown]]

That's puzzling. I see "Didn't find message 1 after refreshing snippets" as though the test has finished...but it is waiting for something. Maybe something related to the cleanup function? I'd add some info() calls in the cleanup function...see if it's getting called?

Comment 16

[:Margaret Leibovic]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=acec751e6d72

Comment 17

[:Margaret Leibovic]

(In reply to Geoff Brown [:gbrown] from comment #15)
> That's puzzling. I see "Didn't find message 1 after refreshing snippets" as
> though the test has finished...but it is waiting for something. Maybe
> something related to the cleanup function? I'd add some info() calls in the
> cleanup function...see if it's getting called?

I updated the test to put the cleanup registration inside the task, because I was wondering if that was causing the problem, and I added info statements. I see that the cleanup function is getting called, but still for some reason the test is timing out:

http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/c33b64400e9847e9c8ff17761a867f70c917cad01a75ba1b26d4bae585e1adde11ab69d6a3954945a9c6a1d92844c666e5f19130196c414c8cd67d913715ff04

Any other ideas?

Comment 18

[Geoff Brown [:gbrown]]

Not really. It looks like a very normal test -- I don't know why it isn't acting normal!

I notice defineLazyServiceGetter() is used here, and not in any of the other Android chrome tests -- could that be related?

Maybe try commenting out parts of the test?

Comment 19

[:Margaret Leibovic]

Comment on attachment 8717136 [details]
MozReview Request: Bug 1227743 - Remove snippet (id) from rotation after the user clicks on it. r=mfinkle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34057/diff/2-3/

Comment 20

[:Margaret Leibovic]

I'll deal with fixing the tests in bug 971107, I don't want to block landing this.

The tests pass locally, so at least that gives me confidence in the logic.

Comment 21

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/7ba6d7255327b319ada588bfa65787cb1b5acba3
Bug 1227743 - Remove snippet (id) from rotation after the user clicks on it. r=mfinkle

Comment 22

[:Margaret Leibovic]

Comment on attachment 8717136 [details]
MozReview Request: Bug 1227743 - Remove snippet (id) from rotation after the user clicks on it. r=mfinkle

Approval Request Comment

[Feature/regressing bug #]: Not a regression, but we suspect this will help fix the issue we're seeing with recent snippets campaigns in bug 1249260.

[User impact if declined]: Snippet message that was clicked will keep appearing until user clicks "x" button.

[Describe test coverage new/current, TreeHerder]: I wrote an automated test that passes locally, but I'm having issues landing it on automation. This code is well isolated, so unlikely to encounter a regression without a change to this file. 

[Risks and why]: Low-risk. If anything breaks, it would be snippets, but it would be in the direction of *not* showing users snippets, which I'm fine with.

[String/UUID change made/needed]: None.

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7ba6d7255327

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8717136 [details]
MozReview Request: Bug 1227743 - Remove snippet (id) from rotation after the user clicks on it. r=mfinkle

Sure, should be in 45 beta 9.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ef6c1a1ed8bf

Comment 26

[Carsten Book [:Tomcat]]

has problems uplifting to beta :

warning: conflicts while merging mobile/android/components/Snippets.js! (edit, then use 'hg resolve --mark')

Comment 27

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-beta/rev/88869e3059df