Centralise notifications for ease of conversion to doorhangers

RESOLVED FIXED

Status

SeaMonkey
UI Design
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [doorhanger])

Attachments

(24 attachments, 8 obsolete attachments)

4.65 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
8.68 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
4.72 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
39.34 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
4.80 KB, patch
Details | Diff | Splinter Review
17.88 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
2.72 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
6.97 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
10.71 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
6.56 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
12.36 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
4.36 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
8.12 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
5.51 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
8.17 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
3.62 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
10.67 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
15.74 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
16.18 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
19.60 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
661 bytes, patch
Ian Neal
: review+
Details | Diff | Splinter Review
5.03 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
9.65 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
21.02 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
I came up with the following scheme to ease the conversion of notifications to doorhangers. The first step is to move all existing notification creation code into notification.xml. We then create a derived binding that is only used by the tabbed browser (doorhangers only work with tabbed browsers). In the derived binding we then add code that implements the notifications using doorhangers.
(Assignee)

Comment 1

7 years ago
Created attachment 474666 [details] [diff] [review]
Part 1: move the rights notification into notification.xml
[Checked in: Comment 13]
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #474666 - Flags: review?(iann_bugzilla)

Updated

7 years ago
Attachment #474666 - Flags: review?(iann_bugzilla) → review+

Comment 2

7 years ago
Looks like notifications will be the next thing I stop working on.
(Assignee)

Comment 3

7 years ago
(In reply to comment #2)
> Looks like notifications will be the next thing I stop working on.
ITYM
"Looks like doorhangers will be the next Firefox code I stop blindly copying"

Comment 4

7 years ago
IMHO, one of the largest failures of the SeaMonkey project is that we waste a lot of precious developer time by re-inventing wheels that Firefox has already invented by trying to skew them our way without changing any bit of functionality. If we used that time to actually make visible-to-the-user improvements, we'd not be running behind Firefox, but make a better and perhaps even innovative suite. But that doesn't seem to be the goal of some people around here.
(In reply to comment #3)
> (In reply to comment #2)
> > Looks like notifications will be the next thing I stop working on.
> ITYM
> "Looks like doorhangers will be the next Firefox code I stop blindly copying"

Heres the rub imo, we need to make sure the code we "copy" meshes with our general code-style (but can divert a bit imo) and meshes with API differences and alternatives we have, nicely.

Doing wheel-reinvention wastes time all around. And review is still useful, as it helps us catch potentially missed bugs in the Firefox implementation, and then BOTH projects can benefit.

Comment 6

7 years ago
In any case, I seriously hope you will backport this new approach to FF, in which case I'm probably happy with it to some extent (i.e. I'm happy as long as it's done in time for the 2.1 feature/beta1 freeze which is tentatively at the end of September).
If you're serious about this, you should do that backport in any case, because we're one Mozilla project after all, Firefox and us are friends, and there's no reasonable argument to differ in code for a thing like that.
If there's a really better way to do doorhanger, I'm sure gavin is happy to have it on their side as well.
What I want to prevent in any way possible is to run into a NIH syndrome of "Firefox always does bad code style and we are so much better" on things where we don't differ in any significant user-facing way. There are good reason for different to Firefox, but only where we can argument an actual user-facing difference. In all other cases, we should either swallow what they are doing or backport our improvements if they are such.
(Assignee)

Comment 7

7 years ago
Most of our current notifications are already handled by notification.xml in a completely different way to the way Firefox does it yet nobody complained then.

Comment 8

7 years ago
You're right, I should have complained and requested a backport back then already, but in the mean time my mindset changed a lot towards liking and respecting Firefox, and seeing the pros of backporting improvements much more. On one hand, way more people get to use better code, on the other, it's much easier to port further work and improvements back and forth. Next to that, if we improve Firefox, they respect us more and see us as a legitimate part of the community - and we can really need that.

Comment 9

7 years ago
(In reply to comment #6)
> What I want to prevent in any way possible is to run into a NIH syndrome of
> "Firefox always does bad code style and we are so much better" on things where
> we don't differ in any significant user-facing way.

Sorry, but that's totally nonsense. 
Porting stuff doesn't mean copying code around one may not even completely understand just because it runs. (That may work, but usually doesn't.)
If you're touching code anyway, it's a very cheap occasion to fix recognized weaknesses and shortcomings - a few minutes spent now will mean a lot of saved minutes later!
Furthermore, visibility to the user is not a valid argument here - even if there were _no_ visible clou that things are different in the backend, it's much better to have maintainable and readable source than burden future patchers with figuring out strange odds and ends...

> There are good reason for different to Firefox, but only where we can argument
> an actual user-facing difference.

That's just wrong. 
Especially you should know that we don't have the resources to alter code a gazillion times until it's straight - any minute spent on better code saves us wasting time later!

> In all other cases, we should either swallow what they are doing or
> backport our improvements if they are such.

Surely we should try to backport stuff, no doubt, but fixing us _and_ them may drain our resources more than necessary, occasionally...

Comment 10

7 years ago
(In reply to comment #9)
> If you're touching code anyway, it's a very cheap occasion to fix recognized
> weaknesses and shortcomings - a few minutes spent now will mean a lot of saved
> minutes later!

True, if I backport them to the "upstream" I have taken the code from.
 If I don't I'm guilty of abusing open source, not using it.

> Especially you should know that we don't have the resources to alter code a
> gazillion times until it's straight - any minute spent on better code saves us
> wasting time later!

Well, we are spending a ton of resources to alter code a gazillion times until our review thinks it's straight. If we seem to not even want to give back the improvements, I don't want to be part of that way of working, so I'll unassigned myself from any bugs where I did some work but we IMHO run afoul of that.

> Surely we should try to backport stuff, no doubt, but fixing us _and_ them may
> drain our resources more than necessary, occasionally...

Not more than we drain our resources already by thinking we need to rewrite every piece of code they have already written.

But thanks for at least admitting we should try to backport our changes.
(If I may, this more global discussion seems legitimate to have, but maybe not in this/a bug?)
Blocks: 570004
(Assignee)

Comment 12

7 years ago
Created attachment 476376 [details] [diff] [review]
Part 2: move the geolocation notification into notification.xml
[Checked in: See comment 16]
Attachment #476376 - Flags: review?(iann_bugzilla)
Comment on attachment 474666 [details] [diff] [review]
Part 1: move the rights notification into notification.xml
[Checked in: Comment 13]

http://hg.mozilla.org/comm-central/rev/bcc0a391a42b
Attachment #474666 - Attachment description: Part 1: move the rights notification into notification.xml → Part 1: move the rights notification into notification.xml [Checked in: Comment 13]

Comment 14

7 years ago
Comment on attachment 476376 [details] [diff] [review]
Part 2: move the geolocation notification into notification.xml
[Checked in: See comment 16]

Why the change from PRIORITY_INFO_HIGH to PRIORITY_INFO_LOW?
r=me with that addressed
Attachment #476376 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 15

7 years ago
(In reply to comment #14)
> (From update of attachment 476376 [details] [diff] [review])
> Why the change from PRIORITY_INFO_HIGH to PRIORITY_INFO_LOW?
Copy & paste error on my part, sorry.
Comment on attachment 476376 [details] [diff] [review]
Part 2: move the geolocation notification into notification.xml
[Checked in: See comment 16]

http://hg.mozilla.org/comm-central/rev/9f57d359479c
Attachment #476376 - Attachment description: Part 2: move the geolocation notification into notification.xml → Part 2: move the geolocation notification into notification.xml [Checked in: See comment 16]
(Assignee)

Comment 17

7 years ago
Created attachment 478252 [details] [diff] [review]
Part 3: Support geolocation notifications from files

This differs slightly from the approaches in other bugs:
This patch uses the host for a site, but the local file path for a file;
bug 570004 uses the host for a site, but the URI path for a file;
bug 580260 uses the spec throughout, with different wording for a file.
Attachment #478252 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 18

7 years ago
Created attachment 478409 [details] [diff] [review]
Part 3: Support geolocation notifications from files
[Checked in: See comment 20]

With string change accidentally omitted from previous patch.
Attachment #478252 - Attachment is obsolete: true
Attachment #478409 - Flags: review?(iann_bugzilla)
Attachment #478252 - Flags: review?(iann_bugzilla)

Comment 19

7 years ago
Comment on attachment 478409 [details] [diff] [review]
Part 3: Support geolocation notifications from files
[Checked in: See comment 20]

If we're using path and host, why are the variables not called that?
Either way can you be consistent, you use file and site in notification.xml and file and host in nsSuiteGlue.js
r=me with that addressed.
Attachment #478409 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 478409 [details] [diff] [review]
Part 3: Support geolocation notifications from files
[Checked in: See comment 20]

http://hg.mozilla.org/comm-central/rev/b30b6002d7f5
Attachment #478409 - Attachment description: Part 3: Support geolocation notifications from files → Part 3: Support geolocation notifications from files [Checked in: See comment 20]

Comment 21

7 years ago
(In reply to comment #20)
> Comment on attachment 478409 [details] [diff] [review]
> Part 3: Support geolocation notifications from files
> [Checked in: See comment 20]
> 
> http://hg.mozilla.org/comm-central/rev/b30b6002d7f5

Serge, please, if there is a conditional review, could you let the patch author make the corrections.

Comment 22

7 years ago
What I expected was either of:
file and site to be used in both notification.xml and nsSuiteGlue.js
path and host to be used in both notification.xml and nsSuiteGlue.js

Instead we got:
file and site used in notification.xml and path and host used in nsSuiteGlue.js

Comment 23

7 years ago
(In reply to comment #21)
> (In reply to comment #20)
> > Comment on attachment 478409 [details] [diff] [review] [details]
> > Part 3: Support geolocation notifications from files
> > [Checked in: See comment 20]
> > 
> > http://hg.mozilla.org/comm-central/rev/b30b6002d7f5
> 
> Serge, please, if there is a conditional review, could you let the patch author
> make the corrections.

Sorry, just looked at the pushlog and saw that Neil did the push and you did the update to the bug. Looks like I was not explicit enough in my review comment.
(Assignee)

Comment 24

7 years ago
(In reply to comment #22)
> What I expected was either of:
> file and site to be used in both notification.xml and nsSuiteGlue.js
> path and host to be used in both notification.xml and nsSuiteGlue.js
> 
> Instead we got:
> file and site used in notification.xml and path and host used in nsSuiteGlue.js
Sorry about that. The string names are "siteWantsToKnow" and "fileWantsToKnow" so I used site and file as the argument names. But the properties used are the URL's host and the file's path so I used host and path as the variable names.
(Assignee)

Comment 25

7 years ago
Created attachment 478648 [details] [diff] [review]
Part 4: implement geolocation doorhanger
[Checked in: See comment 38]

Actually most of this patch is identical to KaiRo's patch in bug 570004.
Attachment #478648 - Flags: review?(iann_bugzilla)

Comment 26

7 years ago
The patch misses a test. Could you please port the test from bug 570004 as well and care that it works?

Thanks a lot!

Comment 27

7 years ago
Comment on attachment 478648 [details] [diff] [review]
Part 4: implement geolocation doorhanger
[Checked in: See comment 38]

What is the reasoning behind having the lazy getter in navigator.js rather than nsBrowserStatusHandler.js?
As one of the patches on this bug please port browser_popupNotification.js from Firefox (as Robert suggests).
Is it expected that when you have a doorhanger showing that it uses the first click on an other window to dismiss the doorhanger and you have to click a second time to bring that other window up?
r=me with those points addressed
Attachment #478648 - Flags: review?(iann_bugzilla) → review+

Comment 28

7 years ago
(In reply to comment #25)
> Created attachment 478648 [details] [diff] [review]
> Part 4: implement geolocation doorhanger
> 
> Actually most of this patch is identical to KaiRo's patch in bug 570004.

Oh and the patch is bitrotted slightly in browser-prefs.js

Comment 29

7 years ago
(In reply to comment #27)
> What is the reasoning behind having the lazy getter in navigator.js rather than
> nsBrowserStatusHandler.js?

IIRC, to be able to have others use it as well and have all the getters for those things in one place.

> Is it expected that when you have a doorhanger showing that it uses the first
> click on an other window to dismiss the doorhanger and you have to click a
> second time to bring that other window up?

At least that matches the Firefox behavior.
(In reply to comment #29)
> (In reply to comment #27)
> > Is it expected that when you have a doorhanger showing that it uses the first
> > click on an other window to dismiss the doorhanger and you have to click a
> > second time to bring that other window up?
> 
> At least that matches the Firefox behavior.

To me that sounds like a bug, but I am ok with matching behavior/bug here for now
Comment on attachment 478648 [details] [diff] [review]
Part 4: implement geolocation doorhanger
[Checked in: See comment 38]

The patch misses adding geolocation-16.png and geolocation-64.png for the Mac theme. This lead to "RuntimeError: File "mac/communicator/icons/geolocation-16.png" not found in /builds/slave/comm-central-trunk-macosx/build/suite/themes/classic" on our MacOSX builders.

Updated

7 years ago
Blocks: 602150
(Assignee)

Comment 32

7 years ago
Created attachment 481165 [details] [diff] [review]
Part 4, l10n only (with fix for bug 602150)
[Checked in: Comment 42]

These are the portions of Part 4 that didn't land because of the l10n freeze.

The fix for bug 602150 affects this part of the patch too.
No longer blocks: 602150
Depends on: 602150

Updated

7 years ago
Depends on: 602538
(In reply to comment #32)
> Created attachment 481165 [details] [diff] [review]
> Part 4, l10n only (with fix for bug 602150)
> 
> These are the portions of Part 4 that didn't land because of the l10n freeze.

So what's the state of this bug in Beta 1, in layman's terms? I'm currently preparing an SMTT blog post and also want to be prepared to answer people's questions in forums/NGs.
(Assignee)

Comment 34

7 years ago
I think the status is that login manager will try to use doorhangers unless you disable them via the pref.
(Assignee)

Comment 35

7 years ago
Created attachment 482157 [details] [diff] [review]
Part 5: Support login manager prompter

The previous patch also makes the login manager try to use doorhangers.

Unfortunately some of our CSS needs to be tweaked to display this correctly.

I copied the mozapps/passwordmgr images to Modern from winstripe.

KaiRo, while writing this patch, I notice that Firefox doesn't appear to set the width and height on the notification icons. Any idea why? (I just copied your patch from bug 570004, so I thought you might have some insight...)
Attachment #482157 - Flags: review?(iann_bugzilla)
Attachment #482157 - Flags: feedback?(kairo)

Comment 36

7 years ago
(In reply to comment #35)
> Created attachment 482157 [details] [diff] [review]
> Part 5: Support login manager prompter
> 
> The previous patch also makes the login manager try to use doorhangers.
> 
> Unfortunately some of our CSS needs to be tweaked to display this correctly.
> 
> I copied the mozapps/passwordmgr images to Modern from winstripe.
> 
> KaiRo, while writing this patch, I notice that Firefox doesn't appear to set
> the width and height on the notification icons. Any idea why? (I just copied
> your patch from bug 570004, so I thought you might have some insight...)

What about for mac too?
(Assignee)

Comment 37

7 years ago
Created attachment 482164 [details] [diff] [review]
Part 5: Support login manager prompter
[Checked in: Comment 44]

With Mac changes.

[My question to KaiRo still stands, but I notice he's CCed anyway.]
Attachment #482157 - Attachment is obsolete: true
Attachment #482164 - Flags: review?(iann_bugzilla)
Attachment #482157 - Flags: review?(iann_bugzilla)
Attachment #482157 - Flags: feedback?(kairo)
Depends on: 603228
Comment on attachment 478648 [details] [diff] [review]
Part 4: implement geolocation doorhanger
[Checked in: See comment 38]

http://hg.mozilla.org/comm-central/rev/c55ed62b37cd
Part 4: implement geolocation doorhanger
+
http://hg.mozilla.org/comm-central/rev/95784e294722
Part 4: missing Mac icons
+
http://hg.mozilla.org/comm-central/rev/2342338033d4
Part 4: fix test failures caused by using reserved attribute name
+
http://hg.mozilla.org/comm-central/rev/aa87a683cf1d
Partial backout of changeset c55ed62b37cd due to l10n freeze
Attachment #478648 - Attachment description: Part 4: implement geolocation doorhanger → Part 4: implement geolocation doorhanger [Checked in: See comment 38]
Comment on attachment 481165 [details] [diff] [review]
Part 4, l10n only (with fix for bug 602150)
[Checked in: Comment 42]

http://hg.mozilla.org/comm-central/rev/317093217f10
Part 4: reland geolocation doorhanger implementation
Attachment #481165 - Attachment description: Part 4, l10n only (with fix for bug 602150) → Part 4, l10n only (with fix for bug 602150) [Checked in: Comment 39]
(In reply to comment #39)
> http://hg.mozilla.org/comm-central/rev/317093217f10
> Part 4: reland geolocation doorhanger implementation

Causing test fails in http://mxr.mozilla.org/comm-central/source/mozilla/content/html/document/test/test_bug369370.html?force=1

I don't see anything at a glance in the patchset to cause this, but helpful:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebProgress.addProgressListener]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://communicator/content/bindings/notification.xml :: addProgressListener :: line 67"  data: no]

Appears twice when the test runs (once for each launched SeaMonkey window)

JS frame :: chrome://communicator/content/bindings/notification.xml :: addProgressListener :: line 71
Source File: chrome://communicator/content/bindings/notification.xml
Line: 73

JS frame :: chrome://communicator/content/bindings/notification.xml ::  :: line 491
Source File: chrome://communicator/content/bindings/notification.xml
Line: 73

From manual frame inspection

I'm backing this out due to the test failures, [sorry].

Neil, can we (please) split out remaining work to new bugs so we can properly track this landing.

Serge filed a bug for this error, not knowing it was causing test failures at the time though, lets track this actual fix [and relanding of this patch] in Bug 603228 because of that, and file new bugs for additional work here...
(In reply to comment #40)
> I'm backing this out due to the test failures, [sorry].

Done: http://hg.mozilla.org/comm-central/rev/8b5776bb0c0e
Attachment #481165 - Attachment description: Part 4, l10n only (with fix for bug 602150) [Checked in: Comment 39] → Part 4, l10n only (with fix for bug 602150) [Backed out: Comment 41]
Comment on attachment 481165 [details] [diff] [review]
Part 4, l10n only (with fix for bug 602150)
[Checked in: Comment 42]

http://hg.mozilla.org/comm-central/rev/d7fd18b29794
Attachment #481165 - Attachment description: Part 4, l10n only (with fix for bug 602150) [Backed out: Comment 41] → Part 4, l10n only (with fix for bug 602150) [Backed out: Comment 42]
Attachment #481165 - Attachment description: Part 4, l10n only (with fix for bug 602150) [Backed out: Comment 42] → Part 4, l10n only (with fix for bug 602150) [Checked in: Comment 42]

Comment 43

7 years ago
Comment on attachment 482164 [details] [diff] [review]
Part 5: Support login manager prompter
[Checked in: Comment 44]

r=me
Really need to document in help how to dismiss and bring back the door hanger as well as any changes that are needed to help due to the change to using door hangers. If it's not going to be in this bug could you open a new one for it. Thanks.
Attachment #482164 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Updated

7 years ago
Blocks: 605755
Comment on attachment 482164 [details] [diff] [review]
Part 5: Support login manager prompter
[Checked in: Comment 44]

http://hg.mozilla.org/comm-central/rev/8033e2000e5d
Attachment #482164 - Attachment description: Part 5: Support login manager prompter → Part 5: Support login manager prompter [Checked in: Comment 44]
(Assignee)

Updated

7 years ago
Duplicate of this bug: 595155
(Assignee)

Comment 46

7 years ago
Created attachment 485616 [details] [diff] [review]
Part 6a: Drop local copy of toEM
[Checked in: Comment 53]

toEM was just too complicated for me to bother duplicating it, so I just look to see if it's been declared in the window scope and use that instead. Part 6c will also want to open the addons manager so this makes it easier to be consistent.
Attachment #485616 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 47

7 years ago
Created attachment 485617 [details] [diff] [review]
Part 6b: Break out addonInstallBlocked into its own method
[Checked in: Comment 54]

This will allow doorhangers to override it in part 6d.
Attachment #485617 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 48

7 years ago
Created attachment 485618 [details] [diff] [review]
Part 6c: Add support for completed and failed addon installation notifications
[Checked in: Comment 55]
Attachment #485618 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 49

7 years ago
Comment on attachment 485616 [details] [diff] [review]
Part 6a: Drop local copy of toEM
[Checked in: Comment 53]

>-          function managePlugins(aEvent){
...
>+            event.target.addEventListener("click", function() {
Oops, the aEvent parameter got lost :-(

Comment 50

7 years ago
Comment on attachment 485616 [details] [diff] [review]
Part 6a: Drop local copy of toEM
[Checked in: Comment 53]

r=me with aEvent issue fixed
Attachment #485616 - Flags: review?(iann_bugzilla) → review+

Comment 51

7 years ago
Comment on attachment 485617 [details] [diff] [review]
Part 6b: Break out addonInstallBlocked into its own method
[Checked in: Comment 54]

>+++ b/suite/common/bindings/notification.xml
>@@ -456,16 +407,72 @@
>+      <method name="addonInstallBlocked">
>+        <parameter name="installInfo"/>
>+        <body>
>+          <![CDATA[
>+            var notificationName, messageString, buttons, host;
As you have moved where host is used, could move the declaration of that variable to there too (just before the try).
>+
>+            if (!this._prefs.getBoolPref("xpinstall.enabled")) {
>+              notificationName = "xpinstall-disabled";
>+              if (this._prefs.prefIsLocked("xpinstall.enabled")) {
>+                messageString = this._stringBundle.GetStringFromName("xpinstallDisabledMessageLocked");
>+                buttons = [];
>+              } else {
>+                var prefBranch = this._prefs;
Also, could the declaration of prefBranch be moved earlier so it can be used instead of this._prefs in the two if statements?

r=me with those points addressed/commented on
Attachment #485617 - Flags: review?(iann_bugzilla) → review+

Comment 52

7 years ago
Comment on attachment 485618 [details] [diff] [review]
Part 6c: Add support for completed and failed addon installation notifications
[Checked in: Comment 55]

>+++ b/suite/common/bindings/notification.xml

>+      <method name="addonInstallFailed">
>+        <parameter name="installInfo"/>
>+        <body>
>+          <![CDATA[
>+            var notificationName = "addon-error";
>+            if (!this.getNotificationWithValue(notificationName)) {
>+              var messageString, host;
You declare messageString later on so no need to here.
r=me with that addressed.
Attachment #485618 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 485616 [details] [diff] [review]
Part 6a: Drop local copy of toEM
[Checked in: Comment 53]

http://hg.mozilla.org/comm-central/rev/85a9c52f1c35
Attachment #485616 - Attachment description: Part 6a: Drop local copy of toEM → Part 6a: Drop local copy of toEM [Checked in: Comment 53]
Comment on attachment 485617 [details] [diff] [review]
Part 6b: Break out addonInstallBlocked into its own method
[Checked in: Comment 54]

http://hg.mozilla.org/comm-central/rev/4418b324bd58
Attachment #485617 - Attachment description: Part 6b: Break out addonInstallBlocked into its own method → Part 6b: Break out addonInstallBlocked into its own method [Checked in: Comment 54]
Comment on attachment 485618 [details] [diff] [review]
Part 6c: Add support for completed and failed addon installation notifications
[Checked in: Comment 55]

http://hg.mozilla.org/comm-central/rev/fad2356f3cf8
Attachment #485618 - Attachment description: Part 6c: Add support for completed and failed addon installation notifications → Part 6c: Add support for completed and failed addon installation notifications [Checked in: Comment 55]
Blocks: 608516
Neil, are you planning more work here or is this bug fixed now?
(Assignee)

Comment 57

7 years ago
Well, I need to write part 6d at the very least.
(Assignee)

Comment 58

7 years ago
Created attachment 491978 [details] [diff] [review]
Part 6d: implement addon doorhanger

Note that currently neither the notificationbar nor the doorhanger are triggered if xpinstall.enabled has been set to false because of bug 613385.
Attachment #491978 - Flags: review?(iann_bugzilla)
> Note that currently neither the notificationbar nor the doorhanger are
> triggered if xpinstall.enabled has been set to false because of bug 613385.

I guess your patch needs to be updated after that bug now.
Depends on: 613385
(Assignee)

Comment 60

7 years ago
(In reply to comment #59)
> > Note that currently neither the notificationbar nor the doorhanger are
> > triggered if xpinstall.enabled has been set to false because of bug 613385.
> I guess your patch needs to be updated after that bug now.
No, the patch has all the right code, but the AddonsManager wasn't calling it.
(Assignee)

Comment 61

7 years ago
Sorry, I hadn't realised he ended up changing the API.
(Assignee)

Comment 62

7 years ago
Created attachment 493220 [details] [diff] [review]
Part 6d: Add support for disabled addon install notifications
[Checked in: Comment 72]
Attachment #491978 - Attachment is obsolete: true
Attachment #493220 - Flags: review?(iann_bugzilla)
Attachment #491978 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 63

7 years ago
Created attachment 493221 [details] [diff] [review]
-w diff of the interesting bits
Comment on attachment 493220 [details] [diff] [review]
Part 6d: Add support for disabled addon install notifications
[Checked in: Comment 72]

Just to be explicit, is it intended that this patch contains notification.xml only, and not the other css+xul?
(Assignee)

Comment 65

7 years ago
(In reply to comment #64)
> (From update of attachment 493220 [details] [diff] [review])
> Just to be explicit, is it intended that this patch contains notification.xml
> only, and not the other css+xul?
No doorhangers are involved here. I'll update the doorhanger patch separately.
(In reply to comment #65)
> No doorhangers are involved here. I'll update the doorhanger patch separately.

'Part 6d' in new patch name confused me then ;->
(Assignee)

Comment 67

7 years ago
Had I known in advance, it would have been part of 6b, so maybe Part 6b++ ;-)
(Assignee)

Comment 68

7 years ago
Created attachment 493272 [details] [diff] [review]
Part 6e: implement addon doorhanger
[Checked in: Comment 73]
Attachment #493272 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 69

7 years ago
Comment on attachment 493221 [details] [diff] [review]
-w diff of the interesting bits

>-              notificationName = "xpinstall-disabled";
>+            var notificationName = "xpinstall-disabled";
Hmm. Should I rename this to "addon-install-disabled" too?

Updated

7 years ago
Attachment #493220 - Flags: review?(iann_bugzilla) → review+

Comment 70

7 years ago
(In reply to comment #69)
> Comment on attachment 493221 [details] [diff] [review]
> -w diff of the interesting bits
> 
> >-              notificationName = "xpinstall-disabled";
> >+            var notificationName = "xpinstall-disabled";
> Hmm. Should I rename this to "addon-install-disabled" too?

Probably best so that it matches the rest.

Updated

7 years ago
Attachment #493272 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 71

7 years ago
Created attachment 499296 [details] [diff] [review]
Part 7: move the update checking notification into notification.xml
[Checked in: See comment 80]

KaiRo actually said that some of these notifications don't need to be converted to doorhangers. But I think they look better in notification.xml anyway.
Attachment #499296 - Flags: review?(iann_bugzilla)
Comment on attachment 493220 [details] [diff] [review]
Part 6d: Add support for disabled addon install notifications
[Checked in: Comment 72]

http://hg.mozilla.org/comm-central/rev/b78db221b0b1
Attachment #493220 - Attachment description: Part 6d: Add support for disabled addon install notifications → Part 6d: Add support for disabled addon install notifications [Checked in: Comment 72]
Comment on attachment 493272 [details] [diff] [review]
Part 6e: implement addon doorhanger
[Checked in: Comment 73]

http://hg.mozilla.org/comm-central/rev/e53cca758048
Attachment #493272 - Attachment description: Part 6e: implement addon doorhanger → Part 6e: implement addon doorhanger [Checked in: Comment 73]
Attachment #493221 - Attachment is obsolete: true
(Assignee)

Comment 74

7 years ago
Created attachment 499576 [details] [diff] [review]
Part 8a: add support for indexedDB notifications
[Checked in: See comment 81]
Attachment #499576 - Flags: review?(iann_bugzilla)
Blocks: 595024

Comment 75

7 years ago
Toolkit Bug 610130 - Doorhanger notifications confused when switching between tabs with different types of doorhangers
Depends on: 610130

Comment 76

7 years ago
Comment on attachment 499296 [details] [diff] [review]
Part 7: move the update checking notification into notification.xml
[Checked in: See comment 80]

>+++ b/suite/common/bindings/notification.xml	Wed Dec 22 15:58:44 2010 +0000
>+            box.persistence = -1; // Until user closes it
Nit: Comment should end with a full stop if it starts with a capital letter.

>+++ b/suite/common/src/nsSuiteGlue.js	Wed Dec 22 15:58:44 2010 +0000
>@@ -247,7 +247,7 @@ SuiteGlue.prototype = {
>     }
>     // Detect if updates are off and warn for outdated builds.
>     if (this._shouldShowUpdateWarning())
>-      this._showUpdateWarning(aWindow);
>+      aWindow.getBrowser().getNotificationBox().showUpdateWarning();
If at the beginning of the _onBrowserStartup function you do something like:
var notifyBox = aWindow.getBrowser().getNotificationBox();

You can then use:
this._showRightsNotification(notifyBox);

and simplify the _showRightsNotification function.

It can be also used for when _showPlacesLockedNotificationBox is moved and for this move too.

r=me with that fixed/addressed
Attachment #499296 - Flags: review?(iann_bugzilla) → review+

Comment 77

7 years ago
Comment on attachment 499576 [details] [diff] [review]
Part 8a: add support for indexedDB notifications
[Checked in: See comment 81]

>+++ b/suite/browser/navigator.js
>@@ -519,16 +519,18 @@ function Startup()
>     XPCOMUtils.defineLazyGetter(window, "PopupNotifications", function() {
>       var tmp = {};
>       Components.utils.import("resource://gre/modules/PopupNotifications.jsm", tmp);
>       return XULBrowserWindow.popupNotifications = new tmp.PopupNotifications(
>           getBrowser(),
>           document.getElementById("notification-popup"),
>           document.getElementById("notification-popup-box"));
>     });
>+    // This calls the constructor again, so we have to destroy it manually.
>+    gBrowser.getNotificationBox().destroy();
>     gBrowser.setAttribute("popupnotification", "true");
>     // This resets popup window scrollbar visibility, so override it.
It would be useful for both the comments to be more explicit as to what "This" is.

>+++ b/suite/locales/en-US/chrome/common/notification.properties
>+# IndexedDB
>+offlineApps.permissions=This website (%S) is asking to store data on your computer for offline use.
>+offlineApps.quota=This website (%1$S) is attempting to store more than %2$SMB of data on your computer for offline use.

Is it always going to be a website? We don't seem to be very consistent (compare with geolocation)

r=me with that fixed/addressed.
Attachment #499576 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 78

6 years ago
(In reply to comment #77)
> >+offlineApps.permissions=This website (%S) is asking to store data on your computer for offline use.
> >+offlineApps.quota=This website (%1$S) is attempting to store more than %2$SMB of data on your computer for offline use.
> 
> Is it always going to be a website? We don't seem to be very consistent
> (compare with geolocation)

Yes, indexedDB only works on a website, while geolocation supports files (although it prompts every time, you can't save geolocation permission.)

Comment 79

6 years ago
(In reply to comment #78)
> (In reply to comment #77)
> > >+offlineApps.permissions=This website (%S) is asking to store data on your computer for offline use.
> > >+offlineApps.quota=This website (%1$S) is attempting to store more than %2$SMB of data on your computer for offline use.
> > 
> > Is it always going to be a website? We don't seem to be very consistent
> > (compare with geolocation)
> 
> Yes, indexedDB only works on a website, while geolocation supports files
> (although it prompts every time, you can't save geolocation permission.)

What I meant was that geolocation has:
geolocation.siteWantsToKnow=%S wants to know your location.
geolocation.fileWantsToKnow=The file %S wants to know your location.

So should they be something like:
geolocation.siteWantsToKnow=This website (%S) wants to know your location.
geolocation.fileWantsToKnow=This file (%S) wants to know your location.
Comment on attachment 499296 [details] [diff] [review]
Part 7: move the update checking notification into notification.xml
[Checked in: See comment 80]

http://hg.mozilla.org/comm-central/rev/59fc64bec3a9
Attachment #499296 - Attachment description: Part 7: move the update checking notification into notification.xml → Part 7: move the update checking notification into notification.xml [Checked in: See comment 80]
Comment on attachment 499576 [details] [diff] [review]
Part 8a: add support for indexedDB notifications
[Checked in: See comment 81]

http://hg.mozilla.org/comm-central/rev/c62bebd4807b
Attachment #499576 - Attachment description: Part 8a: add support for indexedDB notifications → Part 8a: add support for indexedDB notifications [Checked in: Comment 81]
Attachment #499576 - Attachment description: Part 8a: add support for indexedDB notifications [Checked in: Comment 81] → Part 8a: add support for indexedDB notifications [Checked in: See comment 81]
Neil, would you port bug 570012 ("addon-install-started") + bug 623192 here too? (or in a separate bug?)
(Assignee)

Comment 83

6 years ago
(In reply to comment #82)
> Neil, would you port bug 570012 ("addon-install-started") + bug 623192 here
> too? (or in a separate bug?)

I probably need to do it here, as it's a two-step process.

(In reply to comment #79)
> >+offlineApps.permissions=This website (%S) is asking to store data on your computer for offline use.
> >+offlineApps.quota=This website (%1$S) is attempting to store more than %2$SMB of data on your computer for offline use.
> 
> Is it always going to be a website? We don't seem to be very consistent
> (compare with geolocation)
> 
> What I meant was that geolocation has:
> geolocation.siteWantsToKnow=%S wants to know your location.
> geolocation.fileWantsToKnow=The file %S wants to know your location.
> 
> So should they be something like:
> geolocation.siteWantsToKnow=This website (%S) wants to know your location.
> geolocation.fileWantsToKnow=This file (%S) wants to know your location.

KaiRo, can we just change the strings in en-US and notify the l10n community, seeing as we're just changing the appearance rather than the meaning?
Depends on: 570012
Depends on: 623192
(Assignee)

Comment 84

6 years ago
Created attachment 503131 [details] [diff] [review]
Part 9: move the places locked notification into notification.xml
[Checked in: Comment 95]

I notice that we don't actually seem to have help for this button. (Moving it to notification.xml makes it possible to trigger manually, of course; I can't seem to trigger it for real!) KaiRo, is there already a bug on file for that?
Attachment #503131 - Flags: review?(iann_bugzilla)

Comment 85

6 years ago
(In reply to comment #83)
> (In reply to comment #82)
> > Neil, would you port bug 570012 ("addon-install-started") + bug 623192 here
> > too? (or in a separate bug?)
> 
> I probably need to do it here, as it's a two-step process.

Gah I'd think even everything that is in here should have been split across different bugs, but wth...

> (In reply to comment #79)
> KaiRo, can we just change the strings in en-US and notify the l10n community,
> seeing as we're just changing the appearance rather than the meaning?

It's not too god, but it could work. Note that I think "This website (%S)" is bad phrasing altogether, just using "%S" instead should be enough, IMHO.
(Assignee)

Comment 86

6 years ago
(In reply to comment #85)
> (In reply to comment #83)
> > (In reply to comment #79)
> > KaiRo, can we just change the strings in en-US and notify the l10n community,
> > seeing as we're just changing the appearance rather than the meaning?
> It's not too god, but it could work. Note that I think "This website (%S)" is
> bad phrasing altogether, just using "%S" instead should be enough, IMHO.
We also already use "this website (%S)" in the xpinstall blocked notification.
(Assignee)

Comment 87

6 years ago
Created attachment 503674 [details] [diff] [review]
Part 8b: add more support for indexedDB notifications
[Checked in: Comment 96]

Some more stuff landed in Firefox since I wrote the previous patch:
* Option to defer the question without setting a particular permission
* Request automatically times out after 5 minutes (think background tab)
* Allow the back end to cancel the quota request
Attachment #503674 - Flags: review?(iann_bugzilla)

Comment 88

6 years ago
Bug 615315 should be ported in here or as a followup as well, just for your notice. It's a simple change, but apparently one that makes things easier for users to understand.
(Assignee)

Comment 89

6 years ago
(In reply to comment #88)
> Bug 615315 should be ported in here or as a followup as well, just for your
> notice. It's a simple change, but apparently one that makes things easier for
> users to understand.

By my reading of the patch it's a base binding change so we should automatically pick it up.

Comment 90

6 years ago
(In reply to comment #89)
> (In reply to comment #88)
> > Bug 615315 should be ported in here or as a followup as well, just for your
> > notice. It's a simple change, but apparently one that makes things easier for
> > users to understand.
> 
> By my reading of the patch it's a base binding change so we should
> automatically pick it up.

I just saw in the changeset it had browser/ changes as well, something in a binding and some test addition that probably would be good to port as well (I hope we added tests for the other notifications as well, but this is in the basic one I'm pretty sure you added).
(Assignee)

Comment 91

6 years ago
(In reply to comment #90)
> (In reply to comment #89)
> > (In reply to comment #88)
> > > Bug 615315 should be ported in here or as a followup as well, just for your
> > > notice. It's a simple change, but apparently one that makes things easier for
> > > users to understand.
> > By my reading of the patch it's a base binding change so we should
> > automatically pick it up.
> I just saw in the changeset it had browser/ changes as well
That's probably part of bug 570012 in that case.

> (I hope we added tests for the other notifications as well
No, but I'll file a helpwanted bug if you like.

Comment 92

6 years ago
(In reply to comment #91)
> (In reply to comment #90)
> > (In reply to comment #89)
> > > (In reply to comment #88)
> > > > Bug 615315 should be ported in here or as a followup as well, just for your
> > > > notice. It's a simple change, but apparently one that makes things easier for
> > > > users to understand.
> > > By my reading of the patch it's a base binding change so we should
> > > automatically pick it up.
> > I just saw in the changeset it had browser/ changes as well
> That's probably part of bug 570012 in that case.

Well, just look into http://hg.mozilla.org/mozilla-central/rev/bdadb18a5adf yourself, that's what landed for bug 615315.

> > (I hope we added tests for the other notifications as well
> No, but I'll file a helpwanted bug if you like.

At least that would be good.

(In reply to comment #84)
> Created attachment 503131 [details] [diff] [review]
> Part 9: move the places locked notification into notification.xml
> 
> I notice that we don't actually seem to have help for this button. (Moving it
> to notification.xml makes it possible to trigger manually, of course; I can't
> seem to trigger it for real!) KaiRo, is there already a bug on file for that?

I think I filed one, yes.

Comment 93

6 years ago
Comment on attachment 503131 [details] [diff] [review]
Part 9: move the places locked notification into notification.xml
[Checked in: Comment 95]

Didn't apply cleanly to nsSuiteGlue.js due to the changes to the preceding comment, but once corrected was fine.
r=me
Attachment #503131 - Flags: review?(iann_bugzilla) → review+

Comment 94

6 years ago
Comment on attachment 503674 [details] [diff] [review]
Part 8b: add more support for indexedDB notifications
[Checked in: Comment 96]

r=me
Attachment #503674 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 503131 [details] [diff] [review]
Part 9: move the places locked notification into notification.xml
[Checked in: Comment 95]

http://hg.mozilla.org/comm-central/rev/9420f64e395b
Attachment #503131 - Attachment description: Part 9: move the places locked notification into notification.xml → Part 9: move the places locked notification into notification.xml [Checked in: Comment 95]
Comment on attachment 503674 [details] [diff] [review]
Part 8b: add more support for indexedDB notifications
[Checked in: Comment 96]

http://hg.mozilla.org/comm-central/rev/034b1563e3d9
Attachment #503674 - Attachment description: Part 8b: add more support for indexedDB notifications → Part 8b: add more support for indexedDB notifications [Checked in: Comment 96]
Depends on: 617553
(Assignee)

Comment 97

6 years ago
Created attachment 505266 [details] [diff] [review]
Part 10: support default notification icon
[Checked in: Comment 102]

This landed in Firefox as part of bug 617553, although I'm not sure whether they actually use it, so I test it by opening the Error Console and evaluating

> top.opener.PopupNotifications.show(top.opener.getBrowser().selectedBrowser, "test-default", "Test of the default notification icon");

I also tweaked Modern to improve the text display, c.f. bug 615481.
Attachment #505266 - Flags: review?(iann_bugzilla)

Comment 98

6 years ago
Created attachment 507645 [details] [diff] [review]
Revised tests v0.1

This patch ports all the changes to the tests across from mozilla/browser/base/content/test's version of browser_popupNotification.js
It does fail on test 19 though with:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | notification anchored to tab - Got , expected tabbrowser-tab
I'm not sure if that is due to any difference's between implementations or something else though
Attachment #507645 - Flags: review?(neil)

Updated

6 years ago
Attachment #505266 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 99

6 years ago
Created attachment 507708 [details] [diff] [review]
Part 8c: implement indexedDB doorhanger
[Checked in: Comment 112]

Also:
* Fixed a bug where quota requests didn't work. ("usage" is correct usage!)
* Corrected the notification name for indexedDB notifications.
* Added a missing icon for password change doorhangers.
* Tweaked the Modern doorhanger background to work with the question icon.
Note that you cannot cancel the indexedDB doorhanger; you can dismiss it and then wait for it to auto-cancel after 30 seconds.
Attachment #507708 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 100

6 years ago
(In reply to comment #98)
> It does fail on test 19 though with:
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js
> | notification anchored to tab - Got , expected tabbrowser-tab
> I'm not sure if that is due to any difference's between implementations or
> something else though
Odd; an ad hoc test I did locally returned tabbrowser-tab as expected.
(In reply to comment #100)
> (In reply to comment #98)
> > It does fail on test 19 though with:
> Odd; an ad hoc test I did locally returned tabbrowser-tab as expected.

Ian, had you applied part 10 patch? (Because this test part is from bug 617553.)
Comment on attachment 505266 [details] [diff] [review]
Part 10: support default notification icon
[Checked in: Comment 102]

http://hg.mozilla.org/comm-central/rev/864643d7a758
Attachment #505266 - Attachment description: Part 10: support default notification icon → Part 10: support default notification icon [Checked in: Comment 102]
Comment on attachment 507645 [details] [diff] [review]
Revised tests v0.1

Hopefully, this will fix (most of)
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1296201988.1296203807.31400.gz
OS X 10.6 comm-central-trunk debug test mochitest-other on 2011/01/28 00:06:28
{
mochitest-browser-chrome: 10865/127/8
}
(Assignee)

Comment 104

6 years ago
(In reply to comment #98)
> This patch ports all the changes to the tests across from
> mozilla/browser/base/content/test's version of browser_popupNotification.js
> It does fail on test 19 though with:
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js
> | notification anchored to tab - Got , expected tabbrowser-tab
> I'm not sure if that is due to any difference's between implementations or
> something else though
The issue here is that Firefox goes pseudo-fullscreen when about:addons is loaded, which we don't do. We could adapt that test, but we'd have to use an alternate method of temporarily hiding the navigation bar.
(Assignee)

Comment 105

6 years ago
Comment on attachment 507645 [details] [diff] [review]
Revised tests v0.1

>       this.notifyObj = new basicNotification(),
Any chance you could fix these while you're here^

>+  // Test notification when chrome is hidden
r- on this test only. All the other tests are fine.
Attachment #507645 - Flags: review?(neil) → review-

Comment 106

6 years ago
(In reply to comment #104)
> The issue here is that Firefox goes pseudo-fullscreen when about:addons is
> loaded, which we don't do. We could adapt that test, but we'd have to use an
> alternate method of temporarily hiding the navigation bar.

We could trigger the toolbargrippy, i.e. collapse the toolbar. ;-)
(Assignee)

Comment 107

6 years ago
Actually there are at least four ways of doing it...

Updated

6 years ago
Attachment #507708 - Flags: review?(iann_bugzilla) → review+

Comment 108

6 years ago
Created attachment 508179 [details] [diff] [review]
Fixed tests v0.2 [Checked in: Comment 110]

Changes since first version of revised tests:
* Fixed the (), issues.
* Removed test 19 as we don't currently hide the chrome, if/when we do, the test can be added.
Attachment #507645 - Attachment is obsolete: true
Attachment #508179 - Flags: review?(neil)
(In reply to comment #108)
> * Removed test 19 as we don't currently hide the chrome, if/when we do, the
> test can be added.

Maybe just comment it out, with a comment.
(Assignee)

Updated

6 years ago
Attachment #508179 - Flags: review?(neil) → review+

Comment 110

6 years ago
Comment on attachment 508179 [details] [diff] [review]
Fixed tests v0.2 [Checked in: Comment 110]

http://hg.mozilla.org/comm-central/rev/3f017c3e9f46
Attachment #508179 - Attachment description: Fixed tests v0.2 → Fixed tests v0.2 [Checked in: Comment 110]
(In reply to comment #103)
> Hopefully, this will fix (most of)
> mochitest-browser-chrome: 10865/127/8

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1296435270.1296437024.12735.gz
OS X 10.6 comm-central-trunk debug test mochitest-other on 2011/01/30 16:54:30
{
mochitest-browser-chrome: 11122/2/8
}
:-)
Comment on attachment 507708 [details] [diff] [review]
Part 8c: implement indexedDB doorhanger
[Checked in: Comment 112]

http://hg.mozilla.org/comm-central/rev/559827cc8d53
Attachment #507708 - Attachment description: Part 8c: implement indexedDB doorhanger → Part 8c: implement indexedDB doorhanger [Checked in: Comment 112]
Whiteboard: [doorhanger]
Depends on: 630092
Depends on: 521159
(Assignee)

Comment 113

6 years ago
Created attachment 512062 [details] [diff] [review]
Part 6f: Add support for addon download progress notifications
[Checked in: Comment 117]
Attachment #512062 - Flags: review?(iann_bugzilla)

Comment 114

6 years ago
(In reply to comment #113)
> Created attachment 512062 [details] [diff] [review]
> Part 6f: Add support for addon download progress notifications

I get the following in the error console with this patch applied (and strict javascript errors enabled) and I download an add-on:
Warning: reference to undefined property installInfo.installs
Source File: chrome://communicator/content/bindings/notification.xml
Line: 921
Warning: reference to undefined property installInfo.installs
Source File: chrome://communicator/content/bindings/notification.xml
Line: 930
Warning: reference to undefined property installInfo.installs
Source File: chrome://communicator/content/bindings/notification.xml
Line: 942
Warning: reference to undefined property this.installInfo.installs
Source File: chrome://communicator/content/bindings/notification.xml
Line: 1619

As I am prompted through a doorhanger with if I want to "Install Software" or "Not Now" before I start the download, should I still be getting the "Software Installation" dialog after the download has completed with "Cancel" and "Install Now" buttons?
(Assignee)

Comment 115

6 years ago
(In reply to comment #114)
> I get the following in the error console with this patch applied (and strict
> javascript errors enabled) and I download an add-on:
> Warning: reference to undefined property installInfo.installs
> Source File: chrome://communicator/content/bindings/notification.xml
> Line: 921
> Warning: reference to undefined property installInfo.installs
> Source File: chrome://communicator/content/bindings/notification.xml
> Line: 930
> Warning: reference to undefined property installInfo.installs
> Source File: chrome://communicator/content/bindings/notification.xml
> Line: 942
> Warning: reference to undefined property this.installInfo.installs
> Source File: chrome://communicator/content/bindings/notification.xml
> Line: 1619
So do I. The strange part is that installInfo.installs is an array, so it's very much defined... might be a bug in tracemonkey or something.

> As I am prompted through a doorhanger with if I want to "Install Software" or
> "Not Now" before I start the download, should I still be getting the "Software
> Installation" dialog after the download has completed with "Cancel" and
> "Install Now" buttons?
The first prompt is the permission prompt, you don't get that on whitelisted sites. You always get the Software Installation dialog though.

Updated

6 years ago
Attachment #512062 - Flags: review?(iann_bugzilla) → review+
Please, check whether bug 633218 (wrt 'addon-install-complete') needs to be ported.
Comment on attachment 512062 [details] [diff] [review]
Part 6f: Add support for addon download progress notifications
[Checked in: Comment 117]

http://hg.mozilla.org/comm-central/rev/edc24697f35e
Attachment #512062 - Attachment description: Part 6f: Add support for addon download progress notifications → Part 6f: Add support for addon download progress notifications [Checked in: Comment 117]
(Assignee)

Comment 118

6 years ago
(In reply to comment #116)
> Please, check whether bug 633218 (wrt 'addon-install-complete') needs to be
> ported.
I'm not sure. If it did, it would need a separate bug.
Comment on attachment 512062 [details] [diff] [review]
Part 6f: Add support for addon download progress notifications
[Checked in: Comment 117]

>diff --git a/suite/locales/en-US/chrome/common/notification.properties b/suite/locales/en-US/chrome/common/notification.properties
>--- a/suite/locales/en-US/chrome/common/notification.properties
>+++ b/suite/locales/en-US/chrome/common/notification.properties
>@@ -17,16 +17,23 @@ xpinstallHostNotAvailable=unknown host
> xpinstallPromptWarning=%S prevented this website (%S) from asking you to install software on your computer.
> xpinstallPromptInstallButton=Install Software…
> xpinstallPromptInstallButton.accesskey=I
> xpinstallDisabledMessageLocked=Software installation has been disabled by your system administrator.
> xpinstallDisabledMessage=Software installation is currently disabled. Click Enable and try again.
> xpinstallDisabledButton=Enable
> xpinstallDisabledButton.accesskey=n
> 
>+addonDownloading=Add-on downloading:;Add-ons downloading:
>+addonDownloadCancelled=Add-on download cancelled.; Add-on downloads cancelled.
>+addonDownloadCancelButton=Cancel
>+addonDownloadCancelButton.accesskey=C
>+addonDownloadRestartButton=Restart
>+addonDownloadRestartButton.accesskey=R
>+

A L10n note here please.
(Assignee)

Comment 120

6 years ago
(In reply to comment #119)
> A L10n note here please.
Sorry about that. I don't know why I didn't just copy and paste the block.
I'll roll it into the next patch, something like this:
# LOCALIZATION NOTE (addonDownloading, addonDownloadCancelled):
# Semi-colon list of plural forms. See:
# http://developer.mozilla.org/en/Localization_and_Plurals
# The number of add-ons is not itself substituted in the string.
# See https://bugzilla.mozilla.org/show_bug.cgi?id=570012 for mockups
(Assignee)

Comment 121

6 years ago
Created attachment 513890 [details] [diff] [review]
Part 6g: Add rest of addon doorhanger

Also sporting a couple of minor fixes to the existing code.
Attachment #513890 - Flags: review?(iann_bugzilla)
Blocks: 635825

Comment 122

6 years ago
Comment on attachment 513890 [details] [diff] [review]
Part 6g: Add rest of addon doorhanger

>+++ b/suite/common/bindings/notification.xml
>+      <method name="addonInstallStarted">
>+        <parameter name="installInfo"/>
>+        <body>
>+          <![CDATA[
>+            var tmp = {};
>+            Components.utils.import("resource://gre/modules/AddonManager.jsm", tmp);
>+            if (installInfo.installs.every(function(aInstall) {
>+                  return aInstall.state == tmp.AddonManager.STATE_DOWNLOADED;
>+                }))
>+              return;
>+
>+            var sourceURI = installInfo.originatingWindow.document.documentURIObject;
>+            Components.utils.import("resource://gre/modules/PluralForm.jsm", tmp);
>+            var notificationName = "addon-progress";
This variable doesn't seem to be used anywhere, cut and paste slip?

r=me with that addressed.
Attachment #513890 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 123

6 years ago
Created attachment 514612 [details] [diff] [review]
Part 6g: Add rest of addon doorhanger
[Checked in: Comment 124]

Patch as pushed; the review comment also alerted me to check for other nits which I've just rolled in.
Attachment #513890 - Attachment is obsolete: true
Attachment #514612 - Flags: review+
Comment on attachment 514612 [details] [diff] [review]
Part 6g: Add rest of addon doorhanger
[Checked in: Comment 124]

http://hg.mozilla.org/comm-central/rev/59ef5d14407a
Attachment #514612 - Attachment description: Part 6g: Add rest of addon doorhanger → Part 6g: Add rest of addon doorhanger [Checked in: Comment 124]
(Assignee)

Comment 125

6 years ago
Created attachment 519053 [details] [diff] [review]
Part 6h: Missing hunk from part 6f
[Checked in: Comment 126]

D'oh!
Attachment #519053 - Flags: review?(iann_bugzilla)

Updated

6 years ago
Attachment #519053 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 519053 [details] [diff] [review]
Part 6h: Missing hunk from part 6f
[Checked in: Comment 126]

http://hg.mozilla.org/comm-central/rev/333c20646727
Attachment #519053 - Attachment description: Part 6h: Missing hunk from part 6f → Part 6h: Missing hunk from part 6f [Checked in: Comment 126]
[I don't know whether this is the right bug, but probably close enough.]

I've noticed that the add-on download doorhanger (or is it the add-on install doorhanger?) is changing its width while progressing. Since I couldn't see the same with Minefield I guess we're missing some CSS. Do we have a bug for such things already?
(Assignee)

Comment 128

6 years ago
Created attachment 524172 [details] [diff] [review]
Part 2b: restore the Learn More link to the geolocation doorhanger
[Checked in: Comment 130]
Attachment #524172 - Flags: review?(iann_bugzilla)

Comment 129

6 years ago
Comment on attachment 524172 [details] [diff] [review]
Part 2b: restore the Learn More link to the geolocation doorhanger
[Checked in: Comment 130]

r=me - issue about why it ignores link behaviour preferences should be in a followup bug.
Attachment #524172 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 130

6 years ago
Comment on attachment 524172 [details] [diff] [review]
Part 2b: restore the Learn More link to the geolocation doorhanger
[Checked in: Comment 130]

Pushed changeset 18a491487cd9 to comm-central.
Attachment #524172 - Attachment description: Part 2b: restore the Learn More link to the geolocation doorhanger → Part 2b: restore the Learn More link to the geolocation doorhanger [Checked in: Comment 130]
(Assignee)

Comment 131

6 years ago
Created attachment 527873 [details] [diff] [review]
Part 11pre: add support for lightweight themes
[Checked in: Comment 134]

This is a branch-safe patch that doesn't move the strings from navigator.properties into notification.properties; I don't know whether to check this in on trunk first or write a completely separate patch.
Attachment #527873 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 132

6 years ago
Created attachment 527874 [details] [diff] [review]
Part 11w: -w version of attachment 527873 [details] [diff] [review] for comparison

Comment 133

6 years ago
Comment on attachment 527873 [details] [diff] [review]
Part 11pre: add support for lightweight themes
[Checked in: Comment 134]

I rather have separate branch and trunk patches, so if you want this patch to go into the branch, then that is fine, but I'd like the trunk patch to be "not-branch-safe".
Attachment #527873 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 527873 [details] [diff] [review]
Part 11pre: add support for lightweight themes
[Checked in: Comment 134]

http://hg.mozilla.org/releases/comm-2.0/rev/0cd3be53dd8b
Attachment #527873 - Attachment description: Part 11pre: add support for lightweight themes → Part 11pre: add support for lightweight themes [Checked in: Comment 134]
Attachment #527874 - Attachment is obsolete: true

Comment 135

6 years ago
We can haz FIXdz?

Updated

6 years ago
Duplicate of this bug: 593512
(Assignee)

Comment 137

6 years ago
Created attachment 528831 [details] [diff] [review]
Part 11: add support for lightweight themes

Since it's ready this is all one big patch, although I could split it into two smaller patches (70% for the first part and 30% for the rest) if you prefer.
Attachment #528831 - Flags: review?(iann_bugzilla)
Depends on: 573536

Comment 138

6 years ago
Comment on attachment 528831 [details] [diff] [review]
Part 11: add support for lightweight themes

When installing/wearing a new persona I get in the error console:

Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIStringBundle.GetStringFromName]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://communicator/content/bindings/notification.xml :: lwthemeInstallNotification :: line 1821"  data: no]
Attachment #528831 - Flags: review?(iann_bugzilla) → review-
(Assignee)

Comment 139

6 years ago
Created attachment 531058 [details] [diff] [review]
Part 11: add support for lightweight themes

My previous patch was in Windows-1252 instead of UTF-8. Oops.
Attachment #528831 - Attachment is obsolete: true
Attachment #531058 - Flags: review?(iann_bugzilla)

Updated

6 years ago
Attachment #531058 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 140

6 years ago
This is now fixed with my final checkin to comm-central: 9e5f4a666601
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

5 years ago
Duplicate of this bug: 581974

Updated

5 years ago
Blocks: 751081
Blocks: 752216
You need to log in before you can comment on or make changes to this bug.