Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2218703002:
[mapToVisualRectInAncestorSpace] Check for detached iframes. (Closed)
|
Created:
4 years, 4 months ago by szager1 Modified:
4 years, 4 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[mapToVisualRectInAncestorSpace] Check for detached iframes.
If, e.g., display:none is set on an iframe, mapTo... should return an empty rect.
BUG=627616
R=chrishtr@chromium.org,wangxianzhu@chromium.org
Committed: https://crrev.com/ff3e7c584eaff665b8560dc1b6150c95a2062fb0
Cr-Commit-Position: refs/heads/master@{#410185}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Simplify test #
Messages
Total messages: 19 (7 generated)
lgtm with a nit: in other places we call a frame in this situation a "detached frame". Better to mention it in the comments besides mentioning display:none. We can avoid some checks like this if we throttle rendering of detached frames (crbug.com/601634).
Description was changed from ========== [mapToVisualRectInAncestorSpace] Check for display:none on iframes. BUG=627616 R=chrishtr@chromium.org,wangxianzhu@chromium.org ========== to ========== [mapToVisualRectInAncestorSpace] Check for detached iframes. If, e.g., display:none is set on an iframe, mapTo... should return an empty rect. BUG=627616 R=chrishtr@chromium.org,wangxianzhu@chromium.org ==========
On 2016/08/05 00:53:43, Xianzhu wrote: > lgtm with a nit: in other places we call a frame in this situation a "detached > frame". Better to mention it in the comments besides mentioning display:none. > > We can avoid some checks like this if we throttle rendering of detached frames > (crbug.com/601634). Done.
The CQ bit was checked by szager@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2218703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp (right): https://codereview.chromium.org/2218703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:168: Document& frameDocument = setupChildIframe("frame", "<style>body { margin: 0; }</style><span><img style='width: 20px; height: 100px'></span>text text text"); Replace the span + img with a simple div.
The CQ bit was unchecked by chrishtr@chromium.org
https://codereview.chromium.org/2218703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp (right): https://codereview.chromium.org/2218703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:168: Document& frameDocument = setupChildIframe("frame", "<style>body { margin: 0; }</style><span><img style='width: 20px; height: 100px'></span>text text text"); On 2016/08/05 01:07:02, chrishtr wrote: > Replace the span + img with a simple div. The span is inline; you want me to replace with: <span><div style="display:inline-block; width:20px; height:100px"></div></span>text text text ?
https://codereview.chromium.org/2218703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp (right): https://codereview.chromium.org/2218703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:168: Document& frameDocument = setupChildIframe("frame", "<style>body { margin: 0; }</style><span><img style='width: 20px; height: 100px'></span>text text text"); On 2016/08/05 at 01:27:21, szager1 wrote: > On 2016/08/05 01:07:02, chrishtr wrote: > > Replace the span + img with a simple div. > > The span is inline; you want me to replace with: > > <span><div style="display:inline-block; width:20px; height:100px"></div></span>text text text > > ? The whole point of the test is to check that an empty rect is returned if the iframe is display:none. There's no need for anything inline, the contents of the iframe can just be a single div, right?
On 2016/08/05 03:13:34, chrishtr wrote: > https://codereview.chromium.org/2218703002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp (right): > > https://codereview.chromium.org/2218703002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:168: Document& > frameDocument = setupChildIframe("frame", "<style>body { margin: 0; > }</style><span><img style='width: 20px; height: 100px'></span>text text text"); > On 2016/08/05 at 01:27:21, szager1 wrote: > > On 2016/08/05 01:07:02, chrishtr wrote: > > > Replace the span + img with a simple div. > > > > The span is inline; you want me to replace with: > > > > <span><div style="display:inline-block; width:20px; > height:100px"></div></span>text text text > > > > ? > > The whole point of the test is to check that an empty rect is returned if the > iframe is display:none. There's no need for anything inline, the contents of the > iframe can just be a single div, right? OK, Done.
lgtm
The CQ bit was checked by szager@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wangxianzhu@chromium.org Link to the patchset: https://codereview.chromium.org/2218703002/#ps20001 (title: "Simplify test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [mapToVisualRectInAncestorSpace] Check for detached iframes. If, e.g., display:none is set on an iframe, mapTo... should return an empty rect. BUG=627616 R=chrishtr@chromium.org,wangxianzhu@chromium.org ========== to ========== [mapToVisualRectInAncestorSpace] Check for detached iframes. If, e.g., display:none is set on an iframe, mapTo... should return an empty rect. BUG=627616 R=chrishtr@chromium.org,wangxianzhu@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [mapToVisualRectInAncestorSpace] Check for detached iframes. If, e.g., display:none is set on an iframe, mapTo... should return an empty rect. BUG=627616 R=chrishtr@chromium.org,wangxianzhu@chromium.org ========== to ========== [mapToVisualRectInAncestorSpace] Check for detached iframes. If, e.g., display:none is set on an iframe, mapTo... should return an empty rect. BUG=627616 R=chrishtr@chromium.org,wangxianzhu@chromium.org Committed: https://crrev.com/ff3e7c584eaff665b8560dc1b6150c95a2062fb0 Cr-Commit-Position: refs/heads/master@{#410185} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ff3e7c584eaff665b8560dc1b6150c95a2062fb0 Cr-Commit-Position: refs/heads/master@{#410185} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
