Bug 66226 - Chromium Mac: Rubber banding gutter drawing
Summary: Chromium Mac: Rubber banding gutter drawing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-15 07:44 PDT by asvitkine
Modified: 2011-08-16 13:01 PDT (History)
9 users (show)

See Also:


Attachments
Patch (13.39 KB, patch)
2011-08-15 07:53 PDT, asvitkine
no flags Details | Formatted Diff | Diff
Patch (14.18 KB, patch)
2011-08-15 12:10 PDT, asvitkine
no flags Details | Formatted Diff | Diff
Patch (14.15 KB, patch)
2011-08-15 12:26 PDT, asvitkine
no flags Details | Formatted Diff | Diff
Patch (14.21 KB, patch)
2011-08-16 10:25 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-08-15 07:44:59 PDT
This bug is to track the work to do the rubber banding gutter drawing in Webkit on Chromium Mac.
Comment 1 asvitkine 2011-08-15 07:53:27 PDT
Created attachment 103914 [details]
Patch
Comment 2 Sailesh Agrawal 2011-08-15 08:57:47 PDT
Comment on attachment 103914 [details]
Patch

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

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.h:-77
> -    void preferencesChanged();

I think moving this up makes sense but it's probably better not to do these kind of changes until ScrollbarThemeChromiumMac.* and ScrollbarThemeMac.* are merged back together (hopefully soon).
Alternatively, you could make this change in both files.

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:202
> +                NSData* tiffData =  [image TIFFRepresentation];

I don't think this is a good way convert to CGImage. It's much better to create a CGBitmap and draw into it.

Also, if all you need to do is fill a rect with a patter than you don't even need a CGImage. You can use the NSImage directly. See LocalCurrentGraphicsContext.
Comment 3 WebKit Review Bot 2011-08-15 11:15:59 PDT
Attachment 103914 [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]
Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Nico Weber 2011-08-15 11:25:52 PDT
Do you know where WebKit2 does this?
Comment 5 asvitkine 2011-08-15 11:29:48 PDT
(In reply to comment #4)
> Do you know where WebKit2 does this?

In ScrollView::paintOverhangAreas().

In the public source, it paints it white - which you can confirm by playing with MiniBrowser.

My patch makes ScrollView::paintOverhangAreas() delegate the painting to the native ScrollBarTheme, which can then vary by platform.
Comment 6 asvitkine 2011-08-15 12:08:47 PDT
(In reply to comment #2)
> (From update of attachment 103914 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103914&action=review
> 
> > Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.h:-77
> > -    void preferencesChanged();
> 
> I think moving this up makes sense but it's probably better not to do these kind of changes until ScrollbarThemeChromiumMac.* and ScrollbarThemeMac.* are merged back together (hopefully soon).
> Alternatively, you could make this change in both files.
> 
> > Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:202
> > +                NSData* tiffData =  [image TIFFRepresentation];
> 
> I don't think this is a good way convert to CGImage. It's much better to create a CGBitmap and draw into it.

Doing it the way you suggest requires more code and appears to be slower in this case (perhaps the TIFFRepresentation data is already available for this image).

The CGBitmap code consistently takes 6ms to execute compared to the TIFFRepresentation path, which takes 5ms consistently.

FWIW, I tried it using this code:

http://codesearch.google.com/#OAMlx_jo-ck/src/base/mac/mac_util.mm&exact_package=chromium&q=CopyNSImageToCGImage&type=cs&l=382

> 
> Also, if all you need to do is fill a rect with a patter than you don't even need a CGImage. You can use the NSImage directly. See LocalCurrentGraphicsContext.

The current code has the advantage that the drawing code is not Mac specific - so for example it could be easily re-used for other platforms if desired (they just need to provide a different Pattern).
Comment 7 asvitkine 2011-08-15 12:10:19 PDT
Created attachment 103939 [details]
Patch
Comment 8 asvitkine 2011-08-15 12:10:43 PDT
(In reply to comment #2)
> (From update of attachment 103914 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103914&action=review
> 
> > Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.h:-77
> > -    void preferencesChanged();
> 
> I think moving this up makes sense but it's probably better not to do these kind of changes until ScrollbarThemeChromiumMac.* and ScrollbarThemeMac.* are merged back together (hopefully soon).
> Alternatively, you could make this change in both files.

Done.
Comment 9 Nico Weber 2011-08-15 12:17:36 PDT
Comment on attachment 103939 [details]
Patch

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

Looks pretty good overall!

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:684
> +    const unsigned kNumShadowColors = sizeof(kShadowColors)/sizeof(kShadowColors[0]);

WTF_ARRAY_LENGTH

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:712
> +            gradient = Gradient::create(FloatPoint(0, shadowRect.maxY()), FloatPoint(0, shadowRect.y()));

Is there any point in caching the gradients? (Do other client usually do this?)
Comment 10 Nico Weber 2011-08-15 12:20:09 PDT
bdakin, this shouldn't cause problems for Safari, right?
Comment 11 asvitkine 2011-08-15 12:26:59 PDT
Created attachment 103942 [details]
Patch
Comment 12 asvitkine 2011-08-15 12:33:12 PDT
> > Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:684
> > +    const unsigned kNumShadowColors = sizeof(kShadowColors)/sizeof(kShadowColors[0]);
> 
> WTF_ARRAY_LENGTH

Thanks! Done.

> > Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:712
> > +            gradient = Gradient::create(FloatPoint(0, shadowRect.maxY()), FloatPoint(0, shadowRect.y()));
> 
> Is there any point in caching the gradients? (Do other client usually do this?)

I'm not sure we can, since the gradient properties are dynamic.
Comment 13 Beth Dakin 2011-08-15 13:16:50 PDT
(In reply to comment #10)
> bdakin, this shouldn't cause problems for Safari, right?

I don't think it will. Looks okay.

For what it's worth, I am not crazy about the idea of moving paintOverhangAreas() to ScrollbarTheme. The behavior happened to be introduced in concert with a new scrollbar theme on Lion, but it strikes me as a view-level concept and not something necessarily inherently tied to the ScrollbarTheme.
Comment 14 asvitkine 2011-08-15 14:08:36 PDT
> For what it's worth, I am not crazy about the idea of moving paintOverhangAreas() to ScrollbarTheme. The behavior happened to be introduced in concert with a new scrollbar theme on Lion, but it strikes me as a view-level concept and not something necessarily inherently tied to the ScrollbarTheme.

Do you have a suggestion for a better place for this?
Comment 15 asvitkine 2011-08-16 08:48:41 PDT
(In reply to comment #14)
> > For what it's worth, I am not crazy about the idea of moving paintOverhangAreas() to ScrollbarTheme. The behavior happened to be introduced in concert with a new scrollbar theme on Lion, but it strikes me as a view-level concept and not something necessarily inherently tied to the ScrollbarTheme.
> 
> Do you have a suggestion for a better place for this?

If not, is there anything else I need to address before this can be approved?
Comment 16 Nico Weber 2011-08-16 08:51:17 PDT
I think this looks good. cc'd reviewers (mihaip, dglazkov; bdakin is probably not comfortable r+ing webkit/chromium code :-) ): can you do the honors?
Comment 17 WebKit Review Bot 2011-08-16 09:34:18 PDT
Comment on attachment 103942 [details]
Patch

Clearing flags on attachment: 103942

Committed r93114: <http://trac.webkit.org/changeset/93114>
Comment 18 WebKit Review Bot 2011-08-16 09:34:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Tony Chang 2011-08-16 09:57:51 PDT
Reverted r93114 for reason:

broke

Committed r93120: <http://trac.webkit.org/changeset/93120>
Comment 20 asvitkine 2011-08-16 10:02:21 PDT
(In reply to comment #19)
> Reverted r93114 for reason:
> 
> broke
> 
> Committed r93120: <http://trac.webkit.org/changeset/93120>

Link to build failure:

http://build.webkit.org/builders/Chromium%20Mac%20Release/builds/27832/steps/compile-webkit/logs/stdio
Comment 21 asvitkine 2011-08-16 10:25:39 PDT
Created attachment 104065 [details]
Patch
Comment 22 asvitkine 2011-08-16 10:27:53 PDT
Apparently WTF_ARRAY_LENGTH() doesn't work for arrays of structs declared within the function.

I've reverted to the patch to not use WTF_ARRAY_LENGTH().
Comment 23 WebKit Review Bot 2011-08-16 10:43:28 PDT
Comment on attachment 104065 [details]
Patch

Rejecting attachment 104065 [details] from commit-queue.

asvitkine@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 24 WebKit Review Bot 2011-08-16 11:45:48 PDT
Comment on attachment 104065 [details]
Patch

Clearing flags on attachment: 104065

Committed r93136: <http://trac.webkit.org/changeset/93136>
Comment 25 WebKit Review Bot 2011-08-16 11:45:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Tony Chang 2011-08-16 12:58:32 PDT
Is it possible that this regressed
  fast/repaint/background-scaling.html
  fast/repaint/scale-page-shrink.html
?

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Frepaint%2Fbackground-scaling.html%2Cfast%2Frepaint%2Fscale-page-shrink.html

It looks like we're not repainting when we should?  Or maybe I'm reading the results incorrectly.
Comment 27 asvitkine 2011-08-16 13:01:37 PDT
I don't see how it could have affected them in any way.