Closed
Bug 781409
Opened 9 years ago
Closed 9 years ago
remove nsITableLayout
Categories
(Core :: Layout: Tables, defect)
Core
Layout: Tables
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: tbsaunde, Assigned: tbsaunde)
References
(Regressed 1 open bug)
Details
(Keywords: APIchange)
Attachments
(3 files)
72.28 KB,
patch
|
Details | Diff | Splinter Review | |
73.02 KB,
patch
|
roc
:
review+
davidb
:
review+
ehsan
:
review-
|
Details | Diff | Splinter Review |
73.81 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
basically everything about nsITableLayout is bad.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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
Assignee | ||
Comment 4•9 years ago
|
||
>
> @@ +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?(roc) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #650861 -
Flags: review?(ehsan)
Attachment #650861 -
Flags: review?(dbolter)
Comment 5•9 years ago
|
||
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
Comment 6•9 years ago
|
||
Note: overall I like the patch.
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Comment 8•9 years ago
|
||
(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 9•9 years ago
|
||
Comment on attachment 650861 [details] [diff] [review] patch OK r=me. Please fix nits.
Attachment #650861 -
Flags: review?(dbolter) → review+
Comment 10•9 years ago
|
||
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-
Comment 11•9 years ago
|
||
(Note that I did not review anything in accessible/ since dbolter already did that!)
Comment 12•9 years ago
|
||
(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.
Comment 13•9 years ago
|
||
(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!
Assignee | ||
Comment 14•9 years ago
|
||
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 :)
Comment 15•9 years ago
|
||
(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)
Assignee | ||
Comment 16•9 years ago
|
||
(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.
Comment 17•9 years ago
|
||
(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.
Assignee | ||
Comment 18•9 years ago
|
||
(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.
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #691648 -
Flags: review?(ehsan)
Comment 21•9 years ago
|
||
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+
Assignee | ||
Comment 23•9 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d898c39d05e
Comment 24•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3d898c39d05e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment 25•8 years ago
|
||
Fix for this bug caused a regression filed under bug 898321.
Comment 26•8 years ago
|
||
Found another regression testing bug 898321. New bug coming up...
You need to log in
before you can comment on or make changes to this bug.
Description
•