Bug 174731

Summary: Drag and Drop preview image for Twitter link is the wrong shape
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, commit-queue, darin, dbates, esprehn+autocc, kangil.han, rniwa, simon.fraser, wenson_hsieh, zalan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Patch
none
Patch
none
Patch none

Description Tim Horton 2017-07-21 15:30:58 PDT
Drag and Drop preview image for Twitter link is the wrong shape
Comment 1 Tim Horton 2017-07-21 15:31:15 PDT
Created attachment 316128 [details]
Patch
Comment 2 zalan 2017-07-21 15:47:17 PDT
Comment on attachment 316128 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:6568
> +    const auto& frameView = *view();

auto& frameView

> Source/WebCore/dom/Range.cpp:1181
> +        for (const auto& rect : textRects) {

auto& rect

> Source/WebCore/dom/Range.cpp:1193
> +    for (const auto& rect : textRects)

auto& rect

> Source/WebCore/dom/Range.cpp:1834
> +                for (const auto& quad : elementQuads)

auto& quad

> Source/WebCore/dom/Range.h:126
> +    WEBCORE_EXPORT FloatRect absoluteBoundingRect(RespectClippingForTextRects = RespectClippingForTextRects::No) const;

All RespectClippingForTextRects::No should turn into YES as the default value at some point in the function.

> Source/WebCore/page/TextIndicator.cpp:322
> +        for (const auto& rect : absoluteTextRects)

auto& rect
Comment 3 Tim Horton 2017-07-21 15:51:23 PDT
(In reply to zalan from comment #2)
> Comment on attachment 316128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316128&action=review
> 
> > Source/WebCore/dom/Document.cpp:6568
> > +    const auto& frameView = *view();
> 
> auto& frameView
> 
> > Source/WebCore/dom/Range.cpp:1181
> > +        for (const auto& rect : textRects) {
> 
> auto& rect
> 
> > Source/WebCore/dom/Range.cpp:1193
> > +    for (const auto& rect : textRects)
> 
> auto& rect
> 
> > Source/WebCore/dom/Range.cpp:1834
> > +                for (const auto& quad : elementQuads)
> 
> auto& quad
> 
> > Source/WebCore/dom/Range.h:126
> > +    WEBCORE_EXPORT FloatRect absoluteBoundingRect(RespectClippingForTextRects = RespectClippingForTextRects::No) const;
> 
> All RespectClippingForTextRects::No should turn into YES as the default
> value at some point in the function.

You mean in the /future/? Yes, I'm going to write that patch Very Soon :) And just remove this parameter.

> > Source/WebCore/page/TextIndicator.cpp:322
> > +        for (const auto& rect : absoluteTextRects)
> 
> auto& rect

Surely, this was dumb.
Comment 4 zalan 2017-07-21 15:54:01 PDT
> > All RespectClippingForTextRects::No should turn into YES as the default
> > value at some point in the function.
> 
> You mean in the /future/? Yes, I'm going to write that patch Very Soon :)
Whenever I type function I actually mean future.
Comment 5 Build Bot 2017-07-21 16:27:19 PDT
Comment on attachment 316128 [details]
Patch

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

New failing tests:
fast/multicol/client-rects.html
fast/multicol/newmulticol/client-rects.html
fast/dom/Range/getClientRects.html
fast/multicol/client-rects-spanners.html
accessibility/mac/bounds-for-range.html
accessibility/content-editable-as-textarea.html
fast/dom/Range/getBoundingClientRect.html
fast/multicol/client-rects-spanners-complex.html
Comment 6 Build Bot 2017-07-21 16:27:21 PDT
Created attachment 316138 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 7 Build Bot 2017-07-21 16:39:01 PDT
Comment on attachment 316128 [details]
Patch

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

New failing tests:
fast/multicol/client-rects.html
fast/multicol/newmulticol/client-rects.html
fast/dom/Range/getClientRects.html
fast/multicol/client-rects-spanners.html
accessibility/mac/bounds-for-range.html
accessibility/content-editable-as-textarea.html
fast/dom/Range/getBoundingClientRect.html
fast/multicol/client-rects-spanners-complex.html
Comment 8 Build Bot 2017-07-21 16:39:02 PDT
Created attachment 316141 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Tim Horton 2017-07-21 16:40:43 PDT
Created attachment 316142 [details]
Patch
Comment 10 Tim Horton 2017-07-21 16:59:42 PDT
Created attachment 316145 [details]
Patch
Comment 11 Tim Horton 2017-07-21 17:59:17 PDT
Created attachment 316153 [details]
Patch
Comment 12 WebKit Commit Bot 2017-07-21 20:01:56 PDT
Comment on attachment 316153 [details]
Patch

Clearing flags on attachment: 316153

Committed r219756: <http://trac.webkit.org/changeset/219756>
Comment 13 WebKit Commit Bot 2017-07-21 20:01:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Darin Adler 2017-07-21 21:39:33 PDT
Comment on attachment 316153 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:6569
> +        if (inverseFrameScale != 1)
> +            rect.scale(inverseFrameScale);

Why the explicit check against 1? Is always this an important optimization? If so, why doesn’t the scale function itself do it? Is there a special reason this is an important optimization here?

> Source/WebCore/dom/Range.cpp:1185
> +        for (auto& quad : textQuads) {
> +            auto clippedRect = intersection(quad.boundingBox(), absoluteClippedOverflowRect);
> +            if (!clippedRect.isEmpty())
> +                clippedRects.append(clippedRect);
> +        }

Could still do reserveInitialCapacity and uncheckedAppend here even if we might not use all the capacity. Not sure what the tradeoff is.

> Source/WebCore/dom/Range.cpp:1193
> +        floatRects.append(quad.boundingBox());

uncheckedAppend

> Source/WebCore/page/TextIndicator.cpp:318
> +        for (auto& rect : absoluteTextRects)
> +            textRects.append(rect);

reserveInitialCapacity/uncheckedAppend?
Comment 15 Tim Horton 2017-07-21 23:36:23 PDT
(In reply to Darin Adler from comment #14)
> Comment on attachment 316153 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316153&action=review
> 
> > Source/WebCore/dom/Document.cpp:6569
> > +        if (inverseFrameScale != 1)
> > +            rect.scale(inverseFrameScale);
> 
> Why the explicit check against 1? Is always this an important optimization?
> If so, why doesn’t the scale function itself do it? Is there a special
> reason this is an important optimization here?

Good question! This is a copy/paste from the function right above (which operates on quads instead). In retrospect, probably no good reason.

> > Source/WebCore/dom/Range.cpp:1185
> > +        for (auto& quad : textQuads) {
> > +            auto clippedRect = intersection(quad.boundingBox(), absoluteClippedOverflowRect);
> > +            if (!clippedRect.isEmpty())
> > +                clippedRects.append(clippedRect);
> > +        }
> 
> Could still do reserveInitialCapacity and uncheckedAppend here even if we
> might not use all the capacity. Not sure what the tradeoff is.

Oh! Yes, I suppose.

> > Source/WebCore/dom/Range.cpp:1193
> > +        floatRects.append(quad.boundingBox());
> 
> uncheckedAppend
> 
> > Source/WebCore/page/TextIndicator.cpp:318
> > +        for (auto& rect : absoluteTextRects)
> > +            textRects.append(rect);
> 
> reserveInitialCapacity/uncheckedAppend?

I will fix these.
Comment 16 Tim Horton 2017-07-22 01:12:32 PDT
Fixed most of Darin's comments in https://trac.webkit.org/changeset/219761/webkit