Bug 66707 – Chromium Mac: Use a custom pattern image for rubber banding overhang area

• Component: New Bugs
• Status: RESOLVED FIXED

Chromium Mac: Use a custom pattern image for rubber banding overhang area

Created attachment 104728 [details]
Patch

Created attachment 104731 [details]
Patch

[WebKit Review Bot]

Attachment 104731 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Last 3072 characters of output:
rements: ['./']
  
    name: third_party/ffmpeg
    url: From('chromium_deps', 'src/third_party/ffmpeg')
    parsed_url: http://src.chromium.org/svn/trunk/deps/third_party/ffmpeg/source@97428
    should_process: True
    processed: True
    requirements: ['./', 'chromium_deps', 'third_party']
  
    name: tools/generate_stubs
    url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@97698
    parsed_url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@97698
    should_process: True
    processed: True
    requirements: ['./']
  
    name: third_party/libjpeg_turbo
    url: From('chromium_deps', 'src/third_party/libjpeg_turbo')
    parsed_url: http://src.chromium.org/svn/trunk/deps/third_party/libjpeg_turbo@95800
    should_process: True
    processed: True
    requirements: ['./', 'chromium_deps', 'third_party']
  
    name: third_party/v8-i18n
    url: From('chromium_deps', 'src/third_party/v8-i18n')
    parsed_url: http://v8-i18n.googlecode.com/svn/trunk@4
    should_process: True
    processed: True
    requirements: ['./', 'chromium_deps', 'third_party']
  
    name: tools/grit
    url: http://src.chromium.org/svn/trunk/src/tools/grit@97698
    parsed_url: http://src.chromium.org/svn/trunk/src/tools/grit@97698
    should_process: True
    processed: True
    requirements: ['./']
  
    name: base
    url: http://src.chromium.org/svn/trunk/src/base@97698
    parsed_url: http://src.chromium.org/svn/trunk/src/base@97698
    should_process: True
    processed: True
    requirements: ['./']
  
    name: third_party/leveldb
    url: From('chromium_deps', 'src/third_party/leveldb')
    should_process: True
    requirements: ['./', 'chromium_deps', 'third_party']
  
    name: sql
    url: http://src.chromium.org/svn/trunk/src/sql@97698
    should_process: True
    requirements: ['./']
  
    name: v8
    url: From('chromium_deps', 'src/v8')
    should_process: True
    requirements: ['./', 'chromium_deps']
  
    name: testing/gtest
    url: From('chromium_deps', 'src/testing/gtest')
    should_process: True
    requirements: ['./', 'chromium_deps', 'testing']
  
    name: third_party/libwebp
    url: http://src.chromium.org/svn/trunk/src/third_party/libwebp@97698
    should_process: True
    requirements: ['./', 'third_party']
  
    name: googleurl
    url: From('chromium_deps', 'src/googleurl')
    should_process: True
    requirements: ['./', 'chromium_deps']
  
    name: third_party/skia/include
    url: From('chromium_deps', 'src/third_party/skia/include')
    should_process: True
    requirements: ['./', 'chromium_deps', 'third_party']
  
    name: third_party/ots
    url: From('chromium_deps', 'src/third_party/ots')
    should_process: True
    requirements: ['./', 'chromium_deps', 'third_party']
  
    name: third_party/snappy/src
    url: From('chromium_deps', 'src/third_party/snappy/src')
    should_process: True
    requirements: ['./', 'chromium_deps', 'third_party']

Died at Tools/Scripts/update-webkit-chromium line 69.
No such file or directory at Tools/Scripts/update-webkit line 100.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sailesh Agrawal]

You'll probably want to add a webkit reviewer. The changes look good to me.

Note: This depends on http://codereview.chromium.org/7711007/

[Nico Weber]

Comment on attachment 104731 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104731&action=review

LGTM

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:198
> +        Image* patternImage = Image::loadPlatformResource("overhangPattern").releaseRef();

releaseRef() documentation says "        // FIXME: Remove releaseRef once we change all callers to call leakRef instead.", so do you want to call leakRef() here?

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:199
> +        m_overhangPattern = Pattern::create(patternImage, true, true);

Looks like create() takes a PassRefPtr, so maybe you don't even need the leakRef() call. I'm not sure if http://www.webkit.org/coding/RefPtr.html recommends against that or not.

Created attachment 104843 [details]
Patch

(In reply to comment #7)
> Created an attachment (id=104843) [details]
> Patch

I've addressed Nico's concerns from comment 6.

The Chromium revision on which this depends is now WebKit's DEPS, so this is ready to go in once approved.

Dmitri, can you review/approve the patch, please?

[WebKit Review Bot]

Comment on attachment 104843 [details]
Patch

Clearing flags on attachment: 104843

Committed r93645: <http://trac.webkit.org/changeset/93645>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This broke webkit_unit_tests on mac:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6%20%28CG%29%28dbg%29/builds/174/steps/webkit_unit_tests/logs/stdio

[Dimitri Glazkov (Google)]

(In reply to comment #12)
> This broke webkit_unit_tests on mac:
> 
> http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6%20%28CG%29%28dbg%29/builds/174/steps/webkit_unit_tests/logs/stdio

Ok, I ain't rubberstampin' these no more.

[James Robinson]

Reopening, reverted in http://trac.webkit.org/changeset/93676

Seems resource loading takes a different path when running unit tests.

This patch now depends on the following Chromium CL: http://codereview.chromium.org/7714036/

Created attachment 105212 [details]
Patch

I've updated the patch to include DEPS change to roll the Chromium revision forward to 98101 so that it included the needed Chromium change.

[WebKit Review Bot]

Attachment 105212 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 105219 [details]
Patch

I've confirmed that webkit_unit_tests pass with this patch and that there is no difference in layout tests before and after this patch.

[Dimitri Glazkov (Google)]

Comment on attachment 105219 [details]
Patch

oooh, look, donut!

[WebKit Review Bot]

Comment on attachment 105219 [details]
Patch

Clearing flags on attachment: 105219

Committed r93879: <http://trac.webkit.org/changeset/93879>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.