471356 - stop using nsAreaFrame for anything other than xul:label

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Attachment: patch

The only things nsAreaFrame still does are access key handling that is specifically conditioned on being a xul:label.

This patch stops using nsAreaFrame for anything other than xul:label and renames it to nsXULLabelFrame.

One piece of interest is the comment and now-#if 0-ed code in CalcQuirkContainingBlock height.

Full reftests pass.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

 		nsViewportFrame.cpp \
+		nsXULLabelFrame.cpp \
 		$(NULL)

This should be #ifdef MOZ_XUL.

-  AddFrameTypeInfo(nsGkAtoms::areaFrame,             "area",      "area");
+  AddFrameTypeInfo(nsGkAtoms::XULLabelFrame,         "XULLabel",  "XULLabel");

So should this I guess.

+    // This code is commented out since it hasn't been executed since
+    // we stopped creating area frames for the root (bug 243519).
+#if 0

Shouldn't we put this back the way it was pre-243519 by switching the check to blockFrame?

Is there a reason not to move nsXULLabelFrame to layout/xul/base/src?

Comment 2

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

(In reply to comment #1)
>          nsViewportFrame.cpp \
> +        nsXULLabelFrame.cpp \
>          $(NULL)
> 
> This should be #ifdef MOZ_XUL.

So you'd prefer not ifdef-ing the .h and .cpp files?  Or doing both?

> 
> -  AddFrameTypeInfo(nsGkAtoms::areaFrame,             "area",      "area");
> +  AddFrameTypeInfo(nsGkAtoms::XULLabelFrame,         "XULLabel",  "XULLabel");
> 
> So should this I guess.

Right.

> +    // This code is commented out since it hasn't been executed since
> +    // we stopped creating area frames for the root (bug 243519).
> +#if 0
> 
> Shouldn't we put this back the way it was pre-243519 by switching the check to
> blockFrame?

Probably.  It took me a while to figure out when that change happened.

> Is there a reason not to move nsXULLabelFrame to layout/xul/base/src?

Does anything else in layout/xul/base/src inherit from block frame?

Comment 3

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

(In reply to comment #2)
> > Shouldn't we put this back the way it was pre-243519 by switching the check to
> > blockFrame?
> 
> Probably.  It took me a while to figure out when that change happened.

In fact, I should probably separate that change, write a test for it, and land it on the 1.9.1 branch.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #2)
> (In reply to comment #1)
> >          nsViewportFrame.cpp \
> > +        nsXULLabelFrame.cpp \
> >          $(NULL)
> > 
> > This should be #ifdef MOZ_XUL.
> 
> So you'd prefer not ifdef-ing the .h and .cpp files?  Or doing both?

I'd prefer not #ifdefing inside those files.

> > Is there a reason not to move nsXULLabelFrame to layout/xul/base/src?
> 
> Does anything else in layout/xul/base/src inherit from block frame?

Don't think so. Does that matter? If you move the file to layout/xul/base/src we won't have to put an 'if XUL' in the layout/generic makefile.

Comment 5

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

Attachment: separate patch for CalcQuirkContainingBlockHeight regression

Here's the separated patch for the CalcQuirkContainingBlockHeight regression.

The reftest in this patch passes in 2008-09-07-02-mozilla-central (Linux x86_64), fails in 2008-09-08-01-mozilla-central (Linux x86_64), fails in my current mozilla-central build without this patch, and passes in my mozilla-central build with this patch.


I'll update the main patch shortly to apply on top of this.

Comment 6

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

Attachment: patch

And here's the main patch updated to your comments.

I also moved the declaration of NS_NewXULLabelFrame from nsHTMLParts.h to nsXULLabelFrame.h.

Comment 7

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

And full reftests pass with the revised patch.

Comment 8

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

http://hg.mozilla.org/mozilla-central/rev/187ce101bf03
http://hg.mozilla.org/mozilla-central/rev/fb2f301f6d0b

Comment 9

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

Comment on attachment 354644 [details] [diff] [review]
separate patch for CalcQuirkContainingBlockHeight regression

Requesting approval for this fix for a layout regression from earlier in the 1.9.1 cycle.

Comment 10

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

Also http://hg.mozilla.org/mozilla-central/rev/4d579f865308

Comment 11

[Boris Zbarsky [:bzbarsky]]

Should the continuingAreaFrame in the IS_TABLE_CELL case in CreateContinuinFrame be renamed to continuingBlockFrame?

Comment 12

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

Attachment: fix some missed renames

Still not sure what to do about the name nsSelectsAreaFrame, but this fixes the rest of the missed renames, I think.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 354644 [details] [diff] [review]
separate patch for CalcQuirkContainingBlockHeight regression

a191=beltzner

Comment 14

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/93d98e479c6a

Marking fixed1.9.1 even though the main bug isn't.

Comment 15

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

Landed the missed renames as http://hg.mozilla.org/mozilla-central/rev/fa29efa5db42