Full Text Bug Listing

Hin-Chung Lam

Reported 2011-10-31 09:55:36 PDT

Created attachment 113061 [details] layout test See the supplied test. Click on good.png which is when compositor is disabled. Click on bad.png which compositor is enabled and position:fixed is put on a separate layer.

Hin-Chung Lam

Comment 1 2011-10-31 09:56:09 PDT

Created attachment 113063 [details] good resolution

Hin-Chung Lam

Comment 2 2011-10-31 09:56:27 PDT

Created attachment 113064 [details] bad resolution

James Robinson

Comment 3 2011-10-31 17:35:40 PDT

I think your test case is the same as this: <!DOCTYPE html> <style> .scaleUp { -webkit-transform:scale(2, 2); position:absolute; left:50px; } .compositedScaleUp { -webkit-transform:translateZ(0) scale(2,2); position:absolute; left:50px; } </style> <div class="scaleUp">TEST 1</div> <br><br> <div class="compositedScaleUp">TEST 2</div> which boils down to rastering at the pre-scale resolution rather than post-scale.

James Robinson

Comment 4 2011-11-01 13:41:34 PDT

I think we need to port http://trac.webkit.org/changeset/75639 (and http://trac.webkit.org/changeset/91137) to our compositor implementation.

Hin-Chung Lam

Comment 5 2011-11-02 13:46:07 PDT

Created attachment 113369 [details] test case for page scale and position:fixed James: The WebKit patches you pointed out only deals with page scale but not general css transform. I think we should limit the scope of this to just have better resolution for css transform caused by page scale. I've uploaded a test case to demonstrate this problem.

Hin-Chung Lam

Comment 6 2011-11-02 13:57:13 PDT

Created attachment 113374 [details] work in progress 1 James: I also took your advice to look into how WebKit implements, please check the attached patch to see if this is the right approach to take. There are several things I don't understand so I added comments to the code: 1. GraphicsLayer::setAppliesPageScale() is called on GraphicsLayer associated to the render view. I don't know what this actually means because the layer transformation passed to the GraphicsLayer doesn't contain the scale transform. Actual painting of this layer goes through RenderLayer::paintLayer() which has transform applied to the painter. 2. It seems in RenderStlye::setPageScaleTransform() the page scale transform is applied as a CSS scale transform to be inherited by all objects. I observed that GraphicsLayer for render view doesn't have scale transform applied, but GraphicsLayer for position:fixed has scale transform applied. I had to counter-act this scale transform in GraphicsLayerChromium for position:fixed.... I'm sure there's a better way to do this..

James Robinson

Comment 7 2011-11-02 14:15:54 PDT

Comment on attachment 113374 [details] work in progress 1 View in context: https://bugs.webkit.org/attachment.cgi?id=113374&action=review > Source/WebCore/platform/graphics/GraphicsLayer.h:-468 > - please don't make spurious changes to files like this. i think your editor is probably doing this, fix it > Source/WebCore/platform/graphics/chromium/LayerChromium.h:197 > + float contentsScale() const { return m_contentsScale; } > + void setContentsScale(float contentsScale) { m_contentsScale = contentsScale; } we already support the ability for layer bounds != content bounds via contentBounds()/bounds(). is it necessary to introduce another mechanism? > Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:154 > + canvasPainter.context()->scale(FloatSize(contentsScale, contentsScale)); if you're scaling the canvas you need to adjust the size of the allocated canvas as well, right? where is that being done? > Source/WebKit/chromium/src/PageOverlay.cpp:81 > + virtual float deviceScaleFactor() const > + { > + return 1; > + } > + > + virtual float pageScaleFactor() const > + { > + return m_webViewImpl->pageScaleFactor(); > + } i don't understand this change at all. are you using PageOverlay?

Hin-Chung Lam

Comment 8 2011-11-02 14:26:08 PDT

Comment on attachment 113374 [details] work in progress 1 View in context: https://bugs.webkit.org/attachment.cgi?id=113374&action=review >> Source/WebCore/platform/graphics/chromium/LayerChromium.h:197 >> + void setContentsScale(float contentsScale) { m_contentsScale = contentsScale; } > > we already support the ability for layer bounds != content bounds via contentBounds()/bounds(). is it necessary to introduce another mechanism? This is used in the painter of texture updater to apply scale to the canvas. >> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:154 >> + canvasPainter.context()->scale(FloatSize(contentsScale, contentsScale)); > > if you're scaling the canvas you need to adjust the size of the allocated canvas as well, right? where is that being done? I thought setting the bounds of LayerChromium will changes the size of canvas, I'll check again. > Source/WebCore/rendering/RenderLayer.cpp:3846 > + bool paintsForFixedPositionLayer = renderer()->isPositioned() && renderer()->style()->position() == FixedPosition; Don't mind changes in this file, it shouldn't be here.. >> Source/WebKit/chromium/src/PageOverlay.cpp:81 >> + } > > i don't understand this change at all. are you using PageOverlay? deviceScaleFactor and pageScaleFactor are methods in GraphicsLayerClient. This change http://trac.webkit.org/changeset/91137 queries client for these scale factors, where is a better spot for implementing these two methods?

James Robinson

Comment 9 2011-11-02 14:32:58 PDT

I see, so are the layer bounds pre-scale or post-scale?

Hin-Chung Lam

Comment 10 2011-11-02 14:34:11 PDT

(In reply to comment #9) > I see, so are the layer bounds pre-scale or post-scale? Layer bounds are post-scale.

James Robinson

Comment 11 2011-11-02 14:36:08 PDT

The way I was envisioning this is that layer bounds are pre-scale but the content bounds are post-scale and we raster with the scale applied into a post-scale canvas, then take care of the draw transform such that it displays at the right size.

Vangelis Kokkevis

Comment 12 2011-11-02 15:10:11 PDT

Comment on attachment 113374 [details] work in progress 1 View in context: https://bugs.webkit.org/attachment.cgi?id=113374&action=review I'm not sure I understand why we have to be applying the scale in all these places. It seems to me that when the page is scaled there will be a scale transform at the top of the tree that will affect the positions and bounds of all layers. Only when it comes time to paint the contents of an individual layer do we have to scale the canvas by the contentsScale and adjust the draw transform by 1/contentsScale to make sure it renders correctly. That should work for all composited layers, not just the fixed position ones. > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:509 > + IntSize layerSize(m_size.width() * contentsScaleFactor, m_size.height() * contentsScaleFactor); If the whole page is scaled via a scale transform at the very top of the tree, then you shouldn't have to worry about accommodating for scales down here. > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:530 > + primaryLayer()->setAnchorPointZ(m_anchorPoint.z() * contentsScaleFactor); Not sure you need that. You're not adjusting the x and y either... > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:541 > + if (appliesPageScale()) { style: no braces > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:542 > + layerTransform = TransformationMatrix().scale(1 / contentsScale()) * m_transform; Should be 1.0 to force a float division ? Also, the scale probably needs to be applied to all 3d axes so I think you need: layerTransform.scale3d(1/cs, 1/cs, 1/cs); > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:593 > + m_layer->setContentsScale(contentsScale()); Isn't the scale going to be set by updateContentsScale() above? > Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:191 > PlatformContextSkia platformContext(canvas); I believe you'll need to apply the scale here too. > Source/WebCore/rendering/RenderLayer.cpp:3851 > + return transform() && ((paintBehavior & PaintBehaviorFlattenCompositingLayers) || paintsToWindow || paintsForFixedPositionLayer); I don't think you want to be special-casing fixed position elements...

Hin-Chung Lam

Comment 13 2011-11-03 06:25:55 PDT

Created attachment 113474 [details] ignore this patch

Hin-Chung Lam

Comment 14 2011-11-03 08:51:00 PDT

Created attachment 113499 [details] work in progress 2 So what I did in this patch: 1. Bump up the contentBounds() using the idea from James. 2. Apply scale transform in the painter. This makes it much cleaner and not have to mess around with layer transform.. I'm working on some layout tests to make sure this works.

Hin-Chung Lam

Comment 15 2011-11-03 10:00:52 PDT

Created attachment 113511 [details] Patch

Hin-Chung Lam

Comment 16 2011-11-03 10:05:29 PDT

I've updated the patch with some tests, please take another look.

James Robinson

Comment 17 2011-11-03 10:21:04 PDT

Comment on attachment 113511 [details] Patch This looks pretty nice!

Hin-Chung Lam

Comment 18 2011-11-03 10:28:10 PDT

Cool! Let's do a full review.

Gustavo Noronha (kov)

Comment 19 2011-11-03 13:17:35 PDT

Comment on attachment 113511 [details] Patch Attachment 113511 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10249412

Daniel Sievers

Comment 20 2011-11-03 15:03:56 PDT

Does this use the wrong (post-scale contentRect) when calling into webkit below? paintContents(*canvasPainter.context(), contentRect); I guess with a scale smaller than 1 you could potentially test if this misses updating anything. Interestingly, since before your patch contentBounds() and bounds() are the same, I think we end up scaling at draw time simply through the projection from layer to target render surface, i.e.: IntRect layerRect = surfaceToLayer.projectQuad(FloatQuad(FloatRect(minimalSurfaceRect))).enclosingBoundingBox();

James Robinson

Comment 21 2011-11-03 15:11:39 PDT

contentBounds != bounds today for scaled images, which are scaled at draw time. http://trac.webkit.org/browser/trunk/LayoutTests/compositing/color-matching/image-color-matching.html for example

James Robinson

Comment 22 2011-11-03 23:52:41 PDT

Comment on attachment 113511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113511&action=review > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:195 > + updateContentsScale(); is there a specific reason you're doing this before the transform == m_transform check? do you need to run this code even if the transform hasn't changed? > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:651 > + // If page scale is applied as a transform in the compositor then there's nothing to do here. not sure i grok this comment. looking at the code, it appears this bit is set on GraphicsLayers for RenderViews, which would include the main layer and iframes below it have you tested scaled elements inside iframes, etc? > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:655 > + m_layer->setContentsScale(contentsScale()); should you be setting this on the m_contentsLayer? > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:657 > + m_layer->setNeedsDisplay(); hmm, this appears to invalidate the full layer every time this function is called even if nothing has changed - which would be bad. > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:137 > +IntSize TiledLayerChromium::contentBounds() const ImageLayerChromium is a subclass of this and provides an implementation of contentBounds() as well. Will this do the right thing for those layers?

James Robinson

Comment 23 2011-11-03 23:53:19 PDT

there's nothing in this patch specific to fixed position elements that I can see - can you please retitle the bug to capture its scope?

Hin-Chung Lam

Comment 24 2011-11-04 09:00:54 PDT

Created attachment 113663 [details] Patch

Hin-Chung Lam

Comment 25 2011-11-04 09:04:16 PDT

Comment on attachment 113663 [details] Patch I've added one more test to cover position:fixed in iframe. I also added a virtual method to LayerChromium to determine if the layer needs content scaling. For example doing content scaling with ImageLayerChromium is bad because it's slower to do it in software, but for TiledLayerChromium for regular content content scaling is needed for the best quality.

Vangelis Kokkevis

Comment 26 2011-11-04 13:45:24 PDT

Comment on attachment 113663 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113663&action=review > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:64 > + virtual void prepareToUpdate(const IntRect& contentRect, const IntSize& tileSize, int /* borderTexels */, float contentsScale) You'll probably need to add contentsScale in /* */ comments to avoid compiler warnings about unused variable. > Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:153 > + canvasPainter.context()->save(); Adding a check for a scale of 1.0 would help avoid unnecessary saves and restores. > LayoutTests/ChangeLog:6 > + Added test cases that has position:fixed and page scale. I would recommend getting rid of all mentions of fixed position in the Changelogs since this fix applies to all tiled layer types. You can also make the layout tests independent of fixed position > LayoutTests/platform/chromium-linux/compositing/geometry/fixed-position-transform-composited-page-scale-expected.txt:1 > +layer at (0,0) size 785x585 You'll need to change test_expectations.txt to mark win and mac as IMAGE until you can harvest results from the bots.

James Robinson

Comment 27 2011-11-04 16:47:51 PDT

Comment on attachment 113663 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113663&action=review I had feedback to previous patch that wasn't addressed. > Source/WebCore/ChangeLog:3 > + [chromium] when position:fixed in a separate layer it doesn't get full resolution with css transform please update the changelog title to reflect the bug title > Source/WebKit/chromium/ChangeLog:3 > + [chromium] when position:fixed in a separate layer it doesn't get full resolution with css transform same here > LayoutTests/compositing/geometry/fixed-position-composited-page-scale.html:1 > +<html> <!DOCTYPE html> before this please please do not add new pixel results that include scrollbars since that makes the results platform-specific

Hin-Chung Lam

Comment 28 2011-11-07 09:20:19 PST

(In reply to comment #22) > (From update of attachment 113511 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113511&action=review > > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:195 > > + updateContentsScale(); > > is there a specific reason you're doing this before the transform == m_transform check? do you need to run this code even if the transform hasn't changed?\ Yes I do need to do this even if transform == m_transform as the content scale may change. > > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:651 > > + // If page scale is applied as a transform in the compositor then there's nothing to do here. > > not sure i grok this comment. looking at the code, it appears this bit is set on GraphicsLayers for RenderViews, which would include the main layer and iframes below it > > have you tested scaled elements inside iframes, etc? I added a test for this case. > > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:655 > > + m_layer->setContentsScale(contentsScale()); > > should you be setting this on the m_contentsLayer? I tried with m_contentsLayer but it didn't fix the problem. I'm a bit confused with m_layer and m_contentsLayer.. what are the differences? > > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:657 > > + m_layer->setNeedsDisplay(); > > hmm, this appears to invalidate the full layer every time this function is called even if nothing has changed - which would be bad. Removed this. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:137 > > +IntSize TiledLayerChromium::contentBounds() const > > ImageLayerChromium is a subclass of this and provides an implementation of contentBounds() as well. Will this do the right thing for those layers? Addressed in latest patch.

Hin-Chung Lam

Comment 29 2011-11-07 09:22:14 PST

Created attachment 113880 [details] Patch

Hin-Chung Lam

Comment 30 2011-11-07 09:24:14 PST

Comment on attachment 113663 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113663&action=review >> Source/WebCore/ChangeLog:3 >> + [chromium] when position:fixed in a separate layer it doesn't get full resolution with css transform > > please update the changelog title to reflect the bug title done. >> Source/WebKit/chromium/ChangeLog:3 >> + [chromium] when position:fixed in a separate layer it doesn't get full resolution with css transform > > same here done. >> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:64 >> + virtual void prepareToUpdate(const IntRect& contentRect, const IntSize& tileSize, int /* borderTexels */, float contentsScale) > > You'll probably need to add contentsScale in /* */ comments to avoid compiler warnings about unused variable. done. >> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:153 >> + canvasPainter.context()->save(); > > Adding a check for a scale of 1.0 would help avoid unnecessary saves and restores. done. >> LayoutTests/ChangeLog:6 >> + Added test cases that has position:fixed and page scale. > > I would recommend getting rid of all mentions of fixed position in the Changelogs since this fix applies to all tiled layer types. You can also make the layout tests independent of fixed position done. >> LayoutTests/compositing/geometry/fixed-position-composited-page-scale.html:1 >> +<html> > > <!DOCTYPE html> before this please > > please do not add new pixel results that include scrollbars since that makes the results platform-specific done.

James Robinson

Comment 31 2011-11-07 17:30:18 PST

Comment on attachment 113880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113880&action=review Nearly there! > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:144 > + return IntSize(bounds().width() * contentsScale(), bounds().height() * contentsScale()); Are you sure this does the rounding we want? > LayoutTests/ChangeLog:6 > + Added test cases that has position:fixed and page scale. since you are only adding cr-linux expectations, you need to add = MISSING expectations for windows and mac or the bots will go red when this lands. > LayoutTests/compositing/geometry/fixed-position-composited-page-scale.html:23 > + eventSender.scalePageBy(2, 0, 0); could you add a few tests with a scale < 1? > LayoutTests/platform/chromium-linux/compositing/geometry/fixed-position-composited-page-scale-expected.txt:2 > +layer at (0,0) size 800x585 > + RenderView at (0,0) size 800x585 do you need the render tree dump here, or is the test just about checking the pixels? If the latter add this incantation to the test: layoutTestController.dumpAsText(true); so you don't need this stuff

Hin-Chung Lam

Comment 32 2011-11-09 10:32:54 PST

I found that this patch doesn't work with page scale factor less than 1.0.. :( The problem can be illustrated by two separate cases: 1. Painting the root layer This path goes through RenderLayer::paintLayer. paintDirtyRect passed to this method is post-scaled and this method map the rect to a pre-scaled one at this line: http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp&type=cs&l=2721. 2. Painting a non-root tiled layer rect Painter of the layer goes through RenderLayerBacking::paintIntoLayer. The dirty rect passed to RenderLayerBacking needs to be pre-scaled because it doesn't handle transformation. However contentRect passed to LayerTextureUpdaterBitmap::prepareToUpdate is post-scaled so only part of the layer content is painted. The sad part is that LayerTextureUpdaterBitmap::prepareToUpdate handles both cases, but a post-scaled dirt rect works for case (1) but not (2). I feel that the proper way is to merge RenderLayer::paintLayer and RenderLayerBacking::paintIntoLayer, it is crazy to transform dirty rect in one case but not the other.. Suggestions?

Daniel Sievers

Comment 33 2011-11-09 11:10:24 PST

(In reply to comment #32) > I found that this patch doesn't work with page scale factor less than 1.0.. :( > Also see my comment #20 above. Isn't it possible to simply pass the pre-scaled rect to the paint routine, since webkit doesn't know or need to care about how we scale this for composited layers?

Hin-Chung Lam

Comment 34 2011-11-09 11:45:24 PST

(In reply to comment #33) > (In reply to comment #32) > > I found that this patch doesn't work with page scale factor less than 1.0.. :( > > > > Also see my comment #20 above. Isn't it possible to simply pass the pre-scaled rect to the paint routine, since webkit doesn't know or need to care about how we scale this for composited layers? It may be possible but we'll need to make sure WebKit in RenderLayer::paintLayer doesn't scale the rect up otherwise there'll be over painting.

Alexandre Elias

Comment 35 2011-11-10 09:53:28 PST

Minor comment: WebViewImpl has a newly added deviceScaleFactor() method, so you might want to call that from PageOverlay. As for the sub-1.0 problem, merging the root layer/sublayer draw paths sounds like a good idea in principle, but if that proves hard for whatever reason, then it should be possible to hack the bounds sent to WebKit like Daniel says.

Hin-Chung Lam

Comment 36 2011-11-11 12:52:50 PST

Created attachment 114753 [details] Patch

Hin-Chung Lam

Comment 37 2011-11-11 12:58:09 PST

Comment on attachment 114753 [details] Patch I've updated the code with changes included in our discussion. This patch doesn't include pixel test results yet because DumpRenderTree with page scaling is currently broken but I've tested this patch locally. I'm look to see what's wrong with DumpRenderTree now..

WebKit Review Bot

Comment 38 2011-11-11 13:01:08 PST

Attachment 114753 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/comp..." exit_code: 1 Last 3072 characters of output: atabase-interactions.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3863: Path does not exist. fast/js/array-functions-non-arrays.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3864: Path does not exist. fast/js/eval-cross-window.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3865: Path does not exist. fast/js/regexp-caching.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3866: Path does not exist. fast/js/toString-overrides.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3867: Path does not exist. fast/js/toString-recursion.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3868: Path does not exist. http/tests/security/xss-eval.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3870: Path does not exist. fast/dom/javascript-url-exception-isolation.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3871: Path does not exist. fast/borders/inline-mask-overlay-image-outset-vertical-rl.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3873: Path does not exist. fast/dom/rtl-scroll-to-leftmost-and-resize.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3875: Path does not exist. fast/canvas/canvas-transforms-fillRect-shadow.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3877: Path does not exist. http/tests/inspector-enabled/dedicated-workers-list.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3878: Path does not exist. http/tests/inspector-enabled/dedicated-workers-list.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3880: Path does not exist. media/track/tracklist-is-reachable.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3883: Path does not exist. compositing/geometry/fixed-position-composited-page-scale.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3884: Path does not exist. compositing/geometry/fixed-position-iframe-composited-page-scale.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3885: Path does not exist. compositing/geometry/fixed-position-transform-composited-page-scale.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3886: Path does not exist. compositing/geometry/fixed-position-composited-page-scale-down.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3887: Path does not exist. compositing/geometry/fixed-position-iframe-composited-page-scale-down.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3888: Path does not exist. compositing/geometry/fixed-position-transform-composited-page-scale-down.html [test/expectations] [2] Total errors found: 2138 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.

Hin-Chung Lam

Comment 39 2011-11-14 10:57:47 PST

Created attachment 114983 [details] Patch

Hin-Chung Lam

Comment 40 2011-11-14 10:58:39 PST

The latest patch takes care of scale factor less than 1.0 and rounding error that creates empty gaps between tiles.

WebKit Review Bot

Comment 41 2011-11-14 11:00:54 PST

Attachment 114983 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/comp..." exit_code: 1 Last 3072 characters of output: /chromium/test_expectations.txt:3860: Path does not exist. accessibility/adjacent-continuations-cause-assertion-failure.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3862: Path does not exist. inspector/debugger/script-formatter.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3865: Path does not exist. fast/js/mozilla/strict/15.4.5.1.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3867: Path does not exist. fast/canvas/canvas-transforms-fillRect-shadow.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3869: Path does not exist. media/track/tracklist-is-reachable.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3870: Path does not exist. media/track/track-cues-cuechange.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3872: Path does not exist. http/tests/inspector-enabled/dedicated-workers-list.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3873: Path does not exist. http/tests/inspector-enabled/dedicated-workers-list.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3875: Path does not exist. http/tests/security/cross-frame-access-custom.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3877: Path does not exist. fast/loader/javascript-url-in-embed.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3879: Path does not exist. security/crypto-random-values-types.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3881: Path does not exist. media/track/track-webvtt-tc004-magic-header.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3883: Path does not exist. http/tests/inspector/resource-tree/resource-tree-frame-add.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3886: Path does not exist. compositing/geometry/fixed-position-composited-page-scale.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3887: Path does not exist. compositing/geometry/fixed-position-iframe-composited-page-scale.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3888: Path does not exist. compositing/geometry/fixed-position-transform-composited-page-scale.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3889: Path does not exist. compositing/geometry/fixed-position-composited-page-scale-down.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3890: Path does not exist. compositing/geometry/fixed-position-iframe-composited-page-scale-down.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3891: Path does not exist. compositing/geometry/fixed-position-transform-composited-page-scale-down.html [test/expectations] [2] Total errors found: 2136 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.

Hin-Chung Lam

Comment 42 2011-11-14 13:39:29 PST

Please ignore the errors for path does not exist. Eric has a patch to fix that.

James Robinson

Comment 43 2011-11-14 15:08:05 PST

Comment on attachment 114983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114983&action=review This seems very close - the contentScale updating seems a bit off but everything else seems quite nice. Could you please upload the .pngs for at least one platform? That's very useful for the gardener or whoever rebaselines the tests (assuming it isn't you) to have a baseline expectation of what the test should look like so they can eyeball if it's failing horribly on the other platforms or not. > Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:63 > + int x = floor(contentRect.x() * invScale); > + int y = floor(contentRect.y() * invScale); doing all this math component-wise is pretty verbose and runs the risk of X/Y confusion errors that can be a pain to debug. Can you try to find the correct combination of (Int|Rect)/(Rect|Size) methods to do this? I think it's something like: scaledContentRect = FloatRect(contentRect).scale(invScale).enclosingIntRect(); or something along those lines (don't take my word for it, please test) > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:195 > + // Call this method first to assign contents scale to LayerChromium so the painter can apply the scale transform. > + updateContentsScale(); this still seems weird - setting the shouldn't change the contents scale. contentsScale() appears to be a function of appliesPageScale(), pageScaleFactor(), and deviceScaleFactor(). Can we recalculate the scale factor when those things change, and not when setting the transform? we have a deviceOrPageScaleChanged() function already that seems like a good place to update this > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:600 > + updateContentsScale(); this call also confuses me a bit - why do we need to update the contentsScale here when updating the transform hierarchy? we aren't changing the pageScale or deviceScale here > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:687 > + return 1.0f; nit: for constants like this in WebKit we typically just say '1'

Hin-Chung Lam

Comment 44 2011-11-15 04:51:34 PST

Comment on attachment 114983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114983&action=review >> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:63 >> + int y = floor(contentRect.y() * invScale); > > doing all this math component-wise is pretty verbose and runs the risk of X/Y confusion errors that can be a pain to debug. Can you try to find the correct combination of (Int|Rect)/(Rect|Size) methods to do this? I think it's something like: > > scaledContentRect = FloatRect(contentRect).scale(invScale).enclosingIntRect(); > > or something along those lines (don't take my word for it, please test) Done. Thanks for suggestions! >> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:195 >> + updateContentsScale(); > > this still seems weird - setting the shouldn't change the contents scale. contentsScale() appears to be a function of appliesPageScale(), pageScaleFactor(), and deviceScaleFactor(). Can we recalculate the scale factor when those things change, and not when setting the transform? we have a deviceOrPageScaleChanged() function already that seems like a good place to update this Yes deviceOrPageScaleChanged() already submit the contentsScale to LayerChromium and it's a good place to do this stuff. However deviceOrPageScaleChanged() is not called when page_scale * device_scale is not 1.0 when the GraphicsLayer is created. GraphicsLayerChromium needs to call updateContentsScale() somewhere to fetch the current contents scale. I thought of doing this in the constructor, but that is not a good place because WebKit sets setAppliesPageScale() after GraphicsLayer is constructed here: http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderLayerBacking.cpp&exact_package=chromium&q=RenderLayerBacking.cpp&type=cs&l=124. Since updateLayerTransform() is always called when creating a GraphicsLayer I thought it would be a OK place to read the current page scale. Otherwise we'll need to add a method to GraphicsLayer and call it from RenderLayerBacking::createPrimaryGraphicsLayer(). > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:567 > + updateContentsScale(); Remove this one too. >> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:600 >> + updateContentsScale(); > > this call also confuses me a bit - why do we need to update the contentsScale here when updating the transform hierarchy? we aren't changing the pageScale or deviceScale here Removed. >> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:687 >> + return 1.0f; > > nit: for constants like this in WebKit we typically just say '1' Done.

Hin-Chung Lam

Comment 45 2011-11-15 04:53:57 PST

Created attachment 115144 [details] Patch

James Robinson

Comment 46 2011-11-15 11:17:28 PST

This patch doesn't apply to ToT so none of the bots can run, would you mind updating it?

Hin-Chung Lam

Comment 47 2011-11-15 14:22:27 PST

Created attachment 115242 [details] Patch

WebKit Review Bot

Comment 48 2011-11-15 14:27:14 PST

Attachment 115242 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/comp..." exit_code: 1 ERROR: FAILURES FOR <lucid, x86_64, release, cpu> in LayoutTests/platform/chromium/test_expectations.txt ERROR: Line:2735 Path does not exist. plugins/invalidate_rect.html LayoutTests/platform/chromium/test_expectations.txt:2735: Path does not exist. plugins/invalidate_rect.html [test/expectations] [2] Total errors found: 1 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.

Hin-Chung Lam

Comment 49 2011-11-15 14:27:41 PST

Comment on attachment 115242 [details] Patch Whoops.. silly test_expectations.txt.. It's updated now.

Hin-Chung Lam

Comment 50 2011-11-15 14:28:51 PST

Whoops.. I have nothing to do to plugins/invalidate_rect.html :(

James Robinson

Comment 51 2011-11-15 14:35:56 PST

Comment on attachment 115242 [details] Patch What invalidates layers when the contentsScale changes?

James Robinson

Comment 52 2011-11-16 15:30:19 PST

It looks like the CoreAnimation implementation pulls the page and device scales instead of relying on GraphicsLayer pushing them. Since they use a deferred update model, on initialization WebCore has a chance to set up all state (including appliesPageScale) before the GraphicsLayer implementation tries to figure out the active page scale. Triggering off of setTransform() is ugly and fragile (what if RenderLayerBacking reorders those two lines?) but I'm not sure of what else we could do. One possibility would be to make GraphicsLayer::setAppliesPageScale virtual and override it in GraphicsLayerChromium to recalculate the scale factors there.

James Robinson

Comment 53 2011-11-16 15:32:32 PST

It looks like the CoreAnimation implementation pulls the page and device scales instead of relying on GraphicsLayer pushing them. Since they use a deferred update model, on initialization WebCore has a chance to set up all state (including appliesPageScale) before the GraphicsLayer implementation tries to figure out the active page scale. Triggering off of setTransform() is ugly and fragile (what if RenderLayerBacking reorders those two lines?) but I'm not sure of what else we could do. One possibility would be to make GraphicsLayer::setAppliesPageScale virtual and override it in GraphicsLayerChromium to recalculate the scale factors there.

Alexandre Elias

Comment 54 2011-11-16 16:15:46 PST

We're currently using deviceOrPageScaleFactorChanged() to trigger an invalidate. It should be applied to sublayers as well. m_appliesPageScale looks like it's a flag set only during initialization to denote the root layer -- it doesn't seem to have much to do with invalidation.

James Robinson

Comment 55 2011-11-16 16:28:17 PST

Comment on attachment 115242 [details] Patch Yeah, it's not clear cut. Anyway I think we can iterate on that point. This patch looks good, R=me.

WebKit Review Bot

Comment 56 2011-11-16 16:59:21 PST

Comment on attachment 115242 [details] Patch Rejecting attachment 115242 [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: iting/geometry/fixed-position-transform-composited-page-scale-expected.txt patching file LayoutTests/compositing/geometry/fixed-position-transform-composited-page-scale.html patching file LayoutTests/platform/chromium/test_expectations.txt Hunk #1 FAILED at 3922. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'James Robinson', u'--f..." exit_code: 1 Full output: http://queues.webkit.org/results/10487914

Iain Merrick

Comment 57 2011-11-18 08:57:32 PST

Created attachment 115821 [details] Fixed merge conflicts

Iain Merrick

Comment 58 2011-11-18 09:03:45 PST

Uploaded a patch with the same code, merge conflicts resolved. I left the author unchanged in the ChangeLogs, not sure of the protocol there. The main change is that the change that split up LayerTextureUpdaterCanvas.cpp (http://trac.webkit.org/changeset/99813) has been reverted, so I moved the BitmapCanvasLayerTextureUpdater and SkPictureCanvasLayerTextureUpdater diffs back to that file. Ran unit tests and new-run-webkit-tests --accelerated-drawing --threaded-compositing compositing. Alpha's new tests and a few others fail with what look like invisible sub-pixel differences. Not sure if that's the new code, or existing failures. I guess the tests need to be rebaselined? But I'm not sure how to do that. James, hopefully this is at least in a more useful state for you. :)

James Robinson

Comment 59 2011-11-18 15:57:58 PST

(In reply to comment #58) > Uploaded a patch with the same code, merge conflicts resolved. > > I left the author unchanged in the ChangeLogs, not sure of the protocol there. > > The main change is that the change that split up LayerTextureUpdaterCanvas.cpp (http://trac.webkit.org/changeset/99813) has been reverted, so I moved the BitmapCanvasLayerTextureUpdater and SkPictureCanvasLayerTextureUpdater diffs back to that file. > > Ran unit tests and new-run-webkit-tests --accelerated-drawing --threaded-compositing compositing. Alpha's new tests and a few others fail with what look like invisible sub-pixel differences. Not sure if that's the new code, or existing failures. I guess the tests need to be rebaselined? But I'm not sure how to do that. I don't know why you ran with -accelerated-drawing, we don't really support that configuration today. The useful configuration to test is the default one. > James, hopefully this is at least in a more useful state for you. :) Yes, this is helpful. Thanks.

James Robinson

Comment 60 2011-11-18 19:05:20 PST

http://trac.webkit.org/changeset/100839

Simon Fraser (smfr)

Comment 61 2012-03-20 18:59:08 PDT

Use of ::webkit-scrollbar to hide scrollbars doesn't work in WK1, so the WK1 pixel results don't match.

Attachments
layout test (369 bytes, text/html) 2011-10-31 09:55 PDT, Hin-Chung Lam	no flags	Details
good resolution (3.96 KB, image/png) 2011-10-31 09:56 PDT, Hin-Chung Lam	no flags	Details
bad resolution (6.37 KB, image/png) 2011-10-31 09:56 PDT, Hin-Chung Lam	no flags	Details
test case for page scale and position:fixed (479 bytes, text/html) 2011-11-02 13:46 PDT, Hin-Chung Lam	no flags	Details
work in progress 1 (17.04 KB, patch) 2011-11-02 13:57 PDT, Hin-Chung Lam	no flags	Details Formatted Diff Diff
ignore this patch (5.47 KB, patch) 2011-11-03 06:25 PDT, Hin-Chung Lam	no flags	Details Formatted Diff Diff
work in progress 2 (13.14 KB, patch) 2011-11-03 08:51 PDT, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (35.75 KB, patch) 2011-11-03 10:00 PDT, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (44.77 KB, patch) 2011-11-04 09:00 PDT, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (42.52 KB, patch) 2011-11-07 09:22 PST, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (36.00 KB, patch) 2011-11-11 12:52 PST, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (36.50 KB, patch) 2011-11-14 10:57 PST, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (55.88 KB, patch) 2011-11-15 04:53 PST, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (55.96 KB, patch) 2011-11-15 14:22 PST, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Fixed merge conflicts (77.05 KB, patch) 2011-11-18 08:57 PST, Iain Merrick	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Bug 71225