Bug 174731 - Drag and Drop preview image for Twitter link is the wrong shape
Summary: Drag and Drop preview image for Twitter link is the wrong shape
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-21 15:30 PDT by Tim Horton
Modified: 2017-07-22 01:12 PDT (History)
11 users (show)

See Also:


Attachments
Patch (19.32 KB, patch)
2017-07-21 15:31 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.51 MB, application/zip)
2017-07-21 16:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (1.21 MB, application/zip)
2017-07-21 16:39 PDT, Build Bot
no flags Details
Patch (19.30 KB, patch)
2017-07-21 16:40 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (19.38 KB, patch)
2017-07-21 16:59 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (19.31 KB, patch)
2017-07-21 17:59 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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