377696 - general.xml cleanup

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Neil Deakin]

- setters should return values
- statusbarpanel should use image not src property

Comment 1

[Neil Deakin]

Attachment: cleanup

Comment 2

[Neil Deakin]

Attachment: include tests

Comment 3

[Asaf Romano (gone)]

Index: toolkit/content/tests/widgets/test_statusbar.xul
===================================================================
RCS file: toolkit/content/tests/widgets/test_statusbar.xul
diff -N toolkit/content/tests/widgets/test_statusbar.xul
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ toolkit/content/tests/widgets/test_statusbar.xul	3 Aug 2007 14:23:41 -0000
@@ -0,0 +1,42 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
+<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css" type="text/css"?>
+<!--
+  XUL Widget Test for statusbar
+  -->
+<window title="Statusbar Test"
+oncommand="alert('aa');"

?

Comment 4

[Neil Deakin]

(In reply to comment #3)
> Index: toolkit/content/tests/widgets/test_statusbar.xul
> ===================================================================
> RCS file: toolkit/content/tests/widgets/test_statusbar.xul
> diff -N toolkit/content/tests/widgets/test_statusbar.xul
> --- /dev/null   1 Jan 1970 00:00:00 -0000
> +++ toolkit/content/tests/widgets/test_statusbar.xul    3 Aug 2007 14:23:41
> -0000
> @@ -0,0 +1,42 @@
> +<?xml version="1.0"?>
> +<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
> +<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css"
> type="text/css"?>
> +<!--
> +  XUL Widget Test for statusbar
> +  -->
> +<window title="Statusbar Test"
> +oncommand="alert('aa');"
> 
> ?
> 

Oops, I'll remove that.

Comment 5

[Asaf Romano (gone)]

Comment on attachment 275116 [details] [diff] [review]
include tests

+      <xul:image .. xbl:inherits="image,image=src"/>

explain this again please.

r=mano otherwise.

Comment 6

[Neil Deakin]

> +      <xul:image .. xbl:inherits="image,image=src"/>
> 
> explain this again please.

The image attribute is the right attribute to use, yet I still allow src for backwards compatibility.

Comment 7

[Peter Van der Beken [:peterv]]

This turned qm-winxp01 red:

*** 39101 INFO Running /tests/toolkit/content/tests/widgets/test_deck.xul...
...
<a href="#tbend">
<p class="error">buildbot.slave.commands.TimeoutError: SIGKILL failed to kill process</p>
</a>

Comment 8

[Peter van der Woude [:Peter6]]

This broke numerous extensions using an icon in the statusbar

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #6)
> > +      <xul:image .. xbl:inherits="image,image=src"/>
> > 
> > explain this again please.
> 
> The image attribute is the right attribute to use, yet I still allow src for
> backwards compatibility.

But you're removing the property, which is inconsistent and a little bit confusing. I'd support both the attribute and the property or remove them altogether (and I think I prefer the latter.)

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #7)
> This turned qm-winxp01 red:

Could the problem be that chrome mochitests were put into _tests/testing/mochitest/tests/ instead of _tests/testing/mochitest/chrome/ ?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... or that the tests shouldn't be chrome mochitests, but should instead use relative paths to load SimpleTest.js, etc.?

Comment 12

[Asaf Romano (gone)]

Attachment: fix bustage

Comment 13

[Asaf Romano (gone)]

Checking in test_deck.xul;
/cvsroot/mozilla/toolkit/content/tests/widgets/test_deck.xul,v  <--  test_deck.xul
new revision: 1.2; previous revision: 1.1
done
Checking in test_props.xul;
/cvsroot/mozilla/toolkit/content/tests/widgets/test_props.xul,v  <--  test_props.xul
new revision: 1.2; previous revision: 1.1
done
Checking in test_statusbar.xul;
/cvsroot/mozilla/toolkit/content/tests/widgets/test_statusbar.xul,v  <--  test_statusbar.xul
new revision: 1.2; previous revision: 1.1
done

Comment 14

[Asaf Romano (gone)]

Attachment: oops

Comment 15

[Neil Deakin]

Attachment: yeah, didn't want to remove the src property

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 279591 [details] [diff] [review]
yeah, didn't want to remove the src property

> function test_statusbar()
> {
>   var panel = $("panel");
>   ok(panel.label, "OK", "statusbarpanel label");
>   ok(panel.image, "happy.png", "statusbarpanel image");
>+  panel.src = "sad.png";
>+  ok(panel.src, "sad.png", "statusbarpanel set src");

Hm, I'm not familiar with the testing frameworks ... doesn't this succeed no matter if panel supports that property?

Comment 17

[Neil Deakin]

(In reply to comment #16)
> >+  panel.src = "sad.png";
> >+  ok(panel.src, "sad.png", "statusbarpanel set src");
> 
> Hm, I'm not familiar with the testing frameworks ... doesn't this succeed no
> matter if panel supports that property?
> 

True, I'll add a second attribute check as well.

Comment 18

[Neil Deakin]

Attachment: add attribute test

Comment 19

[Asaf Romano (gone)]

Comment on attachment 279592 [details] [diff] [review]
add attribute test

r=mano

Comment 20

[Neil Deakin]

Checked in the last patch.

Comment 21

[Jason Barnabe (np)]

(In reply to comment #8)
> This broke numerous extensions using an icon in the statusbar
> 

So was this supposed to be backwards compatible or not? Stylish's status bar icon doesn't work with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007090701 Minefield/3.0a8pre

The code is:

<statusbarpanel id="stylish-panel" src="chrome://stylish/skin/smallicon.png" class="statusbarpanel-menu-iconic" tooltiptext="Stylish" onclick="stylishCommonOverlay.handleStatusClick(event)">(In reply to comment #8)

Comment 22

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

(In reply to comment #21)

> Stylish's status bar icon doesn't work with Mozilla/5.0 (X11; U; Linux i686;
> en-US; rv:1.9a8pre) Gecko/2007090701 Minefield/3.0a8pre

That's bug 395099.