Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2127303002:
Make the crashed web platform pointerevent tests pass (Closed)
|
Created:
4 years, 5 months ago by lanwei Modified:
4 years, 5 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org, extensions-reviews_chromium.org, Peter Beverloo, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, blink-reviews, piman+watch_chromium.org, jochen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake the crashed web platform pointerevent tests pass.
In the Issue 2130433002, Navid had to skip some pointerevent tests because they crashed for some reasons.
This patch fixes it and also remove them from the TestExpectations file.
BUG=620236
Committed: https://crrev.com/c6d61f2a2afb8560ea805450887d802cb90bf117
Cr-Commit-Position: refs/heads/master@{#405558}
Patch Set 1 : pointer event tests #
Total comments: 3
Patch Set 2 : Use ScrollTo to move the target into the view bound #
Messages
Total messages: 38 (24 generated)
Description was changed from ========== pointer test pointer test BUG=620236 ========== to ========== Make the crashed web platform pointerevent tests pass. In the Issue 2130433002, Navid had to skip some pointerevent tests because they crashed for some reasons. This patch fixes it and also remove them from the TestExpectations file. BUG=620236 ==========
lanwei@chromium.org changed reviewers: + nzolghadr@chromium.org
lanwei@chromium.org changed reviewers: + rbyers@chromium.org
lanwei@chromium.org changed reviewers: + dtapuska@chromium.org, mustaq@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Make the crashed web platform pointerevent tests pass. In the Issue 2130433002, Navid had to skip some pointerevent tests because they crashed for some reasons. This patch fixes it and also remove them from the TestExpectations file. BUG=620236 ========== to ========== Make the crashed web platform pointerevent tests pass. In the Issue 2130433002, Navid had to skip some pointerevent tests because they crashed for some reasons. This patch fixes it and also remove them from the TestExpectations file. BUG=620236 ==========
LGTM overall. That is okay if you skip those tests that are still failing if you believe they are failing due to some other unrelated errors. We can iterate over those again later.
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
https://codereview.chromium.org/2127303002/diff/120001/content/shell/browser/... File content/shell/browser/layout_test/blink_test_controller.cc (right): https://codereview.chromium.org/2127303002/diff/120001/content/shell/browser/... content/shell/browser/layout_test/blink_test_controller.cc:289: // The W3C pointer event layout tests use a different size than the other Ugh. I know you're just repeating the pattern above, but this is pretty magical. Can you not just have the test scroll down (eg. using Element.scrollIntoView) as we had discussed? Some tests may still exceed such a height, really tests should be written not to assume any particular window size (which is probably why we use a pretty small window size by default - better to provoke tests more often).
https://codereview.chromium.org/2127303002/diff/120001/content/shell/browser/... File content/shell/browser/layout_test/blink_test_controller.cc (right): https://codereview.chromium.org/2127303002/diff/120001/content/shell/browser/... content/shell/browser/layout_test/blink_test_controller.cc:289: // The W3C pointer event layout tests use a different size than the other On 2016/07/12 13:54:08, Rick Byers wrote: > Ugh. I know you're just repeating the pattern above, but this is pretty > magical. Can you not just have the test scroll down (eg. using > Element.scrollIntoView) as we had discussed? Some tests may still exceed such a > height, really tests should be written not to assume any particular window size > (which is probably why we use a pretty small window size by default - better to > provoke tests more often). I agree. Navid, what do you think?
On 2016/07/12 14:27:00, lanwei wrote: Sure. That would be good too. Can you give them a try and see if you can get them pass with some scroll?
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
The CQ bit was checked by lanwei@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...
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Patchset #4 (id:180001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2127303002/diff/120001/content/shell/browser/... File content/shell/browser/layout_test/blink_test_controller.cc (right): https://codereview.chromium.org/2127303002/diff/120001/content/shell/browser/... content/shell/browser/layout_test/blink_test_controller.cc:289: // The W3C pointer event layout tests use a different size than the other On 2016/07/12 14:26:54, lanwei wrote: > On 2016/07/12 13:54:08, Rick Byers wrote: > > Ugh. I know you're just repeating the pattern above, but this is pretty > > magical. Can you not just have the test scroll down (eg. using > > Element.scrollIntoView) as we had discussed? Some tests may still exceed such > a > > height, really tests should be written not to assume any particular window > size > > (which is probably why we use a pretty small window size by default - better > to > > provoke tests more often). > > I agree. Navid, what do you think? scrollIntoView can only align either to the top or bottom of the view bound, but we need to align with the left as well, so I use ScrollTo.
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/13 22:27:35, lanwei wrote: > https://codereview.chromium.org/2127303002/diff/120001/content/shell/browser/... > File content/shell/browser/layout_test/blink_test_controller.cc (right): > > https://codereview.chromium.org/2127303002/diff/120001/content/shell/browser/... > content/shell/browser/layout_test/blink_test_controller.cc:289: // The W3C > pointer event layout tests use a different size than the other > On 2016/07/12 14:26:54, lanwei wrote: > > On 2016/07/12 13:54:08, Rick Byers wrote: > > > Ugh. I know you're just repeating the pattern above, but this is pretty > > > magical. Can you not just have the test scroll down (eg. using > > > Element.scrollIntoView) as we had discussed? Some tests may still exceed > such > > a > > > height, really tests should be written not to assume any particular window > > size > > > (which is probably why we use a pretty small window size by default - better > > to > > > provoke tests more often). > > > > I agree. Navid, what do you think? > > scrollIntoView can only align either to the top or bottom of the view bound, but > we need to align with the left as well, so I use ScrollTo. LGTM, thanks!
On 2016/07/14 14:51:58, Rick Byers wrote: > On 2016/07/13 22:27:35, lanwei wrote: > > > https://codereview.chromium.org/2127303002/diff/120001/content/shell/browser/... > > File content/shell/browser/layout_test/blink_test_controller.cc (right): > > > > > https://codereview.chromium.org/2127303002/diff/120001/content/shell/browser/... > > content/shell/browser/layout_test/blink_test_controller.cc:289: // The W3C > > pointer event layout tests use a different size than the other > > On 2016/07/12 14:26:54, lanwei wrote: > > > On 2016/07/12 13:54:08, Rick Byers wrote: > > > > Ugh. I know you're just repeating the pattern above, but this is pretty > > > > magical. Can you not just have the test scroll down (eg. using > > > > Element.scrollIntoView) as we had discussed? Some tests may still exceed > > such > > > a > > > > height, really tests should be written not to assume any particular window > > > size > > > > (which is probably why we use a pretty small window size by default - > better > > > to > > > > provoke tests more often). > > > > > > I agree. Navid, what do you think? > > > > scrollIntoView can only align either to the top or bottom of the view bound, > but > > we need to align with the left as well, so I use ScrollTo. > > LGTM, thanks! lgtm too.
Could you please take a look at content/renderer/gpu/gpu_benchmarking_extension.cc, thanks?
lanwei@chromium.org changed reviewers: + sievers@chromium.org
sievers@ Could you please take a look at content/renderer/gpu/gpu_benchmarking_extension.cc, thanks?
On 2016/07/14 17:04:42, lanwei wrote: > sievers@ Could you please take a look at > content/renderer/gpu/gpu_benchmarking_extension.cc, thanks? lgtm
The CQ bit was checked by lanwei@chromium.org
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 ========== Make the crashed web platform pointerevent tests pass. In the Issue 2130433002, Navid had to skip some pointerevent tests because they crashed for some reasons. This patch fixes it and also remove them from the TestExpectations file. BUG=620236 ========== to ========== Make the crashed web platform pointerevent tests pass. In the Issue 2130433002, Navid had to skip some pointerevent tests because they crashed for some reasons. This patch fixes it and also remove them from the TestExpectations file. BUG=620236 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Make the crashed web platform pointerevent tests pass. In the Issue 2130433002, Navid had to skip some pointerevent tests because they crashed for some reasons. This patch fixes it and also remove them from the TestExpectations file. BUG=620236 ========== to ========== Make the crashed web platform pointerevent tests pass. In the Issue 2130433002, Navid had to skip some pointerevent tests because they crashed for some reasons. This patch fixes it and also remove them from the TestExpectations file. BUG=620236 Committed: https://crrev.com/c6d61f2a2afb8560ea805450887d802cb90bf117 Cr-Commit-Position: refs/heads/master@{#405558} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c6d61f2a2afb8560ea805450887d802cb90bf117 Cr-Commit-Position: refs/heads/master@{#405558} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
