348419 - No way to access the update notification dialog when visiting an extension's homepage

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Simon Bünzli]

Steps to reproduce:
1. Make sure that you get an update notification on startup (e.g. lower an extension's version in extensions.rdf, find the update for it in the Add-ons manager and set extensions.update.notifyUser to true)
2. Launch Firefox
3. Visit that extension's homepage
4. Try to continue the update process...

Actual result:
You have to close the opened browser window before being able to get back (NB: people resuming their previous session will lose it in the process).

Expected result:
? (not sure how to best handle this case - maybe don't allow the homepage to be visited in this case, or don't make the notification dialog modal)

Comment 1

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

For 2.0 this should just not be available and for 3.0 I'd like an app notification area for notification of updates instead of notifying on startup which would solve this.

Comment 2

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

bug 347585 is for the app notification area 

Comment 3

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

Too late to try to fix this for 2.0 and for 3.0 fixing bug 347585 and making the EM use the notification area will fix this.

Comment 4

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

Attachment: patch (remove Homepage and About context menuitems)

Comment 5

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

Dave, what do you think about removing the option to open the homepage and the about dialog?

Comment 6

[Dave Townsend [:mossop]]

I can't say I'm all that keen on it. I think it's hiding potentially crucial information from the user when he might need it most. In my experience most users barely know what add-ons the have installed so when an update reminder appears I think they might want to find out just what that thing is. I guess it depends what happens with bug 297903.

That said I'm willing to bet hardly anyone uses them in the update reminder and I can't think of a decent way to make it work in the "notify before startup" scenario.

Comment 7

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

Agreed on all counts... I'm going to run this by beltzner.

Comment 8

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

Attachment: patch (remove Homepage and About context menuitems for Firefox only)

Seth, I talked with beltzner about this approach and got his buy in to make this change.

Comment 9

[(not reading, please use [e-mail] instead)]

Comment on attachment 271940 [details] [diff] [review]
patch (remove Homepage and About context menuitems for Firefox only)

I think it might be better to explicitly call out what items you are removing.

Something like: 

#ifdef MOZ_PHOENIX
        // If we are a browser when updating on startup don't display context
        // menuitems that can open a browser window.
        gUpdateContextMenus.splice(gUpdateContextMenus.indexOf("menuitem_homepage"), 1);
        gUpdateContextMenus.splice(gUpdateContextMenus.indexOf("menuitem_about"), 1);
        gUpdateContextMenus.splice(gUpdateContextMenus.indexOf("menuseparator_1"), 1);
#endif

Comment 10

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

What is the concern this would address?

Comment 11

[(not reading, please use [e-mail] instead)]

my reason is that so if someone changes the order of those menuitems or changes the code, they're more likely to see the code that dynamically modifies that array.

How about doing something like this, instead:

var gUpdateContextMenusNoBrowser = ["menuitem_installUpdate",
"menuitem_includeUpdate"];
var gUpdateContextMenus = ["menuitem_homepage", "menuitem_about",
"menuseparator_1"].concat(gUpdateContextMenusNoBrowser);

#ifdef MOZ_PHOENIX
  // If we are a browser when updating on startup don't display context
  // menuitems that can open a browser window.
  gUpdateContextMenus = gUpdateContextMenusNoBrowser;
#endif

Comment 12

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

I like the second approach better since they may also be removed instead of just changed. I'll go with that and resubmit. Thanks

Comment 13

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

Attachment: patch rev 3 (remove Homepage and About context menuitems for Firefox only)

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 272073 [details] [diff] [review]
patch rev 3 (remove Homepage and About context menuitems for Firefox only)

r=sspitzer, thanks robert

Comment 15

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

Checked in to trunk

Checking in mozilla/toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.125; previous revision: 1.124
done

Comment 17

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

Comment on attachment 272073 [details] [diff] [review]
patch rev 3 (remove Homepage and About context menuitems for Firefox only)

This patch fixes this bug by preventing users from opening a browser window on firefox during startup which over-writes the session store as seen in Bug 361129.

Comment 18

[Philip Chee]

Now that SeaMonkey is using toolkit can the phrase in the comment "If we are a browser" be changed to "If we are Firefox"?

Comment 19

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

sure

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 272073 [details] [diff] [review]
patch rev 3 (remove Homepage and About context menuitems for Firefox only)

approved for 1.8.1.7, a=dveditz for release-drivers

Comment 21

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

Checked in to MOZILLA_1_8_BRANCH

Checking in mozilla/toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.72.2.43; previous revision: 1.72.2.42
done

Comment 22

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.8 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.8pre) Gecko/20070929 BonEcho/2.0.0.8pre ID:2007092904 - the context menu for visit homepage and about is removed, so no new browser could be opened when you see the update notification - comment #17

- > adding verified keyword