Currently the ScrollbarLayerChromium of the top level frame is in a ScrollbarLayerChromium with a regular 1x scale factor regardless of the current content scale factor. The scroll bar should match the content scale of the rest of the browser drawing a high DPI scrollbar when running on a high DPI display.
Created attachment 160824 [details] Patch
Created attachment 162243 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 162243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162243&action=review As a note for reviewers: Twiz is working on a change (wkb.ug/95094) to set contentsScale on all layers in the tree appropriately, which would eliminate the need for these changes to Frame/FrameView/ScrollingCoordiatorChromium/WebLayer. So I'm not sure that this should land before that change, unless it is at risk for m23. > Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:268 > + IntRect thumbRect = m_geometry->thumbRect(m_scrollbar.get()); Why'd this change to a IntRect? > Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:282 > + // Paint and upload the entire part. I don't feel like this comment is adding much value here. Is there something to explain that the code does not? > Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:293 > + IntRect thumbRect = m_geometry->thumbRect(m_scrollbar.get()); Why switched to IntRect again? > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:108 > - m_geometry->splitTrack(&m_scrollbar, m_geometry->trackRect(&m_scrollbar), backTrackRect, thumbRect, foreTrackRect); > + WebRect trackRect = m_geometry->trackRect(&m_scrollbar); > + m_geometry->splitTrack(&m_scrollbar, trackRect, backTrackRect, thumbRect, foreTrackRect); no-op change? should this be in the diff? > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:113 > - OwnPtr<CCTextureDrawQuad> quad = CCTextureDrawQuad::create(sharedQuadState, IntRect(thumbRect.x, thumbRect.y, thumbRect.width, thumbRect.height), m_thumbResourceId, premultipledAlpha, uvRect, flipped); > + OwnPtr<CCTextureDrawQuad> quad = CCTextureDrawQuad::create(sharedQuadState, IntRect(thumbRect.x * widthScale, thumbRect.y * heightScale, thumbRect.width * widthScale, thumbRect.height * heightScale), m_thumbResourceId, premultipledAlpha, uvRect, flipped); Style-wise, how about we add some local variables here: thumbContentRect and foreTrackContentRect? These will be the original rects scale()d up by width and height scale. Then we don't have to add multiplications inline here. > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:155 > - return WebKit::WebSize(m_owner->contentBounds().width(), m_owner->contentBounds().height()); > + return WebKit::WebSize(m_owner->bounds().width(), m_owner->bounds().height()); Who does this change affect, can you justify this one? It seems unrelated to the other changes somehow, is this fixing a separate bug than rendering scale?
Comment on attachment 162243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162243&action=review >> Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:268 >> + IntRect thumbRect = m_geometry->thumbRect(m_scrollbar.get()); > > Why'd this change to a IntRect? Not sure, changing to WebRect. Maybe this came out of merging, or just testing. >> Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:282 >> + // Paint and upload the entire part. > > I don't feel like this comment is adding much value here. Is there something to explain that the code does not? Removed. >> Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:293 >> + IntRect thumbRect = m_geometry->thumbRect(m_scrollbar.get()); > > Why switched to IntRect again? Changed back. >> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:108 >> + m_geometry->splitTrack(&m_scrollbar, trackRect, backTrackRect, thumbRect, foreTrackRect); > > no-op change? should this be in the diff? No, this was from testing. Removed. >> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:113 >> + OwnPtr<CCTextureDrawQuad> quad = CCTextureDrawQuad::create(sharedQuadState, IntRect(thumbRect.x * widthScale, thumbRect.y * heightScale, thumbRect.width * widthScale, thumbRect.height * heightScale), m_thumbResourceId, premultipledAlpha, uvRect, flipped); > > Style-wise, how about we add some local variables here: thumbContentRect and foreTrackContentRect? These will be the original rects scale()d up by width and height scale. Then we don't have to add multiplications inline here. Done. >> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:155 >> + return WebKit::WebSize(m_owner->bounds().width(), m_owner->bounds().height()); > > Who does this change affect, can you justify this one? It seems unrelated to the other changes somehow, is this fixing a separate bug than rendering scale? splitRect uses the trackRect which comes from WebScrollBar::size to compute the size of the scrollbar components. The size was coming from this class which was giving the pixel size and the resulting calculations were using a combination of pixel size and DIP size which resulted in the incorrect positioning I was originally seeing.
Created attachment 162285 [details] Patch
Comment on attachment 162285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162285&action=review > Source/Platform/chromium/public/WebLayer.h:108 > + // Set to apply a scale factor used when painting and drawing this layer's content. Defaults to 1.0. > + virtual void setContentsScale(float) = 0; > + virtual float contentsScale() = 0; > + No no no. I think twiz's change in bug 95094 makes this unnecessary. > Source/WebCore/page/FrameView.cpp:708 > +void FrameView::deviceOrPageScaleFactorChanged() > +{ > + frame()->page()->scrollingCoordinator()->frameViewHorizontalScrollbarLayerDidChange( > + this, layerForHorizontalScrollbar()); > + frame()->page()->scrollingCoordinator()->frameViewVerticalScrollbarLayerDidChange( > + this, layerForVerticalScrollbar()); > +} I think all this is unnecessary too. > Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:285 > + float widthScale = static_cast<float>(contentBounds().width()) / bounds().width(); > + float heightScale = static_cast<float>(contentBounds().height()) / bounds().height(); > + > + scrollbarOrigin.scale(widthScale, heightScale); Can I suggest a helper function to convert an IntRect from layer space into content space? > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:157 > - return WebKit::WebSize(m_owner->contentBounds().width(), m_owner->contentBounds().height()); > + return WebKit::WebSize(m_owner->bounds().width(), m_owner->bounds().height()); Yeah, this is definitely correct. It only worked before because bounds() == contentBounds() for scrollbar layers.
Created attachment 162656 [details] Patch
Comment on attachment 162285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162285&action=review Thanks for the reviews! >> Source/Platform/chromium/public/WebLayer.h:108 >> + > > No no no. I think twiz's change in bug 95094 makes this unnecessary. Right, I was just waiting on the change to land to remove it :-). >> Source/WebCore/page/FrameView.cpp:708 >> +} > > I think all this is unnecessary too. Done, and other now unnecessary code. >> Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:285 >> + scrollbarOrigin.scale(widthScale, heightScale); > > Can I suggest a helper function to convert an IntRect from layer space into content space? Not quite sure exactly what you're proposing. Something on LayerChromium like IntRect LayerChromium::convertToContentSpace(const IntRect&);?
Comment on attachment 162285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162285&action=review The rest of the new patch looks really good. >>> Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:285 >>> + scrollbarOrigin.scale(widthScale, heightScale); >> >> Can I suggest a helper function to convert an IntRect from layer space into content space? > > Not quite sure exactly what you're proposing. Something on LayerChromium like IntRect LayerChromium::convertToContentSpace(const IntRect&);? Yeah, exactly something like LayerChromium::layerRectToContentRect(IntRect). Mostly just for readability so that you don't have to parse all the additional scale factors on every x, y, w, h value.
Comment on attachment 162656 [details] Patch Attachment 162656 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13785526 New failing tests: compositing/geometry/fixed-position-iframe-composited-page-scale-down.html
Created attachment 162849 [details] Patch
(In reply to comment #10) > (From update of attachment 162285 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162285&action=review > > The rest of the new patch looks really good. > > >>> Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:285 > >>> + scrollbarOrigin.scale(widthScale, heightScale); > >> > >> Can I suggest a helper function to convert an IntRect from layer space into content space? > > > > Not quite sure exactly what you're proposing. Something on LayerChromium like IntRect LayerChromium::convertToContentSpace(const IntRect&);? > > Yeah, exactly something like LayerChromium::layerRectToContentRect(IntRect). Mostly just for readability so that you don't have to parse all the additional scale factors on every x, y, w, h value. Done.
Comment on attachment 162849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162849&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.h:296 > + IntRect layerRectToContentRect(const IntRect& layerRect); > + How about on CCLayerImpl too?
Created attachment 162863 [details] Patch
(In reply to comment #14) > (From update of attachment 162849 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162849&action=review > > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:296 > > + IntRect layerRectToContentRect(const IntRect& layerRect); > > + > > How about on CCLayerImpl too? Done too.
Comment on attachment 162863 [details] Patch I am a bit on the fence about this layerRectToContentRect thing. Content space has a different origin than layer space does. It makes sense to scale a rect's size between these two spaces but not its position.
(In reply to comment #17) > (From update of attachment 162863 [details]) > I am a bit on the fence about this layerRectToContentRect thing. > > Content space has a different origin than layer space does. It makes sense to scale a rect's size between these two spaces but not its position. Sorry, I was out to lunch here completely. This is right as is. We do similar in TLC also: void TiledLayerChromium::setNeedsDisplayRect(const FloatRect& dirtyRect) { float contentsWidthScale = static_cast<float>(contentBounds().width()) / bounds().width(); float contentsHeightScale = static_cast<float>(contentBounds().height()) / bounds().height(); FloatRect scaledDirtyRect(dirtyRect); scaledDirtyRect.scale(contentsWidthScale, contentsHeightScale);
Comment on attachment 162863 [details] Patch R=me. Thanks!
Comment on attachment 162863 [details] Patch Attachment 162863 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13772968 New failing tests: compositing/geometry/fixed-position-iframe-composited-page-scale-down.html
Created attachment 163517 [details] Patch
The failing test compositing/geometry/fixed-position-iframe-composited-page-scale-down.html calls window.internals.settings.setPageScaleFactor(0.5, 0, 0). Trying to draw the scrollbar at a ~0.5 scale factor seems to cause some off by one issues however I'm not sure why we draw the top-level scroll bar differently when the final pixel size in the layout test is the same as at 1x. The latest patch uses rounded sizes and existing FloatRect operations which should be more correct but I think this will still fail the test.
Comment on attachment 163517 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163517&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:152 > + FloatRect contentRect(layerRect); > + contentRect.scale(widthScale, heightScale); > + return roundedIntRect(contentRect); I feel like this should be an containingIntRect not rounded, so that it always includes the whole layerRect given to the method. I think there's very few cases we want to round, typically we take the containing/contained rect. > Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:152 > + return IntSize(lroundf(bounds().width() * contentsScale()), lroundf(bounds().height() * contentsScale())); This is the rounding exception, but it's fine here because it's defining the space, not moving between spaces.
Comment on attachment 163517 [details] Patch Attachment 163517 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13824567 New failing tests: compositing/geometry/fixed-position-iframe-composited-page-scale-down.html
Created attachment 163604 [details] Patch
Comment on attachment 163517 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163517&action=review I found that the test was failing because the page scale was being applied to the scrollbar contentBounds, which I believe is incorrect. See the ScrollbarLayerChromium constructor in the latest patch where I call setBoundsContainPageScale. Not sure if the constructor is the correct place/way to call a parent class method so please correct me if I got it wrong. Thanks! >> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:152 >> + return roundedIntRect(contentRect); > > I feel like this should be an containingIntRect not rounded, so that it always includes the whole layerRect given to the method. I think there's very few cases we want to round, typically we take the containing/contained rect. Done.
(In reply to comment #26) > (From update of attachment 163517 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163517&action=review > > I found that the test was failing because the page scale was being applied to the scrollbar contentBounds, which I believe is incorrect. See the ScrollbarLayerChromium constructor in the latest patch where I call setBoundsContainPageScale. Not sure if the constructor is the correct place/way to call a parent class method so please correct me if I got it wrong. Thanks! Will this affect only the root scrollbar or others too. What's expected here? I think non-root scrollbars will actually be scaled by page scale (though we should be using overlay scrollbars under pinch anyway). Is it possible to set this flag on the root scrollbar layer from CCLTHImpl?
(In reply to comment #27) > (In reply to comment #26) > > (From update of attachment 163517 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=163517&action=review > > > > I found that the test was failing because the page scale was being applied to the scrollbar contentBounds, which I believe is incorrect. See the ScrollbarLayerChromium constructor in the latest patch where I call setBoundsContainPageScale. Not sure if the constructor is the correct place/way to call a parent class method so please correct me if I got it wrong. Thanks! > > Will this affect only the root scrollbar or others too. What's expected here? I think non-root scrollbars will actually be scaled by page scale (though we should be using overlay scrollbars under pinch anyway). Actually, I think we expect non-root scrollbars to be scaled down by page scale. I've locally run a layout test of page scale 0.5 and the image result of the non-root scrollbar is unaffected by this line, in both cases it is scaled down to half size. > Is it possible to set this flag on the root scrollbar layer from CCLTHImpl? I don't think so, perhaps there's a way between them that I haven't found yet.
(In reply to comment #28) > (In reply to comment #27) > > (In reply to comment #26) > > > (From update of attachment 163517 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=163517&action=review > > > > > > I found that the test was failing because the page scale was being applied to the scrollbar contentBounds, which I believe is incorrect. See the ScrollbarLayerChromium constructor in the latest patch where I call setBoundsContainPageScale. Not sure if the constructor is the correct place/way to call a parent class method so please correct me if I got it wrong. Thanks! > > > > Will this affect only the root scrollbar or others too. What's expected here? I think non-root scrollbars will actually be scaled by page scale (though we should be using overlay scrollbars under pinch anyway). > > Actually, I think we expect non-root scrollbars to be scaled down by page scale. I've locally run a layout test of page scale 0.5 and the image result of the non-root scrollbar is unaffected by this line, in both cases it is scaled down to half size. To clarify, this line won't prevent the scrollbar from changing its size, it prevents it from having its contentsScale adjusted by the page scale. So if you pinched in and the scrollbar was 5x as big, you would still get a scrollbar rendered at 1x (or 2x in hi-dpi). If the flag was not set, then the scrollbar would be painted in a bitmap scaled 5x (whether we have resources for that or how it would appear is another issue that I'm not sure about). > > Is it possible to set this flag on the root scrollbar layer from CCLTHImpl? > > I don't think so, perhaps there's a way between them that I haven't found yet.
(In reply to comment #28) > (In reply to comment #27) > > > > Is it possible to set this flag on the root scrollbar layer from CCLTHImpl? > > I don't think so, perhaps there's a way between them that I haven't found yet. NCCH has enough information to find the root scrollbar layers and set flags on them. It used to do this in the past to reserve scrollbar textures (see: https://bugs.webkit.org/show_bug.cgi?id=77251). There's no reason you couldn't do something like that and explicitly set the flag you need on them.
Created attachment 163918 [details] Patch
(In reply to comment #30) > (In reply to comment #28) > > (In reply to comment #27) > > > > > > Is it possible to set this flag on the root scrollbar layer from CCLTHImpl? > > > > I don't think so, perhaps there's a way between them that I haven't found yet. > > NCCH has enough information to find the root scrollbar layers and set flags on them. It used to do this in the past to reserve scrollbar textures (see: https://bugs.webkit.org/show_bug.cgi?id=77251). There's no reason you couldn't do something like that and explicitly set the flag you need on them. Dana, I misunderstood your comment before, thanks for the clarification. Adrienne, thanks for the example! I've updated the patch.
I spoke with twiz and enne today about an idea I had to avoid the need to set boundsContainPageScale on this layer at all: basically we only include pageScale in the contentsScale for layers in the subtree of the rootScrollLayer. I think this patch shouldn't worry about that. When the function to set contentsScale on all layers is changed, then the boundsContainPageScale should be removed here though. And hopefully writing this here will remind us all at that time. Cheers!
Ping, are we happy with this patch now?
Comment on attachment 163918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163918&action=review I like the changes. This code has all moved. Can you create a Chromium bug and rebase your Source/WebCore/platform/graphics/chromium/cc on top of /cc instead? I'll lgtm that. > Source/WebKit/chromium/src/NonCompositedContentHost.cpp:34 > +#include "LayerChromium.h" You can't include or use LayerChromium in NonCompositedContentHost. You have to go through WebContentLayer.
Created attachment 164610 [details] Patch
Comment on attachment 164610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164610&action=review > Source/WebKit/chromium/ChangeLog:8 > + Calls setAppliesPageScale(true) on root scrollbar layers as these are not scaled. can we test this? we have layout tests that cover high-DPI mode and we can use non-mock scrollbars on a given test by calling window.internals.setMockScrollbarsEnabled()
Comment on attachment 164610 [details] Patch R=me. Thanks for moving this code here.
(In reply to comment #37) > (From update of attachment 164610 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164610&action=review > > > Source/WebKit/chromium/ChangeLog:8 > > + Calls setAppliesPageScale(true) on root scrollbar layers as these are not scaled. > > can we test this? we have layout tests that cover high-DPI mode and we can use non-mock scrollbars on a given test by calling window.internals.setMockScrollbarsEnabled() Yes, there is a difference, however it will still be low DPI until after we land http://codereview.chromium.org/10909255/ which we can't land without this fix for page scale.
Comment on attachment 164610 [details] Patch Clearing flags on attachment: 164610 Committed r129023: <http://trac.webkit.org/changeset/129023>
All reviewed patches have been landed. Closing bug.