Closed Bug 1277131 Opened 8 years ago Closed 8 years ago

rename nsTableOuterFrame to nsTableWrapperFrame

Categories

(Core :: Layout: Tables, defect)

Product:
Core
Component:
Layout: Tables
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: dbaron, Assigned: bmo)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

Bug 1277131 : Part 1 - rename nsTableOuterFrame to nsTableWrapperFrame.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1277131 : Part 2 - rename nsMathMLmtableOuterFrame to nsMathMLmtableWrapperFrame.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1277131 : Part 3 - rename nsGkAtoms::tableOuterFrame and nsCSSAnonBoxes::tableOuter.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1277131 : Part 4 - rename -moz-table-outer to -moz-table-wrapper.
58 bytes, text/x-review-board-request
heycam
: review+
Details 
Now that there's a clear name in CSS 2.1 for the "table wrapper box", defined in https://drafts.csswg.org/css2/tables.html#model , we should rename nsTableOuterFrame to nsTableWrapperFrame so that our code's terminology matches the spec's.
David, I'd like to take this bug and have some study on relevant codes around. Thanks.
Assignee: nobody → aschen
Status: NEW → ASSIGNED
Comment on attachment 8760192 [details]
Bug 1277131 : Part 1 - rename nsTableOuterFrame to nsTableWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57874/diff/1-2/
Comment on attachment 8760193 [details]
Bug 1277131 : Part 2 - rename nsMathMLmtableOuterFrame to nsMathMLmtableWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57876/diff/1-2/
Comment on attachment 8760194 [details]
Bug 1277131 : Part 3 - rename nsGkAtoms::tableOuterFrame and nsCSSAnonBoxes::tableOuter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57878/diff/1-2/
Attachment #8760192 - Flags: review?(cam)
Attachment #8760193 - Flags: review?(cam)
Attachment #8760194 - Flags: review?(cam)
Comment on attachment 8760192 [details]
Bug 1277131 : Part 1 - rename nsTableOuterFrame to nsTableWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57874/diff/2-3/
Comment on attachment 8760193 [details]
Bug 1277131 : Part 2 - rename nsMathMLmtableOuterFrame to nsMathMLmtableWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57876/diff/2-3/
Comment on attachment 8760194 [details]
Bug 1277131 : Part 3 - rename nsGkAtoms::tableOuterFrame and nsCSSAnonBoxes::tableOuter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57878/diff/2-3/
Comment on attachment 8760192 [details]
Bug 1277131 : Part 1 - rename nsTableOuterFrame to nsTableWrapperFrame.

https://reviewboard.mozilla.org/r/57874/#review56160

Looks good, but there are a few more things that would be good to fix at the same time:

* There are some comments in files under layout/table/ that mention "outer table frame", "table outer frame", "outer table" that should now talk about wrapper frames.
* Please rename ::-moz-table-outer to ::-moz-table-wrapper.
* Please rename nsGkAtoms::tableOuterFrame to nsGkAtoms::tableWrapperFrame.

::: layout/mathml/nsMathMLmtableFrame.h:49
(Diff revision 3)
>    {
> -    return nsTableOuterFrame::IsFrameOfType(aFlags & ~(nsIFrame::eMathML));
> +    return nsTableWrapperFrame::IsFrameOfType(aFlags & ~(nsIFrame::eMathML));
>    }
>  
>  protected:
> -  explicit nsMathMLmtableOuterFrame(nsStyleContext* aContext) : nsTableOuterFrame(aContext) {}
> +  explicit nsMathMLmtableOuterFrame(nsStyleContext* aContext) : nsTableWrapperFrame(aContext) {}

This line was a bit long to begin with.  Can you wrap it to fit 80 columns?
Attachment #8760192 - Flags: review?(cam)
(In reply to Cameron McCormack (:heycam) from comment #11)
> * Please rename nsGkAtoms::tableOuterFrame to nsGkAtoms::tableWrapperFrame.

Ah, I see you do this in part 3.
Comment on attachment 8760193 [details]
Bug 1277131 : Part 2 - rename nsMathMLmtableOuterFrame to nsMathMLmtableWrapperFrame.

https://reviewboard.mozilla.org/r/57876/#review56162
Attachment #8760193 - Flags: review?(cam) → review+
Comment on attachment 8760194 [details]
Bug 1277131 : Part 3 - rename nsGkAtoms::tableOuterFrame and nsCSSAnonBoxes::tableOuter.

https://reviewboard.mozilla.org/r/57878/#review56164

::: layout/style/nsCSSAnonBoxList.h:55
(Diff revision 3)
>  CSS_ANON_BOX(inlineTable, ":-moz-inline-table")
>  CSS_ANON_BOX(table, ":-moz-table")
>  CSS_ANON_BOX(tableCell, ":-moz-table-cell")
>  CSS_ANON_BOX(tableColGroup, ":-moz-table-column-group")
>  CSS_ANON_BOX(tableCol, ":-moz-table-column")
> -CSS_ANON_BOX(tableOuter, ":-moz-table-outer")
> +CSS_ANON_BOX(tableWrapper, ":-moz-table-outer")

As mentioned in the patch 1 comment, please rename the pseudo to ::-moz-table-wrapper.  (I guess here rather than in patch 1 would be better.)

::: layout/style/nsComputedDOMStyle.h:689
(Diff revision 3)
>    /*
> -   * While computing style data, the primary frame for mContent --- named "outer"
> +   * While computing style data, the primary frame for mContent --- named "wrapper"
>     * because we should use it to compute positioning data.  Null
>     * otherwise.
>     */
> -  nsIFrame* mOuterFrame;
> +  nsIFrame* mWrapperFrame;

I think it might be better to leave this one called mOuterFrame, since it's not always referring to nsTableWrapperFrames but an outer frame more generally.

::: layout/style/nsComputedDOMStyle.cpp:682
(Diff revision 3)
> -    mOuterFrame = mContent->GetPrimaryFrame();
> -    mInnerFrame = mOuterFrame;
> -    if (mOuterFrame) {
> -      nsIAtom* type = mOuterFrame->GetType();
> -      if (type == nsGkAtoms::tableOuterFrame) {
> +    mWrapperFrame = mContent->GetPrimaryFrame();
> +    mInnerFrame = mWrapperFrame;
> +    if (mWrapperFrame) {
> +      nsIAtom* type = mWrapperFrame->GetType();
> +      if (type == nsGkAtoms::tableWrapperFrame) {
>          // If the frame is an outer table frame then we should get the style

Please update the comments around here to talk about table wrapper frames.
Attachment #8760194 - Flags: review?(cam)
Comment on attachment 8760193 [details]
Bug 1277131 : Part 2 - rename nsMathMLmtableOuterFrame to nsMathMLmtableWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57876/diff/3-4/
Comment on attachment 8760194 [details]
Bug 1277131 : Part 3 - rename nsGkAtoms::tableOuterFrame and nsCSSAnonBoxes::tableOuter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57878/diff/3-4/
Comment on attachment 8760193 [details]
Bug 1277131 : Part 2 - rename nsMathMLmtableOuterFrame to nsMathMLmtableWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57876/diff/4-5/
Attachment #8760194 - Attachment is obsolete: true
Attachment #8760194 - Flags: review?(cam)
Attachment #8762585 - Attachment is obsolete: true
Comment on attachment 8760192 [details]
Bug 1277131 : Part 1 - rename nsTableOuterFrame to nsTableWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57874/diff/3-4/
Attachment #8760192 - Flags: review?(cam)
Comment on attachment 8760193 [details]
Bug 1277131 : Part 2 - rename nsMathMLmtableOuterFrame to nsMathMLmtableWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57876/diff/5-6/
Comment on attachment 8762586 [details]
Bug 1277131 : Part 3 - rename nsGkAtoms::tableOuterFrame and nsCSSAnonBoxes::tableOuter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59190/diff/1-2/
Comment on attachment 8762587 [details]
Bug 1277131 : Part 4 - rename -moz-table-outer to -moz-table-wrapper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59192/diff/1-2/
https://reviewboard.mozilla.org/r/57874/#review56160

*1 - fixed and updated in patch part 1.
*2 - fxied in new patch part 4.
*3 - addressed in patch part 3.

> This line was a bit long to begin with.  Can you wrap it to fit 80 columns?

fixed in updated patch part 2.
Attachment #8762586 - Flags: review?(cam)
Attachment #8762587 - Flags: review?(cam)
Attachment #8760192 - Flags: review?(cam) → review+
Comment on attachment 8760192 [details]
Bug 1277131 : Part 1 - rename nsTableOuterFrame to nsTableWrapperFrame.

https://reviewboard.mozilla.org/r/57874/#review56460

Looks good, but for the comment changes can you use the term "table wrapper" and "table wrapper frame" instead of "wrapper table" and "wrapper table frame"?  That sounds more correct.  r=me that and the below comments.

::: layout/base/nsCSSFrameConstructor.cpp:1926
(Diff revisions 1 - 4)
>  
> -// Return whether the given frame is a table pseudo-frame.  Note that
> +// Return whether the given frame is a table pseudo-frame. Note that
>  // cell-content and table-outer frames have pseudo-types, but are always
>  // created, even for non-anonymous cells and tables respectively.  So for those
>  // we have to examine the cell or table frame to see whether it's a pseudo
> -// frame.  In particular, a lone table caption will have an outer table as its
> +// frame. In particular, a lone table caption will have an wrapper table as its

"a"

::: layout/doc/table_reflow_slides.html:378
(Diff revisions 1 - 4)
>      <a href="#s9">&lt;</a>
>      <a href="#s11">&gt;</a>
>    </div>
>    <h1>Table Reflow</h1>
>    <ul>
> -    <li>Outer table reflows table and caption (if present)</li>
> +    <li>wrapper table reflows table and caption (if present)</li>

Keep uppercase letter.

::: layout/doc/table_reflow_slides.html:491
(Diff revisions 1 - 4)
>    <ul>
>      <li>If the table is already balanced, pass 1 constrains the width (like a normal pass 2) based on the current
>  	column widths. The pass 2 will get skipped if the table doesn't need to rebalance. <!--<span class="comment">please clarify when can this happen</span>--></li> 
>      <li>Nested table reflowed with an unconstrained width (i.e. an ancestor is doing a pass 1 reflow)
>  	will only do a pass 1 reflow on its children</li>
> -    <li>Outer table caches last avail width and avoids reflowing children if resize reflow is the same as previous</li>
> +    <li>wrapper table caches last avail width and avoids reflowing children if resize reflow is the same as previous</li>

And here.

::: layout/doc/table_reflow_slides.html:505
(Diff revisions 1 - 4)
>      <a href="#s13">&lt;</a>
>      <a href="#s15">&gt;</a>
>    </div>
>    <h1>Table incremental reflow</h1>
>    <ul>
> -    <li>Outer table is a target when a caption is added or removed (dirty) or the table or caption margin 
> +    <li>wrapper table is a target when a caption is added or removed (dirty) or the table or caption margin 

And here.

::: layout/base/nsCSSFrameConstructor.h:470
(Diff revision 4)
>                                  nsIFrame*                      aPrevSibling,
>                                  bool                           aIsRecursiveCall = false);
>  
>    // BEGIN TABLE SECTION
>    /**
> -   * Construct an outer table frame.  This is the FrameConstructionData
> +   * Construct an wrapper table frame.  This is the FrameConstructionData

"a"

::: layout/doc/obsolete/nav4-html.html:473
(Diff revision 4)
>  <BR>Attributes:
>  <UL><B>COLSPAN</B>=int [clamped: >= 1 &amp;&amp; &lt;= 1000 ]
>  <BR><B>ROWSPAN</B>=int [clamped: >= 1 &amp;&amp; &lt;= 10000 ]
>  <BR><B>NOWRAP</B> [boolean: disables wrapping ]
>  <BR><B>BGCOLOR</B>=color [default: inherit from the row; if not row then
> -table; if not table then inherit from an outer table cell; this works because
> +table; if not table then inherit from an wrapper table cell; this works because

"a"

::: layout/generic/nsFrame.cpp:8353
(Diff revision 4)
>      if (!innerTable->StyleContext()->GetPseudo()) {
>        return innerTable;
>      }
>    }
>  
> -  // Outer tables are always anon boxes; if we're in here for an outer
> +  // wrapper tables are always anon boxes; if we're in here for an outer

Keep uppercase letter.

::: layout/style/nsComputedDOMStyle.cpp:680
(Diff revision 4)
>      mOuterFrame = mContent->GetPrimaryFrame();
>      mInnerFrame = mOuterFrame;
>      if (mOuterFrame) {
>        nsIAtom* type = mOuterFrame->GetType();
>        if (type == nsGkAtoms::tableOuterFrame) {
> -        // If the frame is an outer table frame then we should get the style
> +        // If the frame is an wrapper table frame then we should get the style

"a"
Comment on attachment 8762586 [details]
Bug 1277131 : Part 3 - rename nsGkAtoms::tableOuterFrame and nsCSSAnonBoxes::tableOuter.

https://reviewboard.mozilla.org/r/59190/#review56470

r=me with the same comments: please use "table wrapper frame" and "table wrapper" rather than "wrapper table frame" and "wrapper table".
Attachment #8762586 - Flags: review?(cam) → review+
Attachment #8762587 - Flags: review?(cam) → review+
Comment on attachment 8762587 [details]
Bug 1277131 : Part 4 - rename -moz-table-outer to -moz-table-wrapper.

https://reviewboard.mozilla.org/r/59192/#review56472
Comment on attachment 8760192 [details]
Bug 1277131 : Part 1 - rename nsTableOuterFrame to nsTableWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57874/diff/4-5/
Comment on attachment 8760193 [details]
Bug 1277131 : Part 2 - rename nsMathMLmtableOuterFrame to nsMathMLmtableWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57876/diff/6-7/
Comment on attachment 8762586 [details]
Bug 1277131 : Part 3 - rename nsGkAtoms::tableOuterFrame and nsCSSAnonBoxes::tableOuter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59190/diff/2-3/
Comment on attachment 8762587 [details]
Bug 1277131 : Part 4 - rename -moz-table-outer to -moz-table-wrapper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59192/diff/2-3/
Comment on attachment 8760192 [details]
Bug 1277131 : Part 1 - rename nsTableOuterFrame to nsTableWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57874/diff/5-6/
Comment on attachment 8760193 [details]
Bug 1277131 : Part 2 - rename nsMathMLmtableOuterFrame to nsMathMLmtableWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57876/diff/7-8/
Comment on attachment 8762586 [details]
Bug 1277131 : Part 3 - rename nsGkAtoms::tableOuterFrame and nsCSSAnonBoxes::tableOuter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59190/diff/3-4/
Comment on attachment 8762587 [details]
Bug 1277131 : Part 4 - rename -moz-table-outer to -moz-table-wrapper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59192/diff/3-4/
https://reviewboard.mozilla.org/r/57874/#review56460

all fixed in updated changeset.

> "a"

I'll keep the previous name "outer" as-is since it seems more like talking about outer table cell instead of anon table wrapper.
Comment on attachment 8762586 [details]
Bug 1277131 : Part 3 - rename nsGkAtoms::tableOuterFrame and nsCSSAnonBoxes::tableOuter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59190/diff/4-5/
Comment on attachment 8762587 [details]
Bug 1277131 : Part 4 - rename -moz-table-outer to -moz-table-wrapper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59192/diff/4-5/
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/255baa34b080
Part 1 - rename nsTableOuterFrame to nsTableWrapperFrame. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0b4d09566b2
Part 2 - rename nsMathMLmtableOuterFrame to nsMathMLmtableWrapperFrame. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dfd78777b8d
Part 3 - rename nsGkAtoms::tableOuterFrame and nsCSSAnonBoxes::tableOuter. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/b866a92a8310
Part 4 - rename -moz-table-outer to -moz-table-wrapper. r=heycam
You need to log in before you can comment on or make changes to this bug.