Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2642723009:
Improve cross-origin-subframe-for-scrolling layout test (Closed)
DescriptionImprove cross-origin-subframe-for-scrolling layout test
The layout test that landed in revision 444947 has some style issues.
This CL addresses them.
BUG=675695
Review-Url: https://codereview.chromium.org/2642723009
Cr-Commit-Position: refs/heads/master@{#445396}
Committed: https://chromium.googlesource.com/chromium/src/+/551b36390e5a7e8ce6441a62f4bc458fe3b420b8
Patch Set 1 #
Total comments: 7
Patch Set 2 : Simplifying #
Total comments: 3
Patch Set 3 : Added an assert_unreached clause for invalid messages #
Messages
Total messages: 24 (16 generated)
The CQ bit was checked by kenrb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kenrb@chromium.org changed reviewers: + bokan@chromium.org
bokan@: This is a follow up CL to address the outstanding issues with the layout test. PTAL?
Thanks, looks good overall, some minor comments though. https://codereview.chromium.org/2642723009/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/misc/resources/cross-origin-subframe-for-scrolling.html (right): https://codereview.chromium.org/2642723009/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/misc/resources/cross-origin-subframe-for-scrolling.html:2: <script src="/js-test-resources/js-test.js"></script> You can remove this I think. https://codereview.chromium.org/2642723009/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/misc/resources/cross-origin-subframe-for-scrolling.html:30: port.postMessage({scrolled: frame_id}, "*"); Can't we simplify this and just post the scrollTop in here? https://codereview.chromium.org/2642723009/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html (right): https://codereview.chromium.org/2642723009/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html:20: if (state == 0 && event.data.scrolled == 1) { if state == 0 then event.data.scrolled must be 1, I'd rather we just assert that inside or not check it at all. https://codereview.chromium.org/2642723009/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html:27: iframe2.contentWindow.postMessage("", "*"); Don't we need to wait until iframe2 gets the onscroll and posts us about it? https://codereview.chromium.org/2642723009/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html:29: iframe1.contentWindow.postMessage("", "*"); This is the second time we're posting iframe1 with an empty message, was this maybe meant to be iframe2?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kenrb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Some changes based on your comments are up, so you can have another look, although I won't land this until I can resolve the crash issue on https://crbug.com/683282. https://codereview.chromium.org/2642723009/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/misc/resources/cross-origin-subframe-for-scrolling.html (right): https://codereview.chromium.org/2642723009/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/misc/resources/cross-origin-subframe-for-scrolling.html:2: <script src="/js-test-resources/js-test.js"></script> On 2017/01/20 18:13:26, bokan wrote: > You can remove this I think. Done. https://codereview.chromium.org/2642723009/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html (right): https://codereview.chromium.org/2642723009/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html:20: if (state == 0 && event.data.scrolled == 1) { On 2017/01/20 18:13:26, bokan wrote: > if state == 0 then event.data.scrolled must be 1, I'd rather we just assert that > inside or not check it at all. Okay, I've simplified this quite a bit and removed the state variable.
thanks, lgtm when you're ready to land. https://codereview.chromium.org/2642723009/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html (right): https://codereview.chromium.org/2642723009/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html:22: } Nit: Add an else clause that fails the test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kenrb@chromium.org to run a CQ dry run
https://codereview.chromium.org/2642723009/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html (right): https://codereview.chromium.org/2642723009/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html:22: } On 2017/01/20 19:24:54, bokan wrote: > Nit: Add an else clause that fails the test. Done, although it still had to be an 'else if' because there are a lot of extra scroll messages from iframe1 that we need to ignore.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2642723009/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html (right): https://codereview.chromium.org/2642723009/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/misc/scroll-cross-origin-iframes.html:22: } On 2017/01/23 16:15:44, kenrb wrote: > On 2017/01/20 19:24:54, bokan wrote: > > Nit: Add an else clause that fails the test. > > Done, although it still had to be an 'else if' because there are a lot of extra > scroll messages from iframe1 that we need to ignore. Ah, yah - didn't think of that. else if looks fine.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kenrb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485191805024170,
"parent_rev": "b7309482de559eef7acab55c852dd4a29f6e298d", "commit_rev":
"551b36390e5a7e8ce6441a62f4bc458fe3b420b8"}
Message was sent while issue was closed.
Description was changed from ========== Improve cross-origin-subframe-for-scrolling layout test The layout test that landed in revision 444947 has some style issues. This CL addresses them. BUG=675695 ========== to ========== Improve cross-origin-subframe-for-scrolling layout test The layout test that landed in revision 444947 has some style issues. This CL addresses them. BUG=675695 Review-Url: https://codereview.chromium.org/2642723009 Cr-Commit-Position: refs/heads/master@{#445396} Committed: https://chromium.googlesource.com/chromium/src/+/551b36390e5a7e8ce6441a62f4bc... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/551b36390e5a7e8ce6441a62f4bc... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
