Closed Bug 471356 Opened 11 years ago Closed 11 years ago

stop using nsAreaFrame for anything other than xul:label

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: dbaron, Assigned: dbaron)

Details

(Keywords: fixed1.9.1)

Attachments

(3 files, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #354632 - Flags: superreview?(roc)
Attachment #354632 - Flags: review?(roc)
 		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?
(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?
(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.
(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.
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.
Attachment #354644 - Flags: superreview?(roc)
Attachment #354644 - Flags: review?(roc)
Attached patch patchSplinter Review
And here's the main patch updated to your comments.

I also moved the declaration of NS_NewXULLabelFrame from nsHTMLParts.h to nsXULLabelFrame.h.
Attachment #354632 - Attachment is obsolete: true
Attachment #354645 - Flags: superreview?(roc)
Attachment #354645 - Flags: review?(roc)
Attachment #354632 - Flags: superreview?(roc)
Attachment #354632 - Flags: review?(roc)
And full reftests pass with the revised patch.
Attachment #354644 - Flags: superreview?(roc)
Attachment #354644 - Flags: superreview+
Attachment #354644 - Flags: review?(roc)
Attachment #354644 - Flags: review+
Attachment #354645 - Flags: superreview?(roc)
Attachment #354645 - Flags: superreview+
Attachment #354645 - Flags: review?(roc)
Attachment #354645 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/187ce101bf03
http://hg.mozilla.org/mozilla-central/rev/fb2f301f6d0b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [regression fix needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
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.
Attachment #354644 - Flags: approval1.9.1?
Should the continuingAreaFrame in the IS_TABLE_CELL case in CreateContinuinFrame be renamed to continuingBlockFrame?
Keywords: 4xp
Still not sure what to do about the name nsSelectsAreaFrame, but this fixes the rest of the missed renames, I think.
Attachment #354824 - Flags: superreview?(bzbarsky)
Attachment #354824 - Flags: review?(bzbarsky)
Attachment #354824 - Flags: superreview?(bzbarsky)
Attachment #354824 - Flags: superreview+
Attachment #354824 - Flags: review?(bzbarsky)
Attachment #354824 - Flags: review+
Comment on attachment 354644 [details] [diff] [review]
separate patch for CalcQuirkContainingBlockHeight regression

a191=beltzner
Attachment #354644 - Flags: approval1.9.1? → approval1.9.1+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/93d98e479c6a

Marking fixed1.9.1 even though the main bug isn't.
Keywords: fixed1.9.1
Whiteboard: [regression fix needs 1.9.1 landing]
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.