[Webkit-unassigned] [Bug 9268] Quirksmode: Backgrounds on table rows

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=9268


Beth Dakin <bdakin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #44362|review?                     |review-
               Flag|                            |




--- Comment #14 from Beth Dakin <bdakin at apple.com>  2010-01-05 15:38:44 PST ---
(From update of attachment 44362)
Hi Daniel! I am sorry it took so long to get back to you on this. Dan and I
talked this over today, and here are some of out thoughts.

First, the specification that you link to outlines a whole list of how tables
should be painted, and this patch addresses one of the items on the list. Do
you know if we do all of these things correctly right now? I suspect that we do
not, but I could be wrong. It would be good to have a set of tests to know
exactly where we stand on all of these issues before we try to fix one of the
issues since we may want to take a different approach entirely if we want to
try to fix them all at once. At the very least, it would be good to have bugs
representing any of the other issues and to have a vague plan about how they
might be fixed just to feel confident that all of the code to fix this issue
wouldn't have to be re-written to fix the others. 


> Index: WebCore/rendering/RenderBoxModelObject.cpp
> ===================================================================
…
> @@ -457,11 +459,32 @@ void RenderBoxModelObject::paintFillLaye
>          IntPoint phase;
>          IntSize tileSize;
>  
> +        RenderTableCell* cell = isTableCell() ? toRenderTableCell(this) : 0;
> +        RenderTableRow* row = cell ? toRenderTableRow(cell->parent()) : 0;
> +        bool isBackgroundImageForRow = row && bgLayer == row->style()->backgroundLayers();
> +


I don't like having all of this table-specific code in
paintFillLayerExtended(). Generally, this is something we try to avoid in
RenderBoxModelObject via polymorphism. I know that paintFillLayerExtended() is
not virtual right now, but it might be worth considering making it virtual. Dan
and I doubt there would be a performance hit for that. On the other hand, you
could add a new virtual function, something like doNewBackgoundStuff() (but
with a better name) that would do nothing in the base class
(RenderBoxModelObject) but all kinds of things in RenderTable.

> +        if (isBackgroundImageForRow && !cell->col()) {
> +            // We calculate and store the background image geometry for the table-row with respect to the
> +            // first cell's (tx, ty) coordinates. We will use this geometry to determine which table cells
> +            // need to be painted (via intersection test) and what part of the background to paint in them.
> +            calculateBackgroundImageGeometry(bgLayer, tx, ty, row->width(), row->height(), destRect, phase, tileSize);
> +            row->setBackgroundDestRect(destRect);
> +            row->setBackgroundPhase(phase);
> +        }

Dan pointed out to me that this is insufficient. A lot of painting occurs
incrementally and only certain cells get repainted. When a subset of cells is
being repainted that doesn't include the first cell in the row, this will go
awry. 


> +        if (cell && cell->paintedBackgroundRect().width() < destRect.width())

I don't understand this comparison.Can you explain it to me? This probably
deserves a comment.

> +    IntRect m_paintedBackgroundRect;
> +    int m_paintedBackgroundPhaseX;
>  };


I don't like the idea of adding these bits to RenderTableCell. I don't like it
because generally it's good to use fewer bits, and because very few
RenderTableCells will actually even use these bits. I think it would be better
to do these computations on the fly.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.