Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16799005:
Insert pinch zoom virtual viewport layers to graphics layer tree. (Closed)
|
Created:
7 years, 6 months ago by wjmaclean Modified:
7 years, 6 months ago CC:
blink-reviews, jamesr, eae+blinkwatch, leviw+renderwatch, abarth-chromium, danakj, Rik, jchaffraix+rendering, Stephen Chennney, jeez, pdr., WRONG-USE-chromium Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionInsert pinch zoom virtual viewport layers to graphics layer tree.
This cl adds the required layers to the graphics layer tree in order to support
the pinch zoom virtual viewport for fixed-position objects.
BUG=none
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152777
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add new viewport layers above overflowControlsHost. #
Total comments: 24
Patch Set 3 : Address review comments. #Patch Set 4 : Skip creating WebScrollbarLayers, defer to creation in CC. #
Total comments: 16
Patch Set 5 : Address James' comments, add WebLayerTreeView function interface. #
Total comments: 15
Patch Set 6 : Add WebLayerTreeView::clearPinchViewportLayers(). #Patch Set 7 : Address comments. #
Total comments: 14
Patch Set 8 : Address comments. #
Total comments: 13
Patch Set 9 : Address comments. #
Total comments: 4
Patch Set 10 : Address comments. #Patch Set 11 : Always set existing scrollbar drawsContent. #
Total comments: 1
Patch Set 12 : Patch for landing. #
Messages
Total messages: 34 (0 generated)
I decided to open a new issue for this cl since it is so different from https://codereview.chromium.org/16136004/. This is the Blink side of a CL to add layers required for pinch zoom virtual viewport. It adds these layers at the WebViewImpl level. This cl has a companion cl, https://codereview.chromium.org/16679011. This CL can be compiled and run independently of https://codereview.chromium.org/16679011 so long as the --enable-pinch-virtual-viewport flag is not specified.
https://codereview.chromium.org/16799005/diff/1/Source/WebKit/chromium/src/Pi... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/1/Source/WebKit/chromium/src/Pi... Source/WebKit/chromium/src/PinchViewports.cpp:89: void PinchViewports::insertViewportLayers() I feel this is too intrusive. I can't see a good reason why the PinchViewport layers needs to be inserted in the middle of RLC layers. IMO we should just make innerViewportClipLayer the new root layer, and attach RLC under PinchViewport. The layer tree will look like: *innerViewportClipLayer (root layer, fixed pos container) *pageScaleLayer *innerViewportScrollLayer overflowControlsHostLayer outerViewportClipLayer (fixed pos container) [frame clip layer in RenderLayerCompositor] outerViewportScrollLayer [frame scroll layer in RenderLayerCompositor] content layers ... horizontal ScrollbarLayer (non-overlay) verticalScrollbarLayer (non-overlay) scroll corner (non-overlay) *horizontalScrollbarLayer (overlay) [only added when existing scrollbars are non-overlay] *verticalScrollbarLayer (overlay) It is slightly different from the design doc but I think this is way cleaner. What do you guys think?
https://codereview.chromium.org/16799005/diff/1/Source/WebKit/chromium/src/Pi... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/1/Source/WebKit/chromium/src/Pi... Source/WebKit/chromium/src/PinchViewports.cpp:86: // *horizontalScrollbarLayer (overlay) [only added when existing scrollbars are non-overlay] Is this incorrect? I was under the impression that this set of scrollbars (the ones that are above the page scale layer) would always be overlay scrollbars and that the other scrollbars (the ones under the page scale layer) would only optionally be there for non-Android platforms?
On 2013/06/12 21:43:52, enne wrote: > https://codereview.chromium.org/16799005/diff/1/Source/WebKit/chromium/src/Pi... > File Source/WebKit/chromium/src/PinchViewports.cpp (right): > > https://codereview.chromium.org/16799005/diff/1/Source/WebKit/chromium/src/Pi... > Source/WebKit/chromium/src/PinchViewports.cpp:86: // > *horizontalScrollbarLayer (overlay) [only added when existing scrollbars are > non-overlay] > Is this incorrect? > > I was under the impression that this set of scrollbars (the ones that are above > the page scale layer) would always be overlay scrollbars and that the other > scrollbars (the ones under the page scale layer) would only optionally be there > for non-Android platforms? I think you're right. For Android we want the inner scrollbar as well. (And disable outer scrollbar on main frame. sub-frames will still use outer scrollbar obviously.)
On 2013/06/13 01:19:54, trchen wrote: > On 2013/06/12 21:43:52, enne wrote: > > > https://codereview.chromium.org/16799005/diff/1/Source/WebKit/chromium/src/Pi... > > File Source/WebKit/chromium/src/PinchViewports.cpp (right): > > > > > https://codereview.chromium.org/16799005/diff/1/Source/WebKit/chromium/src/Pi... > > Source/WebKit/chromium/src/PinchViewports.cpp:86: // > > *horizontalScrollbarLayer (overlay) [only added when existing scrollbars are > > non-overlay] > > Is this incorrect? > > > > I was under the impression that this set of scrollbars (the ones that are > above > > the page scale layer) would always be overlay scrollbars and that the other > > scrollbars (the ones under the page scale layer) would only optionally be > there > > for non-Android platforms? > > I think you're right. For Android we want the inner scrollbar as well. (And > disable outer scrollbar on main frame. sub-frames will still use outer scrollbar > obviously.) Sorry, perhaps the code comment is misleading. We will *always* have the inner-frame, overlay scrollbars. But, the way we achieve this differs between Android and non-Android. For Android, I was just going to leave the existing, overlay scrollbars and not move them. They will be attached to the inner viewport and work as expected. For non-Android, the existing scrollbars are non-overlay, so my plan is to just move them to the outer scroll layer, and create new overlay scrollbars. This simplifies the changes, as the necessary clipping mechanisms are already in place.
On 2013/06/12 21:37:59, trchen wrote: > https://codereview.chromium.org/16799005/diff/1/Source/WebKit/chromium/src/Pi... > File Source/WebKit/chromium/src/PinchViewports.cpp (right): > > https://codereview.chromium.org/16799005/diff/1/Source/WebKit/chromium/src/Pi... > Source/WebKit/chromium/src/PinchViewports.cpp:89: void > PinchViewports::insertViewportLayers() > I feel this is too intrusive. I can't see a good reason why the PinchViewport > layers needs to be inserted in the middle of RLC layers. IMO we should just make > innerViewportClipLayer the new root layer, and attach RLC under PinchViewport. > The layer tree will look like: This "intrusiveness" is partly why I wanted to put this creation logic into RLC. In your diagram, the overlay scrollbars appear to be under the page-scroll layer, meaning they'll get scaled, is that really what you intended?
On 2013/06/13 13:05:01, wjmaclean wrote: > On 2013/06/12 21:37:59, trchen wrote: > > > https://codereview.chromium.org/16799005/diff/1/Source/WebKit/chromium/src/Pi... > > File Source/WebKit/chromium/src/PinchViewports.cpp (right): > > > > > https://codereview.chromium.org/16799005/diff/1/Source/WebKit/chromium/src/Pi... > > Source/WebKit/chromium/src/PinchViewports.cpp:89: void > > PinchViewports::insertViewportLayers() > > > I feel this is too intrusive. I can't see a good reason why the PinchViewport > > layers needs to be inserted in the middle of RLC layers. IMO we should just > make > > innerViewportClipLayer the new root layer, and attach RLC under PinchViewport. > > The layer tree will look like: > > This "intrusiveness" is partly why I wanted to put this creation logic into RLC. > > In your diagram, the overlay scrollbars appear to be under the page-scroll > layer, meaning they'll get scaled, is that really what you intended? Oops, that's an indent error. I mean this: *innerViewportClipLayer (root layer, fixed pos container) *pageScaleLayer *innerViewportScrollLayer overflowControlsHostLayer outerViewportClipLayer (fixed pos container) [frame clip layer in RenderLayerCompositor] outerViewportScrollLayer [frame scroll layer in RenderLayerCompositor] content layers ... horizontal ScrollbarLayer (non-overlay) verticalScrollbarLayer (non-overlay) scroll corner (non-overlay) *horizontalScrollbarLayer (overlay) [only added when existing scrollbars are non-overlay] *verticalScrollbarLayer (overlay) So the pinch zoom layers won't interleave with RLC layers.
On 2013/06/13 17:52:37, trchen wrote: > On 2013/06/13 13:05:01, wjmaclean wrote: > > On 2013/06/12 21:37:59, trchen wrote: > > > > > > https://codereview.chromium.org/16799005/diff/1/Source/WebKit/chromium/src/Pi... > > > File Source/WebKit/chromium/src/PinchViewports.cpp (right): > > > > > > > > > https://codereview.chromium.org/16799005/diff/1/Source/WebKit/chromium/src/Pi... > > > Source/WebKit/chromium/src/PinchViewports.cpp:89: void > > > PinchViewports::insertViewportLayers() > > > > > I feel this is too intrusive. I can't see a good reason why the > PinchViewport > > > layers needs to be inserted in the middle of RLC layers. IMO we should just > > make > > > innerViewportClipLayer the new root layer, and attach RLC under > PinchViewport. > > > The layer tree will look like: > > > > This "intrusiveness" is partly why I wanted to put this creation logic into > RLC. > > > > In your diagram, the overlay scrollbars appear to be under the page-scroll > > layer, meaning they'll get scaled, is that really what you intended? > > Oops, that's an indent error. I mean this: > *innerViewportClipLayer (root layer, fixed pos container) > *pageScaleLayer > *innerViewportScrollLayer > overflowControlsHostLayer > outerViewportClipLayer (fixed pos container) [frame clip layer > in RenderLayerCompositor] > outerViewportScrollLayer [frame scroll layer in > RenderLayerCompositor] > content layers ... > horizontal ScrollbarLayer (non-overlay) > verticalScrollbarLayer (non-overlay) > scroll corner (non-overlay) > *horizontalScrollbarLayer (overlay) [only added when existing scrollbars > are non-overlay] > *verticalScrollbarLayer (overlay) > > So the pinch zoom layers won't interleave with RLC layers. So I'm not intrinsically opposed to this ... I was just trying to stay within the confines of the design doc, but I don't see any reason why we couldn't do this, if all the design-doc stakeholders are ok with it.
On 2013/06/13 18:01:09, wjmaclean wrote: > So I'm not intrinsically opposed to this ... I was just trying to stay within > the confines of the design doc, but I don't see any reason why we couldn't do > this, if all the design-doc stakeholders are ok with it. It certainly seems like it'd be easier to just reparent the layers coming out of RLC rather than trying to insert layers. If keeping the overflow controls host layer makes things cleaner, then let's do that.
PTAL
Overall LGTM. Thanks for the great work! https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.h (right): https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:57: GraphicsLayer* rootGraphicsLayer() { return m_innerViewportClipLayer.get(); } nits: 80 chars https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:62: const static size_t kOverlayScrollbarThickness; Should we make it a WebSettings? https://codereview.chromium.org/16799005/diff/11001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerCompositor.h (right): https://codereview.chromium.org/16799005/diff/11001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerCompositor.h:141: GraphicsLayer* clipLayer() const; nits: We probably don't need this accessor anymore.
I'm most skeptical about the scrollbar changes, but I'll take a look at the other CL for that. https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:44: const size_t PinchViewports::kOverlayScrollbarThickness = 10; This seems a little out of place. How do the current overlay scrollbars determine their thickness? https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:82: // Implement destructor here as it needs access to the definition of WebScrollbar I don't think this comment really adds much. https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:90: // Use bounds of scroll layer to communicate size of scrollbars. Scroll layers should be the size of the the scrollable area if you're going to size them. Please communicate this some other way. https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:129: // We only need to create overlay scrollbars for the frame and move the ..."move"? https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:138: if (GraphicsLayer* scrollbar = m_owner->compositor()->layerForHorizontalScrollbar()) This seems a little intrusive to go modify some layers that the RLC has created. Maybe the top-level RLC shouldn't create scrollbars that aren't going to be used? https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:180: scrollbarGraphicsLayer->setNeedsDisplay(); Blink invalidates needlessly too often. Can you early out here if the contents rect hasn't changed? https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:183: // FIXME: this function will be removed before this CL lands, please ignore for review. Instead of FIXME, I'd recommend using DO NOT SUBMIT, which will fail presubmit for committing so you can't accidentally land it. https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:3871: Document* document = page()->mainFrame()->document(); Can any of these things be NULL?
https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:44: const size_t PinchViewports::kOverlayScrollbarThickness = 10; On 2013/06/13 22:45:07, enne wrote: > This seems a little out of place. How do the current overlay scrollbars > determine their thickness? ScrollbarTheme::scrollbarThickness(). Platform implementation provide hardcoded overrides. Although device scale factor will compensate for pixel density, I think we still want different thickness for various kind of devices (phones, tablets, netbooks). Perhaps make it a WebSettings? https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:138: if (GraphicsLayer* scrollbar = m_owner->compositor()->layerForHorizontalScrollbar()) On 2013/06/13 22:45:07, enne wrote: > This seems a little intrusive to go modify some layers that the RLC has created. > Maybe the top-level RLC shouldn't create scrollbars that aren't going to be > used? Currently the scrollability of many objects relies on the existence of scrollbars. bokan@ is working on this issue: crbug.com/247055
PTAL https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:44: const size_t PinchViewports::kOverlayScrollbarThickness = 10; On 2013/06/13 23:10:31, trchen wrote: > On 2013/06/13 22:45:07, enne wrote: > > This seems a little out of place. How do the current overlay scrollbars > > determine their thickness? > > ScrollbarTheme::scrollbarThickness(). Platform implementation provide hardcoded > overrides. Although device scale factor will compensate for pixel density, I > think we still want different thickness for various kind of devices (phones, > tablets, netbooks). Perhaps make it a WebSettings? Done. Right now I've made an accessor and a default value, assuming we can plumb setters in a separate CL once the viewport work is more mature. https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:82: // Implement destructor here as it needs access to the definition of WebScrollbar On 2013/06/13 22:45:07, enne wrote: > I don't think this comment really adds much. Done. https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:90: // Use bounds of scroll layer to communicate size of scrollbars. On 2013/06/13 22:45:07, enne wrote: > Scroll layers should be the size of the the scrollable area if you're going to > size them. Please communicate this some other way. Hmm, ok. Is it reasonable to have the WebScrollbar instance look to the parent of this layer for size information do you think then? If so, do you prefer that we pass both layer pointers to the constructor, or just discover the clip layer as the parent of the scroll layer? In either case, I've removed this sizing. https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:129: // We only need to create overlay scrollbars for the frame and move the On 2013/06/13 22:45:07, enne wrote: > ..."move"? Stale comment, fixed. https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:138: if (GraphicsLayer* scrollbar = m_owner->compositor()->layerForHorizontalScrollbar()) On 2013/06/13 23:10:31, trchen wrote: > On 2013/06/13 22:45:07, enne wrote: > > This seems a little intrusive to go modify some layers that the RLC has > created. > > Maybe the top-level RLC shouldn't create scrollbars that aren't going to be > > used? > > Currently the scrollability of many objects relies on the existence of > scrollbars. bokan@ is working on this issue: crbug.com/247055 I've added a FIXME with this bug number indicating we should just suppress the creation of these scrollbars in RLC if they're not going to be used (though technically that violates the "RLC should not know about the outer viewport" philosophy). https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:180: scrollbarGraphicsLayer->setNeedsDisplay(); On 2013/06/13 22:45:07, enne wrote: > Blink invalidates needlessly too often. Can you early out here if the contents > rect hasn't changed? Done. https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:183: // FIXME: this function will be removed before this CL lands, please ignore for review. On 2013/06/13 22:45:07, enne wrote: > Instead of FIXME, I'd recommend using DO NOT SUBMIT, which will fail presubmit > for committing so you can't accidentally land it. Did not know about that, will definitely use it from now on! Done. https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.h (right): https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:57: GraphicsLayer* rootGraphicsLayer() { return m_innerViewportClipLayer.get(); } On 2013/06/13 21:11:54, trchen wrote: > nits: 80 chars Done. https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:62: const static size_t kOverlayScrollbarThickness; On 2013/06/13 21:11:54, trchen wrote: > Should we make it a WebSettings? Sure, seems reasonable. https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/16799005/diff/11001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:3871: Document* document = page()->mainFrame()->document(); On 2013/06/13 22:45:07, enne wrote: > Can any of these things be NULL? Yes, we should put in checks. I'm not sure what the rules are regarding whether mainFrame() ever has a null document(), or whether those rules could ever change, so I'll just exhaustively check them. Done. https://codereview.chromium.org/16799005/diff/11001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerCompositor.h (right): https://codereview.chromium.org/16799005/diff/11001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerCompositor.h:141: GraphicsLayer* clipLayer() const; On 2013/06/13 21:11:54, trchen wrote: > nits: We probably don't need this accessor anymore. Done.
Based on conversation with jamesr@ I have modified this CL to *not* create WebScrollbarLayers, assuming instead that CC will create and attach the necessary ScrollbarLayers (this will happen when we send the layer ids to LayerTreeHost for the various layers in the viewport/overlay scrollbar structure)
lgtm
https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:32: #include "PinchViewports.h" please specify paths relative to Source/ https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.h (right): https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:49: using namespace WebCore; specific using statements are strongly preferred in headers. https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:53: static WTF::PassOwnPtr<PinchViewports> create(WebViewImpl* owner); no WTF:: needed - wtf/OwnPtr.h has a using statement that puts it in the global namespace (As does most of the rest of WTF). You'll notice other code in Blink does not explicitly qualify these https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:65: PinchViewports(WebViewImpl* owner); explicit. why are all these protected and not private? https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:70: WTF::OwnPtr<GraphicsLayer> m_innerViewportClipLayer; no WTF:: https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebSettingsImpl.cpp (right): https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebSettingsImpl.cpp:61: , m_pinchOverlayScrollbarThickness(10) could this be a named constant if it's meant to be meaningful or initialized to 0 if the embedder is required to set it? https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:3813: m_pinchViewports = PinchViewports::create(this); do you need to tear the pinch viewports down when leaving compositing mode? https://codereview.chromium.org/16799005/diff/26001/Source/core/platform/grap... File Source/core/platform/graphics/GraphicsLayer.cpp (right): https://codereview.chromium.org/16799005/diff/26001/Source/core/platform/grap... Source/core/platform/graphics/GraphicsLayer.cpp:723: if (m_client && m_client->isTrackingRepaints()) { I think it'd be better to always create a GraphicsLayerClient, even if it's a no-op. The GraphicsLayerClient interface is small
PTAL (I'm in the process of updating the companion patch). https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:32: #include "PinchViewports.h" On 2013/06/14 20:55:46, jamesr wrote: > please specify paths relative to Source/ Done. https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.h (right): https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:49: using namespace WebCore; On 2013/06/14 20:55:46, jamesr wrote: > specific using statements are strongly preferred in headers. Done. https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:53: static WTF::PassOwnPtr<PinchViewports> create(WebViewImpl* owner); On 2013/06/14 20:55:46, jamesr wrote: > no WTF:: needed - wtf/OwnPtr.h has a using statement that puts it in the global > namespace (As does most of the rest of WTF). You'll notice other code in Blink > does not explicitly qualify these Done. https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:65: PinchViewports(WebViewImpl* owner); On 2013/06/14 20:55:46, jamesr wrote: > explicit. why are all these protected and not private? D'oh! I have no clue what I was thinking there! Done. https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:70: WTF::OwnPtr<GraphicsLayer> m_innerViewportClipLayer; On 2013/06/14 20:55:46, jamesr wrote: > no WTF:: Done. https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebSettingsImpl.cpp (right): https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebSettingsImpl.cpp:61: , m_pinchOverlayScrollbarThickness(10) On 2013/06/14 20:55:46, jamesr wrote: > could this be a named constant if it's meant to be meaningful or initialized to > 0 if the embedder is required to set it? Done. https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/16799005/diff/26001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:3813: m_pinchViewports = PinchViewports::create(this); On 2013/06/14 20:55:46, jamesr wrote: > do you need to tear the pinch viewports down when leaving compositing mode? I don't *think* so, though I could be convinced otherwise. Here's my reasoning: 1) I assume "leaving compositing mode" corresponds to (layer == 0) in WebViewImpl::setRootGraphicsLayer(). In this case, any existing viewport layers are disconnected from the tree, and so do nothing. 2) When the root graphics layer changes (and typically the RenderLayerCompositor behind it), we disconnect and then re-connect the viewport layers. Since they don't rely on anything outside PinchViewports (e.g. they don't rely on RLC to be a client), this re-use should be OK. In practice building the viewport layer structure once and re-using it seems to have worked fine, and I am unaware if any reason why it shouldn't be ok. But, I'm also OK with tearing it down and rebuilding each time if that seems safer. https://codereview.chromium.org/16799005/diff/26001/Source/core/platform/grap... File Source/core/platform/graphics/GraphicsLayer.cpp (right): https://codereview.chromium.org/16799005/diff/26001/Source/core/platform/grap... Source/core/platform/graphics/GraphicsLayer.cpp:723: if (m_client && m_client->isTrackingRepaints()) { On 2013/06/14 20:55:46, jamesr wrote: > I think it'd be better to always create a GraphicsLayerClient, even if it's a > no-op. The GraphicsLayerClient interface is small OK, done. That being said, it seems weirdly inconsistent that we check in numerous other places to see if we have a valid client, but not here. I would think either we should always check, or never check. (It was primarily the existence of these other checks that convinced me to code it with a null client ... a previous version of this CL had a client ;-) )
https://codereview.chromium.org/16799005/diff/35001/public/platform/WebLayerT... File public/platform/WebLayerTreeView.h (right): https://codereview.chromium.org/16799005/diff/35001/public/platform/WebLayerT... public/platform/WebLayerTreeView.h:126: WebLayer* innerViewportClipLayer, Ooops, I'll change these to be "const WebLayer*"
Lookin' better! https://codereview.chromium.org/16799005/diff/35001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:46: WTF::PassOwnPtr<PinchViewports> PinchViewports::create(WebViewImpl* owner) No WTF:: https://codereview.chromium.org/16799005/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:152: OwnPtr<GraphicsLayer>& scrollbarGraphicsLayer = isHorizontal(orientation) ? i think having this be a reference to an OwnPtr is very confusing since there isn't actually any change in ownership implied here. It'd be clearer to just take a raw pointer to the appropriate scrollbar https://codereview.chromium.org/16799005/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:162: int height = isHorizontal(orientation) ? kOverlayScrollbarThickness : m_innerViewportClipLayer->size().height() - kOverlayScrollbarThickness; perhaps store isHorizontal(orientation) in a local bool instead of calling the function 5 times? https://codereview.chromium.org/16799005/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:168: void PinchViewports::registerViewportLayersWithTreeView(WebLayerTreeView* treeView) const i don't think we use the variable name 'treeView' anywhere - how about 'layerTreeView' ? https://codereview.chromium.org/16799005/diff/35001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.h (right): https://codereview.chromium.org/16799005/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:47: using WebCore::GraphicsContext; since you're in a header and there's only one use of most these types, it'd be cleaner to just fully qualify them https://codereview.chromium.org/16799005/diff/35001/Source/core/platform/grap... File Source/core/platform/graphics/GraphicsLayer.cpp (right): https://codereview.chromium.org/16799005/diff/35001/Source/core/platform/grap... Source/core/platform/graphics/GraphicsLayer.cpp:723: if (m_client && m_client->isTrackingRepaints()) { you don't need this change, do you? https://codereview.chromium.org/16799005/diff/35001/Source/core/platform/grap... Source/core/platform/graphics/GraphicsLayer.cpp:939: if (m_client) ditto
PTAL, I think I got everything ... https://codereview.chromium.org/16799005/diff/35001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:46: WTF::PassOwnPtr<PinchViewports> PinchViewports::create(WebViewImpl* owner) On 2013/06/17 20:42:10, jamesr wrote: > No WTF:: Done. https://codereview.chromium.org/16799005/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:152: OwnPtr<GraphicsLayer>& scrollbarGraphicsLayer = isHorizontal(orientation) ? On 2013/06/17 20:42:10, jamesr wrote: > i think having this be a reference to an OwnPtr is very confusing since there > isn't actually any change in ownership implied here. It'd be clearer to just > take a raw pointer to the appropriate scrollbar Done. https://codereview.chromium.org/16799005/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:162: int height = isHorizontal(orientation) ? kOverlayScrollbarThickness : m_innerViewportClipLayer->size().height() - kOverlayScrollbarThickness; On 2013/06/17 20:42:10, jamesr wrote: > perhaps store isHorizontal(orientation) in a local bool instead of calling the > function 5 times? Done. https://codereview.chromium.org/16799005/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:168: void PinchViewports::registerViewportLayersWithTreeView(WebLayerTreeView* treeView) const On 2013/06/17 20:42:10, jamesr wrote: > i don't think we use the variable name 'treeView' anywhere - how about > 'layerTreeView' ? Done. https://codereview.chromium.org/16799005/diff/35001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.h (right): https://codereview.chromium.org/16799005/diff/35001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:47: using WebCore::GraphicsContext; On 2013/06/17 20:42:10, jamesr wrote: > since you're in a header and there's only one use of most these types, it'd be > cleaner to just fully qualify them Done. https://codereview.chromium.org/16799005/diff/35001/Source/core/platform/grap... File Source/core/platform/graphics/GraphicsLayer.cpp (right): https://codereview.chromium.org/16799005/diff/35001/Source/core/platform/grap... Source/core/platform/graphics/GraphicsLayer.cpp:723: if (m_client && m_client->isTrackingRepaints()) { On 2013/06/17 20:42:10, jamesr wrote: > you don't need this change, do you? No, I guess not. :-) https://codereview.chromium.org/16799005/diff/35001/Source/core/platform/grap... Source/core/platform/graphics/GraphicsLayer.cpp:939: if (m_client) On 2013/06/17 20:42:10, jamesr wrote: > ditto Done.
https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:95: // DO NOT SUBMIT: this next line will be removed before this CL lands, please ignore for review. don't forget this https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:143: // DO NOT SUBMIT: this next line will be removed before this CL lands, please ignore for review. or this https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:198: // DO NOT SUBMIT: this function will be removed before this CL lands, please ignore for review. or this https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.h (right): https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:65: void showGraphicsLayers(); i don't think you want to check this in https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebSettingsImpl.h (right): https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebSettingsImpl.h:195: int m_pinchOverlayScrollbarThickness; why the newline? https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1618: m_pinchViewports->setViewportSize(FloatSize(m_size.width, m_size.height)); this appears to be the only caller to setViewportSize(), so what makes sure the viewport size is set if compositing mode is entered without a resize? why is this a FloatSize if the size is always an int? https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.h (right): https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.h:846: OwnPtr<PinchViewports> m_pinchViewports; please move this up to live by the other compositor-related members up at lines 807-817
PTAL Debugging code is out, and comments addressed. https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:95: // DO NOT SUBMIT: this next line will be removed before this CL lands, please ignore for review. On 2013/06/17 22:16:34, jamesr wrote: > don't forget this Done. https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:143: // DO NOT SUBMIT: this next line will be removed before this CL lands, please ignore for review. On 2013/06/17 22:16:34, jamesr wrote: > or this Done. https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:198: // DO NOT SUBMIT: this function will be removed before this CL lands, please ignore for review. On 2013/06/17 22:16:34, jamesr wrote: > or this Done. https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.h (right): https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:65: void showGraphicsLayers(); On 2013/06/17 22:16:34, jamesr wrote: > i don't think you want to check this in Done. https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebSettingsImpl.h (right): https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebSettingsImpl.h:195: int m_pinchOverlayScrollbarThickness; On 2013/06/17 22:16:34, jamesr wrote: > why the newline? Done. https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1618: m_pinchViewports->setViewportSize(FloatSize(m_size.width, m_size.height)); On 2013/06/17 22:16:34, jamesr wrote: > this appears to be the only caller to setViewportSize(), so what makes sure the > viewport size is set if compositing mode is entered without a resize? Thus far my observation is that we always seem to get a resize, but I can add this to setRootGraphicsLayer() as well. > why is this a FloatSize if the size is always an int? GraphicsLayer::setSize() requires FloatRect, and it seemed easier to convert it here, but I've moved this conversion into PinchViewport.cpp. https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.h (right): https://codereview.chromium.org/16799005/diff/38002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.h:846: OwnPtr<PinchViewports> m_pinchViewports; On 2013/06/17 22:16:34, jamesr wrote: > please move this up to live by the other compositor-related members up at lines > 807-817 Done.
Looking pretty good, all my comments below are minor. https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:89: ASSERT(m_innerViewportClipLayer); nit: no need for this assertion since you create it in constructor. https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:132: // FIXME: If we knew in advance before the overflowControlsHostLayer goes I don't entirely follow what this problem is; why can't you fix this as part of this patch? https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:135: // issue crbug.com/247055, inhibit the creation of these scrollbars when they're not needed. Why should we ever inhibit their creation? They may unexpectedly need to appear at any time due to an impl-thread handled gesture. I suggest deleting this FIXME comment. https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:150: ASSERT(scrollbarGraphicsLayer); nit: also no need for this assert since it's created in the constructor. https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.h (right): https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:64: void setViewportSize(const WebSize&); nit: use WebCore::IntSize since this isn't a Chromium-facing API. https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:3824: if (m_pinchViewports) { nit: to clarify what's guarded by setting, I suggest writing the branches this way: if (page()->settings()->pinchVirtualViewportEnabled()) { if (!m_pinchViewports) m_pinchViewports = PinchViewports::create(this); ... } else { ... }
PTAL - addressed aelias@'s comments. https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:89: ASSERT(m_innerViewportClipLayer); On 2013/06/18 19:52:28, aelias wrote: > nit: no need for this assertion since you create it in constructor. Done. https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:132: // FIXME: If we knew in advance before the overflowControlsHostLayer goes On 2013/06/18 19:52:28, aelias wrote: > I don't entirely follow what this problem is; why can't you fix this as part of > this patch? We only ever find out (I think) that the old compositor has gone away when WVI::setRootGraphicsLayer() is called with layer == 0, by which time it's too late to call WVI::compositor() and retrieve the two scrollbar layers so we can re-enable the drawing. Now, it may not be an issue, as I suspect by this time they're destroyed anyways, but I'd like to return the layers to their original state in case they're reused at a later time. Is there a notification *before* the existing rootGraphicsLayer/compositor go away? https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:135: // issue crbug.com/247055, inhibit the creation of these scrollbars when they're not needed. On 2013/06/18 19:52:28, aelias wrote: > Why should we ever inhibit their creation? They may unexpectedly need to appear > at any time due to an impl-thread handled gesture. I suggest deleting this > FIXME comment. Done. https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:150: ASSERT(scrollbarGraphicsLayer); On 2013/06/18 19:52:28, aelias wrote: > nit: also no need for this assert since it's created in the constructor. Done. https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.h (right): https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.h:64: void setViewportSize(const WebSize&); On 2013/06/18 19:52:28, aelias wrote: > nit: use WebCore::IntSize since this isn't a Chromium-facing API. Done. https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:3824: if (m_pinchViewports) { On 2013/06/18 19:52:28, aelias wrote: > nit: to clarify what's guarded by setting, I suggest writing the branches this > way: > > if (page()->settings()->pinchVirtualViewportEnabled()) { > if (!m_pinchViewports) > m_pinchViewports = PinchViewports::create(this); > ... > } else { > ... > } That is clearer, thanks! Done.
mostly lgtm https://codereview.chromium.org/16799005/diff/14002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/14002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:145: const int kOverlayScrollbarThickness = m_owner->settingsImpl()->pinchOverlayScrollbarThickness(); this isn't a static constant so it shouldn't get a 'k' prefix. just name it like a normal variable https://codereview.chromium.org/16799005/diff/14002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebSettingsImpl.cpp (right): https://codereview.chromium.org/16799005/diff/14002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebSettingsImpl.cpp:63: , m_pinchOverlayScrollbarThickness(10) this has switched between '0' and '10' in different patch sets. which do you intend to land? i think 0 as a default makes more sense
Fixed remaining comments, plus added setter for the default overlay scrollbar width in WebSettings/WebSettingsImpl. https://codereview.chromium.org/16799005/diff/14002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/14002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:145: const int kOverlayScrollbarThickness = m_owner->settingsImpl()->pinchOverlayScrollbarThickness(); On 2013/06/18 20:21:58, jamesr wrote: > this isn't a static constant so it shouldn't get a 'k' prefix. just name it like > a normal variable Done. https://codereview.chromium.org/16799005/diff/14002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebSettingsImpl.cpp (right): https://codereview.chromium.org/16799005/diff/14002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebSettingsImpl.cpp:63: , m_pinchOverlayScrollbarThickness(10) On 2013/06/18 20:21:58, jamesr wrote: > this has switched between '0' and '10' in different patch sets. which do you > intend to land? i think 0 as a default makes more sense Sorry about that ... it was intended to (and will!) land as 0 (see new patch). I changed this for ongoing development work and forgot to set it back!
https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/PinchViewports.cpp (right): https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/PinchViewports.cpp:132: // FIXME: If we knew in advance before the overflowControlsHostLayer goes On 2013/06/18 20:11:40, wjmaclean wrote: > On 2013/06/18 19:52:28, aelias wrote: > > I don't entirely follow what this problem is; why can't you fix this as part > of > > this patch? > > We only ever find out (I think) that the old compositor has gone away when > WVI::setRootGraphicsLayer() is called with layer == 0, by which time it's too > late to call WVI::compositor() and retrieve the two scrollbar layers so we can > re-enable the drawing. > > Now, it may not be an issue, as I suspect by this time they're destroyed > anyways, but I'd like to return the layers to their original state in case > they're reused at a later time. > > Is there a notification *before* the existing rootGraphicsLayer/compositor go > away? I suggest removing the early return and doing: scrollbar->setDrawsContent(!page->mainFrame()->view()->hasOverlayScrollbars()); then you set it to the right value on every attach, and don't have to do anything on detach.
On 2013/06/18 22:25:59, aelias wrote: > https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... > File Source/WebKit/chromium/src/PinchViewports.cpp (right): > > https://codereview.chromium.org/16799005/diff/49001/Source/WebKit/chromium/sr... > Source/WebKit/chromium/src/PinchViewports.cpp:132: // FIXME: If we knew in > advance before the overflowControlsHostLayer goes > On 2013/06/18 20:11:40, wjmaclean wrote: > > On 2013/06/18 19:52:28, aelias wrote: > > > I don't entirely follow what this problem is; why can't you fix this as part > > of > > > this patch? > > > > We only ever find out (I think) that the old compositor has gone away when > > WVI::setRootGraphicsLayer() is called with layer == 0, by which time it's too > > late to call WVI::compositor() and retrieve the two scrollbar layers so we can > > re-enable the drawing. > > > > Now, it may not be an issue, as I suspect by this time they're destroyed > > anyways, but I'd like to return the layers to their original state in case > > they're reused at a later time. > > > > Is there a notification *before* the existing rootGraphicsLayer/compositor go > > away? > > I suggest removing the early return and doing: > > scrollbar->setDrawsContent(!page->mainFrame()->view()->hasOverlayScrollbars()); > > then you set it to the right value on every attach, and don't have to do > anything on detach. We can do that, though it's not the problem I'm worried about. I'm worried about the case where (1) we set drawsContent() == false on the existing scrollbars because they're overlay, and (2) the graphics layer tree is detached and reused somewhere else *without* an inner/outer viewport structure. Now, maybe I'm being overly cautious (I can't easily imaging this scenario really happening). But, could it happen? Also, I generally imagine that page() will always be non-null when this function is invoke, but I'm nervous about removing that early out ... is it possible for page() to be null in this case?
I don't think either of those will happen outside of maybe tests. Anyway, lgtm.
https://codereview.chromium.org/16799005/diff/10003/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebSettingsImpl.cpp (right): https://codereview.chromium.org/16799005/diff/10003/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebSettingsImpl.cpp:63: , m_pinchOverlayScrollbarThickness(10) still 10 ?
On 2013/06/19 20:17:06, jamesr wrote: > https://codereview.chromium.org/16799005/diff/10003/Source/WebKit/chromium/sr... > File Source/WebKit/chromium/src/WebSettingsImpl.cpp (right): > > https://codereview.chromium.org/16799005/diff/10003/Source/WebKit/chromium/sr... > Source/WebKit/chromium/src/WebSettingsImpl.cpp:63: , > m_pinchOverlayScrollbarThickness(10) > still 10 ? Argh ... shouldh've put a "DO NOT SUBMIT" on it ... removed ;-)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/16799005/66002
Message was sent while issue was closed.
Change committed as 152777 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
