538331 - On update perform action based upon the update metadata

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Robert Strong (they/them - no direct email)]

Instead of just opening a web page after an update Firefox should be able to perform an action based upon the update's metadata.

The basic flow would be:
a. update is performed.
b. update service would set a pref so Firefox could easily tell that an update was performed.
c. Firefox would then read the last update's data via the update manager which would contain the custom action value provided by the update snippet and perform the associated action for this value.

Note: I'm leaning towards this value being a string since it is provided by the update snippet and is per app defined in order to avoid bugs where someone wants to use a string. It would be human readable then so the value's meaning would be clear.

The current actions I know of are:
1. do nothing
2. open a web page (as is done today)
3. show the notification bar

Note: the notification bar is meant for notification regarding a page's content but we have made an exception in relation to the license notification and I suspect that is what we will do here as well.

I will be filing a bug for the app update part of this after I have it better flushed out. I've discussed this with ddahl and he is going to take this bug.

beltzner, have I left anything out?

Comment 1

[Robert Strong (they/them - no direct email)]

btw: the pref *might* just contain the action value... not sure yet but that shouldn't stop work on adding the actions.

Comment 2

[David Dahl :ddahl]

this look like the code where the override_url is checked for:

http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#538

Comment 3

[David Dahl :ddahl]

more notes for me:

What or if we should overirde the home page is determined here: 

http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#114

Comment 4

[David Dahl :ddahl]

Attachment: v 0.0 WIP

Just getting started - Rob am I on the right path? 

What bundle / dtd should I use to store button text, etc?

Comment 5

[Robert Strong (they/them - no direct email)]

I believe so... as long as this is where the current page is displayed after an upgrade. You'll need to handle the different cases separately for open page (as it does now), show notification, and do nothing.

Strings should likely be in browser.properties

Comment 6

[David Dahl :ddahl]

This new function, browserUpdateNotification, is executed when a new Gecko version is detected: 'case OVERRIDE_NEW_MSTONE', which is determined inside of a getter called defaultArgs, which in turn is called from the commandline handler. The handle method (line 364) is where we are actually calling openWindow(), etc.

It is inside defaultArgs() getter where session restore is attempted. I added my call browserUpdateNotification below session restore. it seems like this may be premature and I am assuming that session restore is asynchronous. 

I guess I am still investigating...

Comment 7

[Robert Strong (they/them - no direct email)]

beltzner would also like an additional action for showing the toast / slider notification

Comment 8

[David Dahl :ddahl]

Attachment: v 0.0.1 WIP

added observer for "sessionstore-windows-restored", 
added distinct functions for each action type.

Rob:
 Is this still headed in the right direction? we'll need to know how we are getting the messages to display. I assume they are static strings in a bundle, but you never know. also the prefnames for each pref we will access - when you get a chance.

Comment 9

[Robert Strong (they/them - no direct email)]

(In reply to comment #8)
> Created an attachment (id=423916) [details]
> v 0.0.1 WIP
> 
> added observer for "sessionstore-windows-restored", 
> added distinct functions for each action type.
> 
> Rob:
>  Is this still headed in the right direction?
What concerns do you have specifically? In the current place where Firefox opens a new tab there will just need to be a check for whether the app.update.updated pref is true and then it will check the last update's extra1 property which is for app specific info from the update xml. For now just hardcode the different cases you are implementing so you can test.

> we'll need to know how we are
> getting the messages to display. I assume they are static strings in a bundle,
> but you never know.
Not sure what you are asking here. If you are referring to strings in the notification ui and the alert slider faaborg should be able to provide them. You should add these strings to browser.properties as noted in comment #5.

> also the prefnames for each pref we will access - when you
> get a chance.
For this it should just be app.update.updated and after I finish up the backend I'll give you the call to check for the extra1 value.

Comment 10

[Robert Strong (they/them - no direct email)]

After finishing up a bit more of the patch in bug 530872 I've decided to go with app.update.extra1 for the pref to determine that an update has been applied. It will contain the app provided value for the custom action specified in the update xml. The possible values I am leaning towards for Firefox are
undefined = extra1 was not specified in the update snippet. I think this should just be the current action.
silent = do nothing
openURL = open a url. This should be the current action using the pref containing the url
openURL http://etc. = open the url specified after the space.
showNotification = show the notification bar. The url should be the url contained in the current action pref.
showNotification http://etc. = show the notification bar with the url specified after the space.
showAlert = show an alert. The url should be the url contained in the current action pref.
showAlert = show an alert. The url should be the url contained in the current showAlert http://etc. = show an alert with the url specified after the space.

The url being read from the existing pref vs. from the extra1 pref should require very little code. That should be enough detail to finish up this bug.

Comment 11

[David Dahl :ddahl]

Attachment: v 0.0.3 WIP

Will need an automatic test - going to start manually testing what I have now

Comment 12

[David Dahl :ddahl]

Attachment: v 0.0.4 WIP - getting closer

this opens a new tab, but opens 3 of them - the observer is working. getting closer. Cannot wait to see how to test this:)

Comment 13

[David Dahl :ddahl]

Attachment: v 0.1 Still WIP - need to write tests and get feedback on L10N

Comment 14

[David Dahl :ddahl]

for the test it looks like I will have to set this pref as well:

XXbrowser.startup.homepage_override.mstone, which on trunk is "rv:1.9.3a2pre", what value should I set it to?

I am referencing "needHomepageOverride" function

Comment 15

[David Dahl :ddahl]

There is no test directory associated with the nsBrowserContenthandler component. Where should a browser chrome test sit in the tree?

Comment 16

[David Dahl :ddahl]

looks like most of the tests for browser are here: mozilla/browser/base/content/test/

doesn't seem to the best place for it. I'll work on it there for now

Comment 17

[David Dahl :ddahl]

Attachment: 0.2 WIP, testing issues

adding a test to preliminary (still wip) patch. Not sure if a browser chrome test is even possible since we need to test several different actions *on startup*. Mak thinks I might be able to "engineer" it:

<ddahl> dolske: we now have 4 more ways to annoy the user on startup when a new milestone is updated
<mak> ddahl: i think you should "simulate" the startup you need initing services/components you're trying to test
<ddahl> with alerts and notifications and more tab openings
<ddahl> hmmm
<dolske> 4 more ways? O_o
<ddahl> dolske: 4 or more
<ddahl> i am inventorying them right now
<mak> ddahl: for example in browserGlue, you can just notify it the startup messages, and it will act the same (or similar) as if browser would be starting... can't you do something like that?
<ddahl> mak: so can I re-run session restore or something like that?
<dolske> Mozmill would probably be simplest, you could look at having that stuff run off a notification, and the then just fire the notification to trigger it in the middle of test runs.
<ddahl> mak: yes that might work
<mak> just init that service and pass it the messages it is expecting at startup
<mak> you will need to know a lot about how startup of that part works, though.

Comment 18

[David Dahl :ddahl]

forgot to mention that the observe() method is never called in my test's observer, weird:

function obs(){
    dump("*** obs constructed\n\n\n");
    this.register();
  }
  obs.prototype = {
    observe: function (subject, topic, data) {
      dump("*** observe() called: topic:" + topic + "\n\n\n");
      if (topic == "sessionstore-windows-restored") {
        // check for the notificationbox and the tab that should  be open to
        // 'APP_UPDATE_URL'
        var notifyBox = gBrowser.getNotificationBox();
        dump("*** notifyBox" + notifyBox + "\n\n\n");
        is(typeof notifyBox.appendNotification == "function");
        this.unregister();
        finish();
      }
    },
    register: function () {
      var obsService =
      Components.classes["@mozilla.org/observer-service;1"].
      getService(Components.interfaces.nsIObserverService);
      obsService.addObserver(this, "sessionstore-windows-restored", false);
    },
    unregister: function () {
      var obsService =
      Components.classes["@mozilla.org/observer-service;1"].
      getService(Components.interfaces.nsIObserverService);
      obsService.removeObserver(this, "sessionstore-windows-restored");

    }
  };

  var myObs = new obs();

Comment 19

[David Dahl :ddahl]

testing notes:

rs told me the easiest way to test this was to:

1. instantiate nsBrowserContentHandler
2. QI to nsIObserver
3. call observe() with the sessionstore-windows-restored topic multiple times and check for the result: new tab, new tab + notificationbar, newtab + alert, and alert

Comment 20

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Dave, I'm going to take this bug off of you... I have a pretty clear picture of what else needs to be done to finish this up.

Comment 21

[Robert Strong (they/them - no direct email)]

beltzner, I'm ready for the default strings for the notification and alert.

I've made it so it is possible to show any combination of the choices (e.g. page, alert, and notification) as well as current behavior and silent.

Comment 22

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Works... still need to write the tests.

Comment 23

[Robert Strong (they/them - no direct email)]

Attachment: patch for feedback

Hey Dietrich, I wanted to get your feedback on this approach. There appear to be absolutely no tests directly for nsBrowserContentHandler.js and nsBrowserGlue.js... any ideas of how to best test this?

Comment 24

[Dietrich Ayala (:dietrich)]

Comment on attachment 433152 [details] [diff] [review]
patch for feedback

looks ok, other than the few comments below.

wrt to testing - outside of a full-blown mock update server and harness, maybe you could write a mochitest that triggers that nsBrowserContentHandler code, and then tests for the expected UI/pref bits? in the browser/components/places tests there are examples where marco triggers the right nsBrowserGlue notifications to flex code in that component.

>+          if (prefb.prefHasUserValue("app.update.postupdate")) {
>+            var um = Components.classes["@mozilla.org/updates/update-manager;1"]
>+                               .getService(Components.interfaces.nsIUpdateManager);
>+            try {
>+              var update = um.getUpdateAt(0)
>+                             .QueryInterface(Components.interfaces.nsIPropertyBag);
>+              var actions = update.getProperty("actions");
>+            }

should comment on what would fail here.

actually, which line would throw here? if not the getProperty() call, then it could be after the try/catch block, yeah? otherwise, since actions is used outside of the try/catch block, define it beforehand for clarity.

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js
>@@ -205,16 +205,21 @@ BrowserGlue.prototype = {
>           this._backupBookmarks();
>         break;
>       case "distribution-customization-complete":
>         this._observerService
>             .removeObserver(this, "distribution-customization-complete");
>         // Customization has finished, we don't need the customizer anymore.
>         delete this._distributionCustomizer;
>         break;
>+      case "alertclickcallback":

i'd prefer either camelCasing or hyphenation in the notification name.

>+    if (actions.indexOf("showAlert") != -1) {
>+      let props = {};
>+      let title = getNotifyString({propName: "alertTitle",
>+                                   stringName: "puAlertTitle",
>+                                   stringParams: [appName]});
>+      let text = getNotifyString({propName: "alertText",
>+                                  stringName: "puAlertText",
>+                                  stringParams: [appName]});
>+      let url = getNotifyString({propName: "alertURL",
>+                                 prefName: "startup.homepage_override_url"});
>+      try {
>+        var notifier = Cc["@mozilla.org/alerts-service;1"].
>+                       getService(Ci.nsIAlertsService);
>+        notifier.showAlertNotification(null, title, text, true, url, this);
>+      }
>+      catch (e) {
>+      }

since you only need the strings if the alerts service exists, should check for that first before fetching them.

also, props isn't used.

Comment 25

[Robert Strong (they/them - no direct email)]

Thanks Dietrich! I'll take care of your comments unless otherwise noted below.

(In reply to comment #24)
> (From update of attachment 433152 [details] [diff] [review])
> looks ok, other than the few comments below.
> 
> wrt to testing - outside of a full-blown mock update server and harness, maybe
> you could write a mochitest that triggers that nsBrowserContentHandler code,
> and then tests for the expected UI/pref bits? in the browser/components/places
> tests there are examples where marco triggers the right nsBrowserGlue
> notifications to flex code in that component.
Thanks alot! It is by no means clear how this code should be tested especially since it doesn't have its own test dir.

> >+          if (prefb.prefHasUserValue("app.update.postupdate")) {
> >+            var um = Components.classes["@mozilla.org/updates/update-manager;1"]
> >+                               .getService(Components.interfaces.nsIUpdateManager);
> >+            try {
> >+              var update = um.getUpdateAt(0)
> >+                             .QueryInterface(Components.interfaces.nsIPropertyBag);
> >+              var actions = update.getProperty("actions");
> >+            }
> 
> should comment on what would fail here.
> 
> actually, which line would throw here? if not the getProperty() call, then it
> could be after the try/catch block, yeah? otherwise, since actions is used
> outside of the try/catch block, define it beforehand for clarity.
Yep... I hadn't cleaned that up yet.

> >diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
> >--- a/browser/components/nsBrowserGlue.js
> >+++ b/browser/components/nsBrowserGlue.js
> >@@ -205,16 +205,21 @@ BrowserGlue.prototype = {
> >           this._backupBookmarks();
> >         break;
> >       case "distribution-customization-complete":
> >         this._observerService
> >             .removeObserver(this, "distribution-customization-complete");
> >         // Customization has finished, we don't need the customizer anymore.
> >         delete this._distributionCustomizer;
> >         break;
> >+      case "alertclickcallback":
> 
> i'd prefer either camelCasing or hyphenation in the notification name.
That is how the alert service is written so that ain't going to happen ;)
http://mxr.mozilla.org/mozilla-central/search?string=alertclickcallback

Comment 26

[Robert Strong (they/them - no direct email)]

Attachment: main patch rev1

Mike, can I get strings for this?

Dietrich, I'll follow up with a separate patch for the tests

Comment 27

[Robert Strong (they/them - no direct email)]

Attachment: Tests for nsBrowserContentHandler.js changes - in progress

Just about done with these tests

Comment 28

[Robert Strong (they/them - no direct email)]

Attachment: Tests for nsBrowserContentHandler.js changes

I'll finish up the nsBrowserGlue.js later

Comment 29

[Robert Strong (they/them - no direct email)]

Attachment: Tests for nsBrowserContentHandler.js changes

left some cruft in the previous patch

Comment 30

[Robert Strong (they/them - no direct email)]

Comment on attachment 435525 [details] [diff] [review]
main patch rev1

I'm going to rework this slightly for the browser glue tests... the string review is still valid so not removing ui-review.

Comment 31

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2 w/ tests

Comment 32

[Robert Strong (they/them - no direct email)]

Comment on attachment 436655 [details] [diff] [review]
patch rev2 w/ tests

The alert tests fail on Linux and Mac which I think is due to their system alert notification implementations. All other tests passed on the try server.

Comment 33

[Robert Strong (they/them - no direct email)]

Attachment: patch rev3 w/ tests

removed alert tests since Mac and Linux have system provided notifications and I don't see any tests in the tree that would allow me to test their functionality.

Comment 34

[Robert Strong (they/them - no direct email)]

Tests passed on Linux and Windows... waiting on Mac to finish

Comment 35

[Dietrich Ayala (:dietrich)]

Comment on attachment 436712 [details] [diff] [review]
patch rev3 w/ tests


>diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js
>--- a/browser/components/nsBrowserContentHandler.js
>+++ b/browser/components/nsBrowserContentHandler.js

add yourself as a contributor to this file

>+
>+            if (actions.indexOf("showURL") != -1) {
>+              overridePage = update.getProperty("openURL")
>+              if (!overridePage)
>+                overridePage = formatter.formatURLPref("startup.homepage_override_url");
>+            }

why not clearing the pref in this case?

>+
>+            if (actions.indexOf("showAlert") == -1 &&
>+                actions.indexOf("showNotification") == -1)
>+              prefb.clearUserPref("app.update.postupdate");

instead of repeating the code to clear the pref, i'd prefer setting a flag, and clearing the pref at the end.

>+          }
>+          else
>+            overridePage = formatter.formatURLPref("startup.homepage_override_url");
>           break;

nit: can you add brackets to the else clause? looks wonky followed by the break.

>+      let buttons = [
>+                      {
>+                        label:     label,
>+                        accessKey: key,
>+                        popup:     null,
>+                        callback: function(aNotificationBar, aButton) {
>+                          browser.selectedTab = browser.addTab(url);
>+                        }
>+                      }
>+                    ];
>+

nit: up to you, but i prefer

let buttons = [
  stuff
  stuff
];

>+++ b/browser/components/test/browser/browser_bug538331.js
>@@ -0,0 +1,460 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=2 et sw=2 tw=80: */
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */

hm? why not the regular license header here?

the test looks ok to me, but please get additional eyes on it from someone more familiar with how update works.

Comment 36

[Robert Strong (they/them - no direct email)]

(In reply to comment #35)
> (From update of attachment 436712 [details] [diff] [review])
> 
> >diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js
> >--- a/browser/components/nsBrowserContentHandler.js
> >+++ b/browser/components/nsBrowserContentHandler.js
> 
> add yourself as a contributor to this file
meh... not a big fan of this file

> 
> >+
> >+            if (actions.indexOf("showURL") != -1) {
> >+              overridePage = update.getProperty("openURL")
> >+              if (!overridePage)
> >+                overridePage = formatter.formatURLPref("startup.homepage_override_url");
> >+            }
> 
> why not clearing the pref in this case?
It is cleared later if showAlert and showNotification are not also present in the actions so browser glue will do the right thing.

> >+
> >+            if (actions.indexOf("showAlert") == -1 &&
> >+                actions.indexOf("showNotification") == -1)
> >+              prefb.clearUserPref("app.update.postupdate");
> 
> instead of repeating the code to clear the pref, i'd prefer setting a flag, and
> clearing the pref at the end.
It isn't clear to me what you mean. At the end of the if block that checks if the pref is present the pref is cleared if showAlert and showNotification are not present. showAlert and showNotification are not checked for at any other time in this block.

> >+          }
> >+          else
> >+            overridePage = formatter.formatURLPref("startup.homepage_override_url");
> >           break;
> 
> nit: can you add brackets to the else clause? looks wonky followed by the
> break.
Done

> >+      let buttons = [
> >+                      {
> >+                        label:     label,
> >+                        accessKey: key,
> >+                        popup:     null,
> >+                        callback: function(aNotificationBar, aButton) {
> >+                          browser.selectedTab = browser.addTab(url);
> >+                        }
> >+                      }
> >+                    ];
> >+
> 
> nit: up to you, but i prefer
> 
> let buttons = [
>   stuff
>   stuff
> ];
I was following existing style and if that were to change would prefer if someone did it in a followup bug so it is consistent in the file.

> >+++ b/browser/components/test/browser/browser_bug538331.js
> >@@ -0,0 +1,460 @@
> >+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >+/* vim: set ts=2 et sw=2 tw=80: */
> >+/* Any copyright is dedicated to the Public Domain.
> >+ * http://creativecommons.org/publicdomain/zero/1.0/
> >+ */
> 
> hm? why not the regular license header here?
It is now a valid license header for tests and is much shorter which makes it easier to get to the actual code on opening the file in an editor.

> the test looks ok to me, but please get additional eyes on it from someone more
> familiar with how update works.
There are already app update tests for the app update side of this functionality that is app agnostic.

Comment 37

[Robert Strong (they/them - no direct email)]

Comment on attachment 436712 [details] [diff] [review]
patch rev3 w/ tests

Dave, can I get you to do a once over of this patch?

Comment 38

[Robert Strong (they/them - no direct email)]

Tests passed on Mac try as well

Comment 39

[Dave Townsend [:mossop]]

Comment on attachment 436712 [details] [diff] [review]
patch rev3 w/ tests

>diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js
>--- a/browser/components/nsBrowserContentHandler.js
>+++ b/browser/components/nsBrowserContentHandler.js
>@@ -530,19 +530,56 @@ var nsBrowserContentHandler = {
>           // Existing profile, new build
>           copyPrefOverride();
> 
>           // Check whether we have a session to restore. If we do, we assume
>           // that this is an "update" session.
>           var ss = Components.classes["@mozilla.org/browser/sessionstartup;1"]
>                              .getService(Components.interfaces.nsISessionStartup);
>           haveUpdateSession = ss.doRestore();
>-          overridePage = formatter.formatURLPref("startup.homepage_override_url");
>+          if (prefb.prefHasUserValue("app.update.postupdate")) {
>+            var um = Components.classes["@mozilla.org/updates/update-manager;1"]
>+                               .getService(Components.interfaces.nsIUpdateManager);
>+            try {
>+              // If the updates.xml file is deleted then getUpdateAt will throw.
>+              var update = um.getUpdateAt(0)
>+                             .QueryInterface(Components.interfaces.nsIPropertyBag);
>+            }
>+            catch (e) {
>+              overridePage = formatter.formatURLPref("startup.homepage_override_url");
>+              prefb.clearUserPref("app.update.postupdate");
>+              break;
>+            }
>+
>+            var actions = update.getProperty("actions");
>+            if (!actions) {
>+              overridePage = formatter.formatURLPref("startup.homepage_override_url");
>+              prefb.clearUserPref("app.update.postupdate");
>+              break;
>+            }
>+
>+            if (actions.indexOf("silent") != -1) {
>+              prefb.clearUserPref("app.update.postupdate");
>+              break;
>+            }
>+
>+            if (actions.indexOf("showURL") != -1) {
>+              overridePage = update.getProperty("openURL")
>+              if (!overridePage)
>+                overridePage = formatter.formatURLPref("startup.homepage_override_url");
>+            }
>+
>+            if (actions.indexOf("showAlert") == -1 &&
>+                actions.indexOf("showNotification") == -1)
>+              prefb.clearUserPref("app.update.postupdate");
>+          }
>+          else
>+            overridePage = formatter.formatURLPref("startup.homepage_override_url");

Kind of feels like a lot of this logic needs splitting out into a separate function or something to avoid the repeated code, but I guess dietrich has covered that. Otherwise this all looks fine.

Comment 40

[:Gavin Sharp [email: [e-mail]]]

>diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js

To expand on Mossop's suggestion of a seperate function, I think you could move this into a getOverridePage(prefb) function. It seems to me like this function should really only deal with showURL, and leave dealing with all other action types to _showUpdateNotification. So something like:

var overridePage = getOverridePage(prefb));

function getOverridePage(prefb) {
  let defaultOverridePage = formatter.formatURLPref("startup.homepage_override_url");

  let hasUpdateOverride = prefb.prefHasUserValue("app.update.postupdate");
  if (!hasUpdateOverride)
    return defaultOverridePage;

  var um = Components.classes["@mozilla.org/updates/update-manager;1"]
                     .getService(Components.interfaces.nsIUpdateManager);
  try {
    // If the updates.xml file is deleted then getUpdateAt will throw.
    var update = um.getUpdateAt(0)
                   .QueryInterface(Components.interfaces.nsIPropertyBag);
    var actions = update.getProperty("actions");
    if (actions.indexOf("showURL") != -1) {
      let updateOpenURL = update.getProperty("openURL")
      if (updateOpenURL)
        return updateOpenURL;
    }
  } catch (e) {
    Components.utils.reportError(e);
  }
  return defaultOverridePage;
}

Would that work?

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>+      case "alertclickcallback":
>+        var win = this.getMostRecentBrowserWindow();
>+        var browser = win.gBrowser;
>+        browser.selectedTab = browser.addTab(data);
>+        break;

This can just be in a function defined prior to the call to showAlertNotification:

var self = this;
function clickCallback() {
  var win = self.getMostRecentBrowserWindow();
  var browser = win.gBrowser;
  browser.selectedTab = browser.addTab(data);
}
notifier.showAlertNotification(null, title, text, true, url, clickCallback);

>+  _showUpdateNotification: function BG__showUpdateNotification() {

>+    if (actions.indexOf("showAlert") != -1) {

>+      if (notifier) {

Early return if (!notifier) would help readability, I think.

The tests are awesome!

Comment 41

[Robert Strong (they/them - no direct email)]

I considered adding the code to getOverridePage and have no problem with going that route... I'll resubmit with the changes. Thanks for the other suggestions as well Gavin.

Comment 42

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 436712 [details] [diff] [review]
patch rev3 w/ tests

>+# Post Update Notifications
>+pu.notifyButton.label=What's New
>+pu.notifyButton.accesskey=W
>+# LOCALIZATION NOTE %S will be replaced by the short name of the application.
>+puNotifyText=%S has been updated!
>+puAlertTitle=%S Updated
>+puAlertText=%S has been updated!

If I'm reading this patch right (and I like to think that I am) we're talking about the strings to be used by default in the drop-down bar after an update, as well as what might appear in a system alert. I think we want the following:

Firefox has been updated         (Details…) (x)

 .---------------------------.
 | Firefox Updated           |
 |                           |
 | Click here for details    |
 '---------------------------'

So that would mean:

pu.notifyButton.label=Details…
pu.notifyButton.accesskey=D
puNotifyText=%S has been updated
puAlertTitle=%S Updated
puAlertText=Click here for details

uir+ with those sweeping changes! :)

Comment 43

[Robert Strong (they/them - no direct email)]

woo hoo! Thanks Mike

Comment 44

[Robert Strong (they/them - no direct email)]

Attachment: patch rev4 w/ tests

Gavin, I went with your suggestions for the most part but made a few changes that I think make it cleaner / clearer.

Changed the strings so adding ui-r+

Comment 45

[Robert Strong (they/them - no direct email)]

Comment on attachment 437413 [details] [diff] [review]
patch rev4 w/ tests

k
>diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js
>--- a/browser/components/nsBrowserContentHandler.js
>+++ b/browser/components/nsBrowserContentHandler.js
>...
>@@ -144,16 +145,44 @@ function needHomepageOverride(prefb) {
>     
>     prefb.setCharPref("browser.startup.homepage_override.mstone", mstone);
>     return (savedmstone ? OVERRIDE_NEW_MSTONE : OVERRIDE_NEW_PROFILE);
>   }
> 
>   return OVERRIDE_NONE;
> }
> 
>+/**
>+ * Gets the application has just been updated override page.
Forgot to qrefresh... I've changed this to
 * Gets the override page for the first run after the application has been
 * updated.

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 437413 [details] [diff] [review]
patch rev4 w/ tests

>diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js

>+function getPostUpdateOverridePage(defaultOverridePage) {

>+  if (actions.indexOf("silent") != -1 || actions.indexOf("showURL") == -1)
>+    return "";

Add a comment here maybe? "Silent update, or no override page specified"

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>+      case "browser-glue-test": // used by tests
>+        if (data == "update-notification") {

>+          if (this._prefs.prefHasUserValue("app.update.postupdate"))
>+            this._showUpdateNotification();

I wonder if you should factor this out into a little helper (checkForPostUpdate()?) and call it directly from here and _onBrowserStartup, just to avoid the test vs. non-test codepaths from getting out of sync in the future. Only two lines so maybe not worth worrying about.

>+  _showUpdateNotification: function BG__showUpdateNotification() {

>+      let browser = win.gBrowser; // for closure in notification bar callback

I think "tabbrowser" is a better name for this... though this file uses "browser" pretty much everywhere else :(

>+    function clickCallback() {

>+      browser.selectedTab = browser.addTab(data);

Looks like this wants s/data/url/ ? Or add the data argument to the function definition, I guess, but just accessing it via the closure seems easier to understand (and then you can remove it from the call to showAlertNotification below).

>+      notifier.showAlertNotification(null, title, text, true, url, clickCallback);

Probably wouldn't hurt to include a "name" param (per IDL) - "post-update-notification"?.

>diff --git a/browser/components/test/browser/browser_bug538331.js b/browser/components/test/browser/browser_bug538331.js

>+function testShowNotification()

>+        // The last test opens an url and verifies the url from the updates.xml
>+        // is correct.
>+        if (i == (BG_NOTIFY_TESTS.length - 1)) {
>+          button.click();
>+          gBrowser.selectedBrowser.addEventListener("load", testNotificationURL, true);
>+        }

Hmm, does this pass given the s/url/data/ issue mentioned above?

>+function WindowOpenListener(url, opencallback, closecallback)

This looks unused?

Comment 47

[:Gavin Sharp [email: [e-mail]]]

I guess the test would also pass if testNotificationURL() is just never called...

Comment 48

[Robert Strong (they/them - no direct email)]

I'll look into it... I know for a fact it was passing in an earlier version attached to this bug.

note: I personally dislike changing tried / true methods of writing code for an implementation such as how the alert service is used throughout the code for what I think are obvious reasons.

Comment 49

[Alex Faaborg [:faaborg] (Firefox UX)]

>as well as what might appear in a system alert

In what cases do we get the system alert instead of the notification bar?

Comment 50

[Robert Strong (they/them - no direct email)]

The alert is the nsIAlertsService alert and it is shown when the update metadata states an alert should be shown. None of these 'actions' prevent other actions except for the silent action which prevents any action.

Comment 51

[Alex Faaborg [:faaborg] (Firefox UX)]

>it is shown when the update metadata states an alert should be shown.

right, I meant in the more literal sense: do we have any plans to provide this metadata in a Firefox update.

Comment 52

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #48)
> note: I personally dislike changing tried / true methods of writing code for an
> implementation such as how the alert service is used throughout the code for
> what I think are obvious reasons.

I'm not sure what this is referring to - can you clarify?

Comment 53

[Robert Strong (they/them - no direct email)]

The option to show an alert, notification, web page, any combination of the previous three, or do nothing will all be available. There are no plans that I know of to specifically show the alert at this moment and when I discussed this with beltzner it was the least important option of the options available during the initial implementation. If the desire to use it comes up it will be available of course.

note: the app update implementation doesn't care what is provided in the custom / additional attributes of the update xml so app's can implement new actions without any changes to app update.

Comment 54

[Robert Strong (they/them - no direct email)]

(In reply to comment #52)
>...
> I'm not sure what this is referring to - can you clarify?
Just that it isn't used very much in the codebase, it isn't very well tested in the codebase, there doesn't appear to be a way to test it on Linux / Mac OS X since they have custom implementations, and hence by not following the normal code pattern where it is used there is a greater chance that this new pattern will fail in an unforeseen way.

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2907

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/nsExtensionManager.js#5521

Comment 55

[:Gavin Sharp [email: [e-mail]]]

Oh, you're referring to the "name" suggestion? Seems quite unlikely that that would have any kind of negative impact, given it's documented purpose (and from a quick look of the implementations), but that was just a suggestion - I don't feel strongly about it!

Comment 56

[Robert Strong (they/them - no direct email)]

No, the comment wasn't directed at you or the 'name'... it was just a general comment directed at nsIAlertsService, the difference in the code pattern, and that there is no way to test this properly which is why it isn't tested in the tests. What brought it to mind was

(In reply to comment #47)
> I guess the test would also pass if testNotificationURL() is just never
> called...
testNotificationURL is called and I believe you were confusing the notification test vs. alert which isn't tested due to the reasons I stated previously. Also, WindowOpenListener was from my previous attempt to test nsIAlertsService.

Comment 57

[Robert Strong (they/them - no direct email)]

Attachment: patch for checkin

Comment 58

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/9996ac775114

Comment 59

[Robert Strong (they/them - no direct email)]

Attachment: patch for 1.9.2 - requires the rollup patch from bug 530872

Comment 60

[Robert Strong (they/them - no direct email)]

Comment on attachment 437753 [details] [diff] [review]
patch for 1.9.2 - requires the rollup patch from bug 530872

Note: when beltzner discussed backporting this to 1.9.2 he said he'd like to backport the strings as well. If this is too difficult we could choose to require the update xml provide the strings for 1.9.2 and noop when they aren't provided.

Comment 61

[Mike Beltzner [:beltzner, not reading bugmail]]

I think we'll want to hold off until after OOPP lands for this; let's aim for 1.9.2.5

Comment 62

[Mike Beltzner [:beltzner, not reading bugmail]]

Rob, is this branchsafe? If so, can you nominate? It'd be useful there.

Comment 63

[Robert Strong (they/them - no direct email)]

It will need the patches from bug 576939 and bug 530872 for the client side and bug 459972 for the server side. I can put together a patch for bug 530872 to use the previous behavior so the client side will be ready for when releng is able to fix bug 459972.

For this bug, if we want the built in strings I'll need to backport l10n which assumes that all of the l10n repos have translated the strings.

Comment 64

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

FYI, the test for this patch was disabled by bug 1077643.