113083 - View manager should invalidate on clip changes

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

View manager should invalidate when clip regions change.  If this is done, the
fix for bug 88653 can probably be reverted to not reflow on dynamic clip changes.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right now we have a serious problem, which is that whenever there is a style
change affecting values which need to be reflected into views (e.g., opacity,
clipping, z-index, etc), we MUST reflow because
nsContainerFrame::SyncFrameViewAfterReflow is the only place where these styles
get propagated into views. Even worse, sometimes the style change hint actually
has to be REFRAME because the style change might require a view to be created
for a frame which didn't already have one. That sucks.

What we need here is a new style change hint that says "don't reflow, just sync
the frame's view --- or reframe if it doesn't already have a view"

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We don't need a new style change hint to make progress here; "clip" (currently)
only applies to blocks with "overflow: hidden", which always have views. We just
have to modify nsCSSFrameConstructor to propagate clip changes to views on
VISUAL style changes, set the style change hint for "clip" to VISUAL, and fix
any remaining bugs :-).

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm going to use this bug as a place to reorganize the way views are synced with
frames (in response to the new nsChangeHint_SyncFrameView).

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Basically what I plan to to do is provide 2 new methods for interacting with views:

nsFrame::SyncViewPropertiesWithFrame
Map the style properties of a frame into its view (excluding geometrical
properties such as size and position).
This subsumes most of nsContainerFrame::SyncFrameViewAfterReflow,
nsCSSFrameConstructor::SyncAndInvalidateView, and various other duplications of
that code.

nsFrame::NeedsView
Determine whether a frame needs a view or not, based on whether there are any
properties that need to be reflected into the view by
SyncViewPropertiesWithFrame. To be called by
nsHTMLContainerFrame::CreateViewForFrame and similar.

I'm also planning to split SyncAndInvalidateView and friends into "Sync" and
"Invalidate" components, and call them in response to the SyncFrameView and
RepaintFrame change hints.

None of that stuff should change behaviour.

Then I will fix this bug by whacking the change hint for clip to be just
SyncFrameView.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Testcase

This testcase shows the benefits of the fix I am about to propose. With my new
code, you can turn paint flashing on and see that as the clip area shrinks, the
only area that is repainted is the difference between the old and new clip
areas. CPU utilization on my system is nearly zero while this animation is
running.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Promised patch

Here it is! It does pretty much what I said it would. I admit that it looks
scary. Probably 1.3 alpha stuff. Still, would be nice to get it reviewed so it
can land ASAP in 1.3 alpha. I have tested it and it even works with
dhtmlcentral.com :-).

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Most of the patch is refactoring view synchronization and "does this frame need
a view" code into a few common functions in nsContainerFrame, plus modifying
various frame codes to use these common functions. In particular
ApplyRenderingChangesToTree and friends in nsCSSFrameConstructor gets whacked to
only update the view properties if that's all the hint asks for.

Part of the patch is to fix the view manager to work correctly when view
properties are set on views that aren't yet part of the view tree. Another part
is to change nsCSSPropList so that you can specify fine-grained hints in the
list. The rest is to modify the change hints for 'clip' to only require
nsChangeHint_SyncFrameView.

Comment 8

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

Planning to land in 1.3alpha (rather than 1.2beta) sounds like taking the words
"alpha" and "beta" in our new milestone names too seriously (if that's even what
they mean -- doesn't "alpha" normally imply "in the middle of feature work"?).

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, it would be great if we can land this in 1.2. The sooner we get this in, the
sooner we can start working on optimizing the change hints for other CSS properties.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Updated trunk patch

This brings the patch up to date. It also addresses one of kin's comments,
which was that the IsContainerContent optimization which eliminated the need
for a view if the frame's content could not contain anything was potentially
bogus. In fact this optimization is pretty useless because it just means we
eliminate the need for a view on overflow:hidden elements with no children,
which isn't all that common. So I'm just removing the optimization entirely.

Comment 11

[kinmoz]

Comment on attachment 100510 [details] [diff] [review]
Updated trunk patch

These changes look fine to me. sr=[e-mail]

==== There were a couple of places in the patch that used if-else-if constructs
to either return the same value or set some flag to the same value, but I'm
going to assume that you split them out for readability/debuggability. For
example:


+  if (NS_STYLE_POSITION_RELATIVE == display->mPosition) {
+    return PR_TRUE;
+  } else if (display->IsAbsolutelyPositioned()) {
+    return PR_TRUE;
+  } 


==== By the way, I did notice that some of the pre-existing code you were
moving around retrieved pointers (parents, views, viewmanagers) and just used
assertions to check for null, but dereferenced them anyways ... yuck! It turns
out that code has been like that for a long while so I won't worry about it too
much.

Thanks for factoring out some of this stuff!

Comment 12

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

Comment on attachment 100510 [details] [diff] [review]
Updated trunk patch

>+  if (mBinding != aOther.mBinding
>+      || mPosition != aOther.mPosition
>+      || mDisplay != aOther.mDisplay
>+      || mFloats != aOther.mFloats
>+      || mOverflow != aOther.mOverflow)
>+    NS_UpdateHint(hint, nsChangeHint_ReconstructFrame);
>+

Do we need the following hints if it's a frame reconstruct, or can we just
return?  (Does it matter?  Maybe it's better to be consistent since the
performance effects should be negligible since the hint handling should know
that too.  At least I hope the hint handling knows that.  Or would that mess up
dymanic background changes on the root element?)

>+  // the following is conservative, for now: changing float breaking shouldn't
>+  // necessarily require a repaint, reflow should suffice.
>+  if (mBreakType != aOther.mBreakType
>+      || mBreakBefore != aOther.mBreakBefore
>+      || mBreakAfter != aOther.mBreakAfter
>+      || mAppearance != aOther.mAppearance)
>+    NS_UpdateHint(hint, NS_CombineHint(nsChangeHint_ReflowFrame, nsChangeHint_RepaintFrame));

This deserves an XXX comment.

>Index: layout/doc/adding-style-props.html
>===================================================================
>RCS file: /cvsroot/mozilla/layout/doc/adding-style-props.html,v
>retrieving revision 1.3
>diff -u -r1.3 adding-style-props.html
>--- layout/doc/adding-style-props.html	19 Jun 2002 04:31:43 -0000	1.3
>+++ layout/doc/adding-style-props.html	25 Sep 2002 04:12:22 -0000
>@@ -91,15 +91,14 @@
>   Insert the property in the list alphabetically, using the existing
> property names as a template. The format of the entry you will create is:
> </p>
>-  <pre>CSS_PROP(-moz-force-broken-image-iconst, force_broken_image_icons, FRAMECHANGE) // bug 58646</pre>
>+  <pre>CSS_PROP(-moz-force-broken-image-iconst, force_broken_image_icons, NS_STYLE_HINT_FRAMECHANGE) // bug 58646</pre>
> 
> <p>The first value is the formal property name, in other words the property
> name as it is seen by the CSS parser.<br>
> The second value is the name of the property as it will appear internally.<br>
> The last value indicates what must change when the value of the property
>-changes. It should be one of (<a href="http://lxr.mozilla.org/seamonkey/source/layout/base/public/nsStyleConsts.h#46">
>-VISUAL, AURAL, REFLOW, CONTENT, FRAMECHANGE</a>
>-).</p>
>+changes. It should an
>+<a href="http://lxr.mozilla.org/seamonkey/source/content/shared/public/nsChangeHint.h">nsChangeHint</a>.</p>

This sentence no verb. ;-)

>Index: layout/html/base/src/nsContainerFrame.cpp

I need to find a better way to look at this part of the diffs.	More from
nsContainerFrame.cpp to the end in a bit.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Do we need the following hints if it's a frame reconstruct, or can we just
> return?

We don't. I don't think it matters performance-wise. It's true that everywhere
we do a frame reconstruction it subsumes reflow and repaint. I'd like to leave
it as is because it makes everything clearer (and a lot clearer than the old
code, I claim).

> This deserves an XXX comment.

OK. At some point we need to audit the hints in CalcDifference because most of
the hints that currently say "NS_STYLE_HINT_REFLOW" don't necessarily need a
full repaint as well ... but changing them requires testing because I'm quite
sure that reflow doesn't repaint everything it should.

> This sentence no verb.

Oops, will fix.

Comment 14

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

Comment on attachment 100510 [details] [diff] [review]
Updated trunk patch

>Index: layout/html/base/src/nsContainerFrame.cpp

Nits this round (mostly on code you moved, but it's ugly).  No need to clean up
all the GetStyleData typesafety if you don't want to.  It's a little hard to
understand these changes -- the comments in nsContainerFrame.h are rather
sparse -- perhaps you could add in at least what was in the bugzilla comments
when you described what you were going to do (updated to what you actually did
:-).  We really do need comments.  Otherwise somebody will come along and just
call the wrong (doing too much work, most likely, or perhaps too little if the
testcase is limited) function because it happens to fix a bug in a certain
testcase.

(Maybe this nitpicking will teach you to attach a |diff -uw| next time you're
reindenting a lot of code so I at least don't blame you for things you're just
reindenting. :-)

>+  if (hasBG &&
>+      NS_STYLE_BG_ATTACHMENT_FIXED == bg->mBackgroundAttachment) {
>+    // If the frame has a fixed background attachment, then indicate that the
>+    // view's contents should be repainted and not bitblt'd
>+    vm->SetViewBitBltEnabled(aView, PR_FALSE);
>+  } else {
>+    vm->SetViewBitBltEnabled(aView, PR_TRUE);
>+  }

I'll be evil and suggest:

  vm->SetViewBitBltEnabled(aView,
    !hasBG || NS_STYLE_BG_ATTACHMENT_FIXED != bg->mBackgroundAttachment);

(Or is my sense of C++ style being skewed by writing LISP?  Probably a
temporary boolean variable is better, but I still don't like the if-else.)

>+  PRBool  viewHasTransparentContent =
>+    (!hasBG ||
>+     (bg->mBackgroundFlags & NS_STYLE_BG_COLOR_TRANSPARENT) ||
>+     !aFrame->CanPaintBackground());

Why over-parenthesize?	It's a bad habit since it hides useful gcc warnings
about using = when you meant ==.

>+  const nsStyleDisplay* display = (const nsStyleDisplay*)
>+    aStyleContext->GetStyleData(eStyleStruct_Display);

I prefer the typesafe form (makes it much less likely to accidentally cast away
const, and ensures you're not using the wrong enum value for the type):

const nsStyleDisplay* display;
::GetStyleData(aStyleContext, &display);

>+    const nsStyleVisibility* vis = (const nsStyleVisibility*)
>+      aStyleContext->GetStyleData(eStyleStruct_Visibility);

Same here.

>+      const nsStyleBorder* borderStyle = (const nsStyleBorder*)
>+        aStyleContext->GetStyleData(eStyleStruct_Border);
>+      const nsStylePadding* paddingStyle = (const nsStylePadding*)
>+        aStyleContext->GetStyleData(eStyleStruct_Padding);

And here.

>+void
>+nsContainerFrame::SyncFrameViewAfterSizeChange(nsIPresContext*  aPresContext,
>+                                               nsIFrame*        aFrame,
>+                                               nsIStyleContext* aStyleContext,
>+                                               nsIView*         aView,
>+                                               PRUint32         aFlags)
>+{
...
>+  nsCOMPtr<nsIStyleContext> savedStyleContext;
>+  if (nsnull == aStyleContext) {
>+    aFrame->GetStyleContext(getter_AddRefs(savedStyleContext));
>+    aStyleContext = savedStyleContext;
>+  }

What's the deal with this?  See my comment below (on the header file).

>+  const nsStyleDisplay* display = (const nsStyleDisplay*)
>+    aStyleContext->GetStyleData(eStyleStruct_Display);

Same typesafety business. :-)

>+  PRBool  hasClip, hasOverflowClip;
>+  PRBool  isBlockLevel = display->IsBlockLevel() || (0 != (kidState & NS_FRAME_OUT_OF_FLOW));
>+  hasClip = display->IsAbsolutelyPositioned() && (display->mClipFlags & NS_STYLE_CLIP_RECT);
>+  hasOverflowClip = isBlockLevel && (display->mOverflow == NS_STYLE_OVERFLOW_HIDDEN);

How about simultaneous declaration and assignment for all three, rather than
predeclaring two of them?  And what's with the funky two spaces between type
and variable name?

>+nsContainerFrame::SyncFrameViewProperties(nsIPresContext*  aPresContext,
...
>+  nsCOMPtr<nsIStyleContext> savedStyleContext;
>+  if (nsnull == aStyleContext) {
>+    aFrame->GetStyleContext(getter_AddRefs(savedStyleContext));
>+    aStyleContext = savedStyleContext;
>+  }

Same thing about the optional parameter business.  See below, again.

>+  const nsStyleVisibility* vis = (const nsStyleVisibility*)
>+    aStyleContext->GetStyleData(eStyleStruct_Visibility);

Typesafe form preferred.  I'll stop commenting on the rest of these. :-)

>+  // See if the frame is being relatively positioned or absolutely
>+  // positioned
>+  if (NS_STYLE_POSITION_RELATIVE == display->mPosition) {
>+    isTopMostView = PR_TRUE;
>+  } else if (display->IsAbsolutelyPositioned()) {
>+    isTopMostView = PR_TRUE;
>+  }

Kin commented on this.	How about using |display->IsPositioned()|, and
commenting that it includes absolute and relative?  And while you're at it, why
not just declare the variable here too rather than initializing it to PR_FALSE
above?

  // See if the frame is relatively or absolutely positioned.
  PRBool isTopMostView = display->IsPositioned();


>+nsContainerFrame::FrameNeedsView(nsIPresContext* aPresContext,

>+  if (vis->mOpacity != 1.0f) {
>+    return PR_TRUE;
>+  }

How about an XXX that this really really sucks since -moz-opacity is
(incorrectly, of course) inherited?  Or do we have that plastered over some
other file?

>+  if (NS_STYLE_POSITION_RELATIVE == display->mPosition) {
>+    return PR_TRUE;
>+  } else if (display->IsAbsolutelyPositioned()) {
>+    return PR_TRUE;
>+  } 

How about |display->IsPositioned()|?

>+  nsCOMPtr<nsIAtom>  pseudoTag;
>+  aStyleContext->GetPseudoType(*getter_AddRefs(pseudoTag));
>+  if (pseudoTag == nsLayoutAtoms::scrolledContentPseudo) {
>+    return PR_TRUE;
>+  }

Maybe scope this in a block so the atom is released immediately?  Or are all
our compilers smart enough to put all the destructors at the end of the
function and then jump to the appropriate point while preserving the return
value in an appropriate register or location on the stack?  Then again, there's
only one early return after it so it doesn't matter much either way, but maybe
this applies for some of the earlier bits in this function.

>Index: layout/html/base/src/nsContainerFrame.h
>+  // This function calls SyncFrameViewAfterGeometryChange, taking care

SyncFrameViewAfterSizeChange ??

>+  static void SyncFrameViewAfterSizeChange(nsIPresContext*  aPresContext,
>+                                           nsIFrame*        aFrame,
>+                                           nsIStyleContext* aStyleContext,
>+                                           nsIView*         aView,
>+                                           PRUint32         aFlags = 0);

>+  static void SyncFrameViewProperties(nsIPresContext*  aPresContext,
>+                                      nsIFrame*        aFrame,
>+                                      nsIStyleContext* aStyleContext,
>+                                      nsIView*         aView,
>+                                      PRUint32         aFlags = 0);

>+  static PRBool FrameNeedsView(nsIPresContext* aPresContext,
>+                               nsIFrame* aFrame, nsIStyleContext* aStyleContext);

Above, it looked like the |aStyleContext| parameter for at least some of these
was optional.  That should be noted.  (That said, I'm not a big fan of optional
parameters if there's not a good reason.  Is there?)

>Index: layout/html/base/src/nsHTMLContainerFrame.cpp
>+  // Create a view
>+  static NS_DEFINE_CID(kViewCID, NS_VIEW_CID);
>+
>+  nsresult result = nsComponentManager::CreateInstance(kViewCID, 
>+                                                       nsnull, 
>+                                                       NS_GET_IID(nsIView), 
>+                                                       (void **)&view);

This could be the typesafe form:

CallCreateInstance(kViewCID, &view);

>+  if (NS_SUCCEEDED(parentView->QueryInterface(NS_GET_IID(nsIScrollableView), (void**)&scrollingView))) {

if (NS_SUCCEEDED(CallQueryInterface(parentView, &scrollingView))) {

These two changes both apply to nsBoxFrame.cpp as well.

Comment 15

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

Comment on attachment 100510 [details] [diff] [review]
Updated trunk patch


One more nit:  how about condensing these with || ?|

>+    if (nsViewVisibility_kShow == visibility
>+        && NS_STYLE_VISIBILITY_HIDDEN == vis->mVisible) {
>+      viewHasTransparentContent = PR_TRUE;
>+    } else if (NS_STYLE_OVERFLOW_VISIBLE == display->mOverflow
>+               && (kidState & NS_FRAME_OUTSIDE_CHILDREN) != 0) {
>+      viewHasTransparentContent = PR_TRUE;
>+    }

r=dbaron.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

diff -uw. Got it :-).

> We really do need comments.

Yeah... I'll insert my bugzilla comments, modified.

> I'll be evil and suggest:

I'll go with the boolean temporary.

> Or is my sense of C++ style being skewed by writing LISP

Don't let those Harvard freaks mess with your head.

> Why over-parenthesize?

Old code, but I'll fix it.

> I prefer the typesafe form

OK, I'll fix them all.

> How about simultaneous declaration and assignment for all three, rather than
> predeclaring two of them?  And what's with the funky two spaces between type
> and variable name?

Two words: legacy code.

> How about using |display->IsPositioned()|

Good call.

> Why not just declare the variable here

Sure, why not :-).

> How about an XXX that this really really sucks since -moz-opacity is
> (incorrectly, of course) inherited?

I have a long list of bugs that boil down to "implement group opacity instead of
inheritance". I don't think there's any need to plaster XXXs around to drive the
point home. This is certainly not the place, since this code will actually
survive the great correction.

> Maybe scope this in a block so the atom is released immediately?  Or are all
> our compilers smart enough to put all the destructors at the end of the
> function and then jump to the appropriate point while preserving the return
> value in an appropriate register or location on the stack?

Probably not *all* our compilers, but I could get old worrying about this kind
of thing. This style is ubiquitous; if compilers can't handle it, it's their
problem.

> SyncFrameViewAfterSizeChange

Righto.

> (That said, I'm not a big fan of optional
> parameters if there's not a good reason.  Is there?)

At least one caller (can't remember which one right now) wants to call
CreateViewForFrame passing in a style context which is not the frame's own style
context (it adds a pseudo). On the other hand, most callers just want the
frame's context.

> This could be the typesafe form:

OK.

> if (NS_SUCCEEDED(CallQueryInterface(parentView, &scrollingView))) {

OK.

> how about condensing these with || ?|

As you wish.

You should know that some HCI people at CMU did a study which showed that people
are very bad at understanding complex boolean expressions and do better with
more of a decision tree representation. Don't let all that LISP go to your head :-).

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

fix checked in.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

marking fixed.