258377 - [BC] border styles not updated properly after changed via DOM

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Andrew Schultz]

The testcase (attachment 158166 [details]) I created for bug 254053 now appears to cover a
different bug as it rendered properly until recently and the original testcase
for that bug has problems in 1.0.

The testcase sets the border on a table and its only cell via a stylesheet and
then changes them via DOM.  The resulting table border is incorrect.

with older builds, a single solid red border is displayed.
In recent builds, an outer red border is displayed along with an inner dotted
black border when the bug doesn't show up.
When the bug does show up, the solid red border is displayed on the top, left
and right.  A thick dotted black border exists on the bottom.

the change in behavior happened between linux trunk 2004080905 and 2004081006,
indicating a regression from bug 230170.

to get the alternative behavior with current trunk, remove the external stylesheet.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Andrew, does changing the order of the DOM calls affect behavior in current
builds?  What about builds before bug 230170 landed?

I'll debug this in detail when I get back, I guess.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Mental note: testcase is attachment 158167 [details]

Comment 3

[Andrew Schultz]

if the order of the calls is switched, there is a single dotted (1px) black border.

before 230170, a single solid red border is displayed (regardless of order).

the current correct behavior (2 borders) is also the old correct behavior, and
the change in behavior from wrong to bad isn't really a bug.  so the attached
testcase is probably just an alternative way to hit bug 254053?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> the current correct behavior (2 borders) is also the old correct behavior

Why is that correct?  The table has collapsed borders, right?  So we should be
ending up with a 1px solid red border all around, as far as I can tell... (as
in, this bug is valid).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Patch in bug 254053 may help this one, actually...

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, this is not fixed by bug 254053.  I wonder whether this is related to bug
254538, though.  Still looking for time to debug this.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Patch in bug 254538 doesn't seem to fix this either.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing incorrect rendering in pre-bug-230170 builds

The problem is that the dynamic style change is just broken somehow.  The only
reason it worked before my patch is that the style was being set before the
initial reflow, whereas the async setting of style makes it run after.	This
testcase sets the style onload (so after initial reflows) and has identical
effects on both builds.  Testcases also coming up with reflows forced in
between the two settings, which will demonstrate behavior identical to current
builds in a pre-bug-230170 build.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Black too-wide bottom border in all builds

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Black all-around border in all builds

Comment 11

[Boris Zbarsky [:bzbarsky]]

Over to table layout. There's clearly some sort of border info cache that's not
being invalidated right here...

Removing regression keyword, since the regression was just a timing change
exposing an underlying incremental reflow bug.

Comment 12

[Bernd]

We cache the winning border in the cellmap. We cache the border computation
results as much as possible. Only SetBCDamageArea will cause a recomputation of
the BC.

Comment 13

[Bernd]

which we dont call during the corresponding reflows

Comment 14

[Boris Zbarsky [:bzbarsky]]

We probably need to call it any time we have a style change reflow or a repaint
targeted at a table element...

Perhaps ProcessRestyledFrames() needs to know about it?

Comment 15

[Bernd]

I think the style change targeted at a table frame together with a style reflow
if above a table frame should do the job. If one changes only the color we dont
need to do this as we fetch the color every time we paint, it is not cached.

However our there is one place that depends on the color, the start of a segment
is also triggered by a colour change. Arggh, this sucks, the sooner we paint the
borders on the table cells the better.

Comment 16

[Garrett Smith]

(In reply to comment #11)
> Over to table layout. There's clearly some sort of border info cache that's not
> being invalidated right here...

I'm experiencing the same problem when modifying borderWidth on a stylesheet for
td. all other border properties for td (width and color) are not bugged -- just
width). The width is changed in the stylesheet, the page is not reflowed.

To have this not work would break the functionality of the application I'm
developing.

The problem only occurs concurrent with 
border-collapse: collapse;

Try this bookmarklet in the attachment 159433 [details], you'll see what I mean:
javascript:alert(document.getElementById('foo').style.borderTop = "11px solid red");

You'll notice that the border color and style change. The width remains at 1px.

A possible hack for any desparate developers might be to momentarily set border
collapse to 'separate' then change the border width, then set border collapse 
back to 'collapse'.

Comment 17

[Gérard Talbot]

Please: is bug 292922 a duplicate of this bug? If so, then I think attachment
182641 [details] (that bug's testcase) would be excellent for this bug.
Thanks

Comment 18

[Gérard Talbot]

Also attachment 182655 [details] ...

Comment 19

[Bernd]

*** Bug 292922 has been marked as a duplicate of this bug. ***

Comment 20

[Julien "_FrnchFrgg_" RIVAUD]

In nsTableFrame::PaintBCBorders, a BCMapBorderIterator is used to loop through
the border chunks of the table.

Those border chunks are grouped together when they are following each other,
with the same style (size, color, etc).

The problem is that (some) style change does not trigger
any update of the border map. By adding

nsRect rect(0, 0, GetColCount(), GetRowCount());
SetBCDamageArea(rect);
CalcBCBorders();

at the beginning of nsTableFrame::PaintBCBorders, thus forcing the border map
to always be up to date by a ugly hack, I get expected behaviour.

I think there are a lot of bugs in dynamic BC styling that are symptoms of this lack of update (at least this bug, bug 286797, bug 275894, and bug 319234).

Comment 21

[Boris Zbarsky [:bzbarsky]]

Any idea what things in particular are not doing an update and should be?

Comment 22

[Julien "_FrnchFrgg_" RIVAUD]

At least, when the color of a border is changed by a script, the border chunks (stored in the cellmap, I think) aren't calculated anew.

My ugly "fix" corrected this bug and a lot of others, but I am not sure if my two added lines trigger only a few updates or a whole bustle.

I also had difficulties to decypher the BC code and guess where the updates should be triggered (I know according to which actions, but I don't know how that translate into code localisation) and how (what to calculate again... I found tricky the dependancies between the information stored), so I cannot tell where some function call is missing, sorry.

I am wondering if the work of David Baron on his new reflow branch will invalidate all this stuff, since he seems to have to rewrite a lot of the table code to achieve his goals...

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The reflow branch has nothing to do with this.

Comment 24

[Julien "_FrnchFrgg_" RIVAUD]

OK. Since in the wiki page you said that there was a lot of work to make the tables work with the new reflow branch, I just wondered if some of this code would need to be rewritten or go away.

If it is not affected, I can continue to investigate without worrying about the usefulness of my efforts.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Hmm... Yeah, I don't see calls to SetBCDamageArea() for anything but changes to the frame structure (addition/removal of cells/rows/columns).  That doesn't seem right to me at all -- we should be setting the damage area when various border styles change.

Comment 26

[Bernd]

see comment 15, sorry for lately responding, the whole border collapse assembler code should go away and make place for paint on the cell borders. One prerequisite is to remove the style hacks, that cause problems during dynamic style changes. This would require lifting the patch from bug 43178 and make it running. I am heavily time constrained so I will not be able to do it myself but promise to help if you go for it.

Bug 203686 outlines the general issues we have.

Comment 27

[Eli Friedman]

*** Bug 343670 has been marked as a duplicate of this bug. ***

Comment 28

[Jayden]

(In reply to comment #27)
> *** Bug 343670 has been marked as a duplicate of this bug. ***
> 

Hi.  I posted Bug 343670.  I did search and didn't find this.  I also wouldn't have realized this was a duplicate even if I would have found it.  I guess anyways there's now a few more examples to look at.

Comment 29

[mike]

Attachment: Another testcase, maybe -- border missing when set in DOM

I'm not 100% sure if this example shows the same bug, but I hope it helps.  A two-cell table has the class of one cell set in HTML and the other in JavaScript by setting the className property.  The one set in HTML has a border, while the one set in code does not.

Comment 30

[mike]

Oops, the example I submitted uses setAttribute("class", ...) and does not set className as I said, but the result is the same.

Comment 31

[Arthur Blake]

Attachment: border size changes via javascript not respected on ALL firefox versions and platforms for tables with border-collapse:collapse

one more test case...

border size changes aren't respected at all when border-collapse is set to collapse.

This seems to occur on ALL versions of Firefox and on ALL platforms I've been able to test.

In the attachment, 3 tables are shown, and the colors, style and size are changed on each table, progressively, 5 times.  The size is progressively increased at 2 second intervals. But firefox doesn't respect these size changes even though it does respect changes to the style and color of the border (as is also demonstrated) (windows only that is-- linux is not even respecting the color change or even the initial border setting as windows does...)

Problem occurs on Windows XP, with Firefox 1.5, 2.0 and 3.0a3.

Also occurs on Linux (fedora core 6) with Firefox 1.5 and (on ubuntu 6.10) Firefox 2.0.

On Linux the problem is even worse, in that the initial border setting doesn't take at all.

other (non-firefox) browsers

On Windows XP with Internet Explorer 6 and 7 and Opera 9, the test case works correctly.

On linux (fedora core 6) Konquerer 3.5.6, the test case also works correctly

Comment 32

[Arthur Blake]

FWIW, I just built the trunk from CVS under linux, and this build is behaving exactly as windows does.  That is, the border color and border style changes from javascript function correctly, but border sizing changes do not work.

Comment 35

[Bernd]

Attachment: no border recalc

Comment 36

[Bernd]

Attachment: revised testcase with reference

Comment 37

[Bernd]

Boris, shall I implement a nsTable(*)Frame::DidSetStyleContext and set the damage area if the border style changes?

Comment 38

[Boris Zbarsky [:bzbarsky]]

That might not be a bad idea....

Comment 39

[Bernd]

Attachment: patch

Comment 40

[Bernd]

Comment on attachment 341822 [details] [diff] [review]
patch

The patch is a descendant of the patch from bug 426629, where dbaron did already have a look at the stuff outside layout/tables

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 341822 [details] [diff] [review]
patch

>+++ b/layout/tables/nsTableCellFrame.cpp
>+nsTableCellFrame::DidSetStyleContext(nsStyleContext* aOldStyleContext)
>+      nsTableFrame::BorderDifferent(aOldStyleContext,GetStyleContext())) {

Space after comma, please.  Same at other callsites.

>+    nsRect damageArea = nsRect(colIndex, rowIndex, 1,1);

why do we not care about rowspan/colspan here?

>+++ b/layout/tables/nsTableColGroupFrame.cpp
>+    nsRect damageArea = nsRect(GetFirstColumn()->GetColIndex(), 0, GetColCount(), tableFrame->GetRowCount());

80 chars?

By the way, why do you construct and then assign here and elsewhere instead of just constructing:

  nsRect damageArea(x, y, width, height);

>+++ b/layout/tables/nsTableFrame.cpp
>+nsTableFrame::DidSetStyleContext(nsStyleContext* aOldStyleContext)
>+  if (!aOldStyleContext) //avoid this on init or nextinflow

Here and elsewhere...  I don't see what nextinflow has to do with this.

>+nsTableFrame::BorderDifferent(nsStyleContext* aOldStyleContext,
>+  const nsStyleBorder* oldStyleData = aOldStyleContext->GetStyleBorder();

Calling GetStyle* on the old context is bad.  It might give you wrong data.  It might crash.  Don't do it.

You want to PeekStyleData like nsStyleContext::CalcStyleDifference does, and presumably return false here if it returns null.

>+  NS_FOR_CSS_SIDES(side) {

Seems to me like it might be a better idea to just call nsStyleBorder::CalcDifference, and if that reports no difference check whether both borders are foreground-color and if so check the actual colors (again, using PeekStyleData as needed).  For example, it's not clear to me that the code as written handles border-image well.

I think it might be better to call BorderDifferent something like BordersAreDifferent and to clearly document that the old style context is the one we're forgetting, and hence possibly completely bogus for GetStyle* purposes.

Comment 42

[Bernd]

Attachment: revised patch

Boris, I am not certain that the BordersAreDifferent code is correct, at least what it does now is what I grapsed from your comment.

Comment 43

[Bernd]

Attachment: reftests

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 342787 [details] [diff] [review]
revised patch

>+++ b/layout/tables/nsTableCellFrame.cpp
>+nsTableCellFrame::DidSetStyleContext(nsStyleContext* aOldStyleContext)
>+    PresContext()->PresShell()->FrameNeedsReflow(tableFrame,
>+                                 nsIPresShell::eTreeChange, NS_FRAME_IS_DIRTY);

Er...  That's expensive.  _Really_ expensive.  Do we really need to do that on color changes?  I'd think we only need to do this if the borders different by a reflow hint, no?

>+nsTableFrame::BordersAreDifferent(nsStyleContext* aOldStyleContext,
>+                                  nsStyleContext* aNewStyleContext)

You might want to make this return a style hint, not a boolean, so callers can avoid unneeded reflows...

>+  if (newStyleData->CalcDifference(*oldStyleData) == NS_STYLE_HINT_NONE) {

reverse this check, and just return if they're not equal, then outdent the rest of this function?

>+        if (!oldStyleColor)
>+          return PR_TRUE;

That should probably be a PR_FALSE, since if there is no oldStyleColor that means no one has ever used it for anything, and hence a color change can't change anything.

Comment 45

[Bernd]

bernd>	bz: do you have minute?
	<bernd>	nsIPresShell::eTreeChange is there way that causes a reflow on a given frame that is less expensive?
	<bz>	no
	<bz>	but why are you causing a reflow if, say, the color changed?
	<bz>	a complete reflow of the entire table, at that
	<bernd>	the bc compuation is part of a table reflow
	<bernd>	and setting the bc damage are without a reflow is useless
	<bernd>	the color influences the edge computation
	<bz>	hold on
	<bz>	we don't recompute the bc conflict stuff without a reflow?
	<bernd>	exactly
	<bz>	that's idiotic
	<bz>	can we fix that?
	<bernd>	its software so we can fix it
	<bz>	just trigger a recomputation pass without reflow
	<bz>	I mean...
	<bz>	what you have there will blow away all the cached width info
	<bz>	on everything in the table
	<bz>	will then proceed to reflow everything
	<bz>	column rebalancing, the works
	<bz>	can you point me to where the bc stuff is recomputed?
	* bz	is having a hard time believing we need to do a dirty reflow on the _table_
	<bernd>	http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#1630
	<bernd>	http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#1872
	<bz>	so...
	<bz>	why not just post an event to call GetMinWidth() ?
	<bz>	Without actually reflowing?
	<bz>	Except in the case when the border width might have actually changed, in which case we should indeed reflow
	<bz>	but the thing to mark dirty should be the node whose style changed, or _maybe_ its parent
	<bz>	not the whole table
	<bz>	Unless I'm missing something
	<bernd>	couldnt we compare what kind of reflow hint we get?
	<bernd>	if its visual then getminwidth
	<bernd>	if reflow then target a reflow at the node?
	<bz>	yes
	<bz>	that's exactly what I was suggesting in the bug
	<bz>	make the border-difference calculation return a hint
	<bz>	then act accordingly
	<bz>	note that you don't want to synchronously call GetMinWidth()
	<bz>	you want to do it async
	<bz>	in case we're restyling a whole bunch of cells at once
	<bernd>	bz: sorry for the incompetence, but how do I call this async, do you have example where I can see it in the code?

Comment 46

[Bernd]

> Boris:   just post an event to call GetMinWidth(). 

I haven't seen a place where this is done. I would appreciate more detailed hint. 

MarkIntrinsicWidths dirty would do the job but as the code says I should not call it directly and the only function that does this is nsIPresShell::eTreeChange, so thats a circle for me.

Comment 47

[Boris Zbarsky [:bzbarsky]]

class nsDelayedGetMinWidth : public nsRunnable {
public:
  nsDelayedGetMinWidth(nsIFrame* aFrame) :
    mFrame(aFrame) {}

  NS_IMETHOD Run() {
    if (mFrame)
      mFrame->GetMinWidth();
    return NS_OK;
  }
private:
  nsWeakFrame mFrame;
};

Then create one of those and dispatch using NS_DispatchToCurrentThread or something along those lines should do the trick, I would think.  Or really, just have the Run() method call whatever code will recompute the BC stuff and invalidate the right rectangles for painting.

Comment 48

[Bernd]

Attachment: next round

Boris, my comment about colors was wrong. So we need only to generate a call to CalcBCBorder in the case for style changes which would only generate a visual hint, border width changes cause a reflow hint which will pickup the marked bc damage area.
Border images are not used for BC stuff so the visual hint is probably enough. 

Overall this should be less performance demanding than the patch before.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Why aren't we just asking the style struct for the hint instead of trying to guess it?

Comment 51

[Bernd]

Because it does not fit our needs.

we have 3 parameters that influence the border collapse borders:

1. width changes this causes a reflow style hint, we only need to mark the corresponding table area for recomputation, the reflow triggers then the recomputation
2. border style changes, this creates a visual style hint, we need to mark  the corresponding table area for recomputation and we need to cause a recomputation so that the painting will have the correct information.
3. border color and any other change, this creates a visual style hint, painting does all what we need, no need for marking.

I thought that this squeezes out the performance impact, I had the feeling that you wish that I make this least impact performance wise, and my feeling is that responding to all visual hints will be overkill (see point 3.). We can do it but it wastes cycles.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Oh, so the problem is that two things with visual hints need to be handled differently?   And the only one that needs recomputation is a style change?

Then I'd rather we did a CalcDifference and if it comes back visual check for style change to decide whether to mark.

That said, I'm surprised color changes don't need recomputation.  Can't they affect winners and such?

Comment 53

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 343421 [details] [diff] [review]
next round

r- per comment 52.

Comment 54

[Bernd]

" Can't they affect winners and such?"
ehm, mumble mumble, "If border styles differ only in color, then a style set on a cell wins over one on a row, which wins over a row group, column, column group and, lastly, table." mumble mumble.

Comment 55

[Bernd]

lease find inlined what change requires recomputation:
>   1.  Borders with the 'border-style' of 'hidden' take precedence over all other conflicting borders. Any border with this value suppresses all borders at this location.

border style change: visual hint but we need to mark and to trigger a recompute

>  2. Borders with a style of 'none' have the lowest priority. Only if the border properties of all the elements meeting at this edge are 'none' will the border be omitted (but note that 'none' is the default value for the border style.)

border style change: visual hint but we need to mark and to trigger a recompute  

>   3. If none of the styles are 'hidden' and at least one of them is not 'none', then narrow borders are discarded in favor of wider   ones.

border width change: reflow hint, we need to mark the area only as it will be recomputed during the reflow   

> If several have the same 'border-width' then styles are preferred in this order: 'double', 'solid', 'dashed', 'dotted', 'ridge', 'outset', 'groove', and the lowest: 'inset'.

either border width change: reflow hint, we need to mark  or a border style change as described above

>   4. If border styles differ only in color, then a style set on a cell wins over one on a row, which wins over a row group, column, column group and, lastly, table. When two elements of the same type conflict, then the one further to the left (if the table's 'direction' is 'ltr'; right, if it is 'rtl') and further to the top wins. 

   This depends only on the position  of the border color spec but not on the color itself! If the position of the style spec changes due to reframing we will recompute anyway as we did before. There is no scenario where a pure border color change will influence the BCCalc result.
 
In short I stand by my point that the patch is now technical correct, I will implement your latest requirement ("Then I'd rather we did a CalcDifference and if it comes back visual check for style change to decide whether to mark"), although I think it makes unnecessary computations. However they are not expensive. And the patch did improve already very much due to your review comments.

And finally this patch is in my eyes the highlight in layout tables in 3.1. (So I will paint the code pink to get it in ,...just kidding ;-))

Comment 56

[Boris Zbarsky [:bzbarsky]]

> This depends only on the position  of the border color spec but not on the
> color itself!

Ah, ok.  Yeah, I'd still rather do the simpler and more future-proof thing, but if it becomes a problem we can fall back on doing something special for color.

Comment 57

[Bernd]

Attachment: updated to tip

and the BCRecalcNeeded got the required change.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Is is possible to do an interdiff against the last patch I looked at?

Comment 59

[Bernd]

Attachment: interdiff

1.) this patch did bit rot as the method got a decom. Basically I changed all functions to the new signature.

2.) I did already checkin a similar stuff where I made tables to invoke fixed layout if the width changes from auto to fixed. So the didsetstyle function  changed significantly. You need to look at this.

3.) And most changes are in NeedToCalcBCBorders, there I changed it to call CalcDifference on the style context. I have to admit that the code looks more maintainable than the perf squeezed out code. (It just BC code that tricks me into perf assembler, thats Pavlov)

Comment 60

[Bernd]

Comment on attachment 344523 [details] [diff] [review]
updated to tip

f... the bit rotting and merge bite this patch

Comment 61

[Bernd]

Attachment: diff other way around

Comment 62

[Bernd]

Comment on attachment 344523 [details] [diff] [review]
updated to tip

I did panic (I forgot the rule: Don't panic)

The patch is not broken.

I am not sure that diff between the old and new patch is what you are asking for.

Comment 63

[Boris Zbarsky [:bzbarsky]]

I'm wasn't asking for the diff between old and new, no.  I was asking for the diff between a merge of the last thing of what I reviewed to tip, and the new patch.  Basically, a diff showing just the substantive changes, not the merge to the new DidSetStyleContext signature.

I guess I'll just review the new patch in its entirety.  :(

Comment 64

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 344523 [details] [diff] [review]
updated to tip

>+++ b/layout/tables/nsTableCellFrame.cpp
>+nsTableCellFrame::DidSetStyleContext(nsStyleContext* aOldStyleContext)
>+  return;

Not needed at the end here.

Similar for the other DidSetStyleContext impls you add.

>+++ b/layout/tables/nsTableFrame.cpp
>   nsHTMLContainerFrame::Destroy();
> }
>+
> 

Drop that blank line?

>+
>+   /* virtual */ void
>+nsTableFrame::DidSetStyleContext(nsStyleContext* aOldStyleContext)

Drop the extra bank line, and fix the indent?

>+class nsDelayedCalcBCBorders : public nsRunnable {
>+    if (mFrame) {
>+      nsTableFrame* tableFrame = static_cast <nsTableFrame*>(mFrame.GetFrame());
>+      if (tableFrame) {

You don't need both null-checks: one implies the other.

>+nsTableFrame::BCRecalcNeeded(nsStyleContext* aOldStyleContext,
>+        nsCOMPtr<nsIRunnable> evt = new nsDelayedCalcBCBorders(this);
>+        if (evt)
>+          NS_DispatchToCurrentThread(evt);

No need for the null-check.  Just dispatch the event.  NS_DispatchToCurrentThread handles a null input (by returning an error).

I'd prefer that you check the bits of the change, not check it for equality; in particular if we start reporting a change of reflow but not repaint for border widths, your code will get things wrong....

With those comments addressed, r+sr=bzbarsky

Comment 65

[Bernd]

I suck on checking in, please apologize I panicked on a red tree by not adding correct checkin comments

My basic understanding is that PeekStyleData is really hidden on Mac Optimized builds but not on Mac Debug builds. Further it looks like the hiding which should happen also on other platforms does not work.

Comment 66

[Boris Zbarsky [:bzbarsky]]

Uh...  That link error makes no sense.  The two object files are in the same lib, so we should be able to call PeekStyleData. bsmedberg, any idea what's going on here?

Undefined symbols:
  "nsStyleContext::PeekStyleData(nsStyleStructID)", referenced from:
      nsTableFrame::BCRecalcNeeded(nsStyleContext*, nsStyleContext*)in nsTableFrame.o
ld: symbol(s) not found
collect2: ld returned 1 exit status
make[4]: *** [XUL] Error 1

Comment 67

[Benjamin Smedberg]

The problem is that nsStyleContent::PeekStyleData is declared inline:

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext.cpp#238

Remove the inline and all will be well.

Comment 68

[Boris Zbarsky [:bzbarsky]]

Except we do want it inlined in general, I suspect.  Would adding inline to the decl in the header also work?

Comment 69

[Benjamin Smedberg]

If you want it inlined in general, move it to the .h file (and out of the .cpp file) so that this code can inline it or instantiate it also.

Comment 70

[Bernd]

Boris, what shall happen with the two lines (235,236) above the function? I understand that the code from 238 till 244 will go into the header. The declaration with the NS_HIDDEN should probably remain there too.  


235 #include "nsStyleStructList.h"
236 #undef STYLE_STRUCT
237 
238 inline const void* nsStyleContext::PeekStyleData(nsStyleStructID aSID)
239 {
240   const void* cachedData = mCachedStyleData.GetStyleData(aSID); 
241   if (cachedData)
242     return cachedData; // We have computed data stored on this node in the context tree.
243   return mRuleNode->GetStyleData(aSID, this, PR_FALSE); // Our rule node will take care of it for us.
244 }

Comment 71

[Boris Zbarsky [:bzbarsky]]

I checked with dbaron, and let's just drop the inline in the C++.

Comment 72

[Bernd]

Attachment: remove the inline

Boris this removes the inline and incorporates your review comments that are still not checked in (sorry for the later).

Comment 73

[Bernd]

fix + reftests checked in

thanks to boris, david and ben for helping me with this, this fixes an 6.5 year old not fully implemented feature.