We need to test if a scrollable area will be scrolled in fast/slow path in bug 105075 and bug 104950.
Created attachment 180364 [details] Patch to grab missing symbols on ews
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
Created attachment 180375 [details] Patch to grab win symbol
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
Created attachment 180396 [details] Patch for review
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.
(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.
Created attachment 180531 [details] Patch to fail to grab win symbol
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
(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.
(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.
Created attachment 180580 [details] Retry ews-win to grab symbol
(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.
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
Created attachment 181333 [details] Patch
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.
Created attachment 181350 [details] Patch
(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.
@jamesr, does the latest patch look good to you?
Comment on attachment 181350 [details] Patch R=me
Comment on attachment 181350 [details] Patch Clearing flags on attachment: 181350 Committed r138991: <http://trac.webkit.org/changeset/138991>
All reviewed patches have been landed. Closing bug.