Bug 96137

Summary: [chromium] Store the contents scale factor in PlatformContextSkia on initialization
Product: WebKit Reporter: Terry Anderson <tdanderson>
Component: PlatformAssignee: Terry Anderson <tdanderson>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, behdad, cc-bugs, danakj, dglazkov, dpranke, eric, jamesr, peter+ews, rjkroege, senorblanco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 96963    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch
none
Patch none

Description Terry Anderson 2012-09-07 12:44:18 PDT
When initializing a PlatformContextSkia, the scale on |canvas| will be equal to the device scale factor. Set this value as a member on PlatformContextSkia, which will be used to correctly render glyphs when the device scale factor is not 1. The related change can be found here: https://codereview.appspot.com/6495089/
Comment 1 Terry Anderson 2012-09-07 12:48:13 PDT
Created attachment 162847 [details]
Patch
Comment 2 Dana Jansens 2012-09-07 12:55:44 PDT
Comment on attachment 162847 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:191
> +    , m_hintingScaleFactor(canvas ? canvas->getTotalMatrix().getScaleX() : SK_Scalar1)

I think you want to do the same in setCanvas() too. It won't actually affect anything because it's used in ImageBuffer with a canvas that has no scale applied to it. But I dislike the paths being different.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:208
> +    // Returns the scale factor needed for correctly rendering glyphs
> +    // when in high DPI mode with text subpixel positioning disabled.
> +    SkScalar hintingScaleFactor() const { return m_hintingScaleFactor; }

Is this needed with the current approach? You're setting it on the SkPaint, nothing will need to query this will it?
Comment 3 Peter Beverloo (cr-android ews) 2012-09-07 16:19:23 PDT
Comment on attachment 162847 [details]
Patch

Attachment 162847 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13793001
Comment 4 WebKit Review Bot 2012-09-07 17:02:18 PDT
Comment on attachment 162847 [details]
Patch

Attachment 162847 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13774939
Comment 5 Stephen White 2012-09-10 11:47:23 PDT
Comment on attachment 162847 [details]
Patch

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

I'm sure you know this, but this can't land until the skia patch lands, is rolled into chrome DEPS, and then into WebKit via Source/WebKit/chromium/DEPS.

>> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:191
>> +    , m_hintingScaleFactor(canvas ? canvas->getTotalMatrix().getScaleX() : SK_Scalar1)
> 
> I think you want to do the same in setCanvas() too. It won't actually affect anything because it's used in ImageBuffer with a canvas that has no scale applied to it. But I dislike the paths being different.

It seems a little fragile to be depending on the canvas's matrix to contain the scale factor at time of PlatformContextSkia construction.  Is there a place you could point to in WebKit where this scale is set?

It doesn't seem to be the call to 

m_context->scale(FloatSize(m_resolutionScale, m_resolutionScale));

in the ImageBuffer constructor (around line 136) as I first suspected, since that call is made long after the platform context is created.  So what is setting it?  Perhaps we could make an explicit call at that point (or just after) to PlatformContextSkia to explicitly set this value as well?

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:361
> +    paint->setHintingScaleFactor(this->hintingScaleFactor());

Nit:  WebKit code doesn't use this->, only skia code.
Comment 6 Dana Jansens 2012-09-10 12:05:49 PDT
(In reply to comment #5)
> (From update of attachment 162847 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162847&action=review
> 
> I'm sure you know this, but this can't land until the skia patch lands, is rolled into chrome DEPS, and then into WebKit via Source/WebKit/chromium/DEPS.
> 
> >> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:191
> >> +    , m_hintingScaleFactor(canvas ? canvas->getTotalMatrix().getScaleX() : SK_Scalar1)
> > 
> > I think you want to do the same in setCanvas() too. It won't actually affect anything because it's used in ImageBuffer with a canvas that has no scale applied to it. But I dislike the paths being different.
> 
> It seems a little fragile to be depending on the canvas's matrix to contain the scale factor at time of PlatformContextSkia construction.  Is there a place you could point to in WebKit where this scale is set?
> 
> It doesn't seem to be the call to 
> 
> m_context->scale(FloatSize(m_resolutionScale, m_resolutionScale));
> 
> in the ImageBuffer constructor (around line 136) as I first suspected, since that call is made long after the platform context is created.  So what is setting it?  Perhaps we could make an explicit call at that point (or just after) to PlatformContextSkia to explicitly set this value as well?

This change is not actually meant for ImageBuffer. ImageBuffer completely avoids this code because it uses setCanvas() instead of passing a canvas to the constructor of the PlatformContextSkia.

The scale is being set on the canvas by the compositor. It's the scale difference between logical pixels (what webkit will draw/layout in) and the SkDevice physical pixels.

I don't think we want anything to happen necessarily wrt ImageBuffer.  I am not opposed to adding another function to the PlatformContextSkia though for callers to use... to match the current functionality, the compositor would call platformContextSkia.setHintingScaleFactor(foo) but ImageBuffer would not.


> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:361
> > +    paint->setHintingScaleFactor(this->hintingScaleFactor());
> 
> Nit:  WebKit code doesn't use this->, only skia code.

Also, use the variable directly when available please, i.e m_hintingScaleFactor.
Comment 7 Stephen White 2012-09-10 13:34:04 PDT
(In reply to comment #6)
> This change is not actually meant for ImageBuffer. ImageBuffer completely avoids this code because it uses setCanvas() instead of passing a canvas to the constructor of the PlatformContextSkia.
> 
> The scale is being set on the canvas by the compositor. It's the scale difference between logical pixels (what webkit will draw/layout in) and the SkDevice physical pixels.

Ahh, ok.  That makes more sense.  So this is the SkCanvas that is used when software rendering, for upload to textures in the compositor?

> I don't think we want anything to happen necessarily wrt ImageBuffer.  I am not opposed to adding another function to the PlatformContextSkia though for callers to use... to match the current functionality, the compositor would call platformContextSkia.setHintingScaleFactor(foo) but ImageBuffer would not.

I think I would prefer that approach, since it's less magic than automatically deducing it from the canvas's matrix.
Comment 8 Dana Jansens 2012-09-10 13:41:24 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > This change is not actually meant for ImageBuffer. ImageBuffer completely avoids this code because it uses setCanvas() instead of passing a canvas to the constructor of the PlatformContextSkia.
> > 
> > The scale is being set on the canvas by the compositor. It's the scale difference between logical pixels (what webkit will draw/layout in) and the SkDevice physical pixels.
> 
> Ahh, ok.  That makes more sense.  So this is the SkCanvas that is used when software rendering, for upload to textures in the compositor?

Yup!
Comment 9 Terry Anderson 2012-09-10 13:52:07 PDT
This patch has no effect on the pixel tests in fast/hidpi (where the device scale factor is set to 2). When I build DRT with this patch, the image output for these tests still has incorrect spacing between glyphs.
Comment 10 Dana Jansens 2012-09-10 14:00:30 PDT
We'll need https://bugs.webkit.org/show_bug.cgi?id=90192 before we can test this using those tests since this is for the compositor DSF path, and those tests are not using it without bug #90192
Comment 11 Terry Anderson 2012-09-11 09:58:36 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > This change is not actually meant for ImageBuffer. ImageBuffer completely avoids this code because it uses setCanvas() instead of passing a canvas to the constructor of the PlatformContextSkia.
> > 
> > The scale is being set on the canvas by the compositor. It's the scale difference between logical pixels (what webkit will draw/layout in) and the SkDevice physical pixels.
> 
> Ahh, ok.  That makes more sense.  So this is the SkCanvas that is used when software rendering, for upload to textures in the compositor?
> 
> > I don't think we want anything to happen necessarily wrt ImageBuffer.  I am not opposed to adding another function to the PlatformContextSkia though for callers to use... to match the current functionality, the compositor would call platformContextSkia.setHintingScaleFactor(foo) but ImageBuffer would not.
> 
> I think I would prefer that approach, since it's less magic than automatically deducing it from the canvas's matrix.

In this case we'd still be getting the scale factor from the canvas's matrix.  The call to platformContext.setHintingScaleFactor(foo) would be made from OpaqueRectTrackingContentLayerDelegate::paintContents(), which does not have direct access to the device scale factor, so foo would still need to come from |canvas|.
Comment 12 Terry Anderson 2012-09-11 10:54:21 PDT
Created attachment 163401 [details]
Patch
Comment 13 WebKit Review Bot 2012-09-11 11:26:34 PDT
Comment on attachment 163401 [details]
Patch

Attachment 163401 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13819415
Comment 14 Peter Beverloo (cr-android ews) 2012-09-11 12:16:16 PDT
Comment on attachment 163401 [details]
Patch

Attachment 163401 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13833036
Comment 15 Stephen White 2012-09-11 15:08:56 PDT
(In reply to comment #11)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > This change is not actually meant for ImageBuffer. ImageBuffer completely avoids this code because it uses setCanvas() instead of passing a canvas to the constructor of the PlatformContextSkia.
> > > 
> > > The scale is being set on the canvas by the compositor. It's the scale difference between logical pixels (what webkit will draw/layout in) and the SkDevice physical pixels.
> > 
> > Ahh, ok.  That makes more sense.  So this is the SkCanvas that is used when software rendering, for upload to textures in the compositor?
> > 
> > > I don't think we want anything to happen necessarily wrt ImageBuffer.  I am not opposed to adding another function to the PlatformContextSkia though for callers to use... to match the current functionality, the compositor would call platformContextSkia.setHintingScaleFactor(foo) but ImageBuffer would not.
> > 
> > I think I would prefer that approach, since it's less magic than automatically deducing it from the canvas's matrix.
> 
> In this case we'd still be getting the scale factor from the canvas's matrix.  The call to platformContext.setHintingScaleFactor(foo) would be made from OpaqueRectTrackingContentLayerDelegate::paintContents(), which does not have direct access to the device scale factor, so foo would still need to come from |canvas|.

I'd still like to know who is setting the scale, and if necessary, set this hinting factor at the same time.

Is it possible it's the one set in CanvasLayerTextureUpdater::paintContents()?    It does a context.scale(contentsScale, contentsScale), but that seems to be the concatenation of device scale and page scale.
Comment 16 Stephen White 2012-09-11 15:10:36 PDT
Comment on attachment 163401 [details]
Patch

This should probably be set over in Chrome's skia.gyp, and brought in with the skia roll (when you roll WebKit chromium DEPS to bring in the new skia).
Comment 17 Stephen White 2012-09-11 15:12:04 PDT
Comment on attachment 163401 [details]
Patch

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

> Source/WebKit/chromium/features.gypi:121
> +      'SK_SUPPORT_HINTING_SCALE_FACTOR',

This should not be set here, but rather over in Chrome's skia.gyp, and brought in with the WebKit chromium DEPS roll that brings in the new rev of Skia.
Comment 18 Terry Anderson 2012-09-11 15:15:27 PDT
(In reply to comment #17)
> (From update of attachment 163401 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163401&action=review
> 
> > Source/WebKit/chromium/features.gypi:121
> > +      'SK_SUPPORT_HINTING_SCALE_FACTOR',
> 
> This should not be set here, but rather over in Chrome's skia.gyp, and brought in with the WebKit chromium DEPS roll that brings in the new rev of Skia.

I am also going to be including it in src/third_party/skia/skia.gyp along with the skia patch. I am including it in features.gypi too because omitting it gave compilation errors, so I assumed it was required in both files.
Comment 19 Dana Jansens 2012-09-11 17:18:26 PDT
(In reply to comment #15)
> (In reply to comment #11)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > This change is not actually meant for ImageBuffer. ImageBuffer completely avoids this code because it uses setCanvas() instead of passing a canvas to the constructor of the PlatformContextSkia.
> > > > 
> > > > The scale is being set on the canvas by the compositor. It's the scale difference between logical pixels (what webkit will draw/layout in) and the SkDevice physical pixels.
> > > 
> > > Ahh, ok.  That makes more sense.  So this is the SkCanvas that is used when software rendering, for upload to textures in the compositor?
> > > 
> > > > I don't think we want anything to happen necessarily wrt ImageBuffer.  I am not opposed to adding another function to the PlatformContextSkia though for callers to use... to match the current functionality, the compositor would call platformContextSkia.setHintingScaleFactor(foo) but ImageBuffer would not.
> > > 
> > > I think I would prefer that approach, since it's less magic than automatically deducing it from the canvas's matrix.
> > 
> > In this case we'd still be getting the scale factor from the canvas's matrix.  The call to platformContext.setHintingScaleFactor(foo) would be made from OpaqueRectTrackingContentLayerDelegate::paintContents(), which does not have direct access to the device scale factor, so foo would still need to come from |canvas|.
> 
> I'd still like to know who is setting the scale, and if necessary, set this hinting factor at the same time.
> 
> Is it possible it's the one set in CanvasLayerTextureUpdater::paintContents()?    It does a context.scale(contentsScale, contentsScale), but that seems to be the concatenation of device scale and page scale.

That is exactly the scale it is pulling out of the matrix. We could pass the contentsWidthScale and contentsHeightScale along into the paint delegate, but they are already in the matrix so it seemed unnecessary to me at the time. Compositor always sets the scale to exactly the scale between layout and real pixels which is what the font hinter wants.

This bug is a bit misleading to say "device scale factor". It should say contents scale.
Comment 20 Stephen White 2012-09-12 08:49:31 PDT
(In reply to comment #19)
> (In reply to comment #15)
> > (In reply to comment #11)
> > > (In reply to comment #7)
> > > > (In reply to comment #6)
> > > > > This change is not actually meant for ImageBuffer. ImageBuffer completely avoids this code because it uses setCanvas() instead of passing a canvas to the constructor of the PlatformContextSkia.
> > > > > 
> > > > > The scale is being set on the canvas by the compositor. It's the scale difference between logical pixels (what webkit will draw/layout in) and the SkDevice physical pixels.
> > > > 
> > > > Ahh, ok.  That makes more sense.  So this is the SkCanvas that is used when software rendering, for upload to textures in the compositor?
> > > > 
> > > > > I don't think we want anything to happen necessarily wrt ImageBuffer.  I am not opposed to adding another function to the PlatformContextSkia though for callers to use... to match the current functionality, the compositor would call platformContextSkia.setHintingScaleFactor(foo) but ImageBuffer would not.
> > > > 
> > > > I think I would prefer that approach, since it's less magic than automatically deducing it from the canvas's matrix.
> > > 
> > > In this case we'd still be getting the scale factor from the canvas's matrix.  The call to platformContext.setHintingScaleFactor(foo) would be made from OpaqueRectTrackingContentLayerDelegate::paintContents(), which does not have direct access to the device scale factor, so foo would still need to come from |canvas|.
> > 
> > I'd still like to know who is setting the scale, and if necessary, set this hinting factor at the same time.
> > 
> > Is it possible it's the one set in CanvasLayerTextureUpdater::paintContents()?    It does a context.scale(contentsScale, contentsScale), but that seems to be the concatenation of device scale and page scale.
> 
> That is exactly the scale it is pulling out of the matrix. We could pass the contentsWidthScale and contentsHeightScale along into the paint delegate, but they are already in the matrix so it seemed unnecessary to me at the time. Compositor always sets the scale to exactly the scale between layout and real pixels which is what the font hinter wants.

Fair enough, but it doesn't make sense to me to bake that assumption into PlatformContextSkia, which is used by clients other than the compositor.  I'd rather it was explicitly set by the compositor for its particular use.  Does that make sense?

> This bug is a bit misleading to say "device scale factor". It should say contents scale.
Comment 21 Dana Jansens 2012-09-13 13:24:48 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #15)
> > > (In reply to comment #11)
> > > > (In reply to comment #7)
> > > > > (In reply to comment #6)
> > > > > > This change is not actually meant for ImageBuffer. ImageBuffer completely avoids this code because it uses setCanvas() instead of passing a canvas to the constructor of the PlatformContextSkia.
> > > > > > 
> > > > > > The scale is being set on the canvas by the compositor. It's the scale difference between logical pixels (what webkit will draw/layout in) and the SkDevice physical pixels.
> > > > > 
> > > > > Ahh, ok.  That makes more sense.  So this is the SkCanvas that is used when software rendering, for upload to textures in the compositor?
> > > > > 
> > > > > > I don't think we want anything to happen necessarily wrt ImageBuffer.  I am not opposed to adding another function to the PlatformContextSkia though for callers to use... to match the current functionality, the compositor would call platformContextSkia.setHintingScaleFactor(foo) but ImageBuffer would not.
> > > > > 
> > > > > I think I would prefer that approach, since it's less magic than automatically deducing it from the canvas's matrix.
> > > > 
> > > > In this case we'd still be getting the scale factor from the canvas's matrix.  The call to platformContext.setHintingScaleFactor(foo) would be made from OpaqueRectTrackingContentLayerDelegate::paintContents(), which does not have direct access to the device scale factor, so foo would still need to come from |canvas|.
> > > 
> > > I'd still like to know who is setting the scale, and if necessary, set this hinting factor at the same time.
> > > 
> > > Is it possible it's the one set in CanvasLayerTextureUpdater::paintContents()?    It does a context.scale(contentsScale, contentsScale), but that seems to be the concatenation of device scale and page scale.
> > 
> > That is exactly the scale it is pulling out of the matrix. We could pass the contentsWidthScale and contentsHeightScale along into the paint delegate, but they are already in the matrix so it seemed unnecessary to me at the time. Compositor always sets the scale to exactly the scale between layout and real pixels which is what the font hinter wants.
> 
> Fair enough, but it doesn't make sense to me to bake that assumption into PlatformContextSkia, which is used by clients other than the compositor.  I'd rather it was explicitly set by the compositor for its particular use.  Does that make sense?

Perfectly!
Comment 22 Terry Anderson 2012-09-13 14:12:21 PDT
Created attachment 163962 [details]
Patch
Comment 23 Terry Anderson 2012-09-13 14:22:17 PDT
(In reply to comment #22)
> Created an attachment (id=163962) [details]
> Patch

The scale is now set by the compositor after creating a PlatformContextSkia instead of having the PCS constructor do so. Note that |m_hintingScaleFactor| is no longer changed when PlatformContextSkia::setCanvas() is called, since this would require changing the value for all clients of PCS. danajk@ or senorblanco@, would either of you like to see this put back in?
Comment 24 Stephen White 2012-09-13 14:24:20 PDT
Comment on attachment 163962 [details]
Patch

This looks much better to me.  Thanks for the extra effort.  As discussed, once the the skia patch is landed in chrome, please bump the chrome revision in Source/WebKit/Chromium/DEPS past the skia roll, and resubmit this patch with that DEPS roll.  Hopefully then we'll see green bots.
Comment 25 Stephen White 2012-09-13 14:24:53 PDT
(In reply to comment #23)
> (In reply to comment #22)
> > Created an attachment (id=163962) [details] [details]
> > Patch
> 
> The scale is now set by the compositor after creating a PlatformContextSkia instead of having the PCS constructor do so. Note that |m_hintingScaleFactor| is no longer changed when PlatformContextSkia::setCanvas() is called, since this would require changing the value for all clients of PCS. danajk@ or senorblanco@, would either of you like to see this put back in?

I prefer it the way you have it now.  Dana?
Comment 26 Dana Jansens 2012-09-13 14:25:12 PDT
I was also going to say this is much nicer. :) The caller of setCanvas would need to call setHintingScaleFactor() as well if they wanted to use it.
Comment 27 WebKit Review Bot 2012-09-13 14:43:38 PDT
Comment on attachment 163962 [details]
Patch

Attachment 163962 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13839664
Comment 28 Peter Beverloo (cr-android ews) 2012-09-13 15:55:43 PDT
Comment on attachment 163962 [details]
Patch

Attachment 163962 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13844385
Comment 29 Stephen White 2012-09-17 11:17:39 PDT
Comment on attachment 163962 [details]
Patch

This looks good to me, once the other relevant change has landed, and relevant test suppressions added.
Comment 30 Terry Anderson 2012-09-17 13:54:40 PDT
Created attachment 164451 [details]
Patch
Comment 31 WebKit Review Bot 2012-09-17 14:11:29 PDT
Comment on attachment 164451 [details]
Patch

Attachment 164451 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13889020
Comment 32 Peter Beverloo (cr-android ews) 2012-09-17 14:29:00 PDT
Comment on attachment 164451 [details]
Patch

Attachment 164451 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13876447
Comment 33 Terry Anderson 2012-09-18 09:30:31 PDT
Created attachment 164577 [details]
Patch
Comment 34 Terry Anderson 2012-09-18 15:56:48 PDT
Created attachment 164633 [details]
Patch
Comment 35 Dana Jansens 2012-09-18 16:21:11 PDT
Comment on attachment 164633 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:192
> +    , m_hintingScaleFactor(SK_Scalar1)

nit: Can this just be set to "1"?
Comment 36 Stephen White 2012-09-18 17:58:44 PDT
Comment on attachment 164633 [details]
Patch

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

Looks good.  r=me

>> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:192
>> +    , m_hintingScaleFactor(SK_Scalar1)
> 
> nit: Can this just be set to "1"?

For SkScalars, SK_Scalar1 is correct (just in case we decide to compile with SK_SCALAR_IS_FIXED someday, for example).
Comment 37 WebKit Review Bot 2012-09-18 18:14:10 PDT
Comment on attachment 164633 [details]
Patch

Rejecting attachment 164633 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
skia/PlatformContextSkia.h
patching file Source/WebKit/chromium/features.gypi
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/chromium/TestExpectations
Hunk #1 FAILED at 2652.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/TestExpectations.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Stephen Wh..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13905273
Comment 38 Terry Anderson 2012-09-19 07:48:01 PDT
Created attachment 164739 [details]
Patch for landing
Comment 39 Terry Anderson 2012-09-19 12:49:50 PDT
Created attachment 164761 [details]
Patch
Comment 40 Terry Anderson 2012-09-19 12:50:27 PDT
Comment on attachment 164761 [details]
Patch

Added two unit tests.
Comment 41 WebKit Review Bot 2012-09-19 12:51:53 PDT
Attachment 164761 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
LayoutTests/platform/chromium/TestExpectations:3032:  A test can not be both SLOW and TIMEOUT. If it times out indefinitely, then it should be just TIMEOUT.  [test/expectations] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 42 Dana Jansens 2012-09-19 12:56:05 PDT
Comment on attachment 164761 [details]
Patch

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

Thanks so much for the tests! Do they need to be inside ifdefs like the implementation is?

> Source/WebKit/chromium/tests/OpaqueRectTrackingContentLayerDelegateTest.cpp:110
> +    SkScalar m_hintingScale;

nit: public members of structs don't use m_ prefix.

Should add a constructor that sets this to some initial value so you don't have possibility of flaky results.

> Source/WebKit/chromium/tests/OpaqueRectTrackingContentLayerDelegateTest.cpp:221
> +    EXPECT_EQ(callback.m_hintingScale, hintingScale);

Can you test the callback's hintingScale before calling paintContents so we can be sure it's responsible?

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:845
> +    EXPECT_EQ(hintingScale, paint.getHintingScaleFactor());

Can you test the paint's hinting scale factor before calling setupPaintCommon too, to make us sure the function is responsible?
Comment 43 Dana Jansens 2012-09-19 12:57:34 PDT
Comment on attachment 164761 [details]
Patch

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

> LayoutTests/platform/chromium/TestExpectations:2617
> +// Flaky tests from ~r97647
> +BUGWK70298 MAC : fast/table/border-collapsing/cached-69296.html = IMAGE

Did you mean to include this or is this from a rebase?
Comment 44 James Robinson 2012-09-19 13:05:22 PDT
Hey Dirk - what does this mean:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.

? We should have skia_test_expectations.txt checked out in Source/WebKit/chromium/skia/ - does the script just not know to look for that file or do we not want to use it in this configuraiton?
Comment 45 Dirk Pranke 2012-09-19 13:14:47 PDT
(In reply to comment #44)
> Hey Dirk - what does this mean:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
> WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
> 
> ? We should have skia_test_expectations.txt checked out in Source/WebKit/chromium/skia/ - does the script just not know to look for that file or do we not want to use it in this configuraiton?

It means what it says ... skia_test_expectations.txt doesn't exist in that checkout. I suspect that you're wrong and we don't actually have a chromium checkout on the style bots. Adam/Eric, can you confirm this? I don't know how to look at what's actually checked out on the bot or how to see the update commands.
Comment 46 Adam Barth 2012-09-19 13:21:54 PDT
The style-bot does not use a Chromium checkout.  check-webkit-style should not depend on having DEPS-defined dependencies available.  Most contributos who run check-webkit-style won't have these!
Comment 47 Dirk Pranke 2012-09-19 13:41:03 PDT
(In reply to comment #46)
> The style-bot does not use a Chromium checkout.  check-webkit-style should not depend on having DEPS-defined dependencies available.  Most contributos who run check-webkit-style won't have these!

IIRC, check-webkit-style only checks the changes to the files that are affected, meaning that you would only get this warning if you were changing LayoutTests/platform/chromium/TestExpectations. I would expect most people changing that file to have a chromium checkout, and if they don't, it seems to me the warning will be appropriate more often than not so that they can figure out if they're doing something safe or not.

So that part seems at least partially okay, at least for people running check-webkit-style interactively.

However, the actual style correctness of LayoutTests/platform/chromium/TestExpectations only depends on the contents of that file, and so theoretically it should be okay for the bot to run this w/o a chromium checkout and still produce the same output (minus the warning about skia_test_expectations, of course), and so we could perhaps add a flag to tell the chromium.py code to not produce this warning in this situation.

If we get the warning in check-webkit-style even if that file isn't modified, that's a different problem and should be fixed, but I don't believe that's the case.
Comment 48 Stephen White 2012-09-19 13:43:55 PDT
Comment on attachment 164761 [details]
Patch

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

> Source/WebKit/chromium/tests/OpaqueRectTrackingContentLayerDelegateTest.cpp:213
> +    SkScalar hintingScale = SkFloatToScalar(2);

MicroNit:  this could be SkIntToScalar.

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:838
> +    SkScalar hintingScale = SkFloatToScalar(2);

Same here.

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:840
> +    platformContext.setHintingScaleFactor(hintingScale); 

Should this all (maybe the whole test) be wrapped in #ifdef SK_SUPPORT_HINTING_SCALE_FACTOR?
Comment 49 Terry Anderson 2012-09-19 14:02:58 PDT
(In reply to comment #41)
> Attachment 164761 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
> WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
> LayoutTests/platform/chromium/TestExpectations:3032:  A test can not be both SLOW and TIMEOUT. If it times out indefinitely, then it should be just TIMEOUT.  [test/expectations] [5]
> Total errors found: 1 in 10 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

So if I disregard this error since it is not related to my patch, will the CQ still let the patch land?
Comment 50 Terry Anderson 2012-09-19 14:12:47 PDT
Created attachment 164777 [details]
Patch
Comment 51 Terry Anderson 2012-09-19 14:13:17 PDT
Comment on attachment 164777 [details]
Patch

Comments from danakj@ and senorblanco@ addressed.
Comment 52 Dirk Pranke 2012-09-19 14:26:08 PDT
(In reply to comment #49)
> (In reply to comment #41)
> > Attachment 164761 [details] [details] did not pass style-queue:
> > 
> > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
> > WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
> > LayoutTests/platform/chromium/TestExpectations:3032:  A test can not be both SLOW and TIMEOUT. If it times out indefinitely, then it should be just TIMEOUT.  [test/expectations] [5]
> > Total errors found: 1 in 10 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> So if I disregard this error since it is not related to my patch, will the CQ still let the patch land?

I'm not actually sure. I think japhet fixed line 3032 recently so hopefully this goes away and you don't get the error.
Comment 53 Adam Barth 2012-09-19 14:26:35 PDT
I would not assume that people editing LayoutTests/platform/chromium/TestExpectations have all the DEPS dependencies.  I certainly edit the TestExpectations file of many other ports without being able to build them.  (We should probably start a new bug thread on this topic rather than hijacking this one.)
Comment 54 Dirk Pranke 2012-09-19 14:29:58 PDT
(In reply to comment #53)
> I would not assume that people editing LayoutTests/platform/chromium/TestExpectations have all the DEPS dependencies.  I certainly edit the TestExpectations file of many other ports without being able to build them.  (We should probably start a new bug thread on this topic rather than hijacking this one.)

True, and I don't assume that, and we don't require it. However, I think it's fair (and good) to warn in such cases, since their edits may not have the effect they expect them to.
Comment 55 Stephen White 2012-09-19 14:32:06 PDT
Comment on attachment 164777 [details]
Patch

Looks good.  I'm ok with this if Dana is.  r=me
Comment 56 Dana Jansens 2012-09-19 14:32:35 PDT
Comment on attachment 164777 [details]
Patch

LGTM too. Thanks Terry
Comment 57 WebKit Review Bot 2012-09-19 15:43:29 PDT
Comment on attachment 164777 [details]
Patch

Clearing flags on attachment: 164777

Committed r129054: <http://trac.webkit.org/changeset/129054>
Comment 58 WebKit Review Bot 2012-09-19 15:43:36 PDT
All reviewed patches have been landed.  Closing bug.