Summary: | Add test infrastructure to test rubber-banding overhang drawing along with layout tests for existing Chromium Mac overhang drawing in the non-gpu path. | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | asvitkine | ||||||||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | aroben, bdakin, dglazkov, gustavo.noronha, gustavo, jamesr, sam, simon.fraser, thakis, webkit.review.bot, xan.lopez | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||
Bug Blocks: | 66969 | ||||||||||||||||||||||||
Attachments: |
|
Description
asvitkine
2011-09-02 11:44:25 PDT
Created attachment 106167 [details]
Patch
Wait, won't the new tests fail if you land this before the other patch? I'd remove the test cases from this patch and just add the internals stuff here. These are the test cases for the software path, which already works without the patch. The other patch will have test cases for the compositing path. Comment on attachment 106167 [details] Patch Attachment 106167 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9583724 New failing tests: platform/chromium-mac/rubberbanding/overhang-e.html platform/chromium-mac/rubberbanding/overhang-se.html platform/chromium-mac/rubberbanding/overhang-sw.html platform/chromium-mac/rubberbanding/overhang-ne.html platform/chromium-mac/rubberbanding/overhang-s.html platform/chromium-mac/rubberbanding/overhang-nw.html platform/chromium-mac/rubberbanding/overhang-w.html platform/chromium-mac/rubberbanding/overhang-n.html (In reply to comment #4) > (From update of attachment 106167 [details]) > Attachment 106167 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/9583724 > > New failing tests: > platform/chromium-mac/rubberbanding/overhang-e.html > platform/chromium-mac/rubberbanding/overhang-se.html > platform/chromium-mac/rubberbanding/overhang-sw.html > platform/chromium-mac/rubberbanding/overhang-ne.html > platform/chromium-mac/rubberbanding/overhang-s.html > platform/chromium-mac/rubberbanding/overhang-nw.html > platform/chromium-mac/rubberbanding/overhang-w.html > platform/chromium-mac/rubberbanding/overhang-n.html Apparently putting tests in platform/chromium-mac isn't enough to to stop other platforms from running them... Created attachment 106182 [details]
Patch
Updated patch that should hopefully prevent other platforms from trying to run the tests. Comment on attachment 106182 [details] Patch Attachment 106182 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9587192 Created attachment 106415 [details]
Patch
Trying to get the mangled exports right... Created attachment 106422 [details]
Patch
Comment on attachment 106422 [details] Patch Attachment 106422 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9597123 Created attachment 106433 [details]
Patch
Attachment 106433 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Last 3072 characters of output: media url: http://src.chromium.org/svn/trunk/src/media@99393 parsed_url: http://src.chromium.org/svn/trunk/src/media@99393 should_process: True processed: True requirements: set(['./']) name: build url: http://src.chromium.org/svn/trunk/src/build@99393 parsed_url: http://src.chromium.org/svn/trunk/src/build@99393 should_process: True requirements: set(['./']) name: net url: http://src.chromium.org/svn/trunk/src/net@99393 should_process: True requirements: set(['./']) name: tools/data_pack url: http://src.chromium.org/svn/trunk/src/tools/data_pack@99393 should_process: True requirements: set(['./']) name: third_party/ffmpeg url: From('chromium_deps', 'src/third_party/ffmpeg') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: tools/generate_stubs url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@99393 should_process: True requirements: set(['./']) name: third_party/libjpeg_turbo url: From('chromium_deps', 'src/third_party/libjpeg_turbo') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: third_party/v8-i18n url: From('chromium_deps', 'src/third_party/v8-i18n') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: tools/grit url: http://src.chromium.org/svn/trunk/src/tools/grit@99393 should_process: True requirements: set(['./']) name: base url: http://src.chromium.org/svn/trunk/src/base@99393 should_process: True requirements: set(['./']) name: sql url: http://src.chromium.org/svn/trunk/src/sql@99393 should_process: True requirements: set(['./']) name: v8 url: From('chromium_deps', 'src/v8') should_process: True requirements: set(['chromium_deps', './']) name: testing/gtest url: From('chromium_deps', 'src/testing/gtest') should_process: True requirements: set(['testing', 'chromium_deps', './']) name: third_party/libwebp url: http://src.chromium.org/svn/trunk/src/third_party/libwebp@99393 should_process: True requirements: set(['third_party', './']) name: googleurl url: From('chromium_deps', 'src/googleurl') should_process: True requirements: set(['chromium_deps', './']) name: third_party/skia/include url: From('chromium_deps', 'src/third_party/skia/include') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: third_party/ots url: From('chromium_deps', 'src/third_party/ots') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: third_party/snappy/src url: From('chromium_deps', 'src/third_party/snappy/src') should_process: True requirements: set(['third_party', 'chromium_deps', './']) Died at Tools/Scripts/update-webkit-chromium line 80. No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style. Created attachment 106440 [details]
Patch
Attachment 106440 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Last 3072 characters of output: media url: http://src.chromium.org/svn/trunk/src/media@99393 parsed_url: http://src.chromium.org/svn/trunk/src/media@99393 should_process: True requirements: set(['./']) name: build url: http://src.chromium.org/svn/trunk/src/build@99393 parsed_url: http://src.chromium.org/svn/trunk/src/build@99393 should_process: True processed: True requirements: set(['./']) name: net url: http://src.chromium.org/svn/trunk/src/net@99393 should_process: True requirements: set(['./']) name: tools/data_pack url: http://src.chromium.org/svn/trunk/src/tools/data_pack@99393 should_process: True requirements: set(['./']) name: third_party/ffmpeg url: From('chromium_deps', 'src/third_party/ffmpeg') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: tools/generate_stubs url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@99393 should_process: True requirements: set(['./']) name: third_party/libjpeg_turbo url: From('chromium_deps', 'src/third_party/libjpeg_turbo') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: third_party/v8-i18n url: From('chromium_deps', 'src/third_party/v8-i18n') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: tools/grit url: http://src.chromium.org/svn/trunk/src/tools/grit@99393 should_process: True requirements: set(['./']) name: base url: http://src.chromium.org/svn/trunk/src/base@99393 should_process: True requirements: set(['./']) name: sql url: http://src.chromium.org/svn/trunk/src/sql@99393 should_process: True requirements: set(['./']) name: v8 url: From('chromium_deps', 'src/v8') should_process: True requirements: set(['chromium_deps', './']) name: testing/gtest url: From('chromium_deps', 'src/testing/gtest') should_process: True requirements: set(['testing', 'chromium_deps', './']) name: third_party/libwebp url: http://src.chromium.org/svn/trunk/src/third_party/libwebp@99393 should_process: True requirements: set(['third_party', './']) name: googleurl url: From('chromium_deps', 'src/googleurl') should_process: True requirements: set(['chromium_deps', './']) name: third_party/skia/include url: From('chromium_deps', 'src/third_party/skia/include') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: third_party/ots url: From('chromium_deps', 'src/third_party/ots') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: third_party/snappy/src url: From('chromium_deps', 'src/third_party/snappy/src') should_process: True requirements: set(['third_party', 'chromium_deps', './']) Died at Tools/Scripts/update-webkit-chromium line 80. No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style. Created attachment 106448 [details]
Patch
Created attachment 106455 [details]
Patch
Created attachment 106465 [details]
Patch
+aroben for the windows def file Comment on attachment 106465 [details]
Patch
.def changes look good to me!
Can I haz review? Comment on attachment 106465 [details]
Patch
You probably want to add
if (!window.internals)
return;
to your layout tests. Other than that, looks fine to me, but I'm not a reviewer.
(In reply to comment #23) > (From update of attachment 106465 [details]) > You probably want to add > > if (!window.internals) > return; > > to your layout tests. Other than that, looks fine to me, but I'm not a reviewer. I'm just curious - does this really matter? I don't mind adding it if it's desired, but it goes against "keep layout tests as small as possible" and doesn't really add anything useful imho (i.e. the tests aren't going to work outside of the test infrastructure anyway - does it matter that they throw a JS exception in that case?). Though, perhaps there's something I'm missing - in which case, let me know and I'll add these. (In reply to comment #24) > (In reply to comment #23) > > (From update of attachment 106465 [details] [details]) > > You probably want to add > > > > if (!window.internals) > > return; > > > > to your layout tests. Other than that, looks fine to me, but I'm not a reviewer. > > I'm just curious - does this really matter? > > I don't mind adding it if it's desired, but it goes against "keep layout tests as small as possible" and doesn't really add anything useful imho (i.e. the tests aren't going to work outside of the test infrastructure anyway - does it matter that they throw a JS exception in that case?). > > Though, perhaps there's something I'm missing - in which case, let me know and I'll add these. Yes! We should add the checks. The general expectation in LayoutTests directory is that the tests should run standalone. If the harness plumbing is required, they should warn you. Created attachment 106605 [details]
Patch
Comment on attachment 106605 [details]
Patch
I have doubts about whether this would be better as a Chromium DRT method (since no other ports are implementing rubber-banding), but it certainly doesn't appear to stick out of WebCore enough to justify that.
(In reply to comment #27) > (From update of attachment 106605 [details]) > I have doubts about whether this would be better as a Chromium DRT method (since no other ports are implementing rubber-banding) Apple's Mac port implements rubber-banding. Comment on attachment 106605 [details] Patch Clearing flags on attachment: 106605 Committed r94705: <http://trac.webkit.org/changeset/94705> All reviewed patches have been landed. Closing bug. The Internals::setScrollViewPosition() added by this patch is poorly named. The position of the scroll view is its top left corner. This is more about the scroll position (sometimes confusingly called the scroll offset). |