Closed Bug 741738 Opened 9 years ago Closed 8 years ago

Count impressions for each doorhanger notification type

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: tchevalier, Assigned: tchevalier)

References

Details

Attachments

(1 file, 9 obsolete files)

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.
Component: Add-ons Manager → Firefox Sync: UI
Product: Toolkit → Mozilla Services
QA Contact: add-ons.manager → sync-ui
Attached patch Patch V1 (obsolete) — Splinter Review
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)
(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.
Attachment #613263 - Flags: review?(mak77)
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.
typo! s/bit deal/big deal/
Attached patch Patch V2 (obsolete) — Splinter Review
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 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-
Attached patch Patch V3 (obsolete) — Splinter Review
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 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)
(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
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.
Attached patch Patch V4 (obsolete) — Splinter Review
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 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+
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.
sync triage: Is work still happening on this? theo, how's it going?
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.
Good news, I will find some help this weekend at MozCamp in Warsaw, so hopefully this will be landed next week!
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?
Do you know precisely where the exception is being thrown from?
(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.
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.
If that's the case, I would throw a debugger; statement in the _notificationType getter to see what's calling it.
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
(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?
(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.
Attached patch Patch V5 (obsolete) — Splinter Review
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
Attached patch Patch V6 (obsolete) — Splinter Review
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
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
https://hg.mozilla.org/mozilla-central/rev/52b90ba3a2c5
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Sorry, I have to back this out again.
https://hg.mozilla.org/integration/mozilla-inbound/rev/effca1041825
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 → ---
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.
Attached patch Patch V7 (obsolete) — Splinter Review
(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 on attachment 662304 [details] [diff] [review]
Patch V7

ok, flagging myself to look into it
Attachment #662304 - Flags: feedback?(mak77)
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
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.
Attached patch patch V8 (obsolete) — Splinter Review
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)
Attached patch patch V9 (obsolete) — Splinter Review
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
Attached patch patch V10Splinter Review
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
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.
https://hg.mozilla.org/mozilla-central/rev/e025693543bd
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(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.
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.