253225 - Land XTF branch

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Alex Fritze]

XTF is a framework for implementing XML elements for Mozilla outside of
content/layout, currently checked in on branch XTF_20040312_BRANCH.

Comment 1

[Alex Fritze]

Attachment: first attempt

This patch contains most of the XTF branch. Missing are the xtf js framework
which is dependent on other bugs (#238324), and the test directory which only
contains js samples and is thus dependent on the xtf js framework.

Comment 2

[Alex Fritze]

Attachment: second attempt

Patch updated to trunk and including some missing files. Attribute handling is
repaired. New features: 
- insertion points a la XBL (only working for xul visuals so far).
- nsIXTFAttributeHandler::handlesAttribute(in nsIAtom attr): determines whether
an attribute will be handled by xtf element or wrapper.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I think XTF should be able to handle namespaced attributes. Namespace support
*might* be needed by the XForms implementation, when it is used with 
XML Events attributes.

Comment 4

[Alex Fritze]

XTF can handle namespaced attributes, but they are always handled (i.e. stored)
by the wrapper. The non-namespace restriction only applies to attributes which
the 'inner' XTF element wants to store in some custom (i.e. non-string) format.
E.g. an XTF element might have an attribute 'xpos' which it stores internally as
a float. 

Comment 5

[Alex Fritze]

Attachment: Third attempt

New patch, updated to trunk. Main new features:
- cleaned up API/class hierarchy
- added 'nsIXTFVisual' interface with 'applyDocumentStyleSheets' attribute and
'didLayout' notification.

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 157049 [details] [diff] [review]
Third attempt

Setting review requests so that jst and I remember to look at this.

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 157049 [details] [diff] [review]
Third attempt

>--- content/base/src/nsContentUtils.cpp	4 Aug 2004 19:48:55 -0000	1.79
>+++ content/base/src/nsContentUtils.cpp	6 Aug 2004 14:31:10 -0000	1.76.2.3
>@@ -203,6 +210,22 @@ nsContentUtils::GetParserServiceWeakRef(
>   return sParserService;
> }
> 
>+nsIXTFService*
>+nsContentUtils::GetXTFServiceWeakRef()
>+{
>+  // XXX: This isn't accessed from several threads, is it?

Our DOM is not threadsafe, so I wouldn't worry about this.


>--- layout/html/style/src/nsCSSFrameConstructor.cpp	26 Aug 2004 08:34:40 -0000	1.974
>+++ layout/html/style/src/nsCSSFrameConstructor.cpp	26 Aug 2004 11:56:45 -0000	1.921.2.17
>@@ -6789,6 +6816,135 @@ nsCSSFrameConstructor::ConstructMathMLFr
> }
> #endif // MOZ_MATHML
> 
>+// XTF 
>+#ifdef MOZ_XTF
>+nsresult
>+nsCSSFrameConstructor::ConstructXTFFrame(nsIPresShell*            aPresShell,
>+                                         nsPresContext*           aPresContext,
>+                                         nsFrameConstructorState& aState,
>+                                         nsIContent*              aContent,
>+                                         nsIFrame*                aParentFrame,
>+                                         nsIAtom*                 aTag,
>+                                         PRInt32                  aNameSpaceID,
>+                                         nsStyleContext*          aStyleContext,
>+                                         nsFrameItems&            aFrameItems)
>+{
>+#ifdef DEBUG
>+//  printf("nsCSSFrameConstructor::ConstructXTFFrame\n");
>+#endif
>+  nsresult  rv = NS_OK;
>+  PRBool isAbsolutelyPositioned = PR_FALSE;
>+  PRBool isFixedPositioned = PR_FALSE;
>+  PRBool forceView = PR_FALSE;
>+  PRBool isBlock = PR_FALSE;
>+  
>+  //NS_ASSERTION(aTag != nsnull, "null XTF tag");
>+  //if (aTag == nsnull)
>+  //  return NS_OK;
>+
>+  // Initialize the new frame
>+  nsIFrame* newFrame = nsnull;
>+   
>+  // See if the element is absolute or fixed positioned
>+  const nsStyleDisplay* disp = aStyleContext->GetStyleDisplay();
>+  if (NS_STYLE_POSITION_ABSOLUTE == disp->mPosition) {
>+    isAbsolutelyPositioned = PR_TRUE;
>+  }
>+  else if (NS_STYLE_POSITION_FIXED == disp->mPosition) {
>+    isFixedPositioned = PR_TRUE;
>+  }
>+
>+  nsCOMPtr<nsIXTFElementWrapperPrivate> xtfElem = do_QueryInterface(aContent);
>+  NS_ASSERTION(xtfElem, "huh? no xtf element?");
>+  switch(xtfElem->GetElementType()) {
>+    case nsIXTFElement::ELEMENT_TYPE_SVG_VISUAL:
>+#ifdef MOZ_SVG
>+      rv = NS_NewXTFSVGDisplayFrame(aPresShell, aContent, &newFrame);
>+#else
>+      NS_ERROR("xtf svg visuals are only supported in mozilla builds with native svg");
>+#endif
>+      break;
>+    case nsIXTFElement::ELEMENT_TYPE_XML_VISUAL:
>+      // XXX examine display style
>+#ifdef DEBUG
>+      printf("creating xtfxmldisplay frame\n");
>+#endif
>+      rv = NS_NewXTFXMLDisplayFrame(aPresShell, &newFrame);

I'd really like to be able to make XTF elements that have an nsInlineFrame,
either by giving a different type of visual or simply using display: inline on
my element.  I'd hacked this on the XForms branch (though not checked in yet)
by just calling ConstructFrameByDisplayType() here.  However, that's against an
older revision of XTF and predates nsXTFXMLDisplayFrame frame, which seems to
be required for new notification types.  A couple of ideas come to mind:

1. Just worry about display: inline by making a new frame type that inherits
from nsInlineFrame.
2. Instead of inheriting from the base type, construct it on the fly as a
member (from the display type) and forward all of the method calls to it. 
Might be tricky due to the wrapped frame not finding itself in its parent's
child list, etc.


dbaron or bz should probably review the CSSFrameConstructor changes.


>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ content/xtf/readme.txt	26 Aug 2004 11:56:45 -0000	1.1.2.1

This is a good start; I'd love to see as much documentation as we can get.



>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ content/xtf/public/nsIXTFElement.idl	26 Aug 2004 11:56:47 -0000	1.1.2.9
>+  void getScriptingInterfaces(out unsigned long count,
>+                              [array, size_is(count), retval] out nsIIDPtr array);

One thing I found is that I had to explicitly include nsIDOMElement,
nsIDOMEventTarget, and nsIDOM3Node in the list of scriptable interfaces.  Is
that intentional?

>+  // Event notifications:
>+
>+  void willChangeDocument(in nsISupports newDoc);
>+  void documentChanged(in nsISupports newDoc);
>+  
>+  void willChangeParent(in nsISupports newParent);
>+  void parentChanged(in nsISupports newParent);

Why do these (and other) notifications get passed nsISupports instead of
nsI[DOM]Document, nsIDOMElement, etc?

Also, might be useful to have a %{C++ section that has macros for stub
implementations of various categories of notifications, like what
nsIDocumentObserver has.

Additionally, I found that for XForms I needed to be notified about
doneAddingChildren(), which is something the content sink knows about.	The
notification is normally suppressed except for certain cases as an
optimization; I'd say if the element is in an unknown namespace it should
notify unconditionally and that notification should go through to the
XTFElement.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ content/xtf/public/nsIXTFVisual.idl	26 Aug 2004 11:56:47 -0000	1.1.2.1
>+interface nsIXTFVisual : nsIXTFElement
>+{
>+  // The content tree routed in 'visualContent' will be rendered where

Is this supposed to be "rooted"?

>+  // the xtf visual is placed into an appropriate context. It can be
>+  // dynamically manipulated during the lifetime of the xtf visual.

Is it possible to change the visualContent on the fly?	If not, say so,
otherwise, how do you notify the wrapper that you've done this?

>+  readonly attribute nsIDOMElement visualContent;
>+
>+  // Children of the xtf visual which are themselves 'visuals'
>+  // (i.e. built-ins elements, other xtf visuals or xbl elements with
>+  // visual <content>) will be rendered in the context of
>+  // 'insertionPoint'. 'insertionPoint' should either be null (in
>+  // which case the children will not be automatically rendered) or
>+  // point into the 'visualContent' tree.

This needs to be clearer.  Do you mean that they are inserted as children of
insertionPoint, or that they are inserted after insertionPoint as siblings of
it?

>+  readonly attribute nsIDOMElement insertionPoint;
>+
>+  // If 'true', document style sheets will apply to the visual
>+  // content. If 'false', only UA style sheets will be applied.
>+  readonly attribute boolean applyDocumentStyleSheets;
>+  

Another idea for future enhancement would be to support scoped stylesheets like
xbl <resources>.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ content/xtf/public/nsIXTFElementFactory.idl	12 Mar 2004 20:27:45 -0000	1.1.2.1
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ content/xtf/public/nsIXTFElementWrapper.idl	26 Aug 2004 11:56:47 -0000	1.1.2.4
>+interface nsIXTFElementWrapper : nsISupports
>+{  
>+  readonly attribute nsIDOMElement elementNode;
>+  readonly attribute nsIDOMElement documentFrameElement;
>+  readonly attribute nsIDOMDocument ownerDocument;

This is just a shorthand for elementNode.ownerDocument, or something different?

I'll finish this later starting from nsXTFGenericElementWrapper.cpp.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> >+  // content. If 'false', only UA style sheets will be applied.

What about user sheets?

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 157049 [details] [diff] [review]
Third attempt

Comments on the rest of the patch:

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ content/xtf/src/nsXTFGenericElementWrapper.cpp	26 Aug 2004 11:56:47 -0000	1.1.2.9
>+private:
>+  virtual nsIXTFElement *GetXTFElement()const { return mXTFElement; }

space before const here (and elsewhere)

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ content/xtf/src/nsXTFInterfaceAggregator.cpp	1 Jun 2004 11:20:58 -0000	1.1.2.2
>+NS_IMETHODIMP
>+nsXTFInterfaceAggregator::CallMethod(PRUint16 methodIndex,
>+                                     const nsXPTMethodInfo* info,
>+                                     nsXPTCMiniVariant* params)
>+{
>+  if (methodIndex < 3) {
>+    NS_ERROR("huh? indirect nsISupports method call unexpected on nsXTFInterfaceAggregator.");
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  // prepare args:
>+  int paramCount = info->GetParamCount();
>+  nsXPTCVariant* fullPars = paramCount ? new nsXPTCVariant[paramCount] : nsnull;
>+
>+  for (int i=0; i<paramCount; ++i) {
>+    const nsXPTParamInfo& paramInfo = info->GetParam(i);
>+    uint8 flags = paramInfo.IsOut() ? nsXPTCVariant::PTR_IS_DATA : 0;

This should be PRUint8, not uint8.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ layout/xtf/src/nsXTFSVGDisplayFrame.cpp	26 Aug 2004 11:56:48 -0000	1.1.2.4
>+  // nsISVGContainerFrame interface:
>+  nsISVGOuterSVGFrame*GetOuterSVGFrame();

Space before the method name.  Try to be more consistent with your spacing in
general.

>+NS_IMETHODIMP
>+nsXTFSVGDisplayFrame::GetFrameForPoint(float x, float y, nsIFrame** hit)
>+{
>+#ifdef DEBUG
>+//  printf("nsXTFSVGDisplayFrame(%p)::GetFrameForPoint\n", this);
>+#endif
>+  *hit = nsnull;
>+  for (nsIFrame* kid = mFrames.FirstChild(); kid;
>+       kid = kid->GetNextSibling()) {
>+    nsISVGChildFrame* SVGFrame=nsnull;
>+    kid->QueryInterface(NS_GET_IID(nsISVGChildFrame),(void**)&SVGFrame);

These can use CallQueryInterface.

>+    if (SVGFrame) {
>+      nsIFrame* temp=nsnull;
>+      nsresult rv = SVGFrame->GetFrameForPoint(x, y, &temp);
>+      if (NS_SUCCEEDED(rv) && temp) {
>+        *hit = temp;
>+        // return NS_OK; can't return. we need reverse order but only
>+        // have a singly linked list...

Can you elaborate on that?

>+    nsISVGChildFrame* SVGFrame=0;

nsnull.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ layout/xtf/src/nsXTFXMLDisplayFrame.cpp	26 Aug 2004 12:00:42 -0000	1.1.2.1
>+private:
>+  nsIPresShell* mPresShell; // XXX should get rid of this; make
>+                            // GetContentInsertionFrame take as arg
>+                            // instead

Or just use GetPresContext()->PresShell().

In general, you may want to consolidate the headers that just declare a single
factory method into just one header file.

Comment 10

[Alex Vincent [:WeirdAl]]

I didn't see any files in the latest patch for mozilla/xft/tests.

Comment 11

[Jonathan Watt [:jwatt]]

I think you mean content/xft/tests. ;-) I've uploaded a .7z of my latest pull of
the XTF stuff to jwatt.org/moz/xtf.7z . (Here's where I find out you were only
mentioning it and are perfectly capable of getting the files for yourself.) :-)

Comment 12

[Alex Fritze]

Attachment: 4th attempt

New patch, updated to trunk and incorporating Brian's comments.

Comment 13

[Alex Fritze]

(In reply to comment #7)
> I'd really like to be able to make XTF elements that have an nsInlineFrame,
[...]
> A couple of ideas come to mind:
> 
> 1. Just worry about display: inline by making a new frame type that inherits
> from nsInlineFrame.
> 2. Instead of inheriting from the base type, construct it on the fly ...

I implemented 1.

> One thing I found is that I had to explicitly include nsIDOMElement,
> nsIDOMEventTarget, and nsIDOM3Node in the list of scriptable interfaces.  Is
> that intentional?

Yes, I thought it might be useful for some applications to restrict scripting to
a custom DOM that doesn't include nsIDOMElement etc.

 
> >+  // Event notifications:
> >+
> >+  void willChangeDocument(in nsISupports newDoc);
> >+  void documentChanged(in nsISupports newDoc);
> >+  
> >+  void willChangeParent(in nsISupports newParent);
> >+  void parentChanged(in nsISupports newParent);
> 
> Why do these (and other) notifications get passed nsISupports instead of
> nsI[DOM]Document, nsIDOMElement, etc?

In general we don't know which interface the callee might be interested in
(nsIDOMNode, nsIDOMEventTarget, some proprietary interface, ...) so the only
canonical choice seemed nsISupport. 

> Also, might be useful to have a %{C++ section that has macros for stub
> implementations of various categories of notifications, like what
> nsIDocumentObserver has.

Yeah, good idea - can we leave that for a later patch, though?

> Additionally, I found that for XForms I needed to be notified about
> doneAddingChildren(), which is something the content sink knows about.	The
> notification is normally suppressed except for certain cases as an
> optimization; I'd say if the element is in an unknown namespace it should
> notify unconditionally and that notification should go through to the
> XTFElement.

I've implemented this for the xml content sink. I guess it should also be
implemented for the XUL content sink, but I haven't figured out how to do that
yet. Not sure we should do it for html sinks.

> Another idea for future enhancement would be to support scoped stylesheets like
> xbl <resources>.

Yeah, that would be great. I haven't figured out how to do that yet. Any
pointers welcome.

> >+  readonly attribute nsIDOMDocument ownerDocument;
> 
> This is just a shorthand for elementNode.ownerDocument, or something different?
It is supposed to yield the owner document independent on whether the node is
currently inserted into a document or not. Doesn't quite work for xul documents
though...

Comment 14

[Alex Fritze]

(In reply to comment #8)
> > >+  // content. If 'false', only UA style sheets will be applied.
> 
> What about user sheets?
> 

Good question, I'm not sure. Do user sheets apply to anonymous content in general?

Comment 15

[Alex Fritze]

(In reply to comment #9)
> >--- /dev/null	1 Jan 1970 00:00:00 -0000
> >+++ layout/xtf/src/nsXTFSVGDisplayFrame.cpp	26 Aug 2004 11:56:48 -0000	1.1.2.4

Most of this code was just c&p'ed from
content/svg/base/src/nsSVGGenericContainerFrame.cpp.
A better way is to split nsSVGGenericContainerFrame.cpp into .cpp and .h files
and use it directly; which is what I've done now.

Comment 16

[Alex Fritze]

(In reply to comment #10)
> I didn't see any files in the latest patch for mozilla/xft/tests.
Yeah, the test files depend on some other code that's not part of these patches.
I'll remove the corresponding entries in allmakefiles.sh in the next patch.

Comment 17

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #13)
> > This is just a shorthand for elementNode.ownerDocument, or something different?
> It is supposed to yield the owner document independent on whether the node is
> currently inserted into a document or not.

That's exactly what elementNode.ownerDocument would return, no?

> Doesn't quite work for xul documents though...

XUL documents, or XUL elements?  Peterv probably has bugs on this...

(In reply to comment #14)
> Do user sheets apply to anonymous content in general?

At the moment, yes.

Comment 18

[Alex Fritze]

(In reply to comment #17)
> (In reply to comment #13)
> > > This is just a shorthand for elementNode.ownerDocument, or something
different?
> > It is supposed to yield the owner document independent on whether the node is
> > currently inserted into a document or not.
> 
> That's exactly what elementNode.ownerDocument would return, no?

Ah, I thought elementNode.ownerDocument would become null when the node comes
unattached, but on rereading the dom specs that doesn't seem to be the case. I
guess we can remove wrapper.ownerDocument then.

> > Doesn't quite work for xul documents though...
> 
> XUL documents, or XUL elements?  Peterv probably has bugs on this...

Any elements that are being created for a xul document, even if they are not in
the xul namespace.

> (In reply to comment #14)
> > Do user sheets apply to anonymous content in general?
> 
> At the moment, yes.

In that case they'll apply to xtf visual content as well.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> In that case they'll apply to xtf visual content as well.

Change the comment accordingly?

Comment 20

[Brian Ryner (not reading)]

> > >+  // Event notifications:
> > >+
> > >+  void willChangeDocument(in nsISupports newDoc);
> > >+  void documentChanged(in nsISupports newDoc);
> > >+  
> > >+  void willChangeParent(in nsISupports newParent);
> > >+  void parentChanged(in nsISupports newParent);
> > 
> > Why do these (and other) notifications get passed nsISupports instead of
> > nsI[DOM]Document, nsIDOMElement, etc?
> 
> In general we don't know which interface the callee might be interested in
> (nsIDOMNode, nsIDOMEventTarget, some proprietary interface, ...) so the only
> canonical choice seemed nsISupport. 
> 

Sure, but if you pass nsISupports you _know_ the caller is going to have to QI
to do anything useful.  May as well pass something that would likely be useful,
then the caller can QI if they need to.  My suggestions:

  void willChangeDocument(in nsIDOMDocument newDoc);
  void documentChanged(in nsIDOMDocument newDoc);
  void willChangeParent(in nsIDOMElement newParent);
  void parentChanged(in nsIDOMElement newParent);
  void willInsertChild(in nsIDOMNode child, in unsigned long index);
  void childInserted(in nsIDOMNode child, in unsigned long index);
  void willAppendChild(in nsIDOMNode child);
  void childAppended(in nsIDOMNode child);

> > Also, might be useful to have a %{C++ section that has macros for stub
> > implementations of various categories of notifications, like what
> > nsIDocumentObserver has.
> 
> Yeah, good idea - can we leave that for a later patch, though?
> 

Sure.

> > >+  readonly attribute nsIDOMDocument ownerDocument;
> > 
> > This is just a shorthand for elementNode.ownerDocument, or something different?


> It is supposed to yield the owner document independent on whether the node is
> currently inserted into a document or not. Doesn't quite work for xul documents
> though...
> 

As I understand it, the owner document is set at node creation time and doesn't
change by inserting the node into a document.

Comment 21

[Brian Ryner (not reading)]

Comment on attachment 160942 [details] [diff] [review]
4th attempt

r=me if you go ahead and change the types on the notification methods as I said
above.

> XTF (sorry about the name!) allows you to extend Mozilla by

It's probably not clear to most people why you would be sorry about the name; I
think it's a good name despite its similarity to XFT.  Maybe we can come up
with a Marketing Buzzword for it if it's a problem :)

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 160942 [details] [diff] [review]
4th attempt

sr=jst

Comment 23

[Alex Fritze]

Attachment: final patch (hopefully!)

Comment 24

[Alex Fritze]

Thanks for the reviews & comments. 

(In reply to comment #21)
> (From update of attachment 160942 [details] [diff] [review])
> r=me if you go ahead and change the types on the notification methods as I said
> above.

Done.
 
> > XTF (sorry about the name!) allows you to extend Mozilla by
> 
> It's probably not clear to most people why you would be sorry about the name; 

People have been saying it hurts their hands typing it :-)

(In reply to comment #19)
> > In that case they'll apply to xtf visual content as well.
> 
> Change the comment accordingly?

Done.

Comment 25

[Alex Vincent [:WeirdAl]]

Comment on attachment 161391 [details] [diff] [review]
final patch (hopefully!)

>+	content/xtf/tests/Makefile
>+	content/xtf/tests/smiley/Makefile
>+	content/xtf/tests/canvas/Makefile
>+	content/xtf/tests/perf/Makefile
>+        content/xtf/tests/shapes/Makefile

This will break the build!

Comment 26

[Alex Fritze]

(In reply to comment #25)
> (From update of attachment 161391 [details] [diff] [review])
> >+	content/xtf/tests/Makefile
> >+	content/xtf/tests/smiley/Makefile
> >+	content/xtf/tests/canvas/Makefile
> >+	content/xtf/tests/perf/Makefile
> >+        content/xtf/tests/shapes/Makefile
> 
> This will break the build!
> 
Indeed!
Several other things messed up as well:
- Forgot to diff nsXTFXULVisualWraper.idl
- content/xtf/Makefile contains references to test and js dirs
- readme.txt not latest version
New patch forthcoming.

Comment 27

[Alex Fritze]

Attachment: final patch mk II

Comment 28

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

(In reply to comment #25)
> >+	content/xtf/tests/perf/Makefile
> >+        content/xtf/tests/shapes/Makefile
> 
> This will break the build!

my experience is that a missing directory/Makefile.in listed in allmakefiles.sh
does not break the build, just causes configure to output a warning...

Comment 29

[Alex Fritze]

Checked in - marking fixed.
The patch below was missing, so the non-xtf builds were temporarily broken:

Index: nsContentUtils.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/base/src/nsContentUtils.cpp,v
retrieving revision 1.82
retrieving revision 1.83
diff -u -p -u -r1.82 -r1.83
--- nsContentUtils.cpp	7 Oct 2004 20:59:48 -0000	1.82
+++ nsContentUtils.cpp	7 Oct 2004 21:23:52 -0000	1.83
@@ -214,6 +214,7 @@ nsContentUtils::GetParserServiceWeakRef(
   return sParserService;
 }
 
+#ifdef MOZ_XTF
 nsIXTFService*
 nsContentUtils::GetXTFServiceWeakRef()
 {
@@ -226,6 +227,7 @@ nsContentUtils::GetXTFServiceWeakRef()
 
   return sXTFService;
 }
+#endif
 
 template <class OutputIterator>
 struct NormalizeNewlinesCharTraits {

Comment 30

[Boris Zbarsky [:bzbarsky]]

Alex, some of the changes to nsCSSFrameConstructor are wrong.  In particular,
floated XTF elements will not get the right geometric parent, which can lead to
both layout and painting issues.

Please fix the code that gets the geometric parent to use the
GetGeometricParent() function on the frame constructor state (see bug 191151,
where I fixed the same exact bug in the SVG, MathML, etc codepaths).

Reopening to make sure this doesn't get lost, but let me know if you want a
separate bug on this.

Comment 31

[Alex Fritze]

Attachment: nsCSSFrameConstructor geometric parent patch

Comment 32

[Alex Fritze]

Comment on attachment 161448 [details] [diff] [review]
nsCSSFrameConstructor geometric parent patch

Boris: 
Thanks for finding this. Are there any other things in the frameconstructor
that are dodgy or is this it?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 161448 [details] [diff] [review]
nsCSSFrameConstructor geometric parent patch

r+sr=bzbarsky.

I don't see things obviously wrong otherwise (though I'm not clear on the
overall arch here, so not sure what some of the code is doing).

Comment 34

[Alex Fritze]

(In reply to comment #33)
> (From update of attachment 161448 [details] [diff] [review])
> r+sr=bzbarsky.

Thanks. Code checked in.
 

Comment 35

[Boris Zbarsky [:bzbarsky]]

OK, I took a more careful look at the CSSFrameConstructor code and bug 266772 is
the result.