Open Bug 285076 Opened 15 years ago Updated 10 months ago

XUL Template Builder isn't working with Dynamic overlays

Categories

(Core :: XUL, defect)

x86
Windows XP
defect
Not set

Tracking

()

People

(Reporter: mscott, Unassigned)

References

(Depends on 2 open bugs)

Details

Attachments

(8 obsolete files)

With the new preferences design, elements that use the XUL template builder in
Thunderbird (such as the privacy / general / sender's white list menu list) fail
to get their RDF datasources reflected into the drop down menu lists.
just to avoid dupes, this also effects the character encoding language menus.
*** Bug 285302 has been marked as a duplicate of this bug. ***
I can ACK this bug on Windows 2K too. So changing OS to "All" would usefull IMHO
Attached patch the fix (obsolete) — Splinter Review
if we have already finished the initial reflow, then we should notify the
document and any observers of any content changes by the xul template builder.
This is the matching set of changes that were made to the XUL Document to
support dynamic overlays.
Attachment #176783 - Flags: second-review?(bryner)
Attachment #176783 - Flags: first-review?(bugs)
FYI, i changed the comment in this patch to say:

  // If we've already done initial reflow, then set notify to true
  // to force notifications when content elements are added to the document.  

Attachment #176783 - Flags: second-review?(bryner) → second-review+
Comment on attachment 176783 [details] [diff] [review]
the fix

Changing review request to jst, since this isn't technically in my code.
Attachment #176783 - Flags: first-review?(bugs) → first-review?(jst)
*** Bug 285427 has been marked as a duplicate of this bug. ***
Hey Johnny, can you review this patch when you have some time? 
*** Bug 286568 has been marked as a duplicate of this bug. ***
Comment on attachment 176783 [details] [diff] [review]
the fix

r=jst
Attachment #176783 - Flags: first-review?(jst) → first-review+
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Regressions by the fix ????
with BEAST Tinderbox builds (hourly)
on new and old profile i get all the above

   1. double entry in Bookmarks Toolbar Folder
   2. double entry in Download Manager
   3. double entry in Extensions Manager
   4. triple entry in the toolbar for bottons created by costomizable toolbar
bottons extension
in dom inspector i don't see the double entry for the Bookmarks Toolbar Folder.
Re comment 11 

This is bug 286709.
(In reply to comment #12)
> Re comment 11 
> 
> This is bug 286709.

286709 is new bug. but what caused it???. maybe the fix of this bug???

Indeed? I meant that while writing my comment. A PITA bug, to say so :(
I had to back this out because of regressions it caused with the Firefox
bookmarks manager. I guess we'll have to come up with some other way of
determining when to set the notifiation flag to true besides checking for
initial reflow.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch another approach (obsolete) — Splinter Review
this idea builds upon the previous fix but instead of testing to see if we've
done the initial reflow before passing in TRUE for notifications, we instead
look at the xul document and check to see if it thinks we are processing a
dynamic overlay.

This more explicit approach avoids the duplicate content issues the previous
patch had. The downside is that it now exposes dynamic overlay loading state as
a property on a XUL document. And currently, in the case of errors, its
possible that this flag would not get cleared so it still needs some work.
Flags: blocking1.8b4+
Whiteboard: patch by mscott, needs review jst/bryner
Rob/Doron, this seems related to the issues with richlistbox and templates, is
there a more generic solution that could happen here?
(In reply to comment #17)
> Rob/Doron, this seems related to the issues with richlistbox and templates, is
> there a more generic solution that could happen here?

The problem in the managers is that the widget never knows when its template has
finished creating the data, not sure if is is related
Blocks: 301858
Blocks: 301854
Does anyone else have any good ideas on how to know when it is appropriate to
issue notifications when building new content other than the approach I started
to work on in comment 16? Unfortunately checking to see if we've already done an
initial reflow and only sending notifications if we have works for dynamic
overlays but causes problems for non dynamic overlays. Hence my attempt to try
to add a dynamic overlay check on the xul document.
> Does anyone else have any good ideas on how to know when it is appropriate to
> issue notifications

Notifications must ALWAYS be issued, if we want to avoid problems.  If aNotify
is passed as false to AppendChildTo or RemoveChildAt or whatever, then the
caller takes the responsibility of later sending the notification onto himself
(as the content sink does, for example).  Trying to actually not send
notifications based on reflow state or anything else is a kludge which may lead
to "correct" rendering (in that the frame model will be right) but can also lead
to incorrect functioning (for example content lists missing data they should
have in them).
Boris, currently notifications never get generated here as PR_FALSE is always
passed in as the notification parameter. This means that in the case of dynamic
overlays where the template content builder is building content after the
document has loaded, that the UI never shows the new content because it never
gets notified. I tried to fix this once before by always passing in TRUE for
aNotify if we've already done the initial document load, but that change caused
content to show up twice in several areas of the Firefox UI. Hence my attempt to
try to be more precise by knowing if we are loading a dynamic overlay or not
when deciding the value of aNotify. 

http://lxr.mozilla.org/mozilla/source/content/xul/templates/src/nsXULContentBuilder.cpp#1136

Right.  My point is that the current overlay code is just generally buggy if it
always passes PR_FALSE and never notifies after that...

Did someone figure out why we're ending up with doubled content?  That usually
indiecates either doubled notification or content getting a frame constructed
before it's notified...
*** Bug 303357 has been marked as a duplicate of this bug. ***
*** Bug 301934 has been marked as a duplicate of this bug. ***
(In reply to comment #22) 
> Did someone figure out why we're ending up with doubled content?  That usually
> indiecates either doubled notification or content getting a frame constructed
> before it's notified...

Hey Boris, here's what I'm seeing in the debugger when opening up the themes
dialog  with one theme. In the onload handler for the dialog, we set a data
source href attribute on the widget to trigger the content building code
http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/content/extensions.js#177

In reponse to getting the href attribute set, I see two calls to
nsXULDocument::ContentAppended for the element. The first call is triggered by
nsXULElement::InsertChildAt from the XULSortService. Here's the relevant stack
trace.

nsXULDocument::ContentAppended
nsXULElement::InsertChildAt
XULSortServiceImpl::InsertContainerNode
nsXULContentBuilder::BuildContentFromTemplate
nsXULContentBuilder::CreateContainerContents
nsXULContentBuilder::CreateTemplateAndContainerContents
nsXULContentBuilder::RebuildAll
nsXULTemplateBuilder::Rebuild
nsXULTemplateBuilder::AttributeChanged
nsXULContentBuilder::AttributeChanged
nsXULDocument::AttributeChanged
nsXULElement::SetAttrAndNotify
nsXULElement::SetAttr
nsGenericElement::SetAttr
nsGenericElement::SetAttribute
nsXULElement::SetAttribute

Now, the stack starts to unwind all the way down to:
nsXULContentBuilder::RebuildAll. After calling 
CreateTemplateAndContainerElements, it then calls ContentAppended. This is the
second call for the same element:

http://lxr.mozilla.org/mozilla/source/content/xul/templates/src/nsXULContentBuilder.cpp#1909

And that is how we end up showing the same element twice if we always pass in
true for aNotify in nsXULContentBuilder::CreateTemplateAndContainerContents



Attachment #182016 - Attachment is obsolete: true
Attachment #176783 - Attachment is obsolete: true
Attached patch another fix (obsolete) — Splinter Review
Boris, what do you think of something like this?

1) CreateTemplateAndContainerContents now always passes PR_TRUE for aNotify
instead of PR_FALSE into CreateContainerContents. This means that
CreateTemplateAndContainerContents will always result in ContentAdded
notifications at the document level.

2) Get rid of the code in nsXULContentBuilder::RebuildAll which manually called
ContentAppended after CreateTemplateAndContainerContents since
CreateTemplateAndContainerContents will now generate the notification for us.
Attachment #192260 - Flags: first-review?(bzbarsky)
Status: REOPENED → ASSIGNED
Whiteboard: patch by mscott, needs review jst/bryner
Boris, do you have any initial thoughts on my proposed fix? If it seems
reasonable, I'd like to start getting trunk coverage on it as soon as I can.
I don't know this code well enough to judge offhand, and I won't be able to
review this for at least a week or two, if only because I have to learn the
code.  If I understand what's going on at all, though, I suspect this will
severely degrade the performance of XUL templates by making the number of
notifications for an insertion O(number of nodes inserted) instead of 1.
(In reply to comment #28)
> I don't know this code well enough to judge offhand, and I won't be able to
> review this for at least a week or two, if only because I have to learn the
> code.  If I understand what's going on at all, though, I suspect this will
> severely degrade the performance of XUL templates by making the number of
> notifications for an insertion O(number of nodes inserted) instead of 1.

hmm I must have misunderstood you in comment 20 where I thought you were
suggesting we should always pass in PR_TRUE for the aNotify parameter whenever
an element is added or removed on the document.
What I said is that notifications must always be issued, either by passing
aNotify == PR_TRUE or by doing the notifying yourself later in batches like the
content sink.  The latter is faster, which is why the content sink does it.
Attached patch another fix (obsolete) — Splinter Review
Ok, here's another approach that hopefully won't break batching of content
notification calls. 

Make aNotify an argument to CreateTemplateAndContainerContents. This allows
callers that wish to block notifications (such as ::RebuildAll), to pass in
PR_FALSE for aNotify.
Attachment #192260 - Attachment is obsolete: true
Attachment #193515 - Flags: first-review?(bzbarsky)
Comment on attachment 192260 [details] [diff] [review]
another fix

clearing an obsolete request since this patch breaks batching notifications.
Attachment #192260 - Flags: first-review?(bzbarsky)
Flags: blocking1.8b5+
Flags: blocking1.8b5+
So what's the net effect of this patch?  Is it to notify when we didn't use to
before, basically?  It'd really help to have more context and to use the -p
option to diff, by the way....
Attached patch cvs diff -p for boris (obsolete) — Splinter Review
same patch, with cvs diff -p for Boris.

Boris, this patch implements your idea in comment 20 about always generating
notifications in order to avoid problems, without degrading performance in
RebuildAll where we want to batch the construction notifications. Hope that
helps, thanks for looking at the patch.

It might be interesting to check this patch into the trunk for a tinderbox
cycle to see if we get any data from the automated xul performance tests.
Comment on attachment 193515 [details] [diff] [review]
another fix

I meant something more like diff -up8.... ;)

>Index: nsXULContentBuilder.cpp

>     nsresult
>     CreateTemplateAndContainerContents(nsIContent* aElement,
>+                                       PRBool aNotify,
>                                        nsIContent** aContainer,
>                                        PRInt32* aNewIndexInContainer);

Please add the following at the beginning of that method:

NS_PRECONDITION(!aContainer == !aNewIndexInContainer,
		"Must ask for either both or neither");
NS_PRECONDITION(aNotify || aContainer, "Notifications will get lost");

>@@ -1534,7 +1536,7 @@
>+    return CreateTemplateAndContainerContents(aElement, PR_TRUE /* aNotify */, nsnull /* don't care */, nsnull /* don't care */);

Why not manually notify like RebuildAll does?  It seems like that should be
faster, I think, if it gets the right notification This code is called for any
XUL element which might have generated kids, after all.

That said r=bzbarsky for this patch if tinderbox tests are not affected, I
guess.
Attachment #193515 - Flags: first-review?(bzbarsky) → first-review+
Component: Preferences → XP Toolkit/Widgets: XUL
Flags: second-review+
Flags: first-review+
Product: Toolkit → Core
Version: unspecified → Trunk
Attachment #193515 - Flags: superreview+
Attachment #193515 - Flags: review+
Attached patch yet another possible solution (obsolete) — Splinter Review
This approach makes nsXULContentBuilder::CreateContents look a lot like
RebuildAll in that it manually fires the content appended notification. I'm not
sure which approach is better. I'm leaning towards this one though. 

I wonder if I should be doing 

nsCOMPtr<nsIDocument> doc = mRoot->GetDocument();

instead of aElement->GetDocument

although it shouldn't matter. I'll do some more testing.
Ideally you want to use container->GetCurrentDoc() for the document.  That'll
guarantee that it's the right document to notify about content being added to
|container|.

Do templates always only append content to a container?  They can't insert it
somewhere in the middle of it?
The comment for BuildContentFromTemplate 

http://lxr.mozilla.org/seamonkey/source/content/xul/templates/src/nsXULContentBuilder.cpp#401

implies that content is only appended by the template builder:

"|aContainer| is an out parameter that will be set to the first
container element in the "real" content tree to which content
was appended." A brief code inspection of that method seems to support that comment.

Hmm, the other callers of ContentAppended in this class all seem to get the
document from mRoot. My gut tells me we should follow that pattern, but I don't
have a technical opinion one way or the other on using mRoot vs.
container->GetContainerDoc()
Ah, ok.  In that case this last solution is best, imo.
One other thing I just realized.  It's worth calling BeginUpdate and EndUpdate
around the notification.  Not doing that can easily lead to bugs.  We have a
mozAutoDocUpdate class to handle this automatically, though in this case that's
not really necessary since there are no early returns involved.
Attached patch updated fix using mRoot (obsolete) — Splinter Review
Ok, this patch uses mRoot to get the document to be consistent with the rest of
the callers in this file.

None of the other 3 callers of ContentAppended are calling
BeginUpdate/EndUpdate in nsXULContentBuilder. Should they be? If so, I'd break
this into two parts, just the basic change for the 1.8 branch and then fixing
all callers to use BeginUpdate/EndUpdate in the XULContentBuilder for the
trunk.
Attachment #193515 - Attachment is obsolete: true
Attachment #193839 - Attachment is obsolete: true
Attachment #193974 - Attachment is obsolete: true
Attachment #194259 - Flags: superreview?(bzbarsky)
Comment on attachment 194259 [details] [diff] [review]
updated fix using mRoot

>Index: nsXULContentBuilder.cpp

> nsXULContentBuilder::CreateTemplateAndContainerContents(nsIContent*

>+    NS_PRECONDITION(!aContainer == !aNewIndexInContainer,
>+		                "Must ask for either both or neither");

Fix the indent?

>@@ -1124,17 +1126,17 @@ nsXULContentBuilder::CreateTemplateAndCo
>-    if (resource) {
>+    if (resource) {      

Please don't add those trailing spaces.

> +        nsCOMPtr<nsIDocument> doc = mRoot->GetDocument();

This can be a weak nsIDocument* ref, and please use GetCurrentDoc(), not
GetDocument().

sr=bzbarsky, with those changes though in the future we probably want to not
use mRoot in a lot of places in this file...

> None of the other 3 callers of ContentAppended are calling
> BeginUpdate/EndUpdate in nsXULContentBuilder. Should they be?

Yes, if we ever hope to get incremental layout working for XUL... I agree that
this should probably be a separate bug and not something to worry about on the
1.8 branch.
Attachment #194259 - Flags: superreview?(bzbarsky) → superreview+
uses a weak reference to the document, calls GetCurrentDoc and fixes some white
space issues.

carrying forward bz's sr.
Attachment #194259 - Attachment is obsolete: true
Attachment #194378 - Flags: superreview+
Attachment #194378 - Flags: review?(jst)
I've checked this into the trunk so we can get some baking time on the trunk.
I'll also be watching the performance numbers on tinderbox just in case.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago14 years ago
Resolution: --- → FIXED
This checkin caused memory leaks -- see bug 307160
Depends on: 307160
What needs to happen here to get this onto the branch? Time is running short for
1.8b5.
What needs to happen: We're waiting to verify that the leak this introduced is
really fixed on the trunk before we land this on the branch. The leak is BUg
#307160.
Comment on attachment 194378 [details] [diff] [review]
updated patch based on review comments

I've verified that the leak fix (Bug 307160) is fixed on the trunk. So both of
these bugs are ready to go for the branch.
Attachment #194378 - Flags: review?(jst) → approval1.8b5?
Comment on attachment 194378 [details] [diff] [review]
updated patch based on review comments

Approved for 1.8b5.
Attachment #194378 - Flags: approval1.8b5? → approval1.8b5+
Comment on attachment 194378 [details] [diff] [review]
updated patch based on review comments

Approved for 1.8b5.
this is now fixed on the branch.
Keywords: fixed1.8
This caused bug 310833 -- basically, a wide variety of templates (including
especially templates that are interacting with XBL-bound elements) are broken.
this has been backed out due to Bug #310833.

no longer a blocker as I've worked around it for 1.5
Status: RESOLVED → REOPENED
Flags: blocking1.8b5+
Keywords: fixed1.8
Resolution: FIXED → ---
Attachment #194378 - Attachment is obsolete: true
Attachment #194378 - Flags: approval1.8b5+
So for trunk, we should think about how to fix this...  The basic problem the
approach we tried here ran into is that in some cases
nsXULContentBuilder::CreateContents needs to NOT notify.  In particular, if it's
being called by the GetChildCount() method of a XUL element while we're
constructing frames...  otherwise we end up notifying (which constructs frames)
and then just constructing frames.  Which gives us two frames per node, which is
bad.

The problem is that GetChildCount() has no idea who's calling it and why, and
I'd really rather not change every single caller of it.  So what are our options
here?
This seems related to this bug.
When launching Thunderbird 1.5 (20051218) and going to Edition > Preferences > Display > Fonts both "Outgoing Mail" and "Incoming mail" dropdown lists work correctly. If closing the Preferences window and then returning to Display > Fonts item, the dropdown lists don't work anymore --you cannot choose a different charset-- and the text of the charset is superposed over the dropdown list arrows.
If exiting Thunderbird then returning to this preference, it works again the first time this preference is displayed, then don't work the second time (at least for the en-US version, for the French one (build 20051201), I had to remove XUL.mfasl to make this preference work again).
Hum, I hope I was clear in my explanation...
My conf : GNU/Linux 2.6.13-15 (OpenSuse)

Is this a blocker for 1.5 release?
Update :
Sorry for the bugspam. For the French version, same behavior as for the en-US one: you only need to restart Thunderbird for the preference to work fine again. Don't know why I had to remove XUL.mfasl before.
(In reply to comment #55)
> This seems related to this bug.
> both "Outgoing Mail" and "Incoming mail" dropdown lists work correctly.
> If closing the Preferences window and then returning to Display > Fonts item,
> the dropdown lists don't work anymore --you cannot choose a different
> charset-- and the text of the charset is superposed over the dropdown
> list arrows.

This problem is bug 315057.  I agree, it appears related.  It was denied 1.5-blocker status; maybe if we're lucky someone will bother to fix it, and then
we can get it included in a subsequent 1.5.x. release.
No longer blocks: 301858
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9-
What is the story on this bug? Is it fixed, WFM, or what? I was just getting rid of some old bugs in my inbox. Sorry for the bother. Thanks.
No, not fixed AFAIA
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: nobody → xptoolkit.widgets
Assignee: mscott → nobody
Status: REOPENED → NEW
Depends on: 1508119
You need to log in before you can comment on or make changes to this bug.