Bug 96137 – [chromium] Store the contents scale factor in PlatformContextSkia on initialization

• Component: Platform
• Status: RESOLVED FIXED

[Terry Anderson]

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/

[Terry Anderson]

Created attachment 162847 [details]
Patch

[Dana Jansens]

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?

[Peter Beverloo (cr-android ews)]

Comment on attachment 162847 [details]
Patch

Attachment 162847 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13793001

[WebKit Review Bot]

Comment on attachment 162847 [details]
Patch

Attachment 162847 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13774939

[Stephen White]

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.

[Dana Jansens]

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

[Stephen White]

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

[Dana Jansens]

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

[Terry Anderson]

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.

[Dana Jansens]

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

[Terry Anderson]

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

[Terry Anderson]

Created attachment 163401 [details]
Patch

[WebKit Review Bot]

Comment on attachment 163401 [details]
Patch

Attachment 163401 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13819415

[Peter Beverloo (cr-android ews)]

Comment on attachment 163401 [details]
Patch

Attachment 163401 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13833036

[Stephen White]

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

[Stephen White]

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

[Stephen White]

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.

[Terry Anderson]

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

[Dana Jansens]

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

[Stephen White]

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

[Dana Jansens]

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

[Terry Anderson]

Created attachment 163962 [details]
Patch

[Terry Anderson]

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

[Stephen White]

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.

[Stephen White]

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

[Dana Jansens]

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.

[WebKit Review Bot]

Comment on attachment 163962 [details]
Patch

Attachment 163962 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13839664

[Peter Beverloo (cr-android ews)]

Comment on attachment 163962 [details]
Patch

Attachment 163962 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13844385

[Stephen White]

Comment on attachment 163962 [details]
Patch

This looks good to me, once the other relevant change has landed, and relevant test suppressions added.

[Terry Anderson]

Created attachment 164451 [details]
Patch

[WebKit Review Bot]

Comment on attachment 164451 [details]
Patch

Attachment 164451 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13889020

[Peter Beverloo (cr-android ews)]

Comment on attachment 164451 [details]
Patch

Attachment 164451 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13876447

[Terry Anderson]

Created attachment 164577 [details]
Patch

[Terry Anderson]

Created attachment 164633 [details]
Patch

[Dana Jansens]

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

[Stephen White]

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

[WebKit Review Bot]

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

[Terry Anderson]

Created attachment 164739 [details]
Patch for landing

[Terry Anderson]

Created attachment 164761 [details]
Patch

[Terry Anderson]

Comment on attachment 164761 [details]
Patch

Added two unit tests.

[WebKit Review Bot]

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.

[Dana Jansens]

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?

[Dana Jansens]

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?

[James Robinson]

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?

[Dirk Pranke]

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

[Adam Barth]

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!

[Dirk Pranke]

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

[Stephen White]

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?

[Terry Anderson]

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

[Terry Anderson]

Created attachment 164777 [details]
Patch

[Terry Anderson]

Comment on attachment 164777 [details]
Patch

Comments from danakj@ and senorblanco@ addressed.

[Dirk Pranke]

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

[Adam Barth]

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

[Dirk Pranke]

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

[Stephen White]

Comment on attachment 164777 [details]
Patch

Looks good.  I'm ok with this if Dana is.  r=me

[Dana Jansens]

Comment on attachment 164777 [details]
Patch

LGTM too. Thanks Terry

[WebKit Review Bot]

Comment on attachment 164777 [details]
Patch

Clearing flags on attachment: 164777

Committed r129054: <http://trac.webkit.org/changeset/129054>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.