Bug 67511 - Add test infrastructure to test rubber-banding overhang drawing along with layout tests for existing Chromium Mac overhang drawing in the non-gpu path.
Summary: Add test infrastructure to test rubber-banding overhang drawing along with la...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 66969
  Show dependency treegraph
 
Reported: 2011-09-02 11:44 PDT by asvitkine
Modified: 2013-02-19 15:58 PST (History)
11 users (show)

See Also:


Attachments
Patch (297.07 KB, patch)
2011-09-02 11:45 PDT, asvitkine
no flags Details | Formatted Diff | Diff
Patch (297.50 KB, patch)
2011-09-02 13:20 PDT, asvitkine
no flags Details | Formatted Diff | Diff
Patch (297.71 KB, patch)
2011-09-06 07:20 PDT, asvitkine
no flags Details | Formatted Diff | Diff
Patch (298.69 KB, patch)
2011-09-06 08:29 PDT, asvitkine
no flags Details | Formatted Diff | Diff
Patch (299.26 KB, patch)
2011-09-06 10:02 PDT, asvitkine
no flags Details | Formatted Diff | Diff
Patch (299.27 KB, patch)
2011-09-06 10:32 PDT, asvitkine
no flags Details | Formatted Diff | Diff
Patch (296.21 KB, patch)
2011-09-06 11:19 PDT, asvitkine
no flags Details | Formatted Diff | Diff
Patch (298.50 KB, patch)
2011-09-06 11:44 PDT, asvitkine
no flags Details | Formatted Diff | Diff
Patch (298.93 KB, patch)
2011-09-06 12:26 PDT, asvitkine
no flags Details | Formatted Diff | Diff
Patch (301.36 KB, patch)
2011-09-07 11:20 PDT, asvitkine
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description asvitkine 2011-09-02 11:44:25 PDT
Add test infrastructure to test rubber-banding overhang drawing along with layout tests for existing Chromium Mac overhang drawing in the non-gpu path.
Comment 1 asvitkine 2011-09-02 11:45:57 PDT
Created attachment 106167 [details]
Patch
Comment 2 Nico Weber 2011-09-02 12:10:57 PDT
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.
Comment 3 asvitkine 2011-09-02 13:06:26 PDT
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 4 WebKit Review Bot 2011-09-02 13:08:00 PDT
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
Comment 5 asvitkine 2011-09-02 13:10:08 PDT
(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...
Comment 6 asvitkine 2011-09-02 13:20:46 PDT
Created attachment 106182 [details]
Patch
Comment 7 asvitkine 2011-09-02 13:21:13 PDT
Updated patch that should hopefully prevent other platforms from trying to run the tests.
Comment 8 Collabora GTK+ EWS bot 2011-09-02 22:19:41 PDT
Comment on attachment 106182 [details]
Patch

Attachment 106182 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9587192
Comment 9 asvitkine 2011-09-06 07:20:19 PDT
Created attachment 106415 [details]
Patch
Comment 10 asvitkine 2011-09-06 07:20:45 PDT
Trying to get the mangled exports right...
Comment 11 asvitkine 2011-09-06 08:29:00 PDT
Created attachment 106422 [details]
Patch
Comment 12 Gustavo Noronha (kov) 2011-09-06 08:48:42 PDT
Comment on attachment 106422 [details]
Patch

Attachment 106422 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9597123
Comment 13 asvitkine 2011-09-06 10:02:32 PDT
Created attachment 106433 [details]
Patch
Comment 14 WebKit Review Bot 2011-09-06 10:05:10 PDT
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.
Comment 15 asvitkine 2011-09-06 10:32:34 PDT
Created attachment 106440 [details]
Patch
Comment 16 WebKit Review Bot 2011-09-06 10:34:42 PDT
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.
Comment 17 asvitkine 2011-09-06 11:19:27 PDT
Created attachment 106448 [details]
Patch
Comment 18 asvitkine 2011-09-06 11:44:04 PDT
Created attachment 106455 [details]
Patch
Comment 19 asvitkine 2011-09-06 12:26:47 PDT
Created attachment 106465 [details]
Patch
Comment 20 Nico Weber 2011-09-06 13:40:29 PDT
+aroben for the windows def file
Comment 21 Adam Roben (:aroben) 2011-09-06 13:41:35 PDT
Comment on attachment 106465 [details]
Patch

.def changes look good to me!
Comment 22 asvitkine 2011-09-06 15:39:07 PDT
Can I haz review?
Comment 23 Nico Weber 2011-09-06 15:50:18 PDT
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.
Comment 24 asvitkine 2011-09-07 07:00:15 PDT
(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.
Comment 25 Dimitri Glazkov (Google) 2011-09-07 10:46:55 PDT
(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.
Comment 26 asvitkine 2011-09-07 11:20:51 PDT
Created attachment 106605 [details]
Patch
Comment 27 Dimitri Glazkov (Google) 2011-09-07 11:31:12 PDT
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.
Comment 28 Adam Roben (:aroben) 2011-09-07 11:37:19 PDT
(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 29 WebKit Review Bot 2011-09-07 13:12:41 PDT
Comment on attachment 106605 [details]
Patch

Clearing flags on attachment: 106605

Committed r94705: <http://trac.webkit.org/changeset/94705>
Comment 30 WebKit Review Bot 2011-09-07 13:12:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Simon Fraser (smfr) 2013-02-19 15:58:37 PST
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).