Closed
Bug 471356
Opened 16 years ago
Closed 16 years ago
stop using nsAreaFrame for anything other than xul:label
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: dbaron, Assigned: dbaron)
Details
(Keywords: fixed1.9.1)
Attachments
(3 files, 1 obsolete file)
3.41 KB,
patch
|
roc
:
review+
roc
:
superreview+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
63.82 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
8.34 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | 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?
Assignee | ||
Comment 2•16 years ago
|
||
(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?
Assignee | ||
Comment 3•16 years ago
|
||
(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.
Assignee | ||
Comment 5•16 years ago
|
||
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)
Assignee | ||
Comment 6•16 years ago
|
||
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)
Assignee | ||
Comment 7•16 years ago
|
||
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+
Assignee | ||
Comment 8•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/187ce101bf03 http://hg.mozilla.org/mozilla-central/rev/fb2f301f6d0b
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [regression fix needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Comment 9•16 years ago
|
||
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?
Assignee | ||
Comment 10•16 years ago
|
||
Also http://hg.mozilla.org/mozilla-central/rev/4d579f865308
Comment 11•16 years ago
|
||
Should the continuingAreaFrame in the IS_TABLE_CELL case in CreateContinuinFrame be renamed to continuingBlockFrame?
Keywords: 4xp
Assignee | ||
Comment 12•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #354824 -
Flags: superreview?(bzbarsky)
Attachment #354824 -
Flags: superreview+
Attachment #354824 -
Flags: review?(bzbarsky)
Attachment #354824 -
Flags: review+
Comment 13•16 years ago
|
||
Comment on attachment 354644 [details] [diff] [review] separate patch for CalcQuirkContainingBlockHeight regression a191=beltzner
Attachment #354644 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Comment 14•16 years ago
|
||
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]
Assignee | ||
Comment 15•15 years ago
|
||
Landed the missed renames as http://hg.mozilla.org/mozilla-central/rev/fa29efa5db42
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
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.
Description
•