Bug 112236 – [BlackBerry] Allow an embedder to position child windows using window coordinates

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Arvid Nilsson]

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.

[Arvid Nilsson]

Created attachment 192884 [details]
Patch

[Jakob Petsovits]

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?

[Jakob Petsovits]

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

[Arvid Nilsson]

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

[Arvid Nilsson]

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

[Arvid Nilsson]

Created attachment 193106 [details]
Patch

[Jakob Petsovits]

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!

[Arvid Nilsson]

And now for something completely different.

[Arvid Nilsson]

Created attachment 193292 [details]
Patch

[Arvid Nilsson]

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.

[Jakob Petsovits]

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?

[Arvid Nilsson]

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

[Arvid Nilsson]

Created attachment 193374 [details]
Patch

Make LayerRendererClient::layerRendererDestroyed() optional

[Rob Buis]

Comment on attachment 193374 [details]
Patch

This LGTM. Jakob?

[Jakob Petsovits]

(In reply to comment #14)
> (From update of attachment 193374 [details])
> This LGTM. Jakob?

Yes, let's have it. Thanks for your patience, Arvid!

[Arvid Nilsson]

(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 =)

[Rob Buis]

Comment on attachment 193374 [details]
Patch

Ok.

[WebKit Review Bot]

Comment on attachment 193374 [details]
Patch

Clearing flags on attachment: 193374

Committed r145953: <http://trac.webkit.org/changeset/145953>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.