Bug 105546 – Add window.internals.nonFastScrollableRects for testing scrollable areas in fast/slow paths

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Xianzhu Wang]

We need to test if a scrollable area will be scrolled in fast/slow path in bug 105075 and bug 104950.

[Xianzhu Wang]

Created attachment 180364 [details]
Patch to grab missing symbols on ews

[Build Bot]

Comment on attachment 180364 [details]
Patch to grab missing symbols on ews

Attachment 180364 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15459060

[Xianzhu Wang]

Created attachment 180375 [details]
Patch to grab win symbol

[Build Bot]

Comment on attachment 180375 [details]
Patch to grab win symbol

Attachment 180375 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15461038

[Xianzhu Wang]

Created attachment 180396 [details]
Patch for review

[Tien-Ren Chen]

I think it would be more useful to export the rects as javascript arrays. That way it is easier to write test scripts to generate absolute <divs> on the page, in case we want to make it a pixel test.

[Xianzhu Wang]

(In reply to comment #6)
> I think it would be more useful to export the rects as javascript arrays. That way it is easier to write test scripts to generate absolute <divs> on the page, in case we want to make it a pixel test.

Done.

[Xianzhu Wang]

Created attachment 180531 [details]
Patch to fail to grab win symbol

[Build Bot]

Comment on attachment 180531 [details]
Patch to fail to grab win symbol

Attachment 180531 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15455461

[Tien-Ren Chen]

(In reply to comment #7)
> (In reply to comment #6)
> > I think it would be more useful to export the rects as javascript arrays. That way it is easier to write test scripts to generate absolute <divs> on the page, in case we want to make it a pixel test.
> 
> Done.

Thanks! I really like this patch!

nits: By the way, the rects returned will be in absolute coordinates right? I think we can reuse ClientRect as the return type -- not worth the trouble to create another rect type, and some other internal functions are already doing this. However I think we should document it somehow. Maybe a line of comment? Or change the function name to nonFastScrollableAbsoluteRects? I don't have a strong opinion to either. Your call.

[Xianzhu Wang]

(In reply to comment #10)
> Thanks! I really like this patch!
> 
> nits: By the way, the rects returned will be in absolute coordinates right? I think we can reuse ClientRect as the return type -- not worth the trouble to create another rect type, and some other internal functions are already doing this. However I think we should document it somehow. Maybe a line of comment? Or change the function name to nonFastScrollableAbsoluteRects? I don't have a strong opinion to either. Your call.

The rects will be in the coordinates of the frame corresponding to the document parameter of the function.

[Xianzhu Wang]

Created attachment 180580 [details]
Retry ews-win to grab symbol

[Tien-Ren Chen]

(In reply to comment #11)
> (In reply to comment #10)
> > Thanks! I really like this patch!
> > 
> > nits: By the way, the rects returned will be in absolute coordinates right? I think we can reuse ClientRect as the return type -- not worth the trouble to create another rect type, and some other internal functions are already doing this. However I think we should document it somehow. Maybe a line of comment? Or change the function name to nonFastScrollableAbsoluteRects? I don't have a strong opinion to either. Your call.
> 
> The rects will be in the coordinates of the frame corresponding to the document parameter of the function.

It is absolute rect, as seen from:
IntRect RenderLayer::scrollableAreaBoundingBox() const
{
    return renderer()->absoluteBoundingBoxRect();
}

The definition of absolute rect is simple. Means its position is the same as if you add a position:absolute box on the document body.

It becomes complicated with the Chromium Android scaling model (applyPageScaleFactorInCompositor() == false) though. In this mode RenderObject::absoluteRect WILL BE SCALED by the page scale factor. That is why we have another term "normalized coordinate" to describe the unscaled absolute rect. It is unlikely to fix absolute rect without breaking everything. Sorry. :(

Bonus: We also have this client coordinate, which is measured relative to the upper left of the viewport, in CSS pixels. It is essentially the same as if you add a position:fixed box on the document body. (Not entirely true, FrameView::scrollOffsetForFixedPosition() can return arbitrary offset to layout fixed position) For Chromium Android M18 the client coordinates have similar scaling problem as absolute coordinates. I submitted a few patches recently to make sure most of the DOM object to correctly input/output unscaled coordinate. In next release they will work properly.

[Build Bot]

Comment on attachment 180580 [details]
Retry ews-win to grab symbol

Attachment 180580 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15451681

[Xianzhu Wang]

Created attachment 181333 [details]
Patch

[James Robinson]

Comment on attachment 181333 [details]
Patch

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

> Source/WebCore/dom/ClientRectList.h:43
>          static PassRefPtr<ClientRectList> create(const Vector<FloatQuad>& quads) { return adoptRef(new ClientRectList(quads)); }
> +        static PassRefPtr<ClientRectList> create(const Vector<FloatRect>& rects) { return adoptRef(new ClientRectList(rects)); }

Is this necessary?  All FloatRects are also FloatQuads and I don't think this case is performance sensitive enough to need special casing.

[Xianzhu Wang]

Created attachment 181350 [details]
Patch

[Xianzhu Wang]

(In reply to comment #16)
> (From update of attachment 181333 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181333&action=review
> 
> > Source/WebCore/dom/ClientRectList.h:43
> >          static PassRefPtr<ClientRectList> create(const Vector<FloatQuad>& quads) { return adoptRef(new ClientRectList(quads)); }
> > +        static PassRefPtr<ClientRectList> create(const Vector<FloatRect>& rects) { return adoptRef(new ClientRectList(rects)); }
> 
> Is this necessary?  All FloatRects are also FloatQuads and I don't think this case is performance sensitive enough to need special casing.

Agreed. Removed the added create() and constructor.

[Xianzhu Wang]

@jamesr, does the latest patch look good to you?

[James Robinson]

Comment on attachment 181350 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 181350 [details]
Patch

Clearing flags on attachment: 181350

Committed r138991: <http://trac.webkit.org/changeset/138991>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.