persist is being broadcast

RESOLVED FIXED in mozilla0.9.6

Status

()

Core
XUL
P2
normal
RESOLVED FIXED
17 years ago
10 years ago

People

(Reporter: David Hyatt, Assigned: Chris Waterson)

Tracking

({regression})

Trunk
mozilla0.9.6
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed on trunk)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

17 years ago
The persist attribute is being broadcast, and it should never be broadcast. 
This was broken during the whole XULElement attribute cleanup.  

This bug is critical for 0.9.6, since it will cause permanent corruption in the
localstore.rdf file, which will prevent you from ever getting column headers
back in a tree widget.
(Reporter)

Comment 1

17 years ago
Taking the liberty of targeting to 0.9.6.  Hopefully that's ok.
Target Milestone: --- → mozilla0.9.6
(Assignee)

Comment 2

17 years ago
Created attachment 56854 [details] [diff] [review]
proposed fix
(Assignee)

Comment 3

17 years ago
I think that ought to fix it. Can you tell me what to do to test?
Status: NEW → ASSIGNED
Keywords: patch
(Reporter)

Comment 4

17 years ago
I can't see the surrounding code, but it looks like you're in a loop or 
something, so shouldn't it be if (!CanBroadcast) continue?
(Assignee)

Comment 5

17 years ago
Oh good grief. Yes, it should.
(Assignee)

Comment 6

17 years ago
So jag sez, to test this:

1. Bring up bookmarks.
2. Add an additional column (e.g., ``keyword'').
3. Close the window.
4. Open the window again.

Expected: keyword column appears
Actual: a big white hole where the keyword column should be
(Assignee)

Comment 7

17 years ago
(And by the way, this patch doesn't fix that problem.)
(Assignee)

Comment 8

17 years ago
Created attachment 56859 [details]
test case that illustrates this fixes the |persist| pushing problem
(Assignee)

Comment 9

17 years ago
So, the above test case shows that the patch fixes the |persist| pushing
problem, which may be necessary to fix this bug but doesn't appear to be sufficient.

Comment 10

17 years ago
I think the column's supposed to come up white. The bug is that the header (that
thing that says "Keyword" for the keyword column) is coming up white when it
shouldn't.

Comment 11

17 years ago
Created attachment 56861 [details]
screenshot displaying the problem
(Assignee)

Comment 12

17 years ago
You attached a PNG as text/plain, fokker! ;-)

But yeah, I still see white column headers with my patch (modified per hyatt)
that shouldn't be pusing |persist| anymore.

Comment 13

17 years ago
Comment on attachment 56861 [details]
screenshot displaying the problem

Thanks to the attachment manager errors like that can be fixed.
Attachment #56861 - Attachment is patch: false
Attachment #56861 - Attachment mime type: text/plain → image/png
(Assignee)

Comment 14

17 years ago
Pretty sneaky, sis! Yes, this patch _does not_ fix the problem.
(Assignee)

Comment 15

17 years ago
Created attachment 56937 [details] [diff] [review]
proper patch, with hyatt's nit picked.
(Assignee)

Updated

17 years ago
Attachment #56854 - Attachment is obsolete: true

Comment 16

17 years ago
waterson: and that was on a clean profile, right?

Hmmm... Does localstore have anything for that element?
(Assignee)

Comment 17

17 years ago
Yes, on a clean profile. This is what ends up in my localstore.rdf:

<RDF:Description
   about="chrome://communicator/content/bookmarks/bookmarks.xul#ShortcutURL"
   hidden="" />


(Assignee)

Comment 18

17 years ago
So it looks like what is happening here is that the <treecell> is
|hidden="true"|, but the <treecol> is |hidden=""|. I suspect the problem has to
due with the fact that the persist information is set using a ``quiet'' update,
which doesn't trigger the document notification. Since the broadcaster
maintenance now happens during document notification, the empty string is
propagated from teh <treecol> to the <treecell>.
(Assignee)

Comment 19

17 years ago
Created attachment 57183 [details] [diff] [review]
better fix

Okay, this fixes the bug. The problem was that we weren't doing a noisy notify
when restoring persisted attribute values. Since the broadcaster code now
relies on the AttributeChanged notification to work, we were missing these
updates. The fix is to make |ApplyPersistentAttributesToElements| do a noisy
notify. Unfortunately, to avoid sending RDF off into the weeds (gotta file a
separate bug), we need to make sure not to re-enter when the document is
notified, trying to re-persist the value that we just restored!
Attachment #56937 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
OS: Windows 2000 → All
Priority: -- → P2
Hardware: PC → All
(Reporter)

Comment 20

17 years ago
sr=hyatt
(Assignee)

Comment 21

17 years ago
cc'ing shaver since jag is awol.
(Assignee)

Updated

17 years ago
Keywords: regression

Comment 22

17 years ago
Comment on attachment 57183 [details] [diff] [review]
better fix

>Index: nsXULDocument.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v
>retrieving revision 1.455
>diff -u -r1.455 nsXULDocument.cpp
>--- nsXULDocument.cpp	2001/11/02 01:53:04	1.455
>+++ nsXULDocument.cpp	2001/11/09 03:06:39

>@@ -2002,9 +2017,7 @@
>     }
> 
>     // Synchronize broadcast listeners
>-    if (mBroadcasterMap &&
>-        (aNameSpaceID == kNameSpaceID_None) &&
>-        (aAttribute != nsXULAtoms::id)) {
>+    if (mBroadcasterMap && CanBroadcast(aNameSpaceID, aAttribute)) {

This change makes it so that if the namespace isn't "none", it will 
broadcast the attribute (which seems like a good change, but not what
the old code was doing).

If this was intended, r=jag.
(Assignee)

Comment 23

17 years ago
This is what the old, old code was doing. (Well, it's what hyatt meant it to be
doing -- that code stifled |id|, |ref|, and |persist| on _any_ namespace, which
is overkill. See nsXULElement.cpp r1.371.)

Comment 24

17 years ago
Comment on attachment 57183 [details] [diff] [review]
better fix

r=jag then :-)
Attachment #57183 - Flags: review+
(Assignee)

Updated

17 years ago
Whiteboard: fixed on trunk
a=blizzard on behalf of drivers for 0.9.6
Keywords: mozilla0.9.6+
(Assignee)

Comment 26

17 years ago
Fixed on the mozilla-0.9.6 branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Updated

10 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.