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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
15 years ago
9 years ago

People

(Reporter: ajschult784, Assigned: bernd_mozilla)

Tracking

({testcase})

Trunk
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P3], )

Attachments

(11 attachments, 4 obsolete attachments)

581 bytes, text/html
Details
584 bytes, text/html
Details
584 bytes, text/html
Details
427 bytes, text/html
Details
2.36 KB, text/html
Details
642 bytes, text/html
Details
29.98 KB, patch
Details | Diff | Splinter Review
39.90 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
45.96 KB, patch
Details | Diff | Splinter Review
45.96 KB, patch
Details | Diff | Splinter Review
4.41 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Reporter

Description

15 years ago
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.
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.
Reporter

Comment 3

15 years ago
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?
> 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).
Patch in bug 254053 may help this one, actually...
Depends on: 254053
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.
Depends on: 254538
Patch in bug 254538 doesn't seem to fix this either.
Priority: -- → P1
Target Milestone: --- → mozilla1.8alpha4
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.
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.
Assignee: bzbarsky → nobody
Component: Style System (CSS) → Layout: Tables
Keywords: regression
Priority: P1 → --
QA Contact: ian → core.layout.tables
Summary: styles not updated properly after changed via DOM → [BC] border styles not updated properly after changed via DOM
Target Milestone: mozilla1.8alpha4 → ---
Assignee

Comment 12

15 years ago
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.
Assignee

Comment 13

15 years ago
which we dont call during the corresponding reflows
Assignee: nobody → bernd_mozilla
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?
Assignee

Comment 15

15 years ago
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.
Assignee

Updated

15 years ago
Blocks: 172767
Assignee

Updated

15 years ago
Blocks: 265143
Assignee

Updated

15 years ago
Depends on: 275139

Comment 16

14 years ago
(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

14 years ago
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
Assignee

Comment 19

14 years ago
*** Bug 292922 has been marked as a duplicate of this bug. ***
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).
Any idea what things in particular are not doing an update and should be?
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...
The reflow branch has nothing to do with this.
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.
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.
Assignee

Comment 26

14 years ago
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

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

Comment 28

13 years ago
(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

13 years ago
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

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

Comment 31

12 years ago
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

12 years ago
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.

Updated

12 years ago
Blocks: 403386

Updated

11 years ago
Duplicate of this bug: 371248

Updated

11 years ago
Duplicate of this bug: 406393
Whiteboard: [Hixie-P3]
Assignee

Comment 35

11 years ago
Posted file no border recalc (obsolete) —
Assignee

Comment 36

11 years ago
Attachment #338444 - Attachment is obsolete: true
Assignee

Comment 37

11 years ago
Boris, shall I implement a nsTable(*)Frame::DidSetStyleContext and set the damage area if the border style changes?
That might not be a bad idea....
Assignee

Comment 39

11 years ago
Posted patch patch (obsolete) — Splinter Review
Assignee

Comment 40

11 years ago
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
Attachment #341822 - Flags: superreview?(bzbarsky)
Attachment #341822 - Flags: review?(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.
Attachment #341822 - Flags: superreview?(bzbarsky)
Attachment #341822 - Flags: superreview-
Attachment #341822 - Flags: review?(bzbarsky)
Attachment #341822 - Flags: review-
Assignee

Comment 42

11 years ago
Posted patch revised patch (obsolete) — Splinter Review
Boris, I am not certain that the BordersAreDifferent code is correct, at least what it does now is what I grapsed from your comment.
Attachment #342787 - Flags: superreview?(bzbarsky)
Attachment #342787 - Flags: review?(bzbarsky)
Assignee

Updated

11 years ago
Attachment #342787 - Attachment is patch: true
Assignee

Updated

11 years ago
Attachment #341822 - Attachment is obsolete: true
Assignee

Comment 43

11 years ago
Posted patch reftestsSplinter Review
Attachment #342787 - Flags: superreview?(bzbarsky)
Attachment #342787 - Flags: superreview-
Attachment #342787 - Flags: review?(bzbarsky)
Attachment #342787 - Flags: review-
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.
Assignee

Comment 45

11 years ago
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?
Assignee

Comment 46

11 years ago
> 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.
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.
Assignee

Comment 48

11 years ago
Posted patch next round (obsolete) — Splinter Review
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.
Attachment #342787 - Attachment is obsolete: true
Attachment #343421 - Flags: superreview?(bzbarsky)
Attachment #343421 - Flags: review?(bzbarsky)
Assignee

Updated

11 years ago
Duplicate of this bug: 457568
Why aren't we just asking the style struct for the hint instead of trying to guess it?
Assignee

Comment 51

11 years ago
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.
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 on attachment 343421 [details] [diff] [review]
next round

r- per comment 52.
Attachment #343421 - Flags: superreview?(bzbarsky)
Attachment #343421 - Flags: superreview-
Attachment #343421 - Flags: review?(bzbarsky)
Attachment #343421 - Flags: review-
Assignee

Comment 54

11 years ago
" 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.
Assignee

Comment 55

11 years ago
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 ;-))
> 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.
Assignee

Comment 57

11 years ago
and the BCRecalcNeeded got the required change.
Attachment #343421 - Attachment is obsolete: true
Attachment #344523 - Flags: superreview?(bzbarsky)
Attachment #344523 - Flags: review?(bzbarsky)
Attachment #344523 - Attachment is patch: true
Is is possible to do an interdiff against the last patch I looked at?
Assignee

Comment 59

11 years ago
Posted patch interdiffSplinter Review
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)
Assignee

Updated

11 years ago
Attachment #344523 - Flags: superreview?(bzbarsky)
Attachment #344523 - Flags: review?(bzbarsky)
Assignee

Comment 60

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

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

Comment 61

11 years ago
Assignee

Updated

11 years ago
Attachment #344523 - Flags: superreview?(bzbarsky)
Attachment #344523 - Flags: review?(bzbarsky)
Assignee

Comment 62

11 years ago
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.
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 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
Attachment #344523 - Flags: superreview?(bzbarsky)
Attachment #344523 - Flags: superreview+
Attachment #344523 - Flags: review?(bzbarsky)
Attachment #344523 - Flags: review+
Assignee

Comment 65

11 years ago
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.
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

11 years ago
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.
Except we do want it inlined in general, I suspect.  Would adding inline to the decl in the header also work?

Comment 69

11 years ago
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.
Assignee

Comment 70

11 years ago
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 }
I checked with dbaron, and let's just drop the inline in the C++.
Assignee

Comment 72

11 years ago
Boris this removes the inline and incorporates your review comments that are still not checked in (sorry for the later).
Attachment #345158 - Flags: review?(bzbarsky)
Assignee

Updated

11 years ago
Attachment #345158 - Attachment is patch: true
Attachment #345158 - Flags: superreview+
Attachment #345158 - Flags: review?(bzbarsky)
Attachment #345158 - Flags: review+
Assignee

Comment 73

11 years ago
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 462849
Assignee

Updated

11 years ago
Duplicate of this bug: 229712
You need to log in before you can comment on or make changes to this bug.