Bug 59046

Summary: With a non-1 page scale, scrolling to reveal selection fails
Product: WebKit Reporter: mitz
Component: New BugsAssignee: mitz
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch mjs: review+

Description mitz 2011-04-20 17:17:19 PDT
With a non-1 page scale, scrolling to reveal selection fails
Comment 1 mitz 2011-04-20 18:55:07 PDT
<rdar://problem/9095366>
Comment 2 mitz 2011-04-20 18:57:04 PDT
Created attachment 90474 [details]
Patch
Comment 3 Maciej Stachowiak 2011-04-20 19:06:10 PDT
Comment on attachment 90474 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90474&action=review

r=me

> Source/WebCore/rendering/RenderObject.cpp:1164
>          if (!v->hasLayer() || !v->layer()->isComposited() || v->layer()->backing()->paintingGoesToWindow()) {
> -            v->repaintViewRectangle(r, immediate);
> +            IntRect repaintRectangle = r;
> +            if (v->hasLayer() &&  v->layer()->transform() && v->layer()->isComposited() && v->layer()->backing()->paintingGoesToWindow())
> +                repaintRectangle = v->layer()->transform()->mapRect(r);
> +            v->repaintViewRectangle(repaintRectangle, immediate);

The logic here is a bit squarely. You test some conditions and then similar or opposite looking ones right inside that if. I wonder if this can be refactored to be more clear, though I have no specific suggestions.
Comment 4 mitz 2011-04-20 19:11:19 PDT
(In reply to comment #3)
> (From update of attachment 90474 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90474&action=review
> 
> r=me
> 
> > Source/WebCore/rendering/RenderObject.cpp:1164
> >          if (!v->hasLayer() || !v->layer()->isComposited() || v->layer()->backing()->paintingGoesToWindow()) {
> > -            v->repaintViewRectangle(r, immediate);
> > +            IntRect repaintRectangle = r;
> > +            if (v->hasLayer() &&  v->layer()->transform() && v->layer()->isComposited() && v->layer()->backing()->paintingGoesToWindow())
> > +                repaintRectangle = v->layer()->transform()->mapRect(r);
> > +            v->repaintViewRectangle(repaintRectangle, immediate);
> 
> The logic here is a bit squarely. You test some conditions and then similar or opposite looking ones right inside that if. I wonder if this can be refactored to be more clear, though I have no specific suggestions.

I can (and will) remove the “ &&  v->layer()->backing()->paintingGoesToWindow()” part, because it’s redundant, and add a local boolean variable for v->hasLayer() && v->layer()->isComposited()
Comment 5 mitz 2011-04-20 19:14:02 PDT
Fixed in r84454. <http://trac.webkit.org/changeset/84454>
Comment 6 WebKit Review Bot 2011-04-20 20:02:17 PDT
http://trac.webkit.org/changeset/84454 might have broken Qt Linux Release
The following tests are not passing:
fast/transforms/selection-bounds-in-transformed-view.html