Closed
Bug 741738
Opened 12 years ago
Closed 12 years ago
Count impressions for each doorhanger notification type
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: theo, Assigned: theo)
References
Details
Attachments
(1 file, 9 obsolete files)
3.69 KB,
patch
|
Details | Diff | Splinter Review |
As said in https://bugzilla.mozilla.org/show_bug.cgi?id=716643#c4, we should count impressions of each type to display it to old users when we add a new notification.
Updated•12 years ago
|
Component: Add-ons Manager → Firefox Sync: UI
Product: Toolkit → Mozilla Services
QA Contact: add-ons.manager → sync-ui
Assignee | ||
Comment 1•12 years ago
|
||
I tried it, it seems to work: the different prefs are created and decremented. Done with a switch, but maybe you to want to use directly this._notificationType to determine the pref name? (If yes, we should rename addons-sync-disabled)
Attachment #613263 -
Flags: review?(mak77)
Comment 2•12 years ago
|
||
(In reply to Théo Chevalier [:tchevalier] from comment #1) > Done with a switch, but maybe you to want to use directly > this._notificationType to determine the pref name? (If yes, we should rename > addons-sync-disabled) I was rather thinking to a single string pref containing a json hash-like object, with one property per notificationType and the count as value. Should not be too hard t expand it in future then. Though, it would then require some sort or "migration" for the existing pref... But in the end that is something you still need here, even with your approach. Your patch, as it is, is basically resetting the pref showing again the promo to users who already saw it, so would be an r- for that.
Updated•12 years ago
|
Attachment #613263 -
Flags: review?(mak77)
Comment 3•12 years ago
|
||
the migration should try to read the old pref, and if it exists clear it and set the proper initial value in the new one. This is not downgrade compatible but that's not a bit deal imo, just means someone downgrading may see again the promo, it's acceptable.
Comment 4•12 years ago
|
||
typo! s/bit deal/big deal/
Assignee | ||
Comment 5•12 years ago
|
||
I still have a bug: the old pref is well migrated, but when a new notification type is added, it overwrite the new pref.
Attachment #613263 -
Attachment is obsolete: true
Attachment #619377 -
Flags: feedback?(mak77)
Comment 6•12 years ago
|
||
Comment on attachment 619377 [details] [diff] [review] Patch V2 Review of attachment 619377 [details] [diff] [review]: ----------------------------------------------------------------- I think I may have an idea to cleanup this, I didn't double check it but at first glance may work ::: browser/base/content/urlbarBindings.xml @@ +1398,5 @@ > try { > + let oldPref = Services.prefs.getIntPref("browser.syncPromoViewsLeft"); > + Services.prefs.clearUserPref("browser.syncPromoViewsLeft"); > + let viewsLeftList = '{"passwords":{"viewsLeft":"' + oldPref + '"},'; > + viewsLeftList += '"bookmarks":{"viewsLeft":"' + oldPref + '"}}'; As a side note, you should never build a json manually, always .stringify an object, editing objects is easy. Also you don't need a .viewsLeft, you can just have a key => val map @@ +1405,5 @@ > + } catch(ex) {} > + > + // Try to read this._notificationType in the new pref, else return 5 > + try { > + let viewsLeftList = JSON.parse(Services.prefs.getCharPref("browser.syncPromoViewsLeftList")); Rather than complicating things like this, I'd add a new field called _viewsLeftMap, into it you may just do: let viewsLeftMap = {}; try { viewsLeftMap = ..read the new pref.. } catch (ex) { // If the old preference exists, migrate it to the new one. try { ..read the old pref.. ..clear it.. ..add to viewsLeftMap.. Services.prefs.setCharPref("browser.syncPromoViewsLeftMap", JSON.stringify(viewsLeftMap)); } catch (ex2) { } } return viewsLeftMap; @@ +1411,2 @@ > } catch(ex) {} > return 5; then this getter would become a mere return this._viewsLeftMap[this._notificationType] || 5; @@ +1429,5 @@ > + } > + catch(ex) { > + let viewsLeftList = '{"' + this._notificationType + '":{"viewsLeft":"' + val + '"}}'; > + Services.prefs.setCharPref("browser.syncPromoViewsLeftList", viewsLeftList); > + } And here you could just this._viewsLeftMap[this._notificationType] = val; Services.prefs.setCharPref("browser.syncPromoViewsLeftMap", JSON.stringify(this._viewsLeftMap));
Attachment #619377 -
Flags: feedback?(mak77) → feedback-
Assignee | ||
Comment 7•12 years ago
|
||
I guess there is some syntax errors, because it doesn't work, and I can't find them. Is there an easy way to debug this on Windows, please? (To put some alert(), or console.log())
Attachment #619377 -
Attachment is obsolete: true
Attachment #624228 -
Flags: feedback?(mak77)
Comment 8•12 years ago
|
||
Comment on attachment 624228 [details] [diff] [review] Patch V3 Review of attachment 624228 [details] [diff] [review]: ----------------------------------------------------------------- What exactly doesn't work? anything? I think this should help ::: browser/base/content/urlbarBindings.xml @@ +1401,5 @@ > + try { > + let oldPref = Services.prefs.getIntPref("browser.syncPromoViewsLeft"); > + Services.prefs.clearUserPref("browser.syncPromoViewsLeft"); > + viewsLeftMap[bookmarks] = oldPref; > + viewsLeftMap[passwords] = oldPref; bookmarks and passwords should be strings here, or you should use viewsLeftMap.bookmarks =
Attachment #624228 -
Flags: feedback?(mak77)
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #8) > What exactly doesn't work? anything? Yeah, anything. Even the promobox isn't displayed. But I suspect it's due to the syntax error you just have solved. > I think this should help > bookmarks and passwords should be strings here, or you should use > viewsLeftMap.bookmarks = Thanks, I try it right now
Assignee | ||
Comment 10•12 years ago
|
||
Sadly still doesn't work. On a fresh profile, clicking twice on the star to bookmark doesn't display the promo box, and the new pref is not created.
Assignee | ||
Comment 11•12 years ago
|
||
I took my time, but it works :) I made a try build if you want to test: https://tbpl.mozilla.org/?tree=Try&rev=7e591e6cdfd5 If you got me a r+, can you let me push it (with bug 716643) to mizilla-inbound by myself?
Attachment #624228 -
Attachment is obsolete: true
Attachment #642056 -
Flags: review?(mak77)
Comment 12•12 years ago
|
||
Comment on attachment 642056 [details] [diff] [review] Patch V4 Review of attachment 642056 [details] [diff] [review]: ----------------------------------------------------------------- I wonder if we should cache the map, but that may introduce subtle cache synchronization bugs, so let's stay as it is for now and see for future. Thanks for completing this. ::: browser/base/content/urlbarBindings.xml @@ +1356,5 @@ > Services.strings.createBundle("chrome://branding/locale/brand.properties"); > </field> > + <property name="_viewsLeftMap"> > + <getter> > + <![CDATA[ please use the proper indentation here, see the other getters/setters in this file. Should be <getter><![CDATA[ contents @@ +1367,5 @@ > + let oldPref = Services.prefs.getIntPref("browser.syncPromoViewsLeft"); > + Services.prefs.clearUserPref("browser.syncPromoViewsLeft"); > + viewsLeftMap.bookmarks = oldPref; > + viewsLeftMap.passwords = oldPref; > + Services.prefs.setCharPref("browser.syncPromoViewsLeftMap", JSON.stringify(viewsLeftMap)); this is going over 80 chars, and it's easy to reindent as Services.prefs.setCharPref("browser.syncPromoViewsLeftMap", JSON.stringify(viewsLeftMap)); @@ +1372,5 @@ > + } catch (ex2) {} > + } > + return viewsLeftMap; > + ]]> > + </getter> as well as fix indentation here
Attachment #642056 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 13•12 years ago
|
||
Last changes addressed, pushed in m-i https://hg.mozilla.org/integration/mozilla-inbound/rev/08a1e103ec0d
Comment 14•12 years ago
|
||
Sorry, I backed out these patches because they caused exceptions in mochitest-browser-chrome: https://hg.mozilla.org/integration/mozilla-inbound/rev/04d508016743 https://tbpl.mozilla.org/php/getParsedLog.php?id=13809261&tree=Mozilla-Inbound
Comment 15•12 years ago
|
||
Thanks Matt. At first glance looks like _viewsLeft is now accessing _notificationType at a wrong time (it tries to read this._panel.firstChild that for some reason is null). Looking at the patches is more likely the culprit is this bug than the other one, but I don't see a point where we may miscall. Should be investigated running the specific failing tests.
Comment 16•12 years ago
|
||
sync triage: Is work still happening on this? theo, how's it going?
Assignee | ||
Comment 17•12 years ago
|
||
Yes, but it's really hard for me. I will try to find help on IRC tomorrow, but if I still can't fix it, someone else should finish it in order to not miss the train.
Assignee | ||
Comment 18•12 years ago
|
||
Good news, I will find some help this weekend at MozCamp in Warsaw, so hopefully this will be landed next week!
Assignee | ||
Comment 19•12 years ago
|
||
So... I worked with Capella in Warsaw a few hours on this, and we figured out that the exception happened during the execution of :
>// Install another
>executeSoon(function () {
near line 670 in _tests/testing/mochitest/browser/browser/base/content/test/browser_bug553455.js
(Because we displayed something right before and right after this function call, and the exception happens between.)
If I understand correctly the test, it installs an add-on (everything is good), close the notification, and it fails during the beginning of another installation and the closing notification. But why, I don't know...
Any idea? What is next step? Randomly change part of the patch to see what is causing this?
Comment 20•12 years ago
|
||
Do you know precisely where the exception is being thrown from?
Assignee | ||
Comment 21•12 years ago
|
||
(In reply to Josh Matthews [:jdm] from comment #20) > Do you know precisely where the exception is being thrown from? The exception is: TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Capella 14-4 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | an unexpected uncaught JS exception reported through window.onerror - TypeError: this._panel.firstChild is null at chrome://browser/content/urlbarBindings.xml:1625 Stack trace: JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 980 TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Capella 14-5 TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Waiting for notification TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Console message: [JavaScript Error: "TypeError: this._panel.firstChild is null" {file: "chrome://browser/content/urlbarBindings.xml" line: 1625}] (Capella 14-4 and Capella 14-5 are the info(); we put around "executeSoon(function () {" in the test.) And the failing line in urlbarBindings.xml (1625) is: let type = this._panel.firstChild.getAttribute("popupid") || Note that the patch in bug 741738 is also applied.
Comment 22•12 years ago
|
||
So you're saying that the equivalent of
> wait_for_notification_close(function () {
> // Install another
> info("foo");
> executeSoon(function () {
> ...
> });
> info("bar");
> });
produces the output you're seeing? That's... wow. Confusing.
Comment 23•12 years ago
|
||
If that's the case, I would throw a debugger; statement in the _notificationType getter to see what's calling it.
Assignee | ||
Comment 24•12 years ago
|
||
off-topic : There is something wrong, and I don't think it's related to the fail. When viewsLeft is called here, in the body : let viewsLeft = this._viewsLeft; and when the pref is (for instance) {"bookmarks":0}, this._viewsLeft return 5 instead returning 0 (the current value of this._viewsLeftMap[this._notificationType]) , which happens for all the other values. In other words, when the pref is 3, it return 3, then we save 2 in the pref (So it's ok), but when we have 0 in the pref, it can't return 0, so it return the other possibility: 5. Maybe 0 is considered as null in this case? So should we consider 1 as the last value and close the notification at 1?(And add 1 to the older pref when we migrate it) But it's bad code, no? (In reply to Josh Matthews [:jdm] from comment #23) > If that's the case, I would throw a debugger; statement in the > _notificationType getter to see what's calling it. I'll try that
Assignee | ||
Comment 25•12 years ago
|
||
(In reply to Josh Matthews [:jdm] from comment #22) > So you're saying that the equivalent of > > > wait_for_notification_close(function () { > > // Install another > > info("foo"); > > executeSoon(function () { > > ... > > }); > > info("bar"); > > }); > > produces the output you're seeing? That's... wow. Confusing. Actually, we tried > > // Install another > > info("foo"); > > executeSoon(function () { info("bar"); > > ... > > }); > > > > }); but I guess it's the same?
Comment 26•12 years ago
|
||
(In reply to Théo Chevalier [:tchevalier] from comment #25) > (In reply to Josh Matthews [:jdm] from comment #22) > > So you're saying that the equivalent of > > > > > wait_for_notification_close(function () { > > > // Install another > > > info("foo"); > > > executeSoon(function () { > > > ... > > > }); > > > info("bar"); > > > }); > > > > produces the output you're seeing? That's... wow. Confusing. > > Actually, we tried > > > > // Install another > > > info("foo"); > > > executeSoon(function () { > info("bar"); > > > ... > > > }); > > > > > > }); > but I guess it's the same? That's very good to know, since it's not the same at all :) Execute soon defers the execution of the function provided until some indeterminate point in the future (soon!), so any arbitrary code can run between then and when your next info statement runs. Sounds like the debugger statement is what you'll need. >Maybe 0 is considered as null in this case? So should we consider 1 as the last value and >close the notification at 1?(And add 1 to the older pref when we migrate it) >But it's bad code, no? Yes, for truthiness checks (ie. if (someValue)), 0 is false. You probably want something like |if (someValue) === undefined| instead, since === will not perform such coercions.
Assignee | ||
Comment 27•12 years ago
|
||
Exception in mochitest fixed, it was due to return this._viewsLeftMap[this._notificationType] || 5; Try build started: https://tbpl.mozilla.org/?tree=Try&rev=6f2b6c88890e Results in: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/theo.chevalier11@gmail.com-6f2b6c88890e The two patches will be landed if all the tests are passed successfully.
Attachment #642056 -
Attachment is obsolete: true
Assignee | ||
Comment 28•12 years ago
|
||
Fixing a last mochitest fail here: + if (this._viewsLeftMap !== undefined) { + let map = this._viewsLeftMap; + } else { + let map = {}; + } Successful build: https://tbpl.mozilla.org/?tree=Try&rev=de3e8aa4795e http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/theo.chevalier11@gmail.com-de3e8aa4795e
Attachment #661497 -
Attachment is obsolete: true
Assignee | ||
Comment 29•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee6470d54349
Assignee | ||
Comment 30•12 years ago
|
||
Backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/027dc449dee8 Because we should use "devices" instead of "computers" Pushed to inbound again: https://hg.mozilla.org/integration/mozilla-inbound/rev/52b90ba3a2c5
Comment 31•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/52b90ba3a2c5
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Assignee | ||
Comment 32•12 years ago
|
||
Sorry, I have to back this out again. https://hg.mozilla.org/integration/mozilla-inbound/rev/effca1041825
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 33•12 years ago
|
||
Now it works, sorry about the noise https://hg.mozilla.org/integration/mozilla-inbound/rev/1b195e9c5f9d
Comment 34•12 years ago
|
||
Actually, it doesn't. This is causing many mochitest failures. Please make sure this works on Try before pushing again. https://hg.mozilla.org/integration/mozilla-inbound/rev/63186642d2e3 See the failures here: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1b195e9c5f9d
Target Milestone: mozilla18 → ---
Comment 35•12 years ago
|
||
I'm getting lost, please upload the current patch here...
btw this._viewsLeftMap is not a memoized getter, so you should limit times you call into it into a single method by assigning the result to a temp var.
> let map = this._viewsLeftMap !== undefined ? this._viewsLeftMap : {};
How can it be undefined, it is initialized to {} already in the getter... not to be picky but this is not what I reviewed :)
So please post an updated patch we can diff.
Assignee | ||
Comment 36•12 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #35) > I'm getting lost, please upload the current patch here... > > btw this._viewsLeftMap is not a memoized getter, so you should limit times > you call into it into a single method by assigning the result to a temp var. Ok > > > let map = this._viewsLeftMap !== undefined ? this._viewsLeftMap : {}; > > How can it be undefined, it is initialized to {} already in the getter... Yes, it is useless, but apparently mochitest is successful on Linux with that (but not on Mac & Win). Can't explain why. > not to be picky but this is not what I reviewed :) Sorry :( I wasn't sure if it needed review or not (and it seems that I was too excited that worked on my laptop and passed mochitest), but next time I'll ask before. > So please post an updated patch we can diff. I attached the last patch I tried (Results: https://tbpl.mozilla.org/?tree=Try&rev=fbbca4f5ca56) I bet the error is in the viewsLeft setter, but where...
Attachment #661518 -
Attachment is obsolete: true
Comment 37•12 years ago
|
||
Comment on attachment 662304 [details] [diff] [review] Patch V7 ok, flagging myself to look into it
Attachment #662304 -
Flags: feedback?(mak77)
Assignee | ||
Comment 38•12 years ago
|
||
Thanks Mak, I'm very busy, so I'm not sure to have the time to investigate and fix that lonely before the next uplift
Comment 39•12 years ago
|
||
So, the failures are due to this patch, but not fault of this patch. Basically when the count was global there were enough tests running before the failing ones to completely deplete it. This patch makes the count per popup type, so those tests now hit the promo code while before they were not. I suppose the tests do something on popupshown that makes our own fail, the fix may involve fixing the tests, but likely I can just swap a property for a field in the binding and that should be enough. Btw, will run it through try before.
Comment 40•12 years ago
|
||
So basically this add a _panelId field. I originally thought to convert _notificationType to a field, but then figured the pref it's checking may change (for example if the user setups sync) so it's better if it's not cached. The "problem" was lying in the popupShown handler we add, it was trying to access the panel children at the wrong time, through _notificationType. Though, there is another problem, the addon-install-complete notification has a custom binding and doesn't seem to use notification-popup, so the promobox won't ever be shown. I have to find where I can set the attribute on that popup.
Attachment #662304 -
Attachment is obsolete: true
Attachment #662304 -
Flags: feedback?(mak77)
Comment 41•12 years ago
|
||
My previous caching was still wrong, so this goes back to the original version, the only change I added is checking that the panel is actually open on popupShown, a previous popupShown may close it, that is basically what some of the tests are doing. Going through Try now.
Attachment #668580 -
Attachment is obsolete: true
Comment 42•12 years ago
|
||
Also need to check the panel is still a supported one, cause in some case on popupshown we also replace the contents and we don't want to show the wrong message (or throw trying to get a message for an unsupported panel). Tested manually and on try: https://tbpl.mozilla.org/?tree=Try&rev=c2deedb9ebce I don't think the added check needs additional review, it's quite trivial and regardless makes sense to ensure we don't show un unexpected message. I'll push it once try is complete.
Attachment #669139 -
Attachment is obsolete: true
Assignee | ||
Comment 43•12 years ago
|
||
Thanks Marco for all your work, I think I would not have been able to do that! Note that now that FF18 is closed, you will need to push it to Aurora too, because bug 716643 is currently landed in FF18.
Comment 45•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e025693543bd
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 46•12 years ago
|
||
(In reply to Théo Chevalier [:tchevalier] from comment #43) > Note that now that FF18 is closed, you will need to push it to Aurora too, > because bug 716643 is currently landed in FF18. The fact is that I'm not sure this patch meets the needed requirement to uplift, would it be an issue to have the "feature" in FF19 than 18? The fact bug 716643 landed in 18 is not a big deal, once this patch will hit release, the impressions for addons will be basically reset, so the worst annoyance may be that some users will see the impressions again after installing FF19. Since these promos have a close button should be a minor problem.
Updated•6 years ago
|
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in
before you can comment on or make changes to this bug.
Description
•