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.

• Component: New Bugs
• Status: RESOLVED FIXED

Add test infrastructure to test rubber-banding overhang drawing along with layout tests for existing Chromium Mac overhang drawing in the non-gpu path.

Created attachment 106167 [details]
Patch

[Nico Weber]

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.

[WebKit Review Bot]

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.

[Collabora GTK+ EWS bot]

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

[Gustavo Noronha (kov)]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Nico Weber]

+aroben for the windows def file

[Adam Roben (:aroben)]

Comment on attachment 106465 [details]
Patch

.def changes look good to me!

Can I haz review?

[Nico Weber]

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.

[Dimitri Glazkov (Google)]

(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

[Dimitri Glazkov (Google)]

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.

[Adam Roben (:aroben)]

(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.

[WebKit Review Bot]

Comment on attachment 106605 [details]
Patch

Clearing flags on attachment: 106605

Committed r94705: <http://trac.webkit.org/changeset/94705>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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).