Closed Bug 157681 Opened 22 years ago Closed 12 years ago

[DHTML]We reflow when we should just move

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16
Tracking Flags:
Tracking Status
firefox16 --- disabled
firefox17 --- disabled
firefox18 --- disabled

People

(Reporter: markushuebner, Assigned: ehsan.akhgari)

References

(Depends on 2 open bugs, Blocks 2 open bugs,
URL
)

Details

(Keywords: perf, testcase, topembed+, Whiteboard: [adt2][games:p3])

Attachments

(6 files, 10 obsolete files)

Profile
(deleted), application/x-gzip
Details
Part 1: Add a test suite
(deleted), patch
Details | Diff | Splinter Review
Part 2: Implementation
(deleted), patch
Details | Diff | Splinter Review
screenshot of mochitest-browser-chrome test failure
(deleted), image/png
Details
Make sure to reflow when {min,max}-width changes
(deleted), patch
dbaron
: review+
Details | Diff | Splinter Review
Simplify nsStylePosition::CalcDifference
(deleted), patch
Details | Diff | Splinter Review 
results on win-xp pro,1.1ghz,512ram.
trunk 2002071508: 851ms
msie6:            411ms
Keywords: perf
To clarify what the hell this bug report is talking about....

setting .style.left currently reflows the div.  It _should_ just reposition its
view and frame and then invalidate.  This has been talked about a bit, but there
seems to be no bug on it.

Things that would need to get done:

1)  A way to detect that a move is enough and that reflow is not needed.
2)  A way to actually do the moving.
Blocks: 21762, 149216
Keywords: testcase
Athlon 750

Moz trunk 2002070304 - 2340
IE6 - 428
PII 366MHz 160MB win98

Mozilla 2002071508 (1.1+) : 3.680s
IE6                       : 1.040s
Severity: normal → major
Keywords: mozilla1.2
OS: Windows XP → All
Hardware: PC → All
Priority: -- → P2
Nomianting for nsbeta1 as this appears to improve performance.
Keywords: nsbeta1
Whiteboard: [adt2]
Target Milestone: --- → Future
Blocks: 107746
> dbaron: nsAbsoluteContainingBlock reflows the abs pos elements, right/
> dbaron: ?
<dbaron> yes
> dbaron: could it keep a cache of computed position values for each element?
<dbaron> why?
<dbaron> (see also bug 75121)
> dbaron: and optimize away reflows to repositions if the computed props did not
change sufficiently
<dbaron> bz: Doing it that would would require having StyleChangedReflow tell us
what changed.  I think it's easier to make a different type of style hint.
> dbaron: true, since it could involve a font change or something in addition to
the reposition
> dbaron: so we would need a position-prop hint or something
<dbaron> yeah
<dbaron> And it would go to a function that decided whether to reflow or reposition.

nsbeta1+
Assignee: attinasi → kin
Keywords: nsbeta1 → nsbeta1+
Target Milestone: Future → mozilla1.2beta
Target Milestone: mozilla1.2beta → mozilla1.2alpha
Summary: We reflow when we should just move → [DHTML]We reflow when we should just move
Depends on: 160936
Athlon 1000, 512MB RAM, Debian+KDE3

1.1: 2676
konqueror 3.0.2: 1373
oh.. Opera 6.03/linux: 556 (!)
This would be such a huge win, and perhaps not difficult now.

The biggest problem I see is that repositioning a frame might require view
bounds to grow. We'd have to recalculate view bounds up the view tree.
This would be a major performance boost on overal DHTML and highly desired for 
1.2 - let's get things rockin'
Some further thoughts on implementing this (just off the top of my head, so I'm
not sure they're right):

 1. The "additional style hint" (a "move" hint) idea is a good bit easier now
thanks to roc's work for bug 160936.
 2. In order to process that style hint:
    + for non-positioned elements, do nothing
    + for relatively positioned elements, just move.  We might need to cache
      the un-offset position as a frame property or something like that, since
      we don't know the old value.  I'm not quite sure where we could do that
      since just about anything (with just about any parent) can be relatively
      positioned.
    + for absolutely positioned elements, we need to decide whether to move
      or reflow.  We currently cache a bit of information as a frame property
      (I think an nsRect representing the overflow area) in
      nsAbsoluteContainingBlock.cpp.  We should probably turn this rect into
      a struct containing the rect and put more information in it -- in
      particular, whether the positioned element wrapped when it was last
      reflowed, and the values of the four offset properties at the last
      reflow.  It might even be useful to cache the preferred width and
      preferred minimum width.  This information is really only needed to
      handle a 'width' of 'auto' (see section 10.3.7 of CSS 2.1 -- *not* CSS
      2).  We would then need to reflow rather than reposition in the
      following cases for auto-width positioned elements:
        + something wrapped at the previous reflow (i.e., we used the
          available width or the minimum width rather than the preferred
          width -- although it's theoretically not that hard to optimize
          some cases where minimum width is used as well, those cases are
          extremely rare on the web), and we now have *more* available space
        + we now have *less* available space, and the old width doesn't fit
          in the container anymore
      If we don't need to reflow, then we can just reposition. Handling
      switches of the offset properties to and from 'auto' might
      also be a little tricky, although I think not.
      It might be nice to have bug 75121 in before this, although hopefully
      they won't trip over each other too much.  However, bug 75121 is about
      exposing a bunch of that nsAbsoluteContainingBlock code that's been
      unused for years and has bugs in it, so it's not clear how much will
      need to be changed (I haven't even had a chance to even reduce the
      testcases for the things the patch breaks).
 3. Then, given that something has moved, we need to handle view sizes and
    repainting.  I'll let roc talk about this bit. :-)
(Note that it might be worth checking to see if the rules in CSS 2.1 are really
compatible with what MSIE implements for absolute positioning, or whether MSIE
does something that would be easier to implement this optimization on.  If
that's the case, it might be worth trying to change the rules further in CSS 2.1.)
Also note that in the case of relatively positioned elements, we can't forget
continuation frames, since inline elements can be relatively positioned.

Or we could skip the optimization for relatively positioned elements entirely in
the first pass, and just make them reflow.  Or is animation of relative
positioning also a big perf issue in real-world "DHTML"?
We have a number of serious relative positioning bugs that we should tackle
before we worry about optimizing relative positioning :-). (The main issue being
that lines and the reflow state for inline frames need to keep TWO "overflow
areas" --- one which pretends everything is in-flow, and one which accounts for
relative positioning.)
After moving a frame, repainting is easy; we just repaint the old and new frame
overflow areas.

Resizing the views and cached overflow areas is harder. Then we run up the view
stack making views bigger if they need to grow to contain new overflowing
children. If children move so that the view can be smaller, then things will
mostly work fine even if we don't shrink the view (except for scrolling views,
see below). That's good because otherwise we would need to know the overflow
areas of a lot of frames.

A tough problem is fixing up scrollbars if a scrolled area grows or shrinks.
This requires computing the exact size of the scroll view which could require
overflow areas we don't really have. We might want to be able to detect this
case and just reflow.

I think we should focus on bug 75121 before we tackle this. I would be very
interested to see what the performance of an incremental reflow targeted at an
absolutely positioned frame is like after that fix, especially when the
positioned frame is something simple like an image.

BTW the thought just occurred to me that we don't actually need to keep the
overflow area of an absolutely positioned frame in the frame property list; it
should always be the same as the bounds for that frame's view.
Blocks: 139986
Blocks: 124178
Nominating for topembed as DHTML perf is something we should address in the next
milestone for embedding customers.
Keywords: topembed
Actually, having worked on 75121 and with some further reflection, I think we
should concentrate on improving the incremental reflow of absolute frames rather
than adding an entirely new code path.
Why do you think that?
I can't think of a reason why repositioning of absolute frames using incremental
reflow has to do any more work than we'd have to do with a separate non-reflow
frame repositioning code path.

With 75121 we don't do much work as we descend through a stack of absolutely
positioned frames to get to the frame that's moving. On the way back out of the
stack we do some work, but as far as I can tell, we do exactly what we'd need to
do in a frame repositioning code path. With 75121 the big cost will probably be
the cost of processing a stylechange reflow for the moved frame, and that could
be expensive, and that's something we should fix (perhaps by adding a
"PositionFrame" incremental reflow reason?).

If I'm wrong about that and there are some big costs on the incremental reflow
path, then maybe we should see if we can fix those.

If we can get comparable performance, then doing this in the context of reflow
has architectural advantages: we'll add a lot less new code --- and especially
we'll reduce code duplication, which is really evil. We won't have to worry so
much about special cases that reflow already takes care of. The optimization
might also be applicable to more situations.

Is there a convincing case that this can't be done in the context of reflow?
The only time there's a strong case for repositioning instead of full reflow is
when we know the size did not change.  The idea is to optimize away the reflow
of the children of the positioned frame.

If we can usefully optimize this in the general case for reflow, we should by
all means do so.  The problem right now is that to decide what our size is after
style.left is set we have to do a full reflow of all the descendants.  We need
to detect the following constraints during reflow:

1)  Our size is independent of whatever happened to trigger the reflow
or [
2a) Nothing changed about our descendants (this is captured by the "resize"
    reflow type, which we would need to use here, somehow)
 and
2b) Our size depends only on our descendants (this typically only applies to
    things that are shrink-wrapping, like floats, tables, positioned elements).
]

If we can usefully detect these situations in normal reflow, I would be all in
favor of doing so and putting some shortcut code in ::Reflow() to bail out early
without reflowing kids when those conditions are met.
One thing that would make this a lot easier would be if .style.left actually
went through the nsStyleStruct CalcDifference path instead of the nsCSSParser
hint computation path. In the former path you have a lot more information to
work with to generate a good hint.
But yes, we definitely need a way to detect that style changes don't require
reflowing of children, and we need it *everywhere*.
Yeah, dbaron was thinking of not using the parser hint there in any case...  We
should just do it. 
Do you feel like doing it, Mr Style System Peer? :-)
Once I have a tree again (eta: 2.5 weeks) I'll give it a shot, sure.  See bug
158713 for that, probably.
Depends on: 158713
What other style changes require a reflow that doesn't require reflowing of
children?  (OK, vertical margins / border / padding sometimes don't (when floats
aren't present), but they require a much larger reflow of everything afterwards,
and I don't think they're common enough to merit optimization.)
Hmm. Maybe no important properties, once clip and z-index are fixed to not
require reflow.

Maybe we could have CalcDifference produce two extra hints,
nsChangeHint_ReflowRepositionFrame and nsChangeHint_ReflowResizeFrame (subhints
of nsChangeHint_ReflowFrame). Store the hint in the StyleChange reflow command,
and whack nsBlockFrame::Reflow to understand them --- i.e., basically do nothing
if it's just a RepositionFrame, and do a resize reflow if it's just a ResizeFrame.
Note that there are cases in which CalcDifference can't tell whether a reflow is
needed.  In particular, consider changing .left on something with auto width and
right... Whether a reflow is needed will depend on whether the preferred width
is smaller than the available width both before and after the .left change.
Always treating that as a size change probably isn't too bad.
Are there still such cases in the spec?  I hope the revised absolute positioning
rules got rid of them.  (I described this in more details in comment 11, but I
don't remember the summary.)
My previous comment was a response to comment 28.

Also, I'd rather see separate handling of this separate case than trying to make
nsBlockFrame::Reflow do yet more stuff (see comment 11).  It's complicated
enough as-is.
Marking as topembed+, as it is desired by major embedding partners.
Keywords: topembed → topembed+
Target Milestone: mozilla1.2alpha → mozilla1.3alpha
Blocks: 178882
Keywords: mozilla1.3
Keywords: mozilla1.2
Flags: blocking1.3b?
Moz 20021210: 3405
IE6         : 350
Opera7      : 10205
This depends on bz's inline style resolution fix. Marking an explicit
dependency. However, I think once that's fixed, this will not be too hard:

I suggets dividing nsChangeHint_Reflow into two sub-hints
nsChangeHint_ReflowPosition and nsChangeHint_ReflowGeneral. ReflowPosition would
generated when a style change affects 'left', 'top', 'bottom' or 'right' and
appropriate dimension ('width' or 'height') is not 'auto'. When we get a
ReflowPosition hint, we set something in the incremental reflow command to say
that the ResizeReflow for the frame itself can be suppressed.
Depends on: 171830
Target Milestone: mozilla1.3alpha → mozilla1.3beta
Blocks: 186465
Target Milestone: mozilla1.3beta → mozilla1.4alpha
Blocks: 188331
Flags: blocking1.3b? → blocking1.3b-
This is 1.4 material?
could be, although I think for many DHTML benchmarks painting is our bottleneck now
Blocks: 117601
Blocks: 117436
Just checked the testcase again:

results on win-xp pro,1.1ghz,512ram.
trunk 2003022709: 1242ms
msie6 sp1:        281ms

Any idea why we are doing even worse now [compared to when filing the bug]?

I wonder why in this testcase we need to reflow at all between assignments to
style.left.
Attached file Profile (deleted) — 
Do we?	I've been thinking about it, and with the way that testcase is written
I would expect we just post a ton of reflow commands at the div that all get
merged into a single reflow, executed after the loop finishes.	So I don't
think this testcase is in fact a good testcase for the problem this bug is
meant to address.

This is a profile (gzipped HTML) of that testcase (with the loop number bumped
up an order of magnitude).  Nothing really major going on here other than the
usual -- style resolution, parsing of the CSS, change notification processing.
Right. Not a good testcase for this bug. This testcase is only testing
style/DOM. A better testcase for this bug would be one which animates something
simple.
Flags: blocking1.4a?
Flags: blocking1.4a? → blocking1.4a-
Target Milestone: mozilla1.4alpha → mozilla1.5alpha
Some of the work needed for bug 175364 is rather similar to what needs to be
done for this bug.  (Really, it's a small subset of what needs to be done,
except that, as for this bug, we need to double-check that we're following the
absolute positioning rules correctly and avoiding any dependencies on the
containing block for anything other than percentages.  Of course, if we're not,
we'll just end up with incremental reflow bugs.)
Depends on: 201897
Blocks: 203448
is this still on track for 1.5a?
This isn't a priority for me. nsIFrame deCOMtamination and bug fixing is all I
have on my radar.
Moving to 1.6a. 1.5a is not realistic.
Target Milestone: mozilla1.5alpha → mozilla1.6alpha
Maybe 1.5b ?!
We all do agree that this is really important - but unfortunately postponed 
from milestone to milestone (originally 1.2alpha, as indicated by bug history).

roc/dbaron - anyone of you willing to take this ... kin seems to be able to 
get to this in the near future.
The following testcase shows that the size of absolutely positioned elements
does depend on the remaining space within the containing block, as CSS2.1 says:

  http://dbaron.org/css/test/2003/abs-pos-intrinsic3

This means we can't just move in the general case, which makes this a good bit
harder to solve, particularly with our current architecture.  (If min-width /
preferred with computation were separate from reflow, it might be easier to
cache the results and optimize based on those results.)
I guess this again will have to be retargeted ... 1.7a ?
taking bug
Assignee: kinmoz → dbaron
Target Milestone: mozilla1.6alpha → Future
Blocks: 286900
Blocks: 143097
The new testcase performs much better (28+-3% CPU) than the old one (100% CPU peak). Intel Celeron 2.8 GHz on 2.6.17-11-generic K/Ubuntu 6.10 (KDE 3.5.5) with display: :0.0  screen: 0
OpenGL vendor string: ATI Technologies Inc.
OpenGL renderer string: RADEON XPRESS Series Generic
OpenGL version string: 2.0.6011 (8.28.8)

Opera 9.20 took 260 for the old testcase and Firefox 2.0.0.3 took 1316.
Blocks: 500410
Just to update on the state of things here, bug 300030 fixed the issue David talks about in comment 47.
Depends on: reflow-refactor
Depends on: 502288
QA Contact: chrispetersen → layout
Is it still worth keeping this bug open?
Depends on: 524925
Assignee: dbaron → ehsan
I talked to dbaron about this, and here is what we plan to do here.

According to http://www.w3.org/TR/CSS2/visudet.html#abs-non-replaced-width, the only case where the width can change as a result of changing the position of an absolutely positioned element is rule number 5.  This is the only case where we would need to overflow, and we can skip that for all other cases.

The implementation would add two new hints for recomputing the horizontal and vertical position.  These two hints will be returned from nsStylePosition::CalcDifference, and in the processing code:

a) If the element is relatively positioned, we would recompute the position, and we would never need to reflow in this case.
b) If the element is absolutely positioned (including the case for position: fixed), we fall back to reflow in case width is auto and left and right are not auto.  For all other cases, we recompute the position, and avoid reflowing.

Note that in the cases where the reflow is skipped, we also need to use the UpdateOverflow hint in order to update the overflow information of the ancestors (which might have changed because of the position change.)
(In reply to Ehsan Akhgari [:ehsan] from comment #57)
> I talked to dbaron about this, and here is what we plan to do here.
> 
> According to http://www.w3.org/TR/CSS2/visudet.html#abs-non-replaced-width,
> the only case where the width can change as a result of changing the
> position of an absolutely positioned element is rule number 5.  This is the
> only case where we would need to overflow, and we can skip that for all
> other cases.

s/need to overflow/need to reflow/ (I think that's what you meant)

> The implementation would add two new hints for recomputing the horizontal
> and vertical position.  These two hints will be returned from
> nsStylePosition::CalcDifference, and in the processing code:
> 
> a) If the element is relatively positioned, we would recompute the position,
> and we would never need to reflow in this case.
> b) If the element is absolutely positioned (including the case for position:
> fixed), we fall back to reflow in case width is auto and left and right are
> not auto.  For all other cases, we recompute the position, and avoid
> reflowing.

We also might need to fall back to reflow for the cases where the new position is the "static position".  (Though it might be easy enough to refactor that code so it's usable.)  If we do, that's the main reason for wanting separate horizontal and vertical hints, since that would allow us to use this "move and update overflow" code when we have movement in one dimension but the opposite dimension uses a static position.

> Note that in the cases where the reflow is skipped, we also need to use the
> UpdateOverflow hint in order to update the overflow information of the
> ancestors (which might have changed because of the position change.)

We can just return the UpdateOverflow hint unconditionally.  nsCSSFrameConstructor::ProcessRestyledFrames *already* has code to ignore the UpdateOverflow hint when we also did a reflow.  (We should probably add the move-position hint *before* reflow in that function (so that it's easier for it to fall back to doing reflow), but ignore it when we have a hint that already requires reflow.)
Attached patch Part 1: Add a test suite (obsolete) (deleted) — — Splinter Review 

        Attachment #609955 -
        Flags: review?(dbaron)

    
    

    
  
layout/reftests/position-dynamic-changes/horizontal/fromauto-leftN-widthA-rightA-2.html seems to be affected by the same problem as bug 688545, which is probably an invalidation bug.  I will mark the manifest as such before landing.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 1: Add a test suite (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
This is the same as the previous patch, except that I added a couple of test cases for relative positioning as well.

        Attachment #609955 -
        Attachment is obsolete: true

        Attachment #611041 -
        Flags: review?(dbaron)

        Attachment #609955 -
        Flags: review?(dbaron)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 2: Implementation (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
This is the implementation patch, the comments at the beginning of the patch explain what it does.

        Attachment #611042 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 611042 [details] [diff] [review]
Part 2: Implementation

Here are review comments on everything except for the implementation of nsCSSFrameConstructor::RecomputePosition and the commit message (which I hope to get to tomorrow):

We have an assumption right now that the processing of a style hint also
applies that style hint to all descendants, except for the ones listed
in nsFrameManager::ReResolveStyleContext at
http://hg.mozilla.org/mozilla-central/file/4c43cfe73516/layout/base/nsFrameManager.cpp#l1067
You should add your new hint to that code... although, actually, the
better fix would be to add the union of the bitfields listed there and
your new nsChangeHint_RecomputePosition as an additional value to the
enum nsChangeHint, with a comment explaining the distinction, so that
it's more likely that those adding new bits will notice that they might
need to consider this.

Also please add a test that fails without this -- i.e., one that makes
simultaneous (i.e., without a flush between) changes to the offset
properties on an ancestor and a descendant.


nsCSSFrameConstructor.cpp:

>@@ -7931,6 +7933,12 @@ nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList& aChangeList)
>         ApplyRenderingChangeToTree(presContext, frame, hint);
>         didInvalidate = true;
>       }
>+      if (hint & nsChangeHint_RecomputePosition) {

This condition could check && !didReflow just like the UpdateOverflow
hint does.

STILL NEED TO REVIEW CONTENTS OF
nsCSSFrameConstructor::RecomputePosition
Need to think about:
 - does this work correctly when done on the root element's frame?

nsHTMLReflowState.cpp:

In the defintion of the function, please change:
  void
to:
  /* static */ void
so it's clear that it's static.

Please make aComputedOffsets the last parameter of
ComputeRelativeOffsets since it's the output.

Also, just drop the aPresContext parameter entirely and use:
  FrameProperties props = aFrame->Properties();
rather than messing with getting a pres context and messing with
the complicated construction of a FrameProperties object.

nsStyleStruct.cpp:

  You don't need to change the maxHint above the Display struct comparison,
  since you're only changing things related to the Position struct.

  (Also, reading this code led me to file bug 741012.)

    
    

    
  
(In reply to David Baron [:dbaron] from comment #65)
> We have an assumption right now that the processing of a style hint also
> applies that style hint to all descendants, except for the ones listed
> in nsFrameManager::ReResolveStyleContext at
> http://hg.mozilla.org/mozilla-central/file/4c43cfe73516/layout/base/
> nsFrameManager.cpp#l1067
> You should add your new hint to that code... although, actually, the
> better fix would be to add the union of the bitfields listed there and
> your new nsChangeHint_RecomputePosition as an additional value to the
> enum nsChangeHint, with a comment explaining the distinction, so that
> it's more likely that those adding new bits will notice that they might
> need to consider this.

Also, as I said in bug 741012 comment 4, you should also add an assertion
to the DO_STRUCT_DIFFERENCE macro in nsStyleContext::CalcDifference that
  nsStyle##struct_::ForceCompare() || 
  NS_SubtractHint(nsStyle##struct_::MaxDifference(),
                  nsChangeHint_NonInherited_Hints) ==
    nsStyle##struct_::MaxDifference()
where nsChangeHint_NonInherited_Hints is the new value I'm asking that you
add.

(nsStylePosition's ForceCompare does return true, but nothing checks that.)

    
    

    
  
Oh, so... I thought you'd come up with a solution where you compute the width and then decide whether to reflow, rather than taking the conservative route (which won't help nearly as many cases).  What led you not to do that?  I still think it's worth trying.

    
    

    
  
(In reply to David Baron [:dbaron] from comment #67)
> Oh, so... I thought you'd come up with a solution where you compute the
> width and then decide whether to reflow, rather than taking the conservative
> route (which won't help nearly as many cases).  What led you not to do that?
> I still think it's worth trying.

I'd like to take on doing that in a follow-up bug, as it can be done with a small change to nsCSSFrameConstructor::RecomputePosition(), but requires a bit more testing.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 2: Implementation (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Addressed the review comments so far.

        Attachment #611042 -
        Attachment is obsolete: true

        Attachment #611632 -
        Flags: review?(dbaron)

        Attachment #611042 -
        Flags: review?(dbaron)

    
    

    
  
I'm getting reftest failures like this on Windows:

https://tbpl.mozilla.org/php/getParsedLog.php?id=10588925&tree=Try

If you look at the move-top-left.html image difference, it seems like I'm invalidating one pixel less that what is necessary, but I don't know why this happens.  Is invalidating the overflow rect not enough?

    
    

    
  
Invalidating the overflow rect should be enough. Make sure both the old and new areas are invalidated.

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #72)
> Invalidating the overflow rect should be enough. Make sure both the old and
> new areas are invalidated.

I invalid the overflow rect once before I change the position of the frame and once after it.  That should be enough, right?

    
    

    
  
Yes.

This failure looks to me like the text overflow area isn't including all of the antialiased pixels. I thought we added enough fuzz on Windows now to avoid that? Maybe not.

You can work around this by adding padding:1px or 2px to some of those elements, I guess.

    
    

    
  
But wouldn't that be just a wallpaper to make the tests pass?  The trail caused by moving the text node is actually visible on the screen.  I don't know if that's acceptable!

    
    

    
  
Add padding to the tests and then file a bug about the overflow area issue and assign it to Jonathan :-).

    
    

    
  
Heh, OK, filed bug 742176.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 1: Add a test suite (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #611041 -
        Attachment is obsolete: true

        Attachment #612095 -
        Flags: review?(dbaron)

        Attachment #611041 -
        Flags: review?(dbaron)

    
    

    
  
Try run for 2cf3cc3351c4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2cf3cc3351c4
Results (out of 217 total builds):
    success: 180
    warnings: 37
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-2cf3cc3351c4

    
    

    
  
Strangely, adding the padding did not fix the reftest failure on Linux!

    
    

    
  
(In reply to Ehsan Akhgari [:ehsan] from comment #80)
> Strangely, adding the padding did not fix the reftest failure on Linux!

Sorry meant Windows.

    
    

    
  
Comment on attachment 611632 [details] [diff] [review]
Part 2: Implementation

Sorry for dropping this for so long.

>+      if ((hint & nsChangeHint_RecomputePosition) && !didReflow) {

Actually, now that I look at this again, this is broken, as is the
existing code that does this for UpdateOverflow.  If didReflow had a
variant local to an iteration of the while loop, it would be ok.  Or we
could skip it.

(We should add a testcase for the brokenness with UpdateOverflow.  It
involves doing a reflow in one part of the tree and updating transform
in another.)


>+        NS_ASSERTION(nsStyle##struct_::ForceCompare() ||                      \
>+             (int)NS_SubtractHint(nsStyle##struct_::MaxDifference(),          \
>+                                  nsChangeHint_NonInherited_Hints) ==         \
>+             (int)nsStyle##struct_::MaxDifference(),                          \
>+             "nsChangeHint_NonInherited_Hints set but ForceCompare() returns false"); \

Why the (int) casts?  They both return nsChangeHint.

Also, how about making the assertion text:
  Structs that can return non-inherited hints must return true from
  ForceCompare
and wrapping it across two lines (just two strings adjacent to each
other, but with a space at the trailing end of the first)


And why is the test using mozRequestAnimationFrame rather than
a MozReftestInvalidate event?


The remainder of the review is about
nsCSSFrameConstructor::RecomputePosition:

Something should have a comment that the return value of
RecomputePosition returns whether it avoided the need to reflow.

After you call ComputeRelativeOffsets you should assert that its
result has left == -right and top == -bottom.

>+  // For absolute positioning, the width can potentially change if width is auto and
>+  // either of left or right are not.  The height can also potentially change if height
>+  // is auto and either of top or bottom are not.  In these cases we fall back to a
>+  // reflow, and in all other cases, we attempt to move the frame here.

You should make it clearer that "these cases" means "where the
height/width might change" but that we intend to change it to
"where it does change".

Also, please wrap at less than 80 characters.

(There are also a few other 80th column violations that you should fix.)


>+  if (!((position->mWidth.GetUnit() == eStyleUnit_Auto &&
>+         (position->mOffset.GetLeftUnit() != eStyleUnit_Auto ||
>+          position->mOffset.GetRightUnit() != eStyleUnit_Auto)) ||
>+        (position->mHeight.GetUnit() == eStyleUnit_Auto &&
>+         (position->mOffset.GetTopUnit() != eStyleUnit_Auto ||
>+          position->mOffset.GetBottomUnit() != eStyleUnit_Auto)))) {

I think the tests of mOffset here should all go; you should just be checking
mWidth and mHeight.  Without that, I think you'll have the bug that a change from:
  width: auto;
  left: 300px;
  right: auto;
to:
  width: auto;
  left: auto;
  right: auto;
will fail to cause the necessary resizing (since both the before and after
cases are shrink-to-fit, but with 300px of difference in the container
width).  Please fix and add a test for this case.


+    if (parentSize.height != NS_INTRINSICSIZE)
+      parentSize.height += margin.TopBottom();
+    if (parentSize.width != NS_INTRINSICSIZE)
+      parentSize.width += margin.LeftRight();

I think you should instead assert that parentSize.width and
parentSize.height are not NS_INTRINSICSIZE.  Or did you actually
hit this case?

Except, also, you want to just set parentSize.height to NS_AUTOHEIGHT
(which is the same as NS_INTRINSICSIZE).  Setting a constrained height
in the nsSize passed to a reflow state indicates it's for pagination.

(You should be able to construct a test that breaks, I suspect, by
positioning an abs pos element so it overflows the bottom of its parent,
and then triggering this code.)


>+    nsHTMLReflowState parentReflowState(aFrame->PresContext(), parentFrame,
>+                                        rc,
>+                                        parentSize);

Merge the second and third lines.


>+    if (parentSize.width != NS_INTRINSICSIZE)
>+      parentReflowState.SetComputedWidth(NS_MAX(parentSize.width, 0));
>+    if (parentSize.height != NS_INTRINSICSIZE)
>+      parentReflowState.SetComputedHeight(NS_MAX(parentSize.height, 0));

These should be content-box sizes (and should use the real height,
unlike the reflow state constructor).

>+    parentReflowState.mComputedMargin.SizeTo(0, 0, 0, 0);

This is reasonable (and simplifies things), but it means that you
shouldn't have bothered inflating parentSize by the margin earlier on.
(It's a pattern we use in nsFrame::BoxReflow, but the point of it is
that we don't need to worry about anything outside the parent's
border-box, so we pretend the parent's margins are 0.)

>+    parentFrame->GetPadding(parentReflowState.mComputedPadding);
>+    parentFrame->GetBorder(parentReflowState.mComputedBorderPadding);
>+    parentReflowState.mComputedBorderPadding +=
>+      parentReflowState.mComputedPadding;

Those are XUL box layout methods.  Instead, use nsIFrame::GetUsedPadding
and nsIFrame::GetUsedBorderAndPadding.

(Same for GetUsedMargin earlier, except it needs removing.)

>+    nsHTMLReflowState reflowState(aFrame->PresContext(), parentReflowState, aFrame,
>+                                  availSize, -1, -1, false);

I see no reason to use the delayed-init form (false for last argument).
Just move this down to your Init call, wrap at less than 80 columns, and
pass cbSize.width and cbSize.height  instead of -1, -1.

>+    // mComputedWidth and mComputedHeight are content-box, not
>+    // border-box
>+    if (parentSize.width != NS_INTRINSICSIZE) {
>+      nscoord computedWidth =
>+        parentSize.width - reflowState.mComputedBorderPadding.LeftRight();
>+      computedWidth = NS_MAX(computedWidth, 0);
>+      reflowState.SetComputedWidth(computedWidth);
>+    }

Again, parentSize.width should never be NS_INTRINSICSIZE.

Also, why is this needed?  Shouldn't the reflow state constructor have
set it correctly?  And, also, I don't see how
reflowState.ComputedWidth() is used later at any point, so I think it's
pretty clearly unused.


>+    // If we're solving for 'left' or 'top', then compute it now that we know the
>+    // width/height.

This comment is a little misleading.  We always knew it.  It's just that
in the normal reflow codepath we don't, and thus in the normal reflow
codepath it's done after reflow, so we have to do it ourselves here.

Also wrap the comment at less than 80 columns.

>+    nsPoint pos(border.left + reflowState.mComputedOffsets.left + reflowState.mComputedMargin.left,
>+                border.top +reflowState.mComputedOffsets.top + reflowState.mComputedMargin.top);

Fix the weird spacing after the first + on the second line.

(Also, maybe |border| would be better called |parentBorder|?)

r=dbaron with those things fixed


Notes about tests:

Need tests with margin, padding, border (varying values, e.g., "1px 3px 9px 27px" tested separately for each of the 6) on the abs-pos element and on its parent.

Need tests with absolutely positioned root element.

        Attachment #611632 -
        Flags: review?(dbaron) → review+

    
    

    
  
Comment on attachment 612095 [details] [diff] [review]
Part 1: Add a test suite

Why requestAnimationFrame rather than MozReftestInvalidate?

And I think you need to add some tests per the last 2 paragraphs of my previous comment.

But r=dbaron on these (though I can't claim anything close to reading the whole thing in detail)

(Have you double-checked (e.g., with printfs) that these tests are hitting your new codepath.)

        Attachment #612095 -
        Flags: review?(dbaron) → review+

    
    

    
  
(In reply to David Baron [:dbaron] from comment #82)
> Need tests with margin, padding, border (varying values, e.g., "1px 3px 9px
> 27px" tested separately for each of the 6) on the abs-pos element and on its
> parent.

Let me doubly-emphasize this part.  I have low confidence in my ability to catch all mistakes relating to margin, border, and padding during review.  You need tests for it.

> Need tests with absolutely positioned root element.

These don't necessarily need to be as thorough as the previous point, but you should have a few that actually trigger your new code (and, in particular, involving margin, border, and padding).  It's possible some of these could be written as mochitests rather than reftests.

    
  
Blocks: 745485

    
    

    
  
(In reply to David Baron [:dbaron] from comment #82)
> Comment on attachment 611632 [details] [diff] [review]
> Part 2: Implementation
> 
> Sorry for dropping this for so long.
> 
> >+      if ((hint & nsChangeHint_RecomputePosition) && !didReflow) {
> 
> Actually, now that I look at this again, this is broken, as is the
> existing code that does this for UpdateOverflow.  If didReflow had a
> variant local to an iteration of the while loop, it would be ok.  Or we
> could skip it.
> 
> (We should add a testcase for the brokenness with UpdateOverflow.  It
> involves doing a reflow in one part of the tree and updating transform
> in another.)

I couldn't come up with a test case which would trigger this.  Here's one of the things I tried for example: https://gist.github.com/2387812.  The style changes would always end up being processed in two separate change lists.

didReflow is also used outside of the loop, so moving it is not trivial.  Can we do this in another bug?

> >+        NS_ASSERTION(nsStyle##struct_::ForceCompare() ||                      \
> >+             (int)NS_SubtractHint(nsStyle##struct_::MaxDifference(),          \
> >+                                  nsChangeHint_NonInherited_Hints) ==         \
> >+             (int)nsStyle##struct_::MaxDifference(),                          \
> >+             "nsChangeHint_NonInherited_Hints set but ForceCompare() returns false"); \
> 
> Why the (int) casts?  They both return nsChangeHint.

operator== for nsChangeHint is declared to return void, which makes me think that is done on purpose to make sure people who compare nsChangeHints know what they're doing.  :-)

> >+  if (!((position->mWidth.GetUnit() == eStyleUnit_Auto &&
> >+         (position->mOffset.GetLeftUnit() != eStyleUnit_Auto ||
> >+          position->mOffset.GetRightUnit() != eStyleUnit_Auto)) ||
> >+        (position->mHeight.GetUnit() == eStyleUnit_Auto &&
> >+         (position->mOffset.GetTopUnit() != eStyleUnit_Auto ||
> >+          position->mOffset.GetBottomUnit() != eStyleUnit_Auto)))) {
> 
> I think the tests of mOffset here should all go; you should just be checking
> mWidth and mHeight.  Without that, I think you'll have the bug that a change
> from:
>   width: auto;
>   left: 300px;
>   right: auto;
> to:
>   width: auto;
>   left: auto;
>   right: auto;
> will fail to cause the necessary resizing (since both the before and after
> cases are shrink-to-fit, but with 300px of difference in the container
> width).  Please fix and add a test for this case.

Right, good catch.  I fixed this and added a test.

> +    if (parentSize.height != NS_INTRINSICSIZE)
> +      parentSize.height += margin.TopBottom();
> +    if (parentSize.width != NS_INTRINSICSIZE)
> +      parentSize.width += margin.LeftRight();
> 
> I think you should instead assert that parentSize.width and
> parentSize.height are not NS_INTRINSICSIZE.  Or did you actually
> hit this case?

No, I just took the code from here: <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#7697>

> Except, also, you want to just set parentSize.height to NS_AUTOHEIGHT
> (which is the same as NS_INTRINSICSIZE).  Setting a constrained height
> in the nsSize passed to a reflow state indicates it's for pagination.
> 
> (You should be able to construct a test that breaks, I suspect, by
> positioning an abs pos element so it overflows the bottom of its parent,
> and then triggering this code.)

Hmm, I'm not sure what the breakage should look like.  I tried constructing such a test case https://gist.github.com/2387996 but couldn't see any failures.

> >+    if (parentSize.width != NS_INTRINSICSIZE)
> >+      parentReflowState.SetComputedWidth(NS_MAX(parentSize.width, 0));
> >+    if (parentSize.height != NS_INTRINSICSIZE)
> >+      parentReflowState.SetComputedHeight(NS_MAX(parentSize.height, 0));
> 
> These should be content-box sizes (and should use the real height,
> unlike the reflow state constructor).

I took this code from <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#7712>.  The border and padding on the parent RS is not computed, so I'm not sure how to set the content-box size on it.

> Need tests with margin, padding, border (varying values, e.g., "1px 3px 9px
> 27px" tested separately for each of the 6) on the abs-pos element and on its
> parent.

Not sure what you mean by "each of the 6".  I'm assuming you meant "each of the 3" (of course there's going to be 6 cases in total, 3 on the abs-pos element and 3 on its parent.

Will submit the new patches soon.

    
    

    
  
(In reply to David Baron [:dbaron] from comment #83)
> Comment on attachment 612095 [details] [diff] [review]
> Part 1: Add a test suite
> 
> Why requestAnimationFrame rather than MozReftestInvalidate?

MozReftestInvalidate only fires once for these tests, so it's not useful.  Also, I wanted the tests to be useful outside of the reftest framework as well.

> (Have you double-checked (e.g., with printfs) that these tests are hitting
> your new codepath.)

Yeah, I've stepped through the code under the debugger to make sure that these tests do test the new code path.

    
    

    
  
(In reply to Ehsan Akhgari [:ehsan] from comment #85)
> (In reply to David Baron [:dbaron] from comment #82)
> > Comment on attachment 611632 [details] [diff] [review]
> > Part 2: Implementation
> > 
> > Sorry for dropping this for so long.
> > 
> > >+      if ((hint & nsChangeHint_RecomputePosition) && !didReflow) {
> > 
> > Actually, now that I look at this again, this is broken, as is the
> > existing code that does this for UpdateOverflow.  If didReflow had a
> > variant local to an iteration of the while loop, it would be ok.  Or we
> > could skip it.
> > 
> > (We should add a testcase for the brokenness with UpdateOverflow.  It
> > involves doing a reflow in one part of the tree and updating transform
> > in another.)
> 
> I couldn't come up with a test case which would trigger this.  Here's one of
> the things I tried for example: https://gist.github.com/2387812.  The style
> changes would always end up being processed in two separate change lists.

Hmmm.  It does have to be something where both changes end up in the same call to ProcessRestyledFrames.  It's not clear to me why that case wouldn't be.  (And, also, the reflow would have to happen first.)

> didReflow is also used outside of the loop, so moving it is not trivial. 
> Can we do this in another bug?

It's not moving it that's needed -- the fix is pretty trivial:  have a |didReflowThisFrame| inside the loop and a |didReflow| outside the loop.

> > Why the (int) casts?  They both return nsChangeHint.
> 
> operator== for nsChangeHint is declared to return void, which makes me think
> that is done on purpose to make sure people who compare nsChangeHints know
> what they're doing.  :-)

ok.  (You could also write it using NS_IsHintSubset and ~, i.e., saying that MaxDifference() is a subset of ~NonInherited_Hints.)

> > Except, also, you want to just set parentSize.height to NS_AUTOHEIGHT
> > (which is the same as NS_INTRINSICSIZE).  Setting a constrained height
> > in the nsSize passed to a reflow state indicates it's for pagination.
> > 
> > (You should be able to construct a test that breaks, I suspect, by
> > positioning an abs pos element so it overflows the bottom of its parent,
> > and then triggering this code.)
> 
> Hmm, I'm not sure what the breakage should look like.  I tried constructing
> such a test case https://gist.github.com/2387996 but couldn't see any
> failures.

Yeah, it may well be asymptomatic because you manually construct the child's reflow state correctly.

> > >+    if (parentSize.width != NS_INTRINSICSIZE)
> > >+      parentReflowState.SetComputedWidth(NS_MAX(parentSize.width, 0));
> > >+    if (parentSize.height != NS_INTRINSICSIZE)
> > >+      parentReflowState.SetComputedHeight(NS_MAX(parentSize.height, 0));
> > 
> > These should be content-box sizes (and should use the real height,
> > unlike the reflow state constructor).
> 
> I took this code from
> <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.
> cpp#7712>.  The border and padding on the parent RS is not computed, so I'm
> not sure how to set the content-box size on it.

Well, it still seems wrong, but maybe it's right, or maybe it just doesn't matter that it's wrong.

> > Need tests with margin, padding, border (varying values, e.g., "1px 3px 9px
> > 27px" tested separately for each of the 6) on the abs-pos element and on its
> > parent.
> 
> Not sure what you mean by "each of the 6".  I'm assuming you meant "each of
> the 3" (of course there's going to be 6 cases in total, 3 on the abs-pos
> element and 3 on its parent.

Exactly.

    
    

    
  
(In reply to David Baron [:dbaron] from comment #87)
> (In reply to Ehsan Akhgari [:ehsan] from comment #85)
> > (In reply to David Baron [:dbaron] from comment #82)
> > > Comment on attachment 611632 [details] [diff] [review]
> > > Part 2: Implementation
> > > 
> > > Sorry for dropping this for so long.
> > > 
> > > >+      if ((hint & nsChangeHint_RecomputePosition) && !didReflow) {
> > > 
> > > Actually, now that I look at this again, this is broken, as is the
> > > existing code that does this for UpdateOverflow.  If didReflow had a
> > > variant local to an iteration of the while loop, it would be ok.  Or we
> > > could skip it.
> > > 
> > > (We should add a testcase for the brokenness with UpdateOverflow.  It
> > > involves doing a reflow in one part of the tree and updating transform
> > > in another.)
> > 
> > I couldn't come up with a test case which would trigger this.  Here's one of
> > the things I tried for example: https://gist.github.com/2387812.  The style
> > changes would always end up being processed in two separate change lists.
> 
> Hmmm.  It does have to be something where both changes end up in the same
> call to ProcessRestyledFrames.  It's not clear to me why that case wouldn't
> be.  (And, also, the reflow would have to happen first.)
> 
> > didReflow is also used outside of the loop, so moving it is not trivial. 
> > Can we do this in another bug?
> 
> It's not moving it that's needed -- the fix is pretty trivial:  have a
> |didReflowThisFrame| inside the loop and a |didReflow| outside the loop.

Hmm, OK, filed bug 745493 to add a test case once we figure out how to trigger the broken scenario here, but I will fix this in my patch anyway.

> > > Why the (int) casts?  They both return nsChangeHint.
> > 
> > operator== for nsChangeHint is declared to return void, which makes me think
> > that is done on purpose to make sure people who compare nsChangeHints know
> > what they're doing.  :-)
> 
> ok.  (You could also write it using NS_IsHintSubset and ~, i.e., saying that
> MaxDifference() is a subset of ~NonInherited_Hints.)

OK, that is cleaner I think, I'll do that.

> > > >+    if (parentSize.width != NS_INTRINSICSIZE)
> > > >+      parentReflowState.SetComputedWidth(NS_MAX(parentSize.width, 0));
> > > >+    if (parentSize.height != NS_INTRINSICSIZE)
> > > >+      parentReflowState.SetComputedHeight(NS_MAX(parentSize.height, 0));
> > > 
> > > These should be content-box sizes (and should use the real height,
> > > unlike the reflow state constructor).
> > 
> > I took this code from
> > <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.
> > cpp#7712>.  The border and padding on the parent RS is not computed, so I'm
> > not sure how to set the content-box size on it.
> 
> Well, it still seems wrong, but maybe it's right, or maybe it just doesn't
> matter that it's wrong.

I added tests for the border/padding/margin cases, and given that all of them pass, I think one of the two latter cases are true.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 1: Add a test suite (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #612095 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 2: Implementation (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #611632 -
        Attachment is obsolete: true

    
    

    
  
I submitted this to the try server: http://tbpl.mozilla.org/?tree=Try&rev=01e4ec2e01c3

David, please let me know if you want to look at the new patches again, or if you're OK with me landing them.

    
    

    
  
So, there's a test failure with this patch on OS X, which I debugged a bit: https://tbpl.mozilla.org/php/getParsedLog.php?id=10914771&tree=Try

The bug is caused because the overflow event is not fired when it should be <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#3034>.  As far as I know, this is the result of the overflow areas not be updated properly.

I always set the RecomputePosition hint together with the UpdateOverflow hint, which _should_ cause the overflow area on the frame's parent to be updated correctly, right?  Do we also need to update the overflow area on other descendants?  Or do we need to do something differently here?

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #76)
> Add padding to the tests and then file a bug about the overflow area issue
> and assign it to Jonathan :-).

Also, adding the padding does not seem to help with this problem.  Moving the inline text frame still leaves a trail on the edge of the moved area.

I'm not sure how to proceed here.

    
    

    
  
Try run for 01e4ec2e01c3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=01e4ec2e01c3
Results (out of 223 total builds):
    success: 168
    warnings: 55
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-01e4ec2e01c3

    
    

    
  
(In reply to Ehsan Akhgari [:ehsan] from comment #92)
> So, there's a test failure with this patch on OS X, which I debugged a bit:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=10914771&tree=Try
> 
> The bug is caused because the overflow event is not fired when it should be
> <http://mxr.mozilla.org/mozilla-central/source/layout/generic/
> nsGfxScrollFrame.cpp#3034>.  As far as I know, this is the result of the
> overflow areas not be updated properly.
> 
> I always set the RecomputePosition hint together with the UpdateOverflow
> hint, which _should_ cause the overflow area on the frame's parent to be
> updated correctly, right?  Do we also need to update the overflow area on
> other descendants?  Or do we need to do something differently here?

Or it could be the general problem that we don't seem to fire the overflow/underflow events as a result of the UpdateOverflow hint as far as I can tell...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 1: Add a test suite (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
The previous test patch had a mistake in the relative tests which is fixed in this patch.

        Attachment #615094 -
        Attachment is obsolete: true

    
    

    
  
Pushed to try on a more recent m-c base revision: https://tbpl.mozilla.org/?tree=Try&rev=7476088374d6

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 1: Add a test suite (deleted)
        — 
          — Splinter Review
      

    


  
Rebased version of the patch.

        Attachment #616263 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 2: Implementation (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Rebased version of the patch.

        Attachment #615095 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 2: Implementation (deleted)
        — 
          — Splinter Review
      

    


  
Oops, wrong patch.

        Attachment #619651 -
        Attachment is obsolete: true

    
    

    
  
Try run for 7476088374d6 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7476088374d6
Results (out of 238 total builds):
    success: 194
    warnings: 37
    failure: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-7476088374d6

    
    

    
  


  
FWIW, here's a screenshot showing the failure that's happening in browser_tabview_bug696602.js. The testcase creates a series of about 9 new tabs in the window, and expects that the tab-strip will overflow; but as can be seen in the screenshot, with this patch applied, the new tabs seem to all be getting "stacked" behind the right-hand edge of the original tab.

    
    

    
  


  
Adjusting nsStylePosition::CalcDifference so that "auto" height or widths aren't treated as being the same seems to prevent the tab-strip test failure, although I don't really understand the underlying reason it was failing.

Note that this patch triggers some additional nsCoord-overflow assertions in layout/generic/crashtests/265867-1.html (it's probably making us re-calculate stuff that we shouldn't need to), so I'm not sure it's necessarily a good idea, but it may give a clue where to track down the problem.

    
  

        Attachment #620820 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 620820 [details] [diff] [review]
patch, don't consider height/width the same if they're "auto"

>   if (mHeight != aOther.mHeight ||
>       mMinHeight != aOther.mMinHeight ||
>-      mMaxHeight != aOther.mMaxHeight) {
>+      mMaxHeight != aOther.mMaxHeight ||
>+      mHeight.GetUnit() == eStyleUnit_Auto) {

This causes us to reflow for *any* style change, as long as 'height' is auto (and we've previously looked at the position struct).  That's not acceptable.  (And also probably why it's capable of covering up all sorts of failures.)

I also think it's better to keep the when-to-fallback-to-reflow logic concentrated in nsCSSFrameConstructor::RecomputePosition, where the logic already lives.

        Attachment #620820 -
        Flags: review?(dbaron) → review-

    
    

    
  
So I looked into this a bit more.  If we skip reflow and just update the overflow areas, the overflow logic in the scroll frame code does not get triggered at all, and I'm pretty sure that's a bug.  But I don't know what the best way to fix that is.

    
    

    
  
(In reply to Ehsan Akhgari [:ehsan] from comment #106)
> So I looked into this a bit more.  If we skip reflow and just update the
> overflow areas, the overflow logic in the scroll frame code does not get
> triggered at all, and I'm pretty sure that's a bug.  But I don't know what
> the best way to fix that is.

Hmm, actually perhaps that's not the case.  It seems like my patch has a bug which causes us to skip reflowing if {max,min}-{width,height} also changes.  Fixing that seems to fix the test failures.  I'll attach the patch once I do a bit more local testing.

    
    

    
  


  
OK, this patch fixes the remaining test failures.  I also added stand-alone unit tests for this.  Basically we need to make sure that we still reflow when max-width and min-width changes, same as we did before this patch.

I'm submitting this separately to make the review easier, but will squash this into the implementation patch upon landing.

        Attachment #620820 -
        Attachment is obsolete: true

        Attachment #630241 -
        Flags: review?(dbaron)

    
    

    
  
(In reply to Ehsan Akhgari [:ehsan] from comment #109)
> http://tbpl.mozilla.org/?tree=Try&rev=13382c3864f4

Pushed the wrong patch; http://tbpl.mozilla.org/?tree=Try&rev=e3ab610c8333

    
  
Blocks: gecko-games

    
    

    
  
Comment on attachment 630241 [details] [diff] [review]
Make sure to reflow when {min,max}-width changes

r=dbaron, though I think it would be cleaner to restructure a little, by putting the *final* return earlier.  Essentially to restructure all the code that comes after the (untouched):

  if (mHeight != aOther.mHeight ||
      mMinHeight != aOther.mMinHeight ||
      mMaxHeight != aOther.mMaxHeight) {
    // Height changes can affect descendant intrinsic sizes due to replaced
    // elements with percentage heights in descendants which also have
    // percentage heights.  And due to our not-so-great computation of mVResize
    // in nsHTMLReflowState, they do need to force reflow of the whole subtree.
    // XXXbz due to XUL caching heights as well, height changes also need to
    // clear ancestor intrinsics!
    return NS_CombineHint(hint, nsChangeHint_ReflowFrame);
  }

by starting what goes after that with a parallel clause:

  if (mWidth != aOther.mWidth ||
      mMinWidth != aOther.mMinWidth ||
      mMaxWidth != aOther.mMaxWidth) {
     // put what's now the final return here
  }

and then continue with the check of offset (which I think can be condensed back into mOffset == aOther.mOffset -- I don't see why the patch this is on top of separates it out into the four pieces), i.e., the two things that are inside the check that you're modifying here.

I think that's equivalent, but it ends up a good bit easier to understand.

        Attachment #630241 -
        Flags: review?(dbaron) → review+

    
    

    
  
(And it avoids having to duplicate the checks of the {min-,max,}{height,width} properties.)

    
    

    
  


  
Right, this makes the code a lot easier to read!

    
    

    
  
Now, on the new test that I added, there is an OS X failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=12391287&tree=Try#error0.  I'll file a new bug for this and will mark this test as fuzzy on Mac.

    
    

    
  
(In reply to Ehsan Akhgari [:ehsan] from comment #114)
> Now, on the new test that I added, there is an OS X failure:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=12391287&tree=Try#error0. 
> I'll file a new bug for this and will mark this test as fuzzy on Mac.

-> bug 761770

    
    

    
  
Comment on attachment 630296 [details] [diff] [review]
Simplify nsStylePosition::CalcDifference

>+  if (mOffset.GetLeft() != aOther.mOffset.GetLeft() ||
>+      mOffset.GetRight() != aOther.mOffset.GetRight() ||
>+      mOffset.GetTop() != aOther.mOffset.GetTop() ||
>+      mOffset.GetBottom() != aOther.mOffset.GetBottom()) {

But also please switch this back to just mOffset != aOther.mOffset (like it was before the first patch in this bug), unless I'm missing a reason not to do this.

    
    

    
  
(In reply to David Baron [:dbaron] from comment #116)
> Comment on attachment 630296 [details] [diff] [review]
> Simplify nsStylePosition::CalcDifference
> 
> >+  if (mOffset.GetLeft() != aOther.mOffset.GetLeft() ||
> >+      mOffset.GetRight() != aOther.mOffset.GetRight() ||
> >+      mOffset.GetTop() != aOther.mOffset.GetTop() ||
> >+      mOffset.GetBottom() != aOther.mOffset.GetBottom()) {
> 
> But also please switch this back to just mOffset != aOther.mOffset (like it
> was before the first patch in this bug), unless I'm missing a reason not to
> do this.

Will do.

    
    

    
  
Thanks for getting this in.

Please do get a followup bug filed on comment 68 -- I'm concerned that if we have the current behavior for any substantial period of time, authors will start sharing performance advice to prefer the cases that this happens to optimize, which is contradictory to good cross-browser and cross-device design practice.

    
  
Depends on: 761980

    
    

    
  
(In reply to David Baron [:dbaron] from comment #119)
> Thanks for getting this in.
> 
> Please do get a followup bug filed on comment 68 -- I'm concerned that if we
> have the current behavior for any substantial period of time, authors will
> start sharing performance advice to prefer the cases that this happens to
> optimize, which is contradictory to good cross-browser and cross-device
> design practice.

Already did: bug 745485.

    
  
Whiteboard: [adt2] → [adt2][games:p3]

    
  
Depends on: 762181

    
  
Depends on: 762180

    
  
Depends on: 762546

    
  
Depends on: 763223

    
  
Depends on: 765597
Blocks: 729597

    
  
Depends on: 766116

    
  
Depends on: 766843
Blocks: 703873

    
  
Depends on: 775350
Depends on: 782062

    
  

          status-firefox17:
          --- → disabled

    
  

          status-firefox18:
          --- → disabled

    
  
Depends on: 824952

    
  
Depends on: 844178
Depends on: 845837
Depends on: 872254

    
  
Depends on: 884468

    
  
Depends on: 894629

    
  
Depends on: 909667

    
  
Depends on: 1066917
Depends on: 1200585

    
  
Depends on: 1248727

    
  
Regressions: 1564035

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: