Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1781133002:
Change page size for NonBlockingEventBrowserTest (Closed)
DescriptionChange page size for NonBlockingEventBrowserTest
This test increases the page size for NonBlockingEventBrowserTest
from 1000dp to 10000dp.
This will allow the test to scroll on devices with a height
of more than 1000dp (like a Nexus 10)
BUG=593974
Committed: https://crrev.com/5fdb8170b7f49b38da430197fe0ba2b642b77f2e
Cr-Commit-Position: refs/heads/master@{#380699}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Refactored page height into a constant #
Total comments: 1
Patch Set 3 : Removed raw string literal #Messages
Total messages: 23 (7 generated)
kraush@amazon.com changed reviewers: + tdresser@chromium.org
Hi tdresser@, This is a proposed fix for two content_browsertests on Nexus 10 (and other large devices). Can you please take a look? :) Thanks! Holger
+dtapuska@ FYI LGTM with nit. https://codereview.chromium.org/1781133002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/non_blocking_event_browsertest.cc (right): https://codereview.chromium.org/1781133002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/non_blocking_event_browsertest.cc:113: EXPECT_EQ(10000, scrollHeight); Can we factor this out into a constant, with a comment indicating that this must be larger than the height in dips of the device under test?
Sorry, not LGTM, just due to lack of previous patches landed in our codebase (though I'm sure your solution will be fine). I'll take another look once you've made that minor adjustment.
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1781133002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1781133002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/03/11 13:27:23, tdresser wrote: > +dtapuska@ FYI > > LGTM with nit. > > https://codereview.chromium.org/1781133002/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/input/non_blocking_event_browsertest.cc > (right): > > https://codereview.chromium.org/1781133002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/input/non_blocking_event_browsertest.cc:113: > EXPECT_EQ(10000, scrollHeight); > Can we factor this out into a constant, with a comment indicating that this must > be larger than the height in dips of the device under test? Good idea! I'll factor it out into a constant.
Hi Timothy, I've extracted the "10000" into a constant as you suggested. Please let me know if there is anything else you'd like me to change. Thanks, Holger
https://codereview.chromium.org/1781133002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/non_blocking_event_browsertest.cc (right): https://codereview.chromium.org/1781133002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/non_blocking_event_browsertest.cc:42: R"(data:text/html;charset=utf-8, Sadly C++11 raw string literals aren't allowed in chromium yet. :( See https://chromium-cpp.appspot.com/ and https://groups.google.com/a/chromium.org/d/msg/chromium-dev/2kWQHbbuMHI/ilh_p... for details. Let's just include this value explicitly in the const char[] (as before), and use the constant everywhere else.
On 2016/03/11 17:01:24, tdresser wrote: > https://codereview.chromium.org/1781133002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/input/non_blocking_event_browsertest.cc > (right): > > https://codereview.chromium.org/1781133002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/non_blocking_event_browsertest.cc:42: > R"(data:text/html;charset=utf-8, > Sadly C++11 raw string literals aren't allowed in chromium yet. :( > > See https://chromium-cpp.appspot.com/ and > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/2kWQHbbuMHI/ilh_p... > for details. > > Let's just include this value explicitly in the const char[] (as before), and > use the constant everywhere else. Argh, sorry, didn't know :( I'll leave it hardcoded in the literal then and use the const within the tests.
Removed the raw string literal. Please take another look.
LGTM!
The CQ bit was checked by kraush@amazon.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1781133002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1781133002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1781133002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1781133002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Change page size for NonBlockingEventBrowserTest This test increases the page size for NonBlockingEventBrowserTest from 1000dp to 10000dp. This will allow the test to scroll on devices with a height of more than 1000dp (like a Nexus 10) BUG=593974 ========== to ========== Change page size for NonBlockingEventBrowserTest This test increases the page size for NonBlockingEventBrowserTest from 1000dp to 10000dp. This will allow the test to scroll on devices with a height of more than 1000dp (like a Nexus 10) BUG=593974 Committed: https://crrev.com/5fdb8170b7f49b38da430197fe0ba2b642b77f2e Cr-Commit-Position: refs/heads/master@{#380699} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5fdb8170b7f49b38da430197fe0ba2b642b77f2e Cr-Commit-Position: refs/heads/master@{#380699} |
