Bug 56130 – [Qt] Plugin is not scrolled together with page content or jumping when content is rendered using cache (backing store).

• Component: New Bugs
• Status: RESOLVED FIXED

[Misha]

Created attachment 85361 [details]
test page

In order to reproduce tiled backing store has to be on. Load page that has a plugin and try to scroll it. 
Depending on the size of the page and window size plugin will stay on the same place or jump "trying to catch" page content. 
The reason for it is that in void PluginView::updatePluginWidget() we updating plugin position only if 
plugin is outside frame window.  Here is what comment says:

    // (i) in order to move/resize the plugin window at the same time as the
    // rest of frame during e.g. scrolling, we set the window geometry
    // in the paint() function, but as paint() isn't called when the
    // plugin window is outside the frame which can be caused by a
    // scroll, we need to move/resize immediately.
    // (ii) if we are running layout tests from DRT, paint() won't ever get called
    // so we need to call setNPWindowIfNeeded() if window geometry has changed

The problem here is that the PluginView::paint() will be called only when cache (tiled backing store) 
needs to be updated. Until then plugin will stay on the same place. The issue is reproducible with 
windowed plugin on Linux and with both windowed and windowless on Symbian. In case of windowless plugin on
Symbian plugin still needs to have the right position.

[Misha]

Created attachment 85362 [details]
flash for test page

[Misha]

Created attachment 85366 [details]
patch to fix plugin position

[Andreas Kling]

How does this change affect the non-tiling case?
CC'ing the Qt plugin kids.

[Robert Hogan]

https://bugs.webkit.org/show_bug.cgi?id=52735 seens related.

The bug in 52735 was down to relative versus absolute co-ordinates. Pluginwidgetqt was dirtying the relative co-ordinates, while scrollview was looking for dirty absolute co-ordinates with the result that certain scroll actions could happen and scrollview would think that pluginwidgetqt had not dirtied the plugin area.

Is there a need for a tiledBackingStore()->invalidate() of some sort?

[Misha]

(In reply to comment #4)
> https://bugs.webkit.org/show_bug.cgi?id=52735 seens related.
> 
> The bug in 52735 was down to relative versus absolute co-ordinates. Pluginwidgetqt was dirtying the relative co-ordinates, while scrollview was looking for dirty absolute co-ordinates with the result that certain scroll actions could happen and scrollview would think that pluginwidgetqt had not dirtied the plugin area.
> 
> Is there a need for a tiledBackingStore()->invalidate() of some sort?


I've tried with tiledBackingStore()->invalidate(). Yes it makes plugin moving but I dout that it's a right approach. Basically, what is happening in this case we just make  PluginView::paint() get invoked but in addition we also also alwyas re-paint tiles under plugin. So in result for each PluginView::updatePluginWidget() we not only call setNPWindowIfNeeded() but also do a paint() and frequency of setNPWindowIfNeeded() is not lower.
I want to understand better what is wrong with calling setNPWindowIfNeeded() from PluginView::updatePluginWidget()? What is wrong with plugin getting information about new position of the plugin window?

[Alexis Menard (darktears)]

Comment on attachment 85366 [details]
patch to fix plugin position

View in context: https://bugs.webkit.org/attachment.cgi?id=85366&action=review

> Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:-110
> -        setNPWindowIfNeeded();

It seems odd to me. Will it trigger some extra repaint for the normal non tiling case?

[Viatcheslav Ostapenko]

(In reply to comment #6)
> (From update of attachment 85366 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85366&action=review
> 
> > Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:-110
> > -        setNPWindowIfNeeded();
> 
> It seems odd to me. Will it trigger some extra repaint for the normal non tiling case?

setNPWindowIfNeeded() doesn't trigger extra repaint.
Some non-tiling cases are affected also, because widgets can have their own backing store.

[Robert Hogan]

(In reply to comment #5)
> I've tried with tiledBackingStore()->invalidate(). Yes it makes plugin moving but I dout that it's a right approach. Basically, what is happening in this case we just make  PluginView::paint() get invoked but in addition we also also alwyas re-paint tiles under plugin. So in result for each PluginView::updatePluginWidget() we not only call setNPWindowIfNeeded() but also do a paint() and frequency of setNPWindowIfNeeded() is not lower.
> I want to understand better what is wrong with calling setNPWindowIfNeeded() from PluginView::updatePluginWidget()? What is wrong with plugin getting information about new position of the plugin window?

Right, so what you want to do is change the current behaviour which is:


    // do not call setNPWindowIfNeeded immediately, will be called on paint()
    m_hasPendingGeometryChange = true;

to 'call setNPWindowIfNeeded immediately always'. Kenneth, may recall the exact answer to this:

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

but it looks like the short answer is so that plugins will scroll synchronized with the page view.

It makes sense to me that the plugin should get repainted on paint() events rather than updatePluginWidget() - but it seems in your case it also results in repainting the backing store, which is more expensive than you'd like. 

At the moment, the plugin itself only gets geometry information from setNPWindow once, when it's created. PluginViewQt avoids further calls because it can cause crashes. Your code won't change that but it does mean the plugin's position will get updated before the rest of the view.

[Misha]

(In reply to comment #8)
> (In reply to comment #5)
> > I've tried with tiledBackingStore()->invalidate(). Yes it makes plugin moving but I dout that it's a right approach. Basically, what is happening in this case we just make  PluginView::paint() get invoked but in addition we also also alwyas re-paint tiles under plugin. So in result for each PluginView::updatePluginWidget() we not only call setNPWindowIfNeeded() but also do a paint() and frequency of setNPWindowIfNeeded() is not lower.
> > I want to understand better what is wrong with calling setNPWindowIfNeeded() from PluginView::updatePluginWidget()? What is wrong with plugin getting information about new position of the plugin window?
> 
> Right, so what you want to do is change the current behaviour which is:
> 
> 
>     // do not call setNPWindowIfNeeded immediately, will be called on paint()
>     m_hasPendingGeometryChange = true;
> 
> to 'call setNPWindowIfNeeded immediately always'. Kenneth, may recall the exact answer to this:
> 
> http://trac.webkit.org/changeset/44381
> 
> but it looks like the short answer is so that plugins will scroll synchronized with the page view.
> 
> It makes sense to me that the plugin should get repainted on paint() events rather than updatePluginWidget() - but it seems in your case it also results in repainting the backing store, which is more expensive than you'd like. 
> 
> At the moment, the plugin itself only gets geometry information from setNPWindow once, when it's created. PluginViewQt avoids further calls because it can cause crashes. Your code won't change that but it does mean the plugin's position will get updated before the rest of the view.


Well... setNPWindow is invoked from paint() and paint() might be called event more often than  updatePluginWidget(). I'd rather have setNPWindow inside updatePluginWidget() but remove it from the paint().
Any known use cases where we know that extra calls of setNPWindow causing crash?

[Kenneth Rohde Christiansen]

> Well... setNPWindow is invoked from paint() and paint() might be called event more often than  updatePluginWidget(). I'd rather have setNPWindow inside updatePluginWidget() but remove it from the paint().
> Any known use cases where we know that extra calls of setNPWindow causing crash?

Flash 9 on Linux used to at least. Someone would need to test this and eventually use quirks to work around this.

[Robert Hogan]

(In reply to comment #9)
> > At the moment, the plugin itself only gets geometry information from setNPWindow once, when it's created. PluginViewQt avoids further calls because it can cause crashes. Your code won't change that but it does mean the plugin's position will get updated before the rest of the view.
> 
> 
> Well... setNPWindow is invoked from paint() and paint() might be called event more often than  updatePluginWidget(). I'd rather have setNPWindow inside updatePluginWidget() but remove it from the paint().
> Any known use cases where we know that extra calls of setNPWindow causing crash?

I wouldn't get too hung up on the crashes - I don't think your patch will cause any, because the quirk prevents NPP_setWindow() (i.e. the function in the plugin itself) getting called more than once.

What I was trying to get at above is that your patch will cause the painting of the plugin to get out of sync with the other paint events in the view - that's the code introduced in http://trac.webkit.org/changeset/44381 that you would be undoing.

[Viatcheslav Ostapenko]

(In reply to comment #11)
> (In reply to comment #9)
> What I was trying to get at above is that your patch will cause the painting 
> of the plugin to get out of sync with the other paint events in the view - 
> that's the code introduced in http://trac.webkit.org/changeset/44381 that 
> you would be undoing.

It will not. Calling setNPWindowIfNeeded doesn't do any paints.
Actually, why paints should be in sync?
In case of windowed mode plugins will get paint events from their own window.
And this change http://trac.webkit.org/changeset/69981 also gets plugin paints out of sync because it moves painting to graphics layer (QGraphicsItem).

[Robert Hogan]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > What I was trying to get at above is that your patch will cause the painting 
> > of the plugin to get out of sync with the other paint events in the view - 
> > that's the code introduced in http://trac.webkit.org/changeset/44381 that 
> > you would be undoing.
> 
> It will not. Calling setNPWindowIfNeeded doesn't do any paints.

I was referring to, which I think will trigger a repaint:

 platformPluginWidget()->setGeometry(m_windowRect); and
 platformPluginWidget()->setVisible(!clipRegion.isEmpty());
 platformPluginWidget()->setMask(clipRegion);

and I think that's why calling setNPWindowIfNeed() works for Misha in this case.

(On a side note, I'd forgotten that the PluginQuirkDontCallSetWindowMoreThanOnce works by calling NPP_setWindow with the same window dimensions over and over again, rather than calling it just once - that name should be changed.)

I guess the issue here is whether the restriction introduced on calling setNPWindowIfNeeded() introduced in http://trac.webkit.org/changeset/69981 is still valid. I don't know the answer to that.

[Misha]

Have some update on the issue.

I've tried the original test from http://trac.webkit.org/changeset/44381
My steps were:

1. in QtTestBrowser toggle use of QGraphicsWebView
2. turned on tiled backing store.
3. in manual-test/qt/plugin-sibling-frame-include.html I removed type="application/x-shockwave-flash" from embed tag:

<embed src="../plugins/test.swf">
</embed>

4. re-size the QtTestBrowser window to make it scrollable.

When I scrolled plugin didn't move. 

The key thing was to remove type="application/x-shockwave-flash". After http://trac.webkit.org/changeset/78352 we substitute wmode "window" for "opaque" in Flash plugins when we use QGraphicsWebView and thus usually don't have a combination of tiled backing store (implied QGraphicsWebView) and windowed plugin. When type parameter is omitted from embed tag the code from http://trac.webkit.org/changeset/78352 is never invoked and by default plugin is created in windowed mode.

So the issue should be reproducible if mime type is not specified in the embed tag and QGraphicsWebView is used in conjunction with some backing store (tiled backing store for example)
.

[Viatcheslav Ostapenko]

(In reply to comment #14)
> So the issue should be reproducible if mime type is not specified in the embed 
> tag and QGraphicsWebView is used in conjunction with some backing store (tiled 
> backing store for example).

Issue should be also reproducible in QGraphicsWebView if plugin doesn't support windowless mode.

[Robert Hogan]

See also https://bugs.webkit.org/show_bug.cgi?id=57179

I think you are seeing the same problem as that bug - the platformWidget() still has its old geometry when it's updated by invalidate() so sends the wrong rect information in the paint event that update() generates.

Calling setNPWindowIfNeeded() all the time will fix this, but is undesirable because it apparently can cause the movement of the frame and the plugin to get out of sync. From my own testing here, just calling setGeometry() doesn't cause any sync issues - but I think we'd want to call setMask() while we're at it too.

Given that setGeometry() causes an immediate moveEvent I'm not sure I'm right about it not causing any sync issues - but it looks to me like the platformWidget() stuff in setNPWindowIfNeeded() should be split out and called before invalidate() is called.

On the other hand, what's the worst that can happen if setNPWindowIfNeeded() is called all the time - whose testing can tell us if it causes any problems?

[Robert Hogan]

(In reply to comment #16)
> See also https://bugs.webkit.org/show_bug.cgi?id=57179
> 
> I think you are seeing the same problem as that bug - the platformWidget() still has its old geometry when it's updated by invalidate() so sends the wrong rect information in the paint event that update() generates.
> 

Note that the problem isn't the plugin getting the wrong rect information - the problem is the frameView getting the wrong rect information. The result is that it thinks areas where the plugin *was* aren't damaged (because it thinks the plugin hasn't moved) and doesn't paint them - so they appear to stay in the same place or jump.

[Misha]

(In reply to comment #17)
> (In reply to comment #16)
> > See also https://bugs.webkit.org/show_bug.cgi?id=57179
> > 
> > I think you are seeing the same problem as that bug - the platformWidget() still has its old geometry when it's updated by invalidate() so sends the wrong rect information in the paint event that update() generates.
> > 
> 
> Note that the problem isn't the plugin getting the wrong rect information - the problem is the frameView getting the wrong rect information. The result is that it thinks areas where the plugin *was* aren't damaged (because it thinks the plugin hasn't moved) and doesn't paint them - so they appear to stay in the same place or jump.

Well, I think it's slightly different situation. In your case the paint() it called, while the use case in this bug is that paint() is not invoked and there is no other place where we set npwindow.

About sync issue. If backing store is in use plugin can go out of sync anyway and it's completely depends on how plugin painting is done. But at least we can call NP API function (setnpwindow) when the geometry has been changed.

On Symbian it's the only way to notify plugin about movements.

Kenneth, Simon, 
Can you guys comment on this?

[Robert Hogan]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > See also https://bugs.webkit.org/show_bug.cgi?id=57179
> > > 
> > > I think you are seeing the same problem as that bug - the platformWidget() still has its old geometry when it's updated by invalidate() so sends the wrong rect information in the paint event that update() generates.
> > > 
> > 
> > Note that the problem isn't the plugin getting the wrong rect information - the problem is the frameView getting the wrong rect information. The result is that it thinks areas where the plugin *was* aren't damaged (because it thinks the plugin hasn't moved) and doesn't paint them - so they appear to stay in the same place or jump.
> Well, I think it's slightly different situation. In your case the paint() it called, while the use case in this bug is that paint() is not invoked and there is no other place where we set npwindow.
> About sync issue. If backing store is in use plugin can go out of sync anyway and it's completely depends on how plugin painting is done. But at least we can call NP API function (setnpwindow) when the geometry has been changed.
> On Symbian it's the only way to notify plugin about movements.

My point is that the area where the plugin has 'stayed' is not the location of your plugin anymore. It's up to FrameView to paint that area. However because we're not calling platformWidget()->setGemoetry() before calling platformWidget()->update() FrameView does not know that the plugin's 'old' position needs to be repainted.
I think fixing that will at least help to fix the case in this bug where the plugin stays in one place.

I agree that it looks as though you need to do more than just that to solve your issue completely though. 

Have you been able to confirm if only calling the platformWidget() stuff in setNPWindowIfNeeded fixes the issue, or you do actually need to call setNPWindow on the plugin itself to fix your problem?

If it does, then that approach will fix both bugs I think.

[Misha]

Created attachment 87637 [details]
Just a Symbian portion of the fix

I've tried setGeometry() on Linux and it helped with the issue. So if we think the it's enough for Linux it can be addressed in https://bugs.webkit.org/show_bug.cgi?id=57179.

For Symbian setGeometry() is not enough, so it will still require to call setNPWindowIfNeeded().
I modified the patch and kept only Symbian portion from the previous patch.

[Laszlo Gombos]

Comment on attachment 87637 [details]
Just a Symbian portion of the fix

View in context: https://bugs.webkit.org/attachment.cgi?id=87637&action=review

Overall patch looks good to me. r- to fix the ChangeLog.

> Source/WebCore/ChangeLog:3
> +        Reviewed by NOBODY (OOPS!).

Please add an extra empty line here.

> Source/WebCore/ChangeLog:12
> +        No new tests required.

Please list the existing test(s) that is fixed by this patch.

[Misha]

Created attachment 88722 [details]
patch with corrected change log

Corrected change log

[Laszlo Gombos]

Comment on attachment 88722 [details]
patch with corrected change log

r=me.

[WebKit Commit Bot]

Comment on attachment 88722 [details]
patch with corrected change log

Clearing flags on attachment: 88722

Committed r83316: <http://trac.webkit.org/changeset/83316>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r83316 cherry-picked into qtwebkit-2.1.x with commit 8ecd550 <http://gitorious.org/webkit/qtwebkit/commit/8ecd550>