Bug 66226 – Chromium Mac: Rubber banding gutter drawing

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

This bug is to track the work to do the rubber banding gutter drawing in Webkit on Chromium Mac.

Created attachment 103914 [details]
Patch

[Sailesh Agrawal]

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.

[WebKit Review Bot]

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.

[Nico Weber]

Do you know where WebKit2 does this?

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

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

Created attachment 103939 [details]
Patch

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

[Nico Weber]

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

[Nico Weber]

bdakin, this shouldn't cause problems for Safari, right?

Created attachment 103942 [details]
Patch

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

[Beth Dakin]

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

> 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?

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

[Nico Weber]

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?

[WebKit Review Bot]

Comment on attachment 103942 [details]
Patch

Clearing flags on attachment: 103942

Committed r93114: <http://trac.webkit.org/changeset/93114>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

Reverted r93114 for reason:

broke

Committed r93120: <http://trac.webkit.org/changeset/93120>

(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

Created attachment 104065 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 104065 [details]
Patch

Rejecting attachment 104065 [details] from commit-queue.

[e-mail] 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.

[WebKit Review Bot]

Comment on attachment 104065 [details]
Patch

Clearing flags on attachment: 104065

Committed r93136: <http://trac.webkit.org/changeset/93136>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

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.

I don't see how it could have affected them in any way.