Bug 174731 – Drag and Drop preview image for Twitter link is the wrong shape

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Drag and Drop preview image for Twitter link is the wrong shape

[Tim Horton]

Created attachment 316128 [details]
Patch

[zalan]

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

[Tim Horton]

(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.

[zalan]

> > 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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Tim Horton]

Created attachment 316142 [details]
Patch

[Tim Horton]

Created attachment 316145 [details]
Patch

[Tim Horton]

Created attachment 316153 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 316153 [details]
Patch

Clearing flags on attachment: 316153

Committed r219756: <http://trac.webkit.org/changeset/219756>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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?

[Tim Horton]

(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.

[Tim Horton]

Fixed most of Darin's comments in https://trac.webkit.org/changeset/219761/webkit