Currently, child windows may be positioned in document coordinates, which requires the BlackBerry::Platform::Graphics::Window::virtualRect() of the parent window to be kept in sync with the document visible content rect. This is easy if there's a one-to-one correspondence between windows and scrollable frames. However, for an embedder that can display an entire scene graph (where the web page is just one of the nodes) in one window, several scrollable frames may be present in that window, and it's difficult to know which scrollable frame to sync the virtualRect with. For the latter scenario, it makes more sense to use window coordinates to place child windows.
Created attachment 192884 [details] Patch
Comment on attachment 192884 [details] Patch Slightly unfortunate that we have to add m_childWindowsUseDocumentCoordinates twice, but understandable since the LayerRenderer will be lazily constructed. According to coding style rule Naming/5., m_childWindowsUseDocumentCoordinates should probably be called m_doChildWindowsUseDocumentCoordinates or m_shouldChildWindowsUseDocumentCoordinates. It's unfortunate that we have to talk about "windows" at all given they should be more of an abstract concept... then again, if we ever get rid of windows then this setting won't likely be needed anyways. So I guess that's okay. What do you think of using "PixelCoordinates" instead of "WindowCoordinates", to standardize on ViewportAccessor terminology? Also, how does this setting affect plugins and their video output? Is this change sufficient to cover Flash plugins in your use case?
(In reply to comment #2) > (From update of attachment 192884 [details]) > What do you think of using "PixelCoordinates" instead of "WindowCoordinates", to standardize on ViewportAccessor terminology? Retracting that comment, as WebCore itself uses Window and Screen in various client coordinate transformations so sticking with Window seems alright.
(In reply to comment #2) > (From update of attachment 192884 [details]) > Slightly unfortunate that we have to add m_childWindowsUseDocumentCoordinates twice, but understandable since the LayerRenderer will be lazily constructed. > According to coding style rule Naming/5., m_childWindowsUseDocumentCoordinates should probably be called m_doChildWindowsUseDocumentCoordinates or m_shouldChildWindowsUseDocumentCoordinates. > It's unfortunate that we have to talk about "windows" at all given they should be more of an abstract concept... then again, if we ever get rid of windows then this setting won't likely be needed anyways. So I guess that's okay. > What do you think of using "PixelCoordinates" instead of "WindowCoordinates", to standardize on ViewportAccessor terminology? > > Also, how does this setting affect plugins and their video output? Is this change sufficient to cover Flash plugins in your use case? I'm thinking the getter/setter should also be renamed since it's a bool property, shouldBlaBla would be more appropriate there, too.
(In reply to comment #2) > (From update of attachment 192884 [details]) > Slightly unfortunate that we have to add m_childWindowsUseDocumentCoordinates twice, but understandable since the LayerRenderer will be lazily constructed. > ... In the future, this kind of repetition can be avoided by introducing a LayerRendererClient interface with a "virtual bool shouldChildWindowsUseDocumentCoordinates() = 0;" method. But introducing a client interface for just this one method seems like overkill.
Created attachment 193106 [details] Patch
Comment on attachment 193106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193106&action=review > Source/WebCore/platform/graphics/blackberry/LayerRenderer.h:89 > + bool shouldChildWindowsUseDocumentCoordinates() const { return m_shouldChildWindowsUseDocumentCoordinates; } > + void setChildWindowsUseDocumentCoordinates(bool shouldUseDocumentCoordinates) { m_shouldChildWindowsUseDocumentCoordinates = shouldUseDocumentCoordinates; } Other code in WebCore includes the "Should" also in the setter, please update its name as well. Looks good otherwise, thanks!
And now for something completely different.
Created attachment 193292 [details] Patch
The getters/setters were getting more ridiculous with every iteration, time to try something completely different, where the "should" moniker only appears where it makes sense - in a delegate method.
Comment on attachment 193292 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193292&action=review Nice, I really like it! Only a few minor questions. > Source/WebCore/platform/graphics/blackberry/LayerRendererClient.h:38 > + virtual void layerRendererDestroyed() = 0; Should this one get a default implementation of { } so that it's optional for clients to implement it? > Source/WebCore/platform/graphics/blackberry/LayerRendererClient.h:43 > + virtual bool shouldClearSurfaceBeforeCompositing() = 0; > + virtual bool shouldChildWindowsUseDocumentCoordinates() = 0; Can these two be const? > Source/WebKit/blackberry/Api/WebPageCompositor_p.h:116 > + virtual void layerRendererDestroyed() { } > + virtual bool shouldClearSurfaceBeforeCompositing(); > + virtual bool shouldChildWindowsUseDocumentCoordinates(); If those are defined public by the interface, should they remain public after being inherited?
(In reply to comment #11) > (From update of attachment 193292 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193292&action=review > > Nice, I really like it! Only a few minor questions. > > > Source/WebCore/platform/graphics/blackberry/LayerRendererClient.h:38 > > + virtual void layerRendererDestroyed() = 0; > > Should this one get a default implementation of { } so that it's optional for clients to implement it? Yes, I agree - I tend to always add the "Destroyed" method to client interfaces, but in this case it's not actually needed since the client manages the lifetime of the object. > > > Source/WebCore/platform/graphics/blackberry/LayerRendererClient.h:43 > > + virtual bool shouldClearSurfaceBeforeCompositing() = 0; > > + virtual bool shouldChildWindowsUseDocumentCoordinates() = 0; > > Can these two be const? I prefer to keep client interfaces non-const where possible, to give the client maximum freedom in how to implement the decision, even to the degree where it requires mutating state. I looked at FrameLoaderClient and found a totally random assortment of const and non-const delegate methods (why would anyone ever use the bloated FrameLoaderClient as a yardstick), but I think the optimum is to keep them non-const. This leads to the question, why is context() const? Well, it's not really a delegate method, just a getter. > > > Source/WebKit/blackberry/Api/WebPageCompositor_p.h:116 > > + virtual void layerRendererDestroyed() { } > > + virtual bool shouldClearSurfaceBeforeCompositing(); > > + virtual bool shouldChildWindowsUseDocumentCoordinates(); > > If those are defined public by the interface, should they remain public after being inherited? I don't think so, since it's an implementation detail of WebPageCompositorPrivate that it happens to implement LayerRendererClient. But that's a matter of taste.
Created attachment 193374 [details] Patch Make LayerRendererClient::layerRendererDestroyed() optional
Comment on attachment 193374 [details] Patch This LGTM. Jakob?
(In reply to comment #14) > (From update of attachment 193374 [details]) > This LGTM. Jakob? Yes, let's have it. Thanks for your patience, Arvid!
(In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 193374 [details] [details]) > > This LGTM. Jakob? > > Yes, let's have it. Thanks for your patience, Arvid! No problem, just compare the first and last patches to see what improvement a proper review can produce =)
Comment on attachment 193374 [details] Patch Ok.
Comment on attachment 193374 [details] Patch Clearing flags on attachment: 193374 Committed r145953: <http://trac.webkit.org/changeset/145953>
All reviewed patches have been landed. Closing bug.