Bug 66707

Summary: Chromium Mac: Use a custom pattern image for rubber banding overhang area
Product: WebKit Reporter: asvitkine
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, jamesr, mark, sail, thakis, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 66828    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description asvitkine 2011-08-22 13:26:31 PDT
Chromium Mac: Use a custom pattern image for rubber banding overhang area
Comment 1 asvitkine 2011-08-22 13:28:25 PDT
Created attachment 104728 [details]
Patch
Comment 2 asvitkine 2011-08-22 13:50:14 PDT
Created attachment 104731 [details]
Patch
Comment 3 WebKit Review Bot 2011-08-22 13:51:39 PDT
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.
Comment 4 Sailesh Agrawal 2011-08-22 13:54:02 PDT
You'll probably want to add a webkit reviewer. The changes look good to me.
Comment 5 asvitkine 2011-08-22 13:55:51 PDT
Note: This depends on http://codereview.chromium.org/7711007/
Comment 6 Nico Weber 2011-08-23 00:53:24 PDT
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.
Comment 7 asvitkine 2011-08-23 07:19:14 PDT
Created attachment 104843 [details]
Patch
Comment 8 asvitkine 2011-08-23 07:20:55 PDT
(In reply to comment #7)
> Created an attachment (id=104843) [details]
> Patch

I've addressed Nico's concerns from comment 6.
Comment 9 asvitkine 2011-08-23 13:31:07 PDT
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?
Comment 10 WebKit Review Bot 2011-08-23 15:59:47 PDT
Comment on attachment 104843 [details]
Patch

Clearing flags on attachment: 104843

Committed r93645: <http://trac.webkit.org/changeset/93645>
Comment 11 WebKit Review Bot 2011-08-23 15:59:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Dimitri Glazkov (Google) 2011-08-23 18:01:02 PDT
(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.
Comment 14 James Robinson 2011-08-23 18:01:51 PDT
Reopening, reverted in http://trac.webkit.org/changeset/93676
Comment 15 asvitkine 2011-08-24 08:03:24 PDT
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/
Comment 16 asvitkine 2011-08-25 10:37:49 PDT
Created attachment 105212 [details]
Patch
Comment 17 asvitkine 2011-08-25 10:38:27 PDT
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.
Comment 18 WebKit Review Bot 2011-08-25 10:39:32 PDT
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.
Comment 19 asvitkine 2011-08-25 10:52:44 PDT
Created attachment 105219 [details]
Patch
Comment 20 asvitkine 2011-08-26 07:23:30 PDT
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.
Comment 21 Dimitri Glazkov (Google) 2011-08-26 07:50:02 PDT
Comment on attachment 105219 [details]
Patch

oooh, look, donut!
Comment 22 WebKit Review Bot 2011-08-26 08:29:16 PDT
Comment on attachment 105219 [details]
Patch

Clearing flags on attachment: 105219

Committed r93879: <http://trac.webkit.org/changeset/93879>
Comment 23 WebKit Review Bot 2011-08-26 08:29:21 PDT
All reviewed patches have been landed.  Closing bug.