Closed Bug 781409 Opened 9 years ago Closed 9 years ago

remove nsITableLayout

Categories

(Core :: Layout: Tables, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

(Regressed 1 open bug)

Details

(Keywords: APIchange)

Attachments

(3 files)

wip
72.28 KB, patch
Details | Diff | Splinter Review
patch
73.02 KB, patch
roc
: review+
davidb
: review+
ehsan
: review-
Details | Diff | Splinter Review
patch 2
73.81 KB, patch
roc
: review+
Details | Diff | Splinter Review 
basically everything about nsITableLayout is bad.
Attached patch wip — — Splinter Review 
This isn't nearly done, but its getting there.
There's a couple methods impls left that are unused but need to be rm'd, and probably a bit of cleanup.
It also seems this breaks an a11y test that tests selecting and unselecting rows (part of accessible/test/mochitest/table/test_sels_table.html)
Attached patch patch — — Splinter Review 
this should be a bit better.  I removed the remaining cruft, and fixed the a11y test failures.

 note I tried to use stdint types when they wouldn't cause conversion problems because of pointers / references.

I can get someone else to look at the accessible/ and editor/ bits if you like.
Attachment #650861 - Flags: review?(roc)
Comment on attachment 650861 [details] [diff] [review]
patch

Review of attachment 650861 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/HTMLTableAccessible.cpp
@@ +598,5 @@
>  void
>  HTMLTableAccessible::SelectedCells(nsTArray<Accessible*>* aCells)
>  {
> +  nsTableOuterFrame* tableFrame = do_QueryFrame(mContent->GetPrimaryFrame());
> +  if (!tableFrame) 

Nit: trailing whitespace

@@ +611,4 @@
>  
> +      PRInt32 startCol = -1, startRow = -1;
> +      cellFrame->GetRowIndex(startRow);
> +      cellFrame->GetColIndex(startCol);

I think we used to check the results of those functions
> 
> @@ +611,4 @@
> >  
> > +      PRInt32 startCol = -1, startRow = -1;
> > +      cellFrame->GetRowIndex(startRow);
> > +      cellFrame->GetColIndex(startCol);
> 
> I think we used to check the results of those functions

true, but GetColIndex() is atleast as written infalable, and and GetRowIndex() probably should be, but I'm not exactly sure that GetParent() on the cell frame will always have a parent though it seems like it probably should.

Anyway I'll probably try and kill nsITableCellLayout next, which will hopefully involve making both of them infalable, so this would just add work to that.
Attachment #650861 - Flags: review?(ehsan)
Attachment #650861 - Flags: review?(dbolter)
Comment on attachment 650861 [details] [diff] [review]
patch

Review of attachment 650861 [details] [diff] [review]:
-----------------------------------------------------------------

I only really looked at the changes to /accessible. I'm a little concerned about ms2ger's point about checking return types. I imagine GetRowIndex checks for a parent for a reason...

::: accessible/src/html/HTMLTableAccessible.cpp
@@ +624,5 @@
>  void
>  HTMLTableAccessible::SelectedCellIndices(nsTArray<PRUint32>* aCells)
>  {
> +  nsTableOuterFrame* tableFrame = do_QueryFrame(mContent->GetPrimaryFrame());
> +  if (!tableFrame)

It might we worth checking IsDefunct here?

@@ +667,1 @@
>  { 

nit: trailing whitespace (after '{' )

@@ +690,5 @@
>  PRInt32
>  HTMLTableAccessible::ColIndexAt(PRUint32 aCellIdx)
>  {
> +  nsTableOuterFrame* tableFrame = do_QueryFrame(mContent->GetPrimaryFrame());
> +  if (!tableFrame) 

nit: trailing whitespace
Note: overall I like the patch.
(In reply to David Bolter [:davidb] from comment #5)
> Comment on attachment 650861 [details] [diff] [review]
> patch
> 
> Review of attachment 650861 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I only really looked at the changes to /accessible. I'm a little concerned
> about ms2ger's point about checking return types. I imagine GetRowIndex
> checks for a parent for a reason...

maybe? it seems kind of old code I don't know so hard to say, but if you saw that check you should have seen it sets the out arg in the failure case to 0 which should be fine.If I remember the use site correctly we just get it to check that the row / col indexs we are looking at are the origin of a cell, so having the wrong one should be fine.

> ::: accessible/src/html/HTMLTableAccessible.cpp
> @@ +624,5 @@
> >  void
> >  HTMLTableAccessible::SelectedCellIndices(nsTArray<PRUint32>* aCells)
> >  {
> > +  nsTableOuterFrame* tableFrame = do_QueryFrame(mContent->GetPrimaryFrame());
> > +  if (!tableFrame)
> 
> It might we worth checking IsDefunct here?

no, platform api functions should always ensure non defunctness before calling functions on accessibles to get properties.
(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> (In reply to David Bolter [:davidb] from comment #5)
> > Comment on attachment 650861 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 650861 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I only really looked at the changes to /accessible. I'm a little concerned
> > about ms2ger's point about checking return types. I imagine GetRowIndex
> > checks for a parent for a reason...
> 
> maybe? it seems kind of old code I don't know so hard to say, but if you saw
> that check you should have seen it sets the out arg in the failure case to 0
> which should be fine.

I say the zero set but I wasn't sure this was harmless (yet).

> > ::: accessible/src/html/HTMLTableAccessible.cpp
> > @@ +624,5 @@
> > >  void
> > >  HTMLTableAccessible::SelectedCellIndices(nsTArray<PRUint32>* aCells)
> > >  {
> > > +  nsTableOuterFrame* tableFrame = do_QueryFrame(mContent->GetPrimaryFrame());
> > > +  if (!tableFrame)
> > 
> > It might we worth checking IsDefunct here?
> 
> no, platform api functions should always ensure non defunctness before
> calling functions on accessibles to get properties.

I did follow backwards on a few call paths but wasn't sure this happened in every case.
Comment on attachment 650861 [details] [diff] [review]
patch

OK r=me. Please fix nits.
Attachment #650861 - Flags: review?(dbolter) → review+
Comment on attachment 650861 [details] [diff] [review]
patch

Review of attachment 650861 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/Makefile.in
@@ +43,2 @@
>  		nsFrameSelection.h \
> +		nsSplittableFrame.h \

I'm pretty sure we don't want to export all of these headers.  Please revert this change, and in directories where you need these headers, add layout/generic to the LOCAL_INCLUDES variable in the Makefile.

::: layout/generic/nsFrameSelection.h
@@ +651,5 @@
>    nsRefPtr<mozilla::Selection> mDomSelections[nsISelectionController::NUM_SELECTIONTYPES];
>  
>    // Table selection support.
>    // Interfaces that let us get info based on cellmap locations
> +  nsTableOuterFrame* GetTableLayout(nsIContent *aTableContent) const;

Please change the name of this method, as it no longer returns the table layout object.

::: layout/generic/nsSelection.cpp
@@ +2495,5 @@
> +        cellFrame->GetColIndex(origColIndex);
> +        uint32_t actualRowSpan =
> +          tableFrame->GetEffectiveRowSpanAt(origRowIndex, origColIndex);
> +        uint32_t actualColSpan =
> +          tableFrame->GetEffectiveColSpanAt(curRowIndex, curColIndex);

Hmm, maybe it's a good idea to have a helper method on nsTableCellFrame since this similar pattern happens in multiple places.

@@ +2626,2 @@
>  
> +  nsIContent* curCellContent;

Please make this an nsCOMPtr, as it is a reference counted objects, and holding on to the bare pointer is not necessarily safe.

::: layout/tables/Makefile.in
@@ +24,5 @@
> +nsCellMap.h \
> +nsTableCellFrame.h \
> +		nsTableColFrame.h \
> +		nsTableColGroupFrame.h \
> +		nsTableOuterFrame.h \

Same comment about the EXPORTS section.

::: layout/tables/nsTableFrame.cpp
@@ +160,1 @@
>  NS_QUERYFRAME_TAIL_INHERITING(nsContainerFrame)

Hmm, I think you're missing:

NS_QUERYFRAME_ENTRY(nsTableOuterFrame)

here.

I'm surprised if this doesn't cause failures here and there in our tests.

::: layout/tables/nsTableOuterFrame.h
@@ +133,5 @@
>  
> +  /**
> +   * Return the content for the cell at the given row and column.
> +   */
> +  nsIContent* GetCellAt(uint32_t aRowIdx, uint32_t aColIdx);

Nit: please make all of the getter methods here const.
Attachment #650861 - Flags: review?(ehsan) → review-
(Note that I did not review anything in accessible/ since dbolter already did that!)
(In reply to Ehsan Akhgari [:ehsan] from comment #11)
> (Note that I did not review anything in accessible/ since dbolter already
> did that!)

You are welcome to though! I do miss stuff.
(In reply to comment #12)
> (In reply to Ehsan Akhgari [:ehsan] from comment #11)
> > (Note that I did not review anything in accessible/ since dbolter already
> > did that!)
> 
> You are welcome to though! I do miss stuff.

OK, I'll do it on the next version of the patch!
seems bugzilla swallowed this in a midair last night.

out/generic/Makefile.in
> @@ +43,2 @@
> >  		nsFrameSelection.h \
> > +		nsSplittableFrame.h \
> 
> I'm pretty sure we don't want to export all of these headers.  Please revert
> this change, and in directories where you need these headers, add
> layout/generic to the LOCAL_INCLUDES variable in the Makefile.

I'm pretty sure roc is ok with these, so I'm not sure what to say here.  It doesn't matter too much to me one way or another.

> 
> @@ +651,5 @@
> >    nsRefPtr<mozilla::Selection> mDomSelections[nsISelectionController::NUM_SELECTIONTYPES];
> >  
> >    // Table selection support.
> >    // Interfaces that let us get info based on cellmap locations
> > +  nsTableOuterFrame* GetTableLayout(nsIContent *aTableContent) const;
> 
> Please change the name of this method, as it no longer returns the table
> layout object.

opps, actually I'm pretty sure that method was just removed, but I forgot about the declaration.

> ::: layout/generic/nsSelection.cpp
> @@ +2495,5 @@
> > +        cellFrame->GetColIndex(origColIndex);
> > +        uint32_t actualRowSpan =
> > +          tableFrame->GetEffectiveRowSpanAt(origRowIndex, origColIndex);
> > +        uint32_t actualColSpan =
> > +          tableFrame->GetEffectiveColSpanAt(curRowIndex, curColIndex);
> 
> Hmm, maybe it's a good idea to have a helper method on nsTableCellFrame
> since this similar pattern happens in multiple places.

I'm not sure what similar pattern you see, it looks to me like we do similar, but slightly different things.

> @@ +2626,2 @@
> >  
> > +  nsIContent* curCellContent;
> 
> Please make this an nsCOMPtr, as it is a reference counted objects, and
> holding on to the bare pointer is not necessarily safe.

it really should be, and even if it wasn't we never call any method on it or pass it to anyone.  We just immediately stuff it into nsCOMPtrs (I'm not sure if it could be exploited, but I really suspect changing the dom during that loop will cause wrong results).  However I grant this code is not exactly the sainist thing I've seen, and getting rid of this variable all together would probably make it clearer.

> ::: layout/tables/Makefile.in
> @@ +24,5 @@
> > +nsCellMap.h \
> > +nsTableCellFrame.h \
> > +		nsTableColFrame.h \
> > +		nsTableColGroupFrame.h \
> > +		nsTableOuterFrame.h \
> 
> Same comment about the EXPORTS section.
> 
> ::: layout/tables/nsTableFrame.cpp
> @@ +160,1 @@
> >  NS_QUERYFRAME_TAIL_INHERITING(nsContainerFrame)
> 
> Hmm, I think you're missing:
> 
> NS_QUERYFRAME_ENTRY(nsTableOuterFrame)
> 
> here.
> 
> I'm surprised if this doesn't cause failures here and there in our tests.

weird, I'm pretty sure I did,and I *know* it will break mochitest-a11y which I checked unless there is magic going on.

> ::: layout/tables/nsTableOuterFrame.h
> @@ +133,5 @@
> >  
> > +  /**
> > +   * Return the content for the cell at the given row and column.
> > +   */
> > +  nsIContent* GetCellAt(uint32_t aRowIdx, uint32_t aColIdx);
> 
> Nit: please make all of the getter methods here const.

good point, though there's part of me that wants to hope compilers  do that for themselves when its all inline.
(In reply to Ehsan Akhgari [:ehsan] from comment #11)
> (Note that I did not review anything in accessible/ since dbolter already
> did that!)

fair, I really only expected you to look at editor/ since I wasn't sure what roc did, but can hardly complain about more :)
(In reply to comment #14)
> seems bugzilla swallowed this in a midair last night.
> 
> out/generic/Makefile.in
> > @@ +43,2 @@
> > >  		nsFrameSelection.h \
> > > +		nsSplittableFrame.h \
> > 
> > I'm pretty sure we don't want to export all of these headers.  Please revert
> > this change, and in directories where you need these headers, add
> > layout/generic to the LOCAL_INCLUDES variable in the Makefile.
> 
> I'm pretty sure roc is ok with these, so I'm not sure what to say here.  It
> doesn't matter too much to me one way or another.

I still don't like this idea for the following reasons:

* It increases our build times unnecessarily since this means that those headers will be copied/symlinked as part of our build process.
* It encourages other parts of the code base to touch layout internals.  It's already bad enough that editor/ now depends on the implementation of table outer frames, but exporting this header and implicitly encouraging other parts of the code to do the same is not a desirable outcome.
* It increases the size of the patch needlessly.  Adjusting LOCAL_INCLUDES is always easier and more localized, and should always be preferred, unless there is a good reason for exporting a given header.

But of course I will retract my comments if roc explicitly disagrees with me!  :-)

> > ::: layout/generic/nsSelection.cpp
> > @@ +2495,5 @@
> > > +        cellFrame->GetColIndex(origColIndex);
> > > +        uint32_t actualRowSpan =
> > > +          tableFrame->GetEffectiveRowSpanAt(origRowIndex, origColIndex);
> > > +        uint32_t actualColSpan =
> > > +          tableFrame->GetEffectiveColSpanAt(curRowIndex, curColIndex);
> > 
> > Hmm, maybe it's a good idea to have a helper method on nsTableCellFrame
> > since this similar pattern happens in multiple places.
> 
> I'm not sure what similar pattern you see, it looks to me like we do similar,
> but slightly different things.

Sure, but it seems to me like we can have helper methods like GetEffectiveRowColSpanAt, GetRowColIndex, etc.  But I don't care much either way...

> > @@ +2626,2 @@
> > >  
> > > +  nsIContent* curCellContent;
> > 
> > Please make this an nsCOMPtr, as it is a reference counted objects, and
> > holding on to the bare pointer is not necessarily safe.
> 
> it really should be, and even if it wasn't we never call any method on it or
> pass it to anyone.  We just immediately stuff it into nsCOMPtrs (I'm not sure
> if it could be exploited, but I really suspect changing the dom during that
> loop will cause wrong results).  However I grant this code is not exactly the
> sainist thing I've seen, and getting rid of this variable all together would
> probably make it clearer.

It _is_ safe today, but tomorrow someone will add a very innocent looking call before you assign curCellContent to an nsCOMPtr which ends up destroying the node, and then we'll have an exploitable sec-critical bug!  Unless this loop is extremely hot which would cause the extra AddRef/Release calls to show up in a profile, don't do this.  I'm tired of fixing security bugs caused by this exact code pattern.  ;-)

(Oh, and if you get rid of this variable that's fine too, but I have a hard time figuring out how that helps readability.)

> > ::: layout/tables/nsTableOuterFrame.h
> > @@ +133,5 @@
> > >  
> > > +  /**
> > > +   * Return the content for the cell at the given row and column.
> > > +   */
> > > +  nsIContent* GetCellAt(uint32_t aRowIdx, uint32_t aColIdx);
> > 
> > Nit: please make all of the getter methods here const.
> 
> good point, though there's part of me that wants to hope compilers  do that for
> themselves when its all inline.

Compilers can't read our minds, unfortunately!  ;-)  There are cases where you explicitly don't want a method to be const to make sure it cannot be called on const objects... (although those are rare)
(In reply to Ehsan Akhgari [:ehsan] from comment #15)
> (In reply to comment #14)
> > seems bugzilla swallowed this in a midair last night.
> > 
> > out/generic/Makefile.in
> > > @@ +43,2 @@
> > > >  		nsFrameSelection.h \
> > > > +		nsSplittableFrame.h \
> > > 
> > > I'm pretty sure we don't want to export all of these headers.  Please revert
> > > this change, and in directories where you need these headers, add
> > > layout/generic to the LOCAL_INCLUDES variable in the Makefile.
> > 
> > I'm pretty sure roc is ok with these, so I'm not sure what to say here.  It
> > doesn't matter too much to me one way or another.
> 
> I still don't like this idea for the following reasons:
> 
> * It increases our build times unnecessarily since this means that those
> headers will be copied/symlinked as part of our build process.
> * It encourages other parts of the code base to touch layout internals. 
> It's already bad enough that editor/ now depends on the implementation of
> table outer frames, but exporting this header and implicitly encouraging
> other parts of the code to do the same is not a desirable outcome.

true, on the other hand LOCAL_INCLUDES can be seen to encourage that part of editor/ to depend on whatever it likes in that part of layout/ not just the headers layout/ chooses to export.  For example you could see it as encouragement to use nsTableColGroupFrame.h.

I'm not saying that I like this situation I don't, but I think there are trade offs here, and no good solution other than use a different language.

> > > ::: layout/generic/nsSelection.cpp
> > > @@ +2495,5 @@
> > > > +        cellFrame->GetColIndex(origColIndex);
> > > > +        uint32_t actualRowSpan =
> > > > +          tableFrame->GetEffectiveRowSpanAt(origRowIndex, origColIndex);
> > > > +        uint32_t actualColSpan =
> > > > +          tableFrame->GetEffectiveColSpanAt(curRowIndex, curColIndex);
> > > 
> > > Hmm, maybe it's a good idea to have a helper method on nsTableCellFrame
> > > since this similar pattern happens in multiple places.
> > 
> > I'm not sure what similar pattern you see, it looks to me like we do similar,
> > but slightly different things.
> 
> Sure, but it seems to me like we can have helper methods like
> GetEffectiveRowColSpanAt, GetRowColIndex, etc.  But I don't care much either
> way...

oh, you just want to bunch getting the row and col values into one method?  I'd probably avoid that being the only way to get them since there are users that only want one, and having both seems redundant to me but shrug.

> > > @@ +2626,2 @@
> > > >  
> > > > +  nsIContent* curCellContent;
> > > 
> > > Please make this an nsCOMPtr, as it is a reference counted objects, and
> > > holding on to the bare pointer is not necessarily safe.
> > 
> > it really should be, and even if it wasn't we never call any method on it or
> > pass it to anyone.  We just immediately stuff it into nsCOMPtrs (I'm not sure
> > if it could be exploited, but I really suspect changing the dom during that
> > loop will cause wrong results).  However I grant this code is not exactly the
> > sainist thing I've seen, and getting rid of this variable all together would
> > probably make it clearer.
> 
> It _is_ safe today, but tomorrow someone will add a very innocent looking
> call before you assign curCellContent to an nsCOMPtr which ends up
> destroying the node, and then we'll have an exploitable sec-critical bug! 
> Unless this loop is extremely hot which would cause the extra AddRef/Release
> calls to show up in a profile, don't do this.  I'm tired of fixing security
> bugs caused by this exact code pattern.  ;-)

fair, on the other hand it could be argued that its better to die sooner than to try and continue in the weird case where you have a disconnected node.  It would probably be a harder exploit, but it wouldn't suprise me if it was still possible if someone makes assumptions about the nodes in the selection.

> (Oh, and if you get rid of this variable that's fine too, but I have a hard
> time figuring out how that helps readability.)

ok, I'll probably just do that then.

> > > ::: layout/tables/nsTableOuterFrame.h
> > > @@ +133,5 @@
> > > >  
> > > > +  /**
> > > > +   * Return the content for the cell at the given row and column.
> > > > +   */
> > > > +  nsIContent* GetCellAt(uint32_t aRowIdx, uint32_t aColIdx);
> > > 
> > > Nit: please make all of the getter methods here const.
> > 
> > good point, though there's part of me that wants to hope compilers  do that for
> > themselves when its all inline.
> 
> Compilers can't read our minds, unfortunately!  ;-)  There are cases where
> you explicitly don't want a method to be const to make sure it cannot be
> called on const objects... (although those are rare)

yeah, I was only considering the question from the view of what optimizations could be performed, although I think you might be able to come up with a example where the method actually being const matters for optimizing too.
(In reply to comment #16)
> > I still don't like this idea for the following reasons:
> > 
> > * It increases our build times unnecessarily since this means that those
> > headers will be copied/symlinked as part of our build process.
> > * It encourages other parts of the code base to touch layout internals. 
> > It's already bad enough that editor/ now depends on the implementation of
> > table outer frames, but exporting this header and implicitly encouraging
> > other parts of the code to do the same is not a desirable outcome.
> 
> true, on the other hand LOCAL_INCLUDES can be seen to encourage that part of
> editor/ to depend on whatever it likes in that part of layout/ not just the
> headers layout/ chooses to export.  For example you could see it as
> encouragement to use nsTableColGroupFrame.h.

No.  If you use LOCAL_INCLUDES, you'll just advertize to everybody that you feel bad about doing something that layout doesn't want you to do.  If you use EXPORTS, you advertize to the world that it's OK to depend on the internal data structures layout uses.  There lies the difference.

> I'm not saying that I like this situation I don't, but I think there are trade
> offs here, and no good solution other than use a different language.

But from a practical standpoint, nothing other than cross-module visibility settings can prevent me from depending on any code inside Gecko.  So the trade-off that you're talking about doesn't really exist.

> > Sure, but it seems to me like we can have helper methods like
> > GetEffectiveRowColSpanAt, GetRowColIndex, etc.  But I don't care much either
> > way...
> 
> oh, you just want to bunch getting the row and col values into one method?  I'd
> probably avoid that being the only way to get them since there are users that
> only want one, and having both seems redundant to me but shrug.

No, I was talking about helper methods which would call the existing methods on your behalf in order to reduce code repetition where we need to call both of these methods, but like I said, I don't care very much, so feel free to ignore my comment here.

> > It _is_ safe today, but tomorrow someone will add a very innocent looking
> > call before you assign curCellContent to an nsCOMPtr which ends up
> > destroying the node, and then we'll have an exploitable sec-critical bug! 
> > Unless this loop is extremely hot which would cause the extra AddRef/Release
> > calls to show up in a profile, don't do this.  I'm tired of fixing security
> > bugs caused by this exact code pattern.  ;-)
> 
> fair, on the other hand it could be argued that its better to die sooner than
> to try and continue in the weird case where you have a disconnected node.  It
> would probably be a harder exploit, but it wouldn't suprise me if it was still
> possible if someone makes assumptions about the nodes in the selection.

We're not talking about dying sooner.  Using bare pointers to access refcounted objects will cause exploitable sec-critical bugs.  Using nsCOMPtrs in those situations but making the wrong assumptions about whether your node lives in a document for example will result in non-exploitable crashes.  :-)

> > Compilers can't read our minds, unfortunately!  ;-)  There are cases where
> > you explicitly don't want a method to be const to make sure it cannot be
> > called on const objects... (although those are rare)
> 
> yeah, I was only considering the question from the view of what optimizations
> could be performed, although I think you might be able to come up with a
> example where the method actually being const matters for optimizing too.

Yeah, possibly.  I'm not entirely sure if compilers pay attention to const during optimizations, but I wouldn't be surprised if some of them would.
(In reply to Ehsan Akhgari [:ehsan] from comment #17)
> (In reply to comment #16)
> > > I still don't like this idea for the following reasons:
> > > 
> > > * It increases our build times unnecessarily since this means that those
> > > headers will be copied/symlinked as part of our build process.
> > > * It encourages other parts of the code base to touch layout internals. 
> > > It's already bad enough that editor/ now depends on the implementation of
> > > table outer frames, but exporting this header and implicitly encouraging
> > > other parts of the code to do the same is not a desirable outcome.
> > 
> > true, on the other hand LOCAL_INCLUDES can be seen to encourage that part of
> > editor/ to depend on whatever it likes in that part of layout/ not just the
> > headers layout/ chooses to export.  For example you could see it as
> > encouragement to use nsTableColGroupFrame.h.
> 
> No.  If you use LOCAL_INCLUDES, you'll just advertize to everybody that you
> feel bad about doing something that layout doesn't want you to do.  If you
> use EXPORTS, you advertize to the world that it's OK to depend on the
> internal data structures layout uses.  There lies the difference.

My point was that once you've told the world you depend on layout internals your then free to poke at whatever you choose.

> > I'm not saying that I like this situation I don't, but I think there are trade
> > offs here, and no good solution other than use a different language.
> 
> But from a practical standpoint, nothing other than cross-module visibility
> settings can prevent me from depending on any code inside Gecko.  So the
> trade-off that you're talking about doesn't really exist.

I certainly can't prevent you, but having to add local includes or export more stuff or play other games does make it harder for you to do so.

you also want to keep local includes usage rare so people do feel bad about it.
Adding to LOCAL_INCLUDES sounds slightly better to me.
Attached patch patch 2 — — Splinter Review
Attachment #691648 - Flags: review?(ehsan)
Comment on attachment 691648 [details] [diff] [review]
patch 2

I'm happy that you moved to use LOCAL_INCLUDES.  Other than that, I think roc should have the final say over this.
Attachment #691648 - Flags: review?(ehsan) → review?(roc)
Comment on attachment 691648 [details] [diff] [review]
patch 2

Review of attachment 691648 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tables/nsTableOuterFrame.h
@@ +188,5 @@
> +      return nullptr;
> +    }
> +
> +    return map->GetCellInfoAt(aRowIdx, aColIdx);
> +    }

Fix indent
Attachment #691648 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/3d898c39d05e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 857487
Fix for this bug caused a regression filed under bug 898321.
Depends on: 898321
Found another regression testing bug 898321. New bug coming up...
Depends on: 898438
Regressions: 1616225
You need to log in before you can comment on or make changes to this bug.