Summary: | Drag and Drop preview image for Twitter link is the wrong shape | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||||||||
Component: | New Bugs | Assignee: | 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
Tim Horton
2017-07-21 15:30:58 PDT
Created attachment 316128 [details]
Patch
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 (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. > > 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 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 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 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 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
Created attachment 316142 [details]
Patch
Created attachment 316145 [details]
Patch
Created attachment 316153 [details]
Patch
Comment on attachment 316153 [details] Patch Clearing flags on attachment: 316153 Committed r219756: <http://trac.webkit.org/changeset/219756> All reviewed patches have been landed. Closing bug. 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? (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. Fixed most of Darin's comments in https://trac.webkit.org/changeset/219761/webkit |