270443 - Port firefox info bars to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

We need a way to notify the user that an XPI install has been blocked (see bug
270170) without a modal dialog.

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: First pass

This is more or less a direct port of the patches on bug 241705, including a
little code to show a message when an XPI is blocked (for testing purposes).

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Improved patch

On very rare occasions, the window title doesn't seem to match the tab title,
but I can't figure out how to reproduce it.  Yesterday, I thought it was when I
clicked links in external applications, but that doesn't seem to be the case. 
I need help figuring out how to reproduce it so I can find the cause.

Comment 3

[HJ]

"We need a way to notify the user that an XPI install has been blocked"

and for popups too

Comment 4

[Ian Neal]

Incorporating patch on bug 270170 should be fairly easy.

If you look at the firefox browser.js for case "xpinstall-install-edit-permissions":
http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#477
The equivalent of line 483 should be changed to:
var existingWindow = wm.getMostRecentWindow("exceptions-install");

and line 493/494 to:
window.openDialog("chrome://communicator/content/popupManager.xul", "_blank",
                  "chrome,resizable=yes", params);

and for case "xpinstall-install-edit-prefs":
Use something like:
goPreferences("advancedItem",
              "chrome://communicator/content/pref/pref-smartupdate.xul",
              "smartupdateItem");

Though you will need to alter
http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/content/preftree.xul#170
to be:
<treeitem id="smartupdateItem">

Comment 5

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #2)
> On very rare occasions, the window title doesn't seem to match the tab title,
> but I can't figure out how to reproduce it.

Try shift+clicking the Home button or an item in the personal toolbar folder
(shift so the new tab is selected - depending on a pref, you might not need
shift).  That seems to be a case that behaves incorrectly (though it is fixed as
soon as you switch tabs).

Comment 6

[Ian Neal]

What config settings do you have to get that?
browser.tabs.opentabfor ones?

Comment 7

[HJ]

In Mozilla Firefox gBrowser.browsers returns [object HTMLCollection] but in
Mozilla this returns [object nodelist] Can this be caused by this:

<property name="browsers" readonly="true">
onget="return this.mPanelContainer.getElementsByTagName('browser');"/>

Ben, did you modify getElementsByTagName() for Mozilla Firefox?

Comment 8

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #5)
> (In reply to comment #2)
> > On very rare occasions, the window title doesn't seem to match the tab title,
> > but I can't figure out how to reproduce it.
> 
> Try shift+clicking the Home button or an item in the personal toolbar folder
> (shift so the new tab is selected - depending on a pref, you might not need
> shift).  That seems to be a case that behaves incorrectly (though it is fixed as
> soon as you switch tabs).
The code that runs when you middle click the home button or a bookmark sets the
"selectedTab" property of the [tab]browser.  The selectedTab setter did not
update mCurrentTab - changing that fixed the problems I could reproduce.  I'm
not entirely sure that mCurrentTab and selectedTab should ever be different...
I'll have to look into this more at some point.  I'll attach an updated patch
once I've done some more testing.

(In reply to comment #6)
> What config settings do you have to get that?
> browser.tabs.opentabfor ones?
I assume our discussion on IRC a while back resolved your question.

Comment 9

[HJ]

(In reply to comment #7)
> Ben, did you modify getElementsByTagName() for Mozilla Firefox?

Nevermind, I already found out what the source of this bug was.

Comment 10

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Even better patch

This fixes the issue in comment #5 and another one I found.  I don't know of
any remaining bugs, but I've only tested it for about an hour.	I'll have to
test it a lot more before it's ready for review.  This patch contains a lot of
dump() statements for debugging purposes that need to be removed as well.

Comment 11

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Oh, it's possible that there are other bug patches in there as well - I made the
diff from a tree with other in-progress patches.  Sorry.

Comment 12

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Cleaned up patch

+	      b.setAttribute("message", "true"); // what does this do?
http://lxr.mozilla.org/aviarybranch/search?string=%28%22message%22
I'm removing that comment, but I still don't know the answer.

	    this.mTabBox.selectedTab = val;
 +	    this.mTabBox.mCurrentTab = val;
I'm still not 100% sure if those two should ever be different.	However, after
fixing another bug, I removed this line and couldn't reproduce the original
reason I added it - maybe this was a bandaid and I fixed the actual problem. 
Firefox doesn't have this line, so I'm not worried about not adding it.

I also stripped out the xpinstall stuff.

Comment 13

[HJ]

(In reply to comment #12)
> Created an attachment (id=171951) [edit]
> Cleaned up patch

This is not all of it, you forgot/lost some parts like the observer and
stylesheet. You also need to copy the image for this line;

+        <xul:image       anonid="messageImage"   class="messageImage"/>


>  +	      b.setAttribute("message", "true"); // what does this do?

Don't add something you don't need it.

> I also stripped out the xpinstall stuff.

Why? 

And if you want to strip out the XPInstall stuff, take out everything because
you don't need this:

+            if (aSource) {
+              message.source = aSource;
+              message.popup = null;
+            }
+            else if (aPopup) {

because that is used for XPInstall only.

+      <method name="_createMessage">

Why this underscore? You won't find any other in tabbrowser.xml, right?

Also, you don't need the following lines, because you removed it (top/bottom) in
all other places.

+        <parameter name="aType"/>
+            message.setAttribute("type", aType);

Change the following line:
+      <handler event="click">
into:
+      <handler event="command"> 
to prevent accessibility issues

The following ID is wrong:
anonid="messagePopupContainer"
because XPInstall is no popup, and XPInstall will have to be included sooner or
later, so I would like to suggest to strip the "Popup" part.

Please replace the lines with:
document.getAnonymousElementByAttribute(this, "anonid", "messagePopupContainer").

with this._messageContainer.

After you've included the following lines:
      <field name="_messageContainer">
        document.getAnonymousElementByAttribute(this, "anonid", "messageContainer");
      </field>

Remove the comment from the following lines:
+        <parameter name="aDocShell"/> <!-- subject for messages sent to
observers on click -->
+        <parameter name="aSource"/> <!-- topic for messages sent to observers
on click -->

Comment 14

[Robert Kaiser]

>  +	      b.setAttribute("message", "true"); // what does this do?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/tabbrowser.xml&rev=1.36.6.29#914
gives no big clue, right...

Look to me like it has some connection with message="true" in
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/tabbrowser.xml&rev=1.36.6.29#104

and therefore the clue might be in the browser binding... I don't know the code
though and I'm only guessing...

Anyways, the small the diff between toolkit and xpfe gets, the better ;-)

Comment 15

[Robert Kaiser]

> On very rare occasions, the window title doesn't seem to match the tab title,

BTW, isn't this the same as bug 250423?

Comment 16

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

> Anyways, the small the diff between toolkit and xpfe gets, the better ;-)
That's the main reason I kept it.

(In reply to comment #15)
> > On very rare occasions, the window title doesn't seem to match the tab title,
> 
> BTW, isn't this the same as bug 250423?
From the patch on that bug:
-            for ( ; i < this.parentNode.childNodes.length; i++) {
-              if (this.parentNode.childNodes[i] == this)
+            for ( ; i < this.parentNode.parentNode.childNodes.length; i++) {
+              if
(this.parentNode.parentNode.childNodes[i].firstChild.nextSibling == this)
                 break;
So yes.  Had I known about that, I could have saved MANY hours of debugging - I
came up with the same solution on my own :-(


(In reply to comment #13)
> (In reply to comment #12)
> > Created an attachment (id=171951) [edit] [edit]
> > Cleaned up patch
> 
> This is not all of it, you forgot/lost some parts like the observer and
> stylesheet.
What stylesheet?  I took out the observer intentionally.


> You also need to copy the image for this line; 
> +        <xul:image       anonid="messageImage"   class="messageImage"/>
I don't think anything is wrong with that.  When a message is received, the
.imageURL is set, and the imageURL setter changes the src property of the image,
so it gets loaded.


> >  +	      b.setAttribute("message", "true"); // what does this do?
> Don't add something you don't need it.
See above, and KaiRo's replies: I left it in even if only to make it easier to
merge xpfe and toolkit later.


> > I also stripped out the xpinstall stuff.
> Why? 
See bug 270170 - Ian Neal is handling xpinstall differently.  See comment #1 of
this bug - I only added the xpinstall code so that *something* would call
showmessage for testing purposes.  I could have used the popup blocker, or even
something fake.


> And if you want to strip out the XPInstall stuff, take out everything because
> you don't need this:
> 
> +            if (aSource) {
> +              message.source = aSource;
> +              message.popup = null;
> +            }
> +            else if (aPopup) {
> 
> because that is used for XPInstall only.
Bug 270170 may (will, I hope) use showMessage for xpinstall once this lands. 
Also, I see no reason to remove it, because we want to match Firefox, and
there's no guarantee some other caller in the future might not want it.


> +      <method name="_createMessage">
> Why this underscore? You won't find any other in tabbrowser.xml, right?
Firefox consistency.  As far as I can tell, _ is used for internal functions -
if Neil wants me to remove it I will (but basically, nobody outside of
tabbrowser should call it).  In browser.xml there are many fields that use _.


> Also, you don't need the following lines, because you removed it (top/bottom) in
> all other places.
> +        <parameter name="aType"/>
> +            message.setAttribute("type", aType);
You're right, that's used for top vs. bottom, but I'm only supporting top.


> Change the following line:
> +      <handler event="click">
> into:
> +      <handler event="command"> 
> to prevent accessibility issues
Good point.


> The following ID is wrong:
> anonid="messagePopupContainer"
> because XPInstall is no popup, and XPInstall will have to be included sooner or
> later, so I would like to suggest to strip the "Popup" part.
I don't think I understand what you mean.  Firefox calls it messagePopupContainer.

> Please replace the lines with:
> document.getAnonymousElementByAttribute(this, "anonid", "messagePopupContainer").
> 
> with this._messageContainer.
>
> After you've included the following lines:
>       <field name="_messageContainer">
>         document.getAnonymousElementByAttribute(this, "anonid",
"messageContainer");
>       </field>
Good idea.


> Remove the comment from the following lines:
> +        <parameter name="aDocShell"/> <!-- subject for messages sent to
> observers on click -->
> +        <parameter name="aSource"/> <!-- topic for messages sent to observers
> on click -->
Without the comment, it's entirely nonobvious what those do.

Comment 17

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Addresses issues in comment 16 that I agreed with.

Comment 18

[HJ]

> So yes.  Had I known about that, I could have saved MANY hours of debugging -
I came up with the same solution on my own :-(

You will need many more hours, to fix the at least 23 bugs, after you
re-initiated the popup observer.

> What stylesheet?  
Take a look at:
http://lxr.mozilla.org/aviarybranch/source/toolkit/themes/winstripe/global/browser.css#49
http://lxr.mozilla.org/aviarybranch/source/browser/themes/pinstripe/browser/browser.css#868

You don't need anything of that?

> I took out the observer intentionally.
I'm talking about the gPopupBlockerObserver so tell me, what triggers popup
messages?

Tip: take a look at this:
http://lxr.mozilla.org/aviarybranch/source/browser/locales/en-US/chrome/browser/browser.properties#71

> I don't think anything is wrong with that.  When a message is received, the
> .imageURL is set, and the imageURL setter changes the src property of the image,
> so it gets loaded.
Read again i.e. where is that image in your patch? The image URL is set in the
observer, but 
without observer, and no stylesheet mods, no image will ever be displayed.

> See above, and KaiRo's replies: I left it in even if only to make it easier to
> merge xpfe and toolkit later.
Well, if that's going to hurt, we're dead already ;)

> See bug 270170 - Ian Neal is handling xpinstall differently. 
Please look at that bug again because that is to provide a UI to manipulate the
list of white /
black listed sites so it is not the same thing.

> Bug 270170 may (will, I hope) use showMessage for xpinstall once this lands. 
> Also, I see no reason to remove it, because we want to match Firefox, and
> there's no guarantee some other caller in the future might not want it.
Again, no it wont. That's why you need gXPInstallObserver, or nothing will ever
happen.

> Firefox consistency.
Give me a break. Oh, you also want the FireFox bug consistency I guess. I
already fixed 173 
bug in this implementation, so good luck and happy hacking, thanks to Mr.
asshole Blake!

> You're right, that's used for top vs. bottom, but I'm only supporting top.
Please re-read you own coments "Firefox consistency" so make sure to re-initiate
top and bottom. 
It is all or nothing, right? 

> Without the comment, it's entirely nonobvious what those do.
To you maybe but not to me. (p.s. comments in XBL are nodes as well)

LOL to Neil; remember this one:
news://news.mozdev.org:119/cs2v9q$9uc$[e-mail] :-)

Comment 19

[HJ]

Let me explain the "thanks to Mr. asshole Blake!" bits a little. I *warned*
Blake to stop bitching about me, but he didn't, and so a result of his own
actions; I am  awaiting Blake's apology, or he needs serious treatment soon!

Blake, you can't joke about my girlfriend (killed in the 9/11 attacks) without
getting hurt my friend, you won't have anything left after I'm done with you!

Comment 20

[HJ]

(In reply to comment #19)

I've send e-mail to Brendan and Asa about my comment, and I rather file a
lawsuit instead of hurting Blakes so don't you worry... Its just that another
e-mail triggered my aggression, frustration and unbelieve. I can't believe he's
the one sending me these e-mails, so Blake, can you PLEASE send me a prive
e-mail (no text needed) to verify your e-mail address and IP number, thanks!

Comment 21

[Frank Wein [:mcsmurf]]

HJ: Could you please stop commenting about that stuff on that bug here? I think
most people already have a strong opinion about blake so far...

Comment 22

[HJ]

Frank, I won't, and comment 20 should have made that clear already. 

Chris, I hope that my points in comment 18 helps you, and if you have any
question, just e-mail because I have this working for MultiZilla already ;)

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 172019 [details] [diff] [review]
patch

xpfe/browser/resources/content/nsBrowserStatusHandler.js
+    // Hide any info bars
+    getBrowser().hideMessage(browser);

I don't think this is a good place... you placed it amidst the code that the
comment above refers to...

xpfe/global/resources/content/bindings/browser.xml

this dealing with docshells seems like a not so good solution to me. why not
send the notification directly to the tabbrowser or something, instead of doing
it globally?

+	   var subject = this.docShell ? this.docShell : null;

this seems pointless, what is it for?
(i.e. why not directly pass subject to notifyObservers. that is, if you really
want to go this route)

+	 if (event.originalTarget.getAttribute("anonid") == "messageButton") {

how about comparing the nodes itself, instead of an attribute on the node?

Comment 24

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #23)
> (From update of attachment 172019 [details] [diff] [review] [edit])
> xpfe/browser/resources/content/nsBrowserStatusHandler.js
> +    // Hide any info bars
> +    getBrowser().hideMessage(browser);
> 
> I don't think this is a good place... you placed it amidst the code that the
> comment above refers to...
Fixed

> xpfe/global/resources/content/bindings/browser.xml
> 
> this dealing with docshells seems like a not so good solution to me. why not
> send the notification directly to the tabbrowser or something, instead of doing
> it globally?
On IRC, biesi suggested replacing the use of observers with a function and
parameters - so showMessage would have aFunction and aFunctionParams (or
something like that) and instead of using notifyObservers, just call the
function with its parameters.  This sounds reasonable to me - I'll check the
Firefox callers, and see if they can all be changed.


> 
> +	   var subject = this.docShell ? this.docShell : null;
> this seems pointless, what is it for?
Nothing.  Removed.

> 
> +	 if (event.originalTarget.getAttribute("anonid") == "messageButton") {
> 
> how about comparing the nodes itself, instead of an attribute on the node?
Fixed.

Comment 25

[HJ]

(In reply to comment #21)
> I think most people already have a strong opinion about blake so far...

To prevent others from adding more comments like this, it wasn't Blake Ross but
a so called 'impersonator', someone that doesn't like me and / or Blake Ross,
but don't you worry, everything is under control now. Peace ...

Comment 26

[HJ]

"On IRC, biesi suggested replacing the use of observers with a function and
parameters - so showMessage would have aFunction and aFunctionParams (or
something like that) and instead of using notifyObservers, just call the
function with its parameters.  This sounds reasonable to me - I'll check the
Firefox callers, and see if they can all be changed."

Ben Goodger used observers to enable extension writers, like me, to add their
own copy, without hacking the original code, so it is in fact a bad idea, not a
good one.

> this seems pointless, what is it for?
Nothing.  Removed.

To prevent errors in case it is 'undefined' so he needs this check.

> how about comparing the nodes itself, instead of an attribute on the node?
Fixed.

It is easier to maintain the code the old way, because you can simply insert
nodes and move code around without serious problems.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #26)
> Ben Goodger used observers to enable extension writers, like me, to add their
> own copy, without hacking the original code, so it is in fact a bad idea, not a
> good one.

hm? own copy of what?

> To prevent errors in case it is 'undefined' so he needs this check.

undefined converts implicitly to null.

> It is easier to maintain the code the old way, because you can simply insert
> nodes and move code around without serious problems.

You can still do that in the new code.

Comment 28

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Seeing as I volunteered to sync up xpfe and toolkit's tabbrowsers, firefox
consistency is probably less important now (I just have to port whatever changes
we make).

Comment 29

[timeless]

Comment on attachment 172019 [details] [diff] [review]
patch

don't compare val against ""
don't false check docshell

Comment 30

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Previous patch had bitrotted.

Comment 31

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 177146 [details] [diff] [review]
patch

carrying forward r=timeless

Comment 32

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch to call infobar on blocked XPI

If you apply this patch, a blocked XPI will show an infobar.  This patch isn't
meant for review or checkin - just to make the other patch testable.

Comment 33

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 177146 [details] [diff] [review]
patch

Bug 268590 significantly changed the toolkit implementation; since we're going to be moving to toolkit in the future, forking it now would be unwise.

Comment 34

[Robert Kaiser]

I think we should update this and get it into trunk as soon as possible. We'll probably need it for plugin finder and other things.

Chris, are you still working on this?

Comment 35

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

No.  Should be trivial to do now, though.

Comment 36

[Teune van Steeg]

I'd like to take this bug.

Duplicate Tab, an extension I have created, makes use of notification bars in Firefox, so I know something about it.  

I shall upload a first patch, which makes it possible to show notifications in SeaMonkey, soon.

Comment 37

[Teune van Steeg]

Attachment: patch to make it possible to show notifications in SeaMonkey using toolkits notification bars

Most of this patch is simply copied from toolkits tabbrowser.xml

Comment 38

[Robert Kaiser]

You should probably request review and super-review on this, probably r?Ctho and sr?Neil

Comment 39

[Teune van Steeg]

Attachment: make it possible to show notifications in SeaMonkey (updated)

Same patch as before, now with diff -8

Comment 40

[Teune van Steeg]

Attachment: show notification bar with blocked xpinstall

This will show a notification when a xpinstall is blocked. Allowing the user to unblock general installation of addons or to add the domain to the allowed list for addons installation.

Comment 41

[Philip Chee]

(In reply to comment #39)
> Created an attachment (id=275757) [details]
> make it possible to show notifications in SeaMonkey (updated)

Some notes:

The modern theme is missing:
/global/notification.css
/messenger/primaryToolbar.css
You'll need to add at least the first file to your patch.

Implementing getBrowserAtIndex will help you simplify your patch e.g.

> -            var newBrowser = this.mPanelContainer.selectedPanel;
> +            var newBrowser = this.mPanelContainer.selectedPanel.firstChild;
becomes:
-            var newBrowser = this.mPanelContainer.selectedPanel;
+            var newBrowser = this.getBrowserAtIndex(this.mPanelContainer.selectedIndex);

> -            b.id = uniqueId;
> +            this.mPanelContainer.lastChild.id = uniqueId;

I wonder if |notificationbox = uniqueId;| would work here?

>        <constructor>
>          <![CDATA[
           var browser = this.mPanelContainer.childNodes[0].firstChild;

> -          this.mCurrentBrowser = this.mPanelContainer.firstChild;
> +          this.mCurrentBrowser = this.mPanelContainer.childNodes[0].firstChild;
           this.mCurrentBrowser = browser;

>            this.mCurrentTab = this.mTabContainer.firstChild;
>            document.addEventListener("keypress", this._keyEventHandler, false);
>            this.mTabBox.handleCtrlTab = !/Mac/.test(navigator.platform);
>            this.arrowKeysShouldWrap = /Mac/.test(navigator.platform);
>
>            var uniqueId = "panel" + this.nextTabNumber++;
>            this.mPanelContainer.childNodes[0].id = uniqueId;
>            this.mTabs[0].linkedPanel = uniqueId;
> -          this.mTabs[0].linkedBrowser = this.mPanelContainer.childNodes[0];
> +          this.mTabs[0].linkedBrowser = this.mPanelContainer.childNodes[0].firstChild;
          this.mTabs[0].linkedBrowser = browser;

> -            this.mPanelContainer.childNodes[i].removeEventListener("DOMTitleChanged", this.onTitleChanged, true);
> +            this.mPanelContainer.childNodes[i].firstChild.removeEventListener("DOMTitleChanged", this.onTitleChanged, true);
             this.browsers[i].removeEventListener("DOMTitleChanged", this.onTitleChanged, true);

...to be continued...

Comment 42

[Philip Chee]

(In reply to comment #40)
> Created an attachment (id=275760) [details]
> show notification bar with blocked xpinstall

> diff -u -8 -r1.611 navigator.js
Nit: use -u8p

You also need to deal with hiding notification boxes in:
  browser.js -> toggleAffectedChrome()
and: 
  nsBrowserStatusHandler.js -> onLocationChange:

Followup bugs should be filed for notification box handling in the popup blocker and in mailnews.

Comment 43

[Teune van Steeg]

(In reply to comment #41)
> Some notes:
> 
> The modern theme is missing:
> /global/notification.css
> /messenger/primaryToolbar.css
> You'll need to add at least the first file to your patch.

It seems to me that toolkits theme for this is okay. Does it not match the
modern theme enough?

> Implementing getBrowserAtIndex will help you simplify your patch 

Yes, but even simpler would be using the existing "browsers" property (as used
in the latest simplification you suggest.)

> I wonder if |notificationbox = uniqueId;| would work here?
That seems more logical, indeed. And that works too.

I'll update the patch with your suggested simplifications.

(in reply to comment #42)
> Nit: use -u8p

> You also need to deal with hiding notification boxes in:
>   browser.js -> toggleAffectedChrome()
> and: 
>   nsBrowserStatusHandler.js -> onLocationChange:

I'll look into that

Comment 44

[[e-mail]]

(In reply to comment #43)
>(In reply to comment #41)
>>The modern theme is missing:
>>/global/notification.css
>>/messenger/primaryToolbar.css
>>You'll need to add at least the first file to your patch.
>It seems to me that toolkits theme for this is okay. Does it not match the
>modern theme enough?
Toolkit themes almost certainly never match the modern theme.

Comment 45

[Teune van Steeg]

Attachment: make it possible to show notifications in Seamonkey (version 2)

update with notes in comment #41 addressed

This also adds notification.css to the patch. I hope this is the correct way of doing it, and I assume that the theming will be improved by the people responsible for the SeaMonkey theme. 
Currently notification.css from toolkit is used in SeaMonkey, will that file now be ignored if this patch is applied?

Comment 46

[Robert Kaiser]

Teune, your theme changes are wrong, I think you didn't get what was meant with the other comments:
We have two themes included with SeaMonkey, which both need to be kept up-to-date. The "classic" or default theme uses the parts from toolkit for global and does not need a change for this. The second theme, "modern", does not use anything from the toolkit's theme and needs such an addition of this file - and it's your task if adding a feature to keep that theme working as well, which might need some digging into what colors are right for it, but if you get it wrong, I guess the reviewer or super-reviewer will correct you.

Comment 47

[Teune van Steeg]

Attachment: make it possible to show notifications in Seamonkey (version 3)

Same patch as before, but this updates the Modern theme instead of the Classic theme.
 
For the color of the (warning priority) information bar I used the same color as the location bar when a secure site is visited. (As in the toolkit version)
For critical priority I left the color red.
And for the info priority it is just the background color of dialogs.

Comment 48

[Teune van Steeg]

Attachment: show notification bar with blocked xpinstall (version 2)

This version has the missing parts mentioned in comment #42 added.

Comment 49

[Teune van Steeg]

Attachment: screenshot of blocked xpinstall with modern theme

This is what the information bar looks like with the modern theme when an extension installation is blocked.

(in reply to comment #46)
> We have two themes included with SeaMonkey, which both need to be kept
> up-to-date. The "classic" or default theme uses the parts from toolkit for
> global and does not need a change for this. The second theme, "modern"
It's confusing ;): the classic theme is newer then the modern theme

Comment 50

[Teune van Steeg]

Attachment: show notification bar with blocked xpinstall (version 3)

Forgot to add check for in page navigation, when hiding notification bar.

Comment 51

[[e-mail]]

Comment on attachment 275845 [details] [diff] [review]
make it possible to show notifications in Seamonkey (version 3)

>+          <xul:notificationbox flex="1">
Doesn't need flex, it's in a deck.

>+            <xul:browser flex="1" message="true" type="content-primary" xbl:inherits="tooltip=contenttooltip,contextmenu=contentcontextmenu,autocompletepopup"/>
What does message="true" do?

>+            if (aBrowser)
>+              return aBrowser.parentNode;
>+            else if (this.mCurrentBrowser)
>+              return this.mCurrentBrowser.parentNode;
>+            return null;
1. this.mCurrentBrowser isn't going to be null
2. Don't use else after return (maybe ?: would be a better idea)

>+            b.setAttribute("message", "true");
(Ditto)

>+            var notificationbox = document.createElementNS(
>+                                    "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
>+                                    "notificationbox");
Strange wrapping.

>+            notificationbox.setAttribute("flex", "1");
(Ditto)

>+          this.mCurrentBrowser = this.mPanelContainer.childNodes[0].firstChild;
Could be .firstChild, not .childNodes[0]!
(I don't know why the old code historically used .childNodes[0])

>-            this.mPanelContainer.childNodes[i].removeEventListener("DOMTitleChanged", this.onTitleChanged, true);
>+            this.browsers[i].removeEventListener("DOMTitleChanged", this.onTitleChanged, true);
Strictly speaking this is a minor semantic change, but it doesn't matter.

I haven't looked at the Modern styles yet.

Comment 52

[[e-mail]]

Comment on attachment 275876 [details] [diff] [review]
show notification bar with blocked xpinstall (version 3)

>+  _findChildShell: function (aDocShell, aSoughtShell)
>+  {
>+    if (aDocShell == aSoughtShell)
>+      return aDocShell;
>+
>+    var node = aDocShell.QueryInterface(Components.interfaces.nsIDocShellTreeNode);
>+    for (var i = 0; i < node.childCount; ++i) {
>+      var docShell = node.getChildAt(i);
>+      docShell = this._findChildShell(docShell, aSoughtShell);
Rather than mucking about with a recursive search, first
a) check the tree item's rootTreeItem to see if it belongs to this window
b) check its sameTypeRootTreeItem to see which browser owns it
I don't know what you're supposed to do if the sidebar opens an XPI.

>+    switch (aTopic) {
>+    case "xpinstall-install-blocked":
Indent case blocks please.

>+        var host = browser.docShell.QueryInterface(Components.interfaces.nsIWebNavigation).currentURI.host;
browser.currentURI might come in useful here. Also, not all URIs have hosts. (Maybe you only need it when you know the URL has a host?)

>+        var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                    .getService(Components.interfaces.nsIPrefService);
We have this in a global somewhere, don't we?

>+            callback: function() { return xpinstallEditPermissions(shell); }
Why not pass the host? You already have it at this point.

>+  <script type="application/x-javascript" src="chrome://communicator/content/permissions/permissionsOverlay.js"/>
Don't we already include this script via permissionsOverlay.xul?

>+    this.lastURI         = null;
>+    if (browser.lastURI) {
So, which is it to be, this or browser? (Clue: the code is wrong either way)

>+      var oldIndexOfHash = oldSpec.indexOf("#");
>+      if (oldIndexOfHash != -1)
>+        oldSpec = oldSpec.substr(0, oldIndexOfHash);
That's not the way to remove the hash from a spec. Use replace.

>+xpinstallPromptWarning=%S prevented this site (%S) from asking you to install software on your computer.
How do you trigger this? I wasn't able to find a suitable about:config setting.

Comment 53

[[e-mail]]

Comment on attachment 275845 [details] [diff] [review]
make it possible to show notifications in Seamonkey (version 3)

>Index: themes/modern/jar.mn
>===================================================================
>RCS file: /cvsroot/mozilla/themes/modern/jar.mn,v
>retrieving revision 1.171
>diff -u -8 -p -r1.171 jar.mn
>--- jar.mn	2 Aug 2007 20:06:40 -0000	1.171
>+++ jar.mn	8 Aug 2007 20:55:34 -0000
Hand-edited diff? (should be themes/modern/jar.mn all three times)

>+notification {
>+  background-color: #E8DB99;
>+  color: InfoText;
No system colours.

>+notification[type="info"] {
>+  background-color: -moz-Dialog;
Ditto.

>+notification[type="critical"] {
>+  background-color: red;
Prefer # notation.

>+.messageText {
>+  -moz-margin-start: 5px;
Nit: Modern isn't currently RTL-friendly

>+.messageButton {
>+  margin: 0px 5px 0px 5px;
Nit: Shorten this to 0px 5px;

>+  -moz-appearance: none;
Modern never has -moz-apperance styles.

>+  border: none !important;
Nit: Doesn't need to be !important

Comment 54

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

(In reply to comment #52)
> >+        var host = browser.docShell.QueryInterface(Components.interfaces.nsIWebNavigation).currentURI.host;
> browser.currentURI might come in useful here. Also, not all URIs have hosts.
> (Maybe you only need it when you know the URL has a host?)

This line of code is being removed by the patch for bug 307770.

Comment 55

[Teune van Steeg]

(In reply to comment #51)
> >+            <xul:browser flex="1" message="true" type="content-primary" xbl:inherits="tooltip=contenttooltip,contextmenu=contentcontextmenu,autocompletepopup"/>
> What does message="true" do?

I think it is supposed to distinguish browsers which have notification boxes, from browsers which don't in Firefox (like the browsers in the sidebar in SeaMonkey). But is not used in the current code, shall I remove it?

(In reply to comment #52)
> Rather than mucking about with a recursive search, first
> a) check the tree item's rootTreeItem to see if it belongs to this window
> b) check its sameTypeRootTreeItem to see which browser owns it
> I don't know what you're supposed to do if the sidebar opens an XPI.
is adding a notificationbox in the sidebar on option (screenshot coming up)?

> >+    this.lastURI         = null;
> >+    if (browser.lastURI) {
> So, which is it to be, this or browser? (Clue: the code is wrong either way)
this.lastURI seems to be an unused remainder of a previous version of this code in Firefox. I shall leave it out in an updated patch. 
Can you tell me what is wrong with using browser.lastURI? Can I retrieve this information from somewhere else?

> >+xpinstallPromptWarning=%S prevented this site (%S) from asking you to install software on your computer.
> How do you trigger this? I wasn't able to find a suitable about:config setting.
the setting is "xpinstall.whitelist.required" if set to true, it will show this warning every time a site which is not on the white list tries to install an extension 

Comment 56

[Teune van Steeg]

Attachment: screenshot of blocked xpinstall in sidebar

This screenshots shows what a notificationbox in the sidebar looks like when a XPI is blocked from opening in the sidebar.

Comment 57

[mv_van_rantwijk]

Teune, I have a few nits:

+            buttons = [];

Where's the declaration of buttons?

+    showPermissionsManager("install", webNav.currentURI.host);

What about XPInstall from file: or other third party protocols?

+  // observer for XPInstall events

Do you really need this comment?  Isn't it obvious enough already?

+  // remove XPInstall observer

Ditto.

+    var browser = getBrowser().selectedBrowser;

Why not simply use gBrowser here?

+        getBrowser().getNotificationBox(browser).removeAllNotifications(true);

Ditto.

+      var oldIndexOfHash = oldSpec.indexOf("#");
+      if (oldIndexOfHash != -1)
+        oldSpec = oldSpec.substr(0, oldIndexOfHash);
+      var newSpec = location;
+      var newIndexOfHash = newSpec.indexOf("#");
+      if (newIndexOfHash != -1)
+        newSpec = newSpec.substr(0, newSpec.indexOf("#"));

Let's not copy Firefox bugs ok?

Comment 58

[[e-mail]]

(In reply to comment #55)
>(In reply to comment #51)
>>What does message="true" do?
>But is not used in the current code, shall I remove it?
Yes, I don't see the point otherwise.

>>I don't know what you're supposed to do if the sidebar opens an XPI.
>Is adding a notificationbox in the sidebar on option (screenshot coming up)?
Looks good to me.

>Can you tell me what is wrong with using browser.lastURI?
browser.lastURI only works when you load pages in the current tab. When you switch tabs you're actually looking at a different browser's lastURI. Worse, when other tabs change URI in the background you don't notice at all.

>Can I retrieve this information from somewhere else?
Only the tabbrowser can monitor URI changes in all browsers.

>the setting is "xpinstall.whitelist.required"
It turns out that it's ignored in the case I was trying to test... oops!

Comment 59

[Teune van Steeg]

(In reply to comment #57)
> +    showPermissionsManager("install", webNav.currentURI.host);
> 
> What about XPInstall from file: or other third party protocols?
XPInstall from file and chrome protocols are not blocked anyhow
(nsInstallTrigger.cpp). As for other protocols: I don't know, but if there's a
host to whitelist, it could be handled here. 
Anyway, work being done in bug #252830, will change a lot of what has to be
done here.

> +  // observer for XPInstall events
> Do you really need this comment?  Isn't it obvious enough already?
> +  // remove XPInstall observer
> Ditto.
true

> +    var browser = getBrowser().selectedBrowser;
> 
> Why not simply use gBrowser here?
this line is just old code placed a few lines forward in the file.
> +        getBrowser().getNotificationBox(browser).removeAllNotifications(true);
> 
> Ditto.
I don't know what the official guidelines are for using gBrowser and
getBrowser() but they are used both throughout the code. And I chose to use
getBrowser().

Comment 60

[mv_van_rantwijk]

(In reply to comment #59)
> (In reply to comment #57)
> > +    showPermissionsManager("install", webNav.currentURI.host);
> > 
> > What about XPInstall from file: or other third party protocols?
> XPInstall from file and chrome protocols are not blocked anyhow
> (nsInstallTrigger.cpp). As for other protocols: I don't know, but if there's a
> host to whitelist, it could be handled here. 
> Anyway, work being done in bug #252830, will change a lot of what has to be
> done here.

Ah yes, I was just thinking too much MultiZilla here.

> > +    var browser = getBrowser().selectedBrowser;
> > 
> > Why not simply use gBrowser here?
> this line is just old code placed a few lines forward in the file.

If you touch something, do it right and fix it ;)

> I don't know what the official guidelines are for using gBrowser and
> getBrowser() but they are used both throughout the code. And I chose to use
> getBrowser().

There are valid use cases, yes, but people should use gBrowser whenever possible because it a tad faster ;)

Comment 61

[Teune van Steeg]

(In reply to comment #58)
> >Can you tell me what is wrong with using browser.lastURI?
> browser.lastURI only works when you load pages in the current tab. When you
> switch tabs you're actually looking at a different browser's lastURI. Worse,
> when other tabs change URI in the background you don't notice at all.

I think this code handles it correctly (apart from the replace method):
When switching tabs onlocationchange is notified, thus retrieving lastURI for the correct browser (gBrowser.selectedBrowser as that browser is now selected) and there is no need the hide the notificationbox in a tab where the location did not change.

The same applies for when a new location is loaded in the background: if that location is different from that browsers lastURI, it will be noticed when you switch back to that tab (onlocationchange is notified), and the notificationbox will be hidden.

Comment 62

[Teune van Steeg]

Attachment: make it possible to show notifications in Seamonkey (v4)

This patch is only to make notifications available in SeaMonkey's tab browser.

A patch to show notifications on blocked XPInstall events will follow when bug #252830 is fixed (can anyone make this bug dependent on bug #252830 ?)

A patch which makes notifications available in the sidebar will follow later.

Comment 63

[[e-mail]]

(In reply to comment #62)
>A patch to show notifications on blocked XPInstall events will follow when bug
>#252830 is fixed (can anyone make this bug dependent on bug #252830 ?)
Actually I think a new bug would be a better idea.

Comment 64

[Teune van Steeg]

Filed bug #393108 (notifications for xpinstall) and bug #393120 (notifications for popup blocker)

Comment 65

[Philip Chee]

>+@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>
>+
>
>+notification {
>
>+  background-color: #E8DB99;
>
>+  color: #000000;
>
>+}

There seem to be a lot of unnecessary blank lines in this file. Editor misconfiguration?

Comment 66

[Teune van Steeg]

Attachment: make it possible to show notifications in Seamonkey (v5)

Same patch as v4 but without unnecessary blank lines

Comment 67

[[e-mail]]

Comment on attachment 277694 [details] [diff] [review]
make it possible to show notifications in Seamonkey (v5)

>+            return (aBrowser)? aBrowser.parentNode: this.mCurrentBrowser.parentNode;
Nits: ()s unnecessary; spaces before as well as after ? and :

>             var b = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
>                                              "browser");
>             b.setAttribute("type", "content-targetable");
>             b.setAttribute("contextmenu", this.getAttribute("contentcontextmenu"));
>             b.setAttribute("tooltip", this.getAttribute("contenttooltip"));
>             b.setAttribute("autocompletepopup", this.getAttribute("autocompletepopup"));
>
>-            this.mPanelContainer.appendChild(b);
>+            // Add the Message and the Browser to the box
>+            var notificationbox = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
>+                                                           "notificationbox");
>+            notificationbox.appendChild(b);
>+            b.setAttribute("flex", "1");
I think this attribute belongs at the end of the list of attributes above.

>-            this.mPanelContainer.childNodes[i].removeEventListener("DOMTitleChanged", this.onTitleChanged, true);
>+            this.browsers[i].removeEventListener("DOMTitleChanged", this.onTitleChanged, true);
Nit: needs commenting that we're actually removing these in display order

>+    if (browser.lastURI) {
>+      var re = /#.*/;
>+      var oldSpec = browser.lastURI.spec.replace(re, "");
>+      var newSpec = location.replace(re, "");
>+      if (newSpec != oldSpec) {
>+        gBrowser.getNotificationBox(browser).removeAllNotifications(true);
>+      }
>+    }
>+    browser.lastURI = locationURI;
OK, so this doesn't quite work. For instance, if you open a link into a new tab in the background which then needs a plugin/popup/install/whatever, this code fires when you switch tab because you've never seen that URI before.

>+  background-color: #E8DB99;
An excellent choice :-)

Comment 68

[Teune van Steeg]

(In reply to comment #67)
> >+    if (browser.lastURI) {
> >+      var re = /#.*/;
> >+      var oldSpec = browser.lastURI.spec.replace(re, "");
> >+      var newSpec = location.replace(re, "");
> >+      if (newSpec != oldSpec) {
> >+        gBrowser.getNotificationBox(browser).removeAllNotifications(true);
> >+      }
> >+    }
> >+    browser.lastURI = locationURI;
> OK, so this doesn't quite work. For instance, if you open a link into a new tab
> in the background which then needs a plugin/popup/install/whatever, this code
> fires when you switch tab because you've never seen that URI before.
 
When you open a link in a new tab, browser.lastURI is not yet defined for that tabs browser (because this code has not run yet for that browser). 
This means that when you switch to the new tab (if it is loaded in the background), onlocationchange will be notified but this block of code will not run (it will not pass the check for browser.lastURI), so that the notification(s) will not be removed.  

Comment 69

[[e-mail]]

You're right, it does work in that case.

The case it doesn't work in is if there's already a page loaded into the tab, and you drag a link onto that tab to load in the background, and that new page fires off a notification. Or perhaps you switch tabs, and the page you had loaded loads up a new page that fires off a notification. Or something like that.

Comment 70

[[e-mail]]

Comment on attachment 277694 [details] [diff] [review]
make it possible to show notifications in Seamonkey (v5)

>+    if (browser.lastURI) {
>+      var re = /#.*/;
>+      var oldSpec = browser.lastURI.spec.replace(re, "");
>+      var newSpec = location.replace(re, "");
>+      if (newSpec != oldSpec) {
>+        gBrowser.getNotificationBox(browser).removeAllNotifications(true);
>+      }
>+    }
I now think that the best place to remove the notifications is in the tabbrowser's onLocationChange handler. You'll need to verify that the location change is for the content window and that the a new document is loading. Note that testing the spec doesn't detect a new document in some cases.

Comment 71

[Teune van Steeg]

Attachment: make it possible to show notifications in Seamonkey (v6)

Update of v5 with all the nits/suggestions from Neil addressed. 

> You'll need to verify that the location
> change is for the content window and that the a new document is loading. 
Thanks for a hint in the right direction. This is much cleaner.

Comment 72

[mv_van_rantwijk]

What happens if you have two or more frames and one of the frames, which isn't linked to the browser message, is being (re)loaded?

Comment 73

[[e-mail]]

Comment on attachment 277759 [details] [diff] [review]
make it possible to show notifications in Seamonkey (v6)

>-              if (!this.mBlank && this.mTabBrowser.mCurrentTab == this.mTab) {
>+              if (this.mBlank)
>+                return;
>+
>+              if (aWebProgress.DOMWindow == this.mBrowser.contentWindow && aWebProgress.isLoadingDocument)
I don't mind but I'm sure you could have put this test before the blank test.

>+                this.mTabBrowser.getNotificationBox(this.mBrowser).removeAllNotifications(true);
You don't need to call getNotificationBox - just use .parentNode

>+            var notificationbox = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
I was tempted to ask you to rename this variable to "n" ;-)

Comment 74

[[e-mail]]

(In reply to comment #72)
>What happens if you have two or more frames and one of the frames, which isn't
>linked to the browser message, is being (re)loaded?
We don't link the message to an individual frame. Messages are only cleared once the frameset or other master document is replaced.

Comment 75

[Teune van Steeg]

Attachment: let the sidebar use notifications

This patch implements notification boxes for the sidebar so that you will be notified when for instance an extension or a popup is blocked when it is loaded in the sidebar. 

At the same time I felt free to remove some duplicate code in sidebarOverlay.js:
>-        if (panel.is_sandboxed()) {
>-          if (!panel.is_persistent())
>-            iframe.setAttribute('src', 'about:blank');
>-        }
because that same code is run a few lines back at line 461 - 462.

There are however 2 problems with this patch:

1) When a notification is showing and you navigate to a different page (maybe different site) in the same sidebar panel, the notification will not disappear. However, what I have seen from (custom) sidebars thus far is that navigation is mostly limited to one page.   

2) Sidebars are small, the notification box may be too wide for the sidebar. This may require different styling for a sidebar notification box, or using very compact messages.

I would like to hear ideas about those problems before I ask review for this patch.

Comment 76

[Teune van Steeg]

(In reply to comment #73)
> (From update of attachment 277759 [details] [diff] [review])
> >-              if (!this.mBlank && this.mTabBrowser.mCurrentTab == this.mTab) {
> >+              if (this.mBlank)
> >+                return;
> >+
> >+              if (aWebProgress.DOMWindow == this.mBrowser.contentWindow && aWebProgress.isLoadingDocument)
> I don't mind but I'm sure you could have put this test before the blank test.
I figured that there is no need to run this code when it is not necessary. 
 
> >+                this.mTabBrowser.getNotificationBox(this.mBrowser).removeAllNotifications(true);
> You don't need to call getNotificationBox - just use .parentNode
> 
> >+            var notificationbox = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
> I was tempted to ask you to rename this variable to "n" ;-)
I don't mind changing that. But I'll wait for a review first to see what else needs to be changed. 
 

Comment 77

[[e-mail]]

Another issue with sidebar notifications is that you'd need one set of code to deal with tabbrowser notifications and another set of code to deal with sidebar notifications (which would be needed in all windows that allow sidebars).

Unless maybe an extended <notificationbox> could detect all the notifications?

Comment 78

[Teune van Steeg]

Attachment: preview of an extended notificationbox

> Unless maybe an extended <notificationbox> could detect all the notifications?

I went ahead and created such an extended notificationbox.
This shows simple notifications for blocked popups and blocked xpi installs in the sidebar and the main browser. 

prerequisites:
- Both notificationbox patches for browser and sidebar must be applied.
- Bind this extended notificationbox to <notificationbox> 
- Extended notificationbox assumes there is an <popup id="blockedPopups"> in navigator.xul to list blocked popups in a popup menu.

Comment 79

[Robert Kaiser]

Hmm, would it make sense to provide this extended notification box in toolkit, and bind it to <notificationbox class="browser"> or such, so multiple browser projects (including Firefox and SeaMonkey) can use it?
If so, I guess it should go into a new bug against toolkit and can be picked up by our notification boxes created here later.

Comment 80

[[e-mail]]

Comment on attachment 278045 [details]
preview of an extended notificationbox

Nice stuff! KaiRo is probably right about getting this into toolkit.

>              const NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
Nit: we normally call this XUL_NS

>            // check if the sought docShell shares the browser with the reference docShell
>            if (treeItem.sameTypeRootTreeItem != soughtTreeItem.sameTypeRootTreeItem)
>              return null;
There is actually an easier way; the soughtTreeItem.sameTypeRootTreeItem should be the treeItem retrived from the browser (the browser docShell type is different from its parent's type so that it's always the same type root). If that is true then that implies all of your other tests are also true.

>          this.addEventListener("DOMPopupBlocked", this.onPopupBlocked, true)
I guess this works but it's much better to use
<handler event="DOMPopupBlocked" phase="capturing">

Comment 81

[Teune van Steeg]

I filed bug #393857 for adding an extended notificationbox to toolkit

Comment 82

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 277759 [details] [diff] [review]
make it possible to show notifications in Seamonkey (v6)

I did not test this.

+            // eventListeners are removed from the browsers in display order of the browsers
Was the old code removing listeners and filters in a different order?

Comment 83

[[e-mail]]

(In reply to comment #82)
>+            // eventListeners are removed from the browsers in display order of the browsers
>Was the old code removing listeners and filters in a different order?
No, the old code was removing them in the same order.

Comment 84

[Teune van Steeg]

Attachment: make it possible to show notifications in Seamonkey (v7, for checkin)

patch for check in, same as v6, but addresses the nits from comment 73

Comment 85

[Teune van Steeg]

Attachment: make it possible to show notifications in Seamonkey (checked in)

I found one small error in the patch, which is fixed now: I changed gChromeState.notificationsHidden to the correct gChromeState.notificationsWereHidden in the browser.js part.

The rest is the same patch as v7.

This makes patch v6 obsolete, but I can't seem to carry over the sr+ of that patch. Can you do that for me Neil?

Still carrying over r+

Does this patch need asking for approval before it can be checked in?

Comment 86

[Teune van Steeg]

Attachment: let the sidebar use notifications, v2

This patch makes it possible to show notifications in the sidebar. I added a style change for those notifications, so they won't appear too wide (close button not in view).

Comment 87

[[e-mail]]

Comment on attachment 279185 [details] [diff] [review]
make it possible to show notifications in Seamonkey (checked in)

SeaMonkey-only code doesn't need separate approval.

Comment 88

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 279186 [details] [diff] [review]
let the sidebar use notifications, v2

I'm not familiar with the sidebar code, so I'd prefer not to review this unless nobody else knows it either.  Mnyromyr, would you be qualified?

Comment 89

[Robert Kaiser]

The main patch can go in now, right?
Teune, do you have CVS access or would you be available on IRC for questions or problem fixing when someone else checks in the patch?

Comment 90

[Robert Kaiser]

Comment on attachment 279185 [details] [diff] [review]
make it possible to show notifications in Seamonkey (checked in)

I just checked in this part. Thanks, Teune!

Comment 91

[Teune van Steeg]

Attachment: fixing issues for checked in patch (checked in)

I tested SeaMonkey with the checked in patch on Linux today. There I found two issues: 
The first is obvious (double quote after URL declaration in style sheet).
The second shows when a notification is showing, and you press the back button. Then when the notification disappears, the page doesn't stretch to the statusbar (it only moves up to fill the space left by the notification area).

This patch fixes both issues.

Comment 92

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

That sounds like a bug in notificationbox.  Neil Deakin, is it an API requirement that the notification boxes not slide out in this case?

Comment 93

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 279368 [details] [diff] [review]
fixing issues for checked in patch (checked in)

r=me.  Even if it's a toolkit bug, I still don't think the notification bars should slide in this case, so the new behavior is arguably better.

Comment 94

[Karsten Düsterloh]

Comment on attachment 279186 [details] [diff] [review]
let the sidebar use notifications, v2

It's actually rather hard to test a patch without a test case...

>Index: suite/common/sidebar/sidebarOverlay.xul
>===================================================================
>+                <notificationbox flex="1" collapsed="true">
>+                  <browser flex="1" class="browser-sidebar" src="about:blank"
>+                    hidden="true" collapsed="true" content="?content"/>
>+                  <browser flex="1" class="browser-sidebar" src="about:blank"
>+                    hidden="true" collapsed="true" content="?content"
>+                    type="content" context="contentAreaContextMenu" tooltip="aHTMLTooltip"/>
>+                </notificationbox>

Please add ids to all (these three and other) elements and use them below (we souldn't have more than one sidebar overlay in any given window).

>Index: suite/common/sidebar/sidebarOverlay.js
>===================================================================
>   this.removeEventListener("load", panel_loader, true);
>   this.removeAttribute('collapsed');
>+  this.parentNode.removeAttribute('collapsed');
>   this.setAttribute('loadstate','loaded');
>-  this.parentNode.firstChild.setAttribute('hidden','true');
>+  this.parentNode.parentNode.firstChild.setAttribute('hidden','true');

This is particularly ugly - and what ids and such have actually been invented for...
Add ids as needed/possible and use getElementById instead.

>         if (sidebarObj.collapsed && panel.is_sandboxed()) {
>           if (!panel.is_persistent()) {
>             debug("    set src=about:blank");
>             iframe.setAttribute('src', 'about:blank');
>+            // content will be reloaded: remove notifications
>+            // to prevent them from showing when expanding the sidebar
>+            iframe.parentNode.removeAllNotifications(true);

Use the notification box' id here...

>             iframe.setAttribute('src', saved_src);
>+            // content is reloaded: remove notifications
>+            iframe.parentNode.removeAllNotifications(true);

Dito.

>@@ -462,20 +468,16 @@ function (force_reload)
>           if (load_state == 'loading') {
>             iframe.removeEventListener("load", panel_loader, true);
>             content.setAttribute('hidden','true');
>             iframe.setAttribute('loadstate', 'never loaded');
>           }
>         }
>-        if (panel.is_sandboxed()) {
>-          if (!panel.is_persistent())
>-            iframe.setAttribute('src', 'about:blank');
>-        }

This needs to be explained. Why do you drop that?

>   if (typeof this.sandboxed == "undefined") {
>     var content = this.get_content();
>-    var unsandboxed_iframe = content.childNodes.item(1);
>+    var unsandboxed_iframe = content.childNodes.item(1).childNodes.item(0);

Again, the sidebar code is ugly enough, we don't want it get even worse...
Use ids.

>     if (this.is_sandboxed()) {
>-      var unsandboxed_iframe = content.childNodes.item(2);
>+      var unsandboxed_iframe = content.childNodes.item(1).childNodes.item(1);
>       this.iframe = unsandboxed_iframe;
>     } else {
>-      var sandboxed_iframe = content.childNodes.item(1);
>+      var sandboxed_iframe = content.childNodes.item(1).childNodes.item(0);
>       this.iframe = sandboxed_iframe;

Dito.

Comment 95

[mv_van_rantwijk]

(In reply to comment #94)
> (From update of attachment 279186 [details] [diff] [review])
> It's actually rather hard to test a patch without a test case...
> 
> >Index: suite/common/sidebar/sidebarOverlay.xul
> >===================================================================
> >+                <notificationbox flex="1" collapsed="true">
> >+                  <browser flex="1" class="browser-sidebar" src="about:blank"
> >+                    hidden="true" collapsed="true" content="?content"/>
> >+                  <browser flex="1" class="browser-sidebar" src="about:blank"
> >+                    hidden="true" collapsed="true" content="?content"
> >+                    type="content" context="contentAreaContextMenu" tooltip="aHTMLTooltip"/>
> >+                </notificationbox>
> 
> Please add ids to all (these three and other) elements and use them below (we
> souldn't have more than one sidebar overlay in any given window).
> 
> >Index: suite/common/sidebar/sidebarOverlay.js
> >===================================================================
> >   this.removeEventListener("load", panel_loader, true);
> >   this.removeAttribute('collapsed');
> >+  this.parentNode.removeAttribute('collapsed');
> >   this.setAttribute('loadstate','loaded');
> >-  this.parentNode.firstChild.setAttribute('hidden','true');
> >+  this.parentNode.parentNode.firstChild.setAttribute('hidden','true');
> 
> This is particularly ugly - and what ids and such have actually been invented
> for...
> Add ids as needed/possible and use getElementById instead.

Why do you want people to slow down things?

> >         if (sidebarObj.collapsed && panel.is_sandboxed()) {
> >           if (!panel.is_persistent()) {
> >             debug("    set src=about:blank");
> >             iframe.setAttribute('src', 'about:blank');
> >+            // content will be reloaded: remove notifications
> >+            // to prevent them from showing when expanding the sidebar
> >+            iframe.parentNode.removeAllNotifications(true);
> 
> Use the notification box' id here...

What about getting the notification box, give it a clear name, and call removeAllNotification?  That will look better IMHO, but there is really no need to use getElementById if you _know_ where it is already. Or is someone actually going to (re)move this element?

> >             iframe.setAttribute('src', saved_src);
> >+            // content is reloaded: remove notifications
> >+            iframe.parentNode.removeAllNotifications(true);
> 
> Dito.

Same here.

> >@@ -462,20 +468,16 @@ function (force_reload)
> >           if (load_state == 'loading') {
> >             iframe.removeEventListener("load", panel_loader, true);
> >             content.setAttribute('hidden','true');
> >             iframe.setAttribute('loadstate', 'never loaded');
> >           }
> >         }
> >-        if (panel.is_sandboxed()) {
> >-          if (!panel.is_persistent())
> >-            iframe.setAttribute('src', 'about:blank');
> >-        }
> 
> This needs to be explained. Why do you drop that?
> 
> >   if (typeof this.sandboxed == "undefined") {
> >     var content = this.get_content();
> >-    var unsandboxed_iframe = content.childNodes.item(1);
> >+    var unsandboxed_iframe = content.childNodes.item(1).childNodes.item(0);
> 
> Again, the sidebar code is ugly enough, we don't want it get even worse...
> Use ids.

Why?  The name unsandboxed_iframe is pretty clear to me, and I haven't even looked at the code!

> >     if (this.is_sandboxed()) {
> >-      var unsandboxed_iframe = content.childNodes.item(2);
> >+      var unsandboxed_iframe = content.childNodes.item(1).childNodes.item(1);
> >       this.iframe = unsandboxed_iframe;
> >     } else {
> >-      var sandboxed_iframe = content.childNodes.item(1);
> >+      var sandboxed_iframe = content.childNodes.item(1).childNodes.item(0);
> >       this.iframe = sandboxed_iframe;
> 
> Dito.

Same for sandboxed_iframe here.

Comment 96

[Teune van Steeg]

(In reply to comment #94)
> (From update of attachment 279186 [details] [diff] [review])
> It's actually rather hard to test a patch without a test case...

To test this for blocked popups you'll have to apply the patch of attachment 279320 [details] [diff] [review] from bug 393857. 
Then you'll have to add the attributes class="browser" and popupmenu="popupBlockerMenu" to the <notificationbox> added in this patch.
And of course you'll have to enable popup blocking.

Further more, you need a sidebar panel which opens popups: I'll upload a page which spawns popups as attachment to this bug so you can install it as a sidebarpanel. You can use  
http://edmullen.net/Mozilla/sidebarpreview.html to install the panel.

> Please add ids to all (these three and other) elements and use them below (we
> souldn't have more than one sidebar overlay in any given window).

The sidebar of SeaMonkey is propagated using RDF. Each panel or tab in the sidebar uses the same elements (among which the two browser elements and the newly add notificationbox element). If I set an id on those elements, a default sidebar would consist of for example 4 notificationboxes with the same id. 
I can give each browser and notificationbox a different id based on a RDF predicate, but the code is designed to have a common object for each sidebar panel, so getElementById won't be an option.

What I could do is reference the notificationbox with a property on sbPanel, just like the active browser element is referenced by the sbPanels iframe property, but I think that is not needed considering the few times iframe.parentNode is called.    

> >-        if (panel.is_sandboxed()) {
> >-          if (!panel.is_persistent())
> >-            iframe.setAttribute('src', 'about:blank');
> >-        }
> 
> This needs to be explained. Why do you drop that?

Here is the full block of code: 
 461         if (!panel.is_persistent()) {
 462           iframe.setAttribute('src', 'about:blank');
 463           load_state = content.getAttribute('loadstate');
 464           if (load_state == 'loading') {
 465             iframe.removeEventListener("load", panel_loader, true);
 466             content.setAttribute('hidden','true');
 467             iframe.setAttribute('loadstate', 'never loaded');
 468           }
 469         }
 470         if (panel.is_sandboxed()) {
 471           if (!panel.is_persistent())
 472             iframe.setAttribute('src', 'about:blank');
 473         }

Notice that on line 461 and 462 that if a panel is not persistant, the 'src' attribute of the iframe is set to 'about:blank'. It does not matter if the panel is sandboxed or not.
On line 470 - 473 the same is done, but only when the panel is sandboxed. So this part is not needed.

Comment 97

[Teune van Steeg]

Attachment: Page to test popup blocking

This html page opens popups at page load. To install it as a sidebar panel go to http://edmullen.net/Mozilla/sidebarpreview.html , fill the "tab URL" textbox with the URL of the attachment. Give the sidebarpanel a appropriate title ("Tab Name"), and click "install tab".

To enable popup blocking in the sidebar, see comment 96

Comment 98

[Karsten Düsterloh]

(In reply to comment #95)
> > This is particularly ugly - and what ids and such have actually
> > been invented for...
> > Add ids as needed/possible and use getElementById instead.
> 
> Why do you want people to slow down things?

"Slowness" is not actually the point. Superfast but unmaintainable code doesn't help anyone.
That said, Teune is right, of course, that "real" ids are not suitable here (see below). My apologies, I see too few sidebar code these days. :(
 
> > >+    var unsandboxed_iframe = content.childNodes.item(1).childNodes.item(0);
> > 
> > Again, the sidebar code is ugly enough, we don't want it get even worse...
> > Use ids.
> 
> Why?  The name unsandboxed_iframe is pretty clear to me, and I haven't even
> looked at the code!

That's not the point, the name is unsandboxed_iframe is okay.
But the "content.childNodes.item(1).childNodes.item(0);" bit is quite unstable, because it makes complicated assumptions which break easily.
It's just not as robust as it could be.



(In reply to comment #96)
> To test this

Thanks.

> If I set an id on those elements, a default
> sidebar would consist of for example 4 notificationboxes with the same id. 

Yeah, sorry about that.
We already _have_ doubled ids, both the hbox and the vbox of a panel share it (eg. urn:sidebar:panel:bookmarks), I for sure don't want to make it worse.

> What I could do is reference the notificationbox with a property on sbPanel,
> just like the active browser element is referenced by the sbPanels iframe
> property, but I think that is not needed considering the few times
> iframe.parentNode is called.    

Okay, agreed. But we should have comments along these childNodes cascades explaining which elements they try to grab, eg.

// grab notificationbox.firstChild
var sandboxed_iframe = content.childNodes.item(1).childNodes.item(0);

> Notice that on line 461 and 462 that if a panel is not persistant, the 'src'
> attribute of the iframe is set to 'about:blank'. It does not matter if the
> panel is sandboxed or not.
> On line 470 - 473 the same is done, but only when the panel is sandboxed. So
> this part is not needed.

Okay. 


I'll have a go with the test case and reevaluate after that, so supposing I don't find something else, the patch is r=me with that comment(s) added.

Comment 99

[[e-mail]]

(In reply to comment #94)
>>Index: suite/common/sidebar/sidebarOverlay.xul
>>===================================================================
>>+                <notificationbox flex="1" collapsed="true">
>>+                  <browser flex="1" class="browser-sidebar" src="about:blank"
>>+                    hidden="true" collapsed="true" content="?content"/>
>>+                  <browser flex="1" class="browser-sidebar" src="about:blank"
>>+                    hidden="true" collapsed="true" content="?content"
>>+                    type="content" context="contentAreaContextMenu" tooltip="aHTMLTooltip"/>
>>+                </notificationbox>
>Please add ids to all (these three and other) elements and use them below
You can't, they're in a template...

Comment 100

[Teune van Steeg]

One thing I forgot to mention in comment 96 about testing popups in the sidebar:
The following functions in navigator.js need to be changed as follows

function popupBlockerMenuShowing(event)
{
  var separator = document.getElementById("popupMenuSeparator");

  if (separator)
    separator.hidden = !createShowPopupsMenu(event.target, event.rangeParent);
}

function createShowPopupsMenu(parent, rangeParent)
{
  while (parent.lastChild && ("popup" in parent.lastChild))
    parent.removeChild(parent.lastChild);

  // rangeParent is currently either <notification> or <statusbar> depending on where the popup was opened from.
  // only notification has a control property.
  var browser = (rangeParent.control) ? rangeParent.control.activeBrowser : gBrowser.selectedBrowser;

  if (!browser)
    return false;

  var popups = browser.pageReport;

  if (popups.length == 0)
    return false;

  for (var i = 0; i < popups.length; i++) {
    var popup = popups[i];
    var menuitem = document.createElement("menuitem");
    var str = gNavigatorBundle.getFormattedString('popupMenuShow',
                                                  [popup.popupWindowURI.spec]);
    menuitem.setAttribute("label", str);
    menuitem.popup = popup;
    parent.appendChild(menuitem);
  }

  return true;
}

Comment 101

[Teune van Steeg]

(In reply to comment #93)
> (From update of attachment 279368 [details] [diff] [review])
> r=me.  Even if it's a toolkit bug, I still don't think the notification bars
> should slide in this case, so the new behavior is arguably better.
> 
(In reply to comment #92)
> That sounds like a bug in notificationbox.  Neil Deakin, is it an API
> requirement that the notification boxes not slide out in this case?

This could actually be the same problem as described in bug 363707. 

Comment 102

[[e-mail]]

Comment on attachment 279368 [details] [diff] [review]
fixing issues for checked in patch (checked in)

>-                this.mBrowser.parentNode.removeAllNotifications(true);
>+                this.mBrowser.parentNode.removeAllNotifications(false);
I guess this is OK for now but check back later e.g. when 363707 is fixed.

Comment 103

[Teune van Steeg]

Attachment: let the sidebar use notifications, v3

Addresses comments from comment #98; clarifies what is actually accessed by the parentNode/childNode cascades.

Comment 104

[Robert Kaiser]

Comment on attachment 279368 [details] [diff] [review]
fixing issues for checked in patch (checked in)

I checked in this part per IRC request from Teune.

Comment 105

[mv_van_rantwijk]

+                <notificationbox flex="1" collapsed="true">
+                  <browser flex="1" class="browser-sidebar" src="about:blank"
+                    hidden="true" collapsed="true" content="?content"/>
+                  <browser flex="1" class="browser-sidebar" src="about:blank"
+                    hidden="true" collapsed="true" content="?content"
+                    type="content" context="contentAreaContextMenu" tooltip="aHTMLTooltip"/>
+                </notificationbox>

Attributes should line up here.

+  this.parentNode.parentNode.firstChild.setAttribute('hidden','true');

Missing space between 'hidden', and 'true'.

+            // content is reloaded: remove notifications

This comment is incorrect.

+    var notificationbox = content.childNodes.item(1);
     if (this.is_sandboxed()) {
-      var unsandboxed_iframe = content.childNodes.item(2);
+      var unsandboxed_iframe = notificationbox.lastChild;
       this.iframe = unsandboxed_iframe;
     } else {
-      var sandboxed_iframe = content.childNodes.item(1);
+      var sandboxed_iframe = notificationbox.firstChild;

This could become a two liner.

Comment 106

[Teune van Steeg]

Attachment: let the sidebar use notifications, v4

Updated patch, addressing issues from comment 105. 
Taking over review.

Comment 107

[[e-mail]]

Comment on attachment 285731 [details] [diff] [review]
let the sidebar use notifications, v4

Sorry for taking so long to get around to this.

I wouldn't go overboard, but where you change code you could consider replacing the hidden and collapsed attributes with their equivalent properties.

The addition of the notification box actually gives you another opportunity for code simplification, as all the interesting boxes have two children:

>+  this.parentNode.parentNode.firstChild.setAttribute('hidden', 'true');
You can optimise this to this.parentNode.previousSibling.hidden = true;

>+    var notificationbox = this.get_content().childNodes.item(1);
You can optimise this to var notificationbox = this.get_content().lastChild;

>-        if (panel.is_sandboxed()) {
>-          if (!panel.is_persistent())
>-            iframe.setAttribute('src', 'about:blank');
>-        }
Can you explain why you removes this?

Comment 108

[Teune van Steeg]

Attachment: let the sidebar use notifications, v5

(In reply to comment #107)
> (From update of attachment 285731 [details] [diff] [review])
> Sorry for taking so long to get around to this.
> 
> I wouldn't go overboard, but where you change code you could consider replacing
> the hidden and collapsed attributes with their equivalent properties.
I went overboard ;), but it was easier to just do
%s/setAttribute('hidden', 'true')/hidden = true
etc. (I hope you don't mind reviewing these extra changes)

> >+    var notificationbox = this.get_content().childNodes.item(1);
> You can optimise this to var notificationbox = this.get_content().lastChild;
This won't work actually for me in the get_iframe function. It results in an error that notificationbox has no properties, and the sidebar just remains blank. (maybe because it is not populated properly yet?) 

> >-        if (panel.is_sandboxed()) {
> >-          if (!panel.is_persistent())
> >-            iframe.setAttribute('src', 'about:blank');
> >-        }
> Can you explain why you removes this?
> 
yes, it is explained in the last part of comment 96.

Comment 109

[[e-mail]]

(In reply to comment #108)
>>>+    var notificationbox = this.get_content().childNodes.item(1);
>>You can optimise this to var notificationbox = this.get_content().lastChild;
>This won't work actually for me in the get_iframe function. It results in an
>error that notificationbox has no properties, and the sidebar just remains
>blank. (maybe because it is not populated properly yet?) 
Actually it's a regression introduced almost three years ago by bug 198533...

Comment 110

[[e-mail]]

Comment on attachment 285731 [details] [diff] [review]
let the sidebar use notifications, v4

In that case, I like this version.

Comment 111

[Teune van Steeg]

Attachment: let the sidebar use notifications, final (checked in)

Final patch, v4 with css changes to avoid using the descendant selector.

Comment 112

[mv_van_rantwijk]

(In reply to comment #111)
> Created an attachment (id=286577) [details]
> let the sidebar use notifications, final
> 
> Final patch, v4 with css changes to avoid using the descendant selector.

Very nice, r+ me (your XUL BootCamp guru and notification's implementor for MultiZilla).

Comment 113

[Robert Kaiser]

Comment on attachment 286577 [details] [diff] [review]
let the sidebar use notifications, final (checked in)

Checked in this part on Teune's IRC request.

Comment 114

[Teune van Steeg]

With the last patch checked in, I believe this bug is fixed now.
The work to enable the notification bars for blocked popups, blocked xpinstall and missing plugins is done in bug 393120