Bug 76063

Created attachment 122028 [details] Patch

Created attachment 122033 [details] Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122028&action=review First look. > Source/WebCore/ChangeLog:8 > + > + Adds specialized reasons for suspend. These are needed > + because those types of suspend are treated slightly > + different. Is this code all your own or based on existing code from our branch? If so, add a comment > Source/WebCore/dom/ActiveDOMObject.h:67 > + PanningAndZooming I wonder if we should use Scaling as that seems to be what is used teh most in WebKit... maybe grep to check whether I am right or not. At least WebCore has pageScale, cssScale etc > Source/WebKit2/UIProcess/WebPageProxy.cpp:1118 > +{ > + if (!isValid() || !m_isPageSuspended) > + return; Are you handing web process crashes? > Source/WebKit2/UIProcess/WebPageProxy.cpp:1127 > +// m_drawingArea->resumePainting(); ups! > Source/WebKit2/UIProcess/WebPageProxy.cpp:1142 > +void WebPageProxy::suspendForPanningAndZooming() Would it make sense with an overloaded suspend method here? taking say an enum? > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:770 > if (m_frame->isMainFrame()) > - webPage->send(Messages::WebPageProxy::DidStartProgress()); > + webPage->didStartProgress(); > } please explain these changes in the changelog > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1595 > +void WebPage::didStartProgress() i am not sure the naming is good as the did* methods are normally generated by the generator and thus callbacks being called by the IPC system > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1623 > + // Suspend at a latter time. later* > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1636 > + doc->suspendActiveDOMObjects(why); > + doc->scriptRunner()->suspend(); What about DeviceMotion, Geolocation etc... I guess the former takes care of that, but maybe a comment to verify that would be good Also from the non-invited, can you add a comment to explain teh difference between scriptRunner->suspend(), setPaused() etc? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1641 > + AnimationController* controller = frame->animation(); > + if (controller) these lines could be merged > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1653 > + if (m_suspendIsDelayed) { > + // Do not run resume, if suspend is pending and > + // make sure delayed pending is cancelled, when resume is called. > + m_suspendIsDelayed = false; > + return; > + } could we make this more clear? // We are not suspended yet as the suspend was delayed, so we just cancel the it. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1664 > + AnimationController* controller = frame->animation(); > + if (controller) Merge: if (Animation.... = frame->animation()) ... > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1668 > + // Resume loading last. Order is important, because setDefersLoading Make sure to resume loading as the last step. The order... > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1672 > + // Listeners need to be resumed by the time load deferring is turned off. > + m_page->setDefersLoading(false); > +} Anyway to test these things? Have you given that some thoughts?

Comment on attachment 122033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122033&action=review > Source/WebKit2/UIProcess/WebPageProxy.cpp:1123 > + process()->send(Messages::WebPage::SetFixedVisibleContentRect(m_pendingVisibleContentRectUpdate), m_pageID); This needs #if USE(TILED_BACKING_STORE) > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1627 > + 'why' value is lost and defaulted to 'DocumentWillGoToBackground' when the suspend is delayed. it should be preserved.

(In reply to comment #3) > View in context: https://bugs.webkit.org/attachment.cgi?id=122028&action=review > > First look. > > Source/WebCore/ChangeLog:8 > > + > > + Adds specialized reasons for suspend. These are needed > > + because those types of suspend are treated slightly > > + different. > > Is this code all your own or based on existing code from our branch? If so, add a comment > It is based of 6 commits from 3 different contributors, yourself one of them. I couldn't see any precedence for attributing a patch extracted from the work of several others. Should I add all the names or mention that it is from our branch? > > Source/WebCore/dom/ActiveDOMObject.h:67 > > + PanningAndZooming > > I wonder if we should use Scaling as that seems to be what is used teh most in WebKit... maybe grep to check whether I am right or not. At least WebCore has pageScale, cssScale etc > I was considering renaming it completely to something like ExternalAnimation, since we might use it for more than pan and pinch gestures in the future. These are just the names from our branch for simplicity. > > Source/WebKit2/UIProcess/WebPageProxy.cpp:1118 > > +{ > > + if (!isValid() || !m_isPageSuspended) > > + return; > > Are you handing web process crashes? > There is code to do handle it. I could see no reason why not to port it. > > Source/WebKit2/UIProcess/WebPageProxy.cpp:1127 > > +// m_drawingArea->resumePainting(); > > ups! Already fixed in the second patch. There were two other misplaced changes in this patch. > > Source/WebKit2/UIProcess/WebPageProxy.cpp:1142 > > +void WebPageProxy::suspendForPanningAndZooming() > > Would it make sense with an overloaded suspend method here? taking say an enum? Good idea, but enums just becomes numbers in the IPC, so it would not be much clearer. The rest of the comments are on style and clarity. I will fix that in the next version. > > Anyway to test these things? Have you given that some thoughts? Not that I know of, but I guess we could have a test runner on QML level that tests exposed API, but it could still not guarantee the API graphically does what it is supposed to.

Comment on attachment 122033 [details] Patch Attachment 122033 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11108475

Comment on attachment 122033 [details] Patch Attachment 122033 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11108493

(In reply to comment #5) > (In reply to comment #3) > > View in context: https://bugs.webkit.org/attachment.cgi?id=122028&action=review > > > > First look. > > > Source/WebCore/ChangeLog:8 > > > + > > > + Adds specialized reasons for suspend. These are needed > > > + because those types of suspend are treated slightly > > > + different. > > > > Is this code all your own or based on existing code from our branch? If so, add a comment > > > It is based of 6 commits from 3 different contributors, yourself one of them. I couldn't see any precedence for attributing a patch extracted from the work of several others. Should I add all the names or mention that it is from our branch? Give them credit :) "Based on the initial work of XXX, ABC and 123" is for example valid.

A closely related case is printing, where we don't want the document to change, and even more strongly, we don't want on-screen rendering to change under a print preview dialog. Modulo some bugs, that works already. I personally haven't heard of issues during panning and zoom. I'm not sure if WebCore needs to know about such cases at all. In particular, executing the code to suspend active DOM objects means poorer responsiveness when starting a gesture.

> It is based of 6 commits from 3 different contributors, yourself one of them. I couldn't see any precedence for attributing a patch extracted from the work of several others. Should I add all the names or mention that it is from our branch? We normally write something like "Based on patch(es) by ..." > > > > Source/WebCore/dom/ActiveDOMObject.h:67 > > > + PanningAndZooming > > > > I wonder if we should use Scaling as that seems to be what is used teh most in WebKit... maybe grep to check whether I am right or not. At least WebCore has pageScale, cssScale etc > > > I was considering renaming it completely to something like ExternalAnimation, since we might use it for more than pan and pinch gestures in the future. These are just the names from our branch for simplicity. I have no strong opinion on this, but panning is not an animation, though an animation might follow it. > > > Source/WebKit2/UIProcess/WebPageProxy.cpp:1118 > > > +{ > > > + if (!isValid() || !m_isPageSuspended) > > > + return; > > > > Are you handing web process crashes? > > > There is code to do handle it. I could see no reason why not to port it. I was more wondering if you really though about the case and tested it. It is easy to end up in a weird state and we have had such bugs in the past. > > > Source/WebKit2/UIProcess/WebPageProxy.cpp:1142 > > > +void WebPageProxy::suspendForPanningAndZooming() > > > > Would it make sense with an overloaded suspend method here? taking say an enum? > Good idea, but enums just becomes numbers in the IPC, so it would not be much clearer. I am fine with the current method. > > Anyway to test these things? Have you given that some thoughts? > > Not that I know of, but I guess we could have a test runner on QML level that tests exposed API, but it could still not guarantee the API graphically does what it is supposed to. I was more thinking about testing the state maybe exposing some methods to the testing system (There should be an internal object now). It would be really cool to have such tests and we have had regressions/bugs before. Zalan might know what needs to be tested and it can possible be in a separate patch

(In reply to comment #9) > A closely related case is printing, where we don't want the document to change, and even more strongly, we don't want on-screen rendering to change under a print preview dialog. > > Modulo some bugs, that works already. I personally haven't heard of issues during panning and zoom. > > I'm not sure if WebCore needs to know about such cases at all. In particular, executing the code to suspend active DOM objects means poorer responsiveness when starting a gesture. For Qt we have a "rendering model" where scaling/panning is done by the UI process by manipulating the position and scale of a tiled backing store. To always ensure 60fps on low-end devices we suspend whatever is possible. When the panning or position/scale animation ends we send the new position etc to WebCore, so suspending also makes sure that WebCore will not use wrong values.

> > Modulo some bugs, that works already. I personally haven't heard of issues during panning and zoom. This feature was originally implemented for mobile environment with relatively slow CPUs in order to support smooth scrolling.

(In reply to comment #11) > (In reply to comment #9) > > A closely related case is printing, where we don't want the document to change, and even more strongly, we don't want on-screen rendering to change under a print preview dialog. > > > > Modulo some bugs, that works already. I personally haven't heard of issues during panning and zoom. > > > > I'm not sure if WebCore needs to know about such cases at all. In particular, executing the code to suspend active DOM objects means poorer responsiveness when starting a gesture. > > For Qt we have a "rendering model" where scaling/panning is done by the UI process by manipulating the position and scale of a tiled backing store. To always ensure 60fps on low-end devices we suspend whatever is possible. When the panning or position/scale animation ends we send the new position etc to WebCore, so suspending also makes sure that WebCore will not use wrong values. I think this is not a good way to solve this. If your scrolling/panning is in another process/thread, then why not just increase the priority of it relative to webkit/webcore? Then you can let the kernel/scheduler help you instead of manually disabling stuff on one thread so another thread runs smoother. Something I'm missing?

(In reply to comment #13) > I think this is not a good way to solve this. If your scrolling/panning is in another process/thread, then why not just increase the priority of it relative to webkit/webcore? Then you can let the kernel/scheduler help you instead of manually disabling stuff on one thread so another thread runs smoother. > > Something I'm missing? The position and dimensions would still be out of sync between the UIProcess and the WebProcess. Suspending the WebProcess ensures the WebProcess is in sync since it is paused while the UIProcess is changing the values. I might find another solution for panning, but for scaling and rotation I still believe suspending is the best solution.

Created attachment 122213 [details] Patch

Comment on attachment 122213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122213&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1624 > + // Suspend at a later time. Does that commetn provide any value? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1637 > + // suspend active DOM objects such as HTML <video> Our coding style says to use proper sentences, even for single line comments, ie start with capital end with punctuation mark. > Source/WebKit2/WebProcess/WebPage/WebPage.h:581 > + inline void suspend() { suspendWithReason(WebCore::ActiveDOMObject::DocumentWillGoToBackground); } > + void suspendWithReason(uint32_t reasonForSuspension); wouldn't a default value not make more sense? void suspend (uint32_t reason = WebCore::ActiveDOMObject::DocumentWillGoToBackground) ? > Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:102 > + SuspendWithReason(uint32_t reasonForSuspension) > + Suspend() > + Resume() Maybe it is better to enforce always giving a reason?

Comment on attachment 122213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122213&action=review > Source/WebKit2/UIProcess/WebPageProxy.cpp:1131 > + process()->send(Messages::DrawingArea::ResumePainting(), m_pageID); Wouldn't resuming the painting first be more logical? and suspending the painting after the page has been suspended. Probably it makes no difference, just looks more logical to me.

Comment on attachment 122213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122213&action=review > Source/WebKit2/UIProcess/WebPageProxy.cpp:1153 > + process()->send(Messages::DrawingArea::SuspendPainting(), m_pageID); > + process()->send(Messages::WebPage::Suspend(), m_pageID); > +} > + > +void WebPageProxy::suspendWithReason(WebCore::ActiveDOMObject::ReasonForSuspension why) > +{ > + if (!isValid() || m_isPageSuspended) > + return; > + > + m_isPageSuspended = true; > + > + process()->send(Messages::WebPage::SuspendWithReason(why), m_pageID); > +} Hm, am I missing something here? suspend() also implies SuspendPainting() but suspendWithReason doesn't.

(In reply to comment #18) > (From update of attachment 122213 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122213&action=review > > > Source/WebKit2/UIProcess/WebPageProxy.cpp:1153 > > + process()->send(Messages::DrawingArea::SuspendPainting(), m_pageID); > > + process()->send(Messages::WebPage::Suspend(), m_pageID); > > +} > > + > > +void WebPageProxy::suspendWithReason(WebCore::ActiveDOMObject::ReasonForSuspension why) > > +{ > > + if (!isValid() || m_isPageSuspended) > > + return; > > + > > + m_isPageSuspended = true; > > + > > + process()->send(Messages::WebPage::SuspendWithReason(why), m_pageID); > > +} > > Hm, am I missing something here? suspend() also implies SuspendPainting() but suspendWithReason doesn't. Oh right. I might need to rename that back to suspendForPanningZooming. The reason paiting isn't suspended for panning is because videos can still play during panning, it doesn't stop replaced content.

Created attachment 122239 [details] Patch

(In reply to comment #20) > Created an attachment (id=122239) [details] > Patch Updated comments. I have removed painting suspension again. I am not entirely sure it is even necessary, but since the paint-suspend in our branch was also introduced by different set of patches, I will also port it as a separate patch later if necessary.

(In reply to comment #14) > (In reply to comment #13) > > I think this is not a good way to solve this. If your scrolling/panning is in another process/thread, then why not just increase the priority of it relative to webkit/webcore? Then you can let the kernel/scheduler help you instead of manually disabling stuff on one thread so another thread runs smoother. > > > > Something I'm missing? > > The position and dimensions would still be out of sync between the UIProcess and the WebProcess. Suspending the WebProcess ensures the WebProcess is in sync since it is paused while the UIProcess is changing the values. > > I might find another solution for panning, but for scaling and rotation I still believe suspending is the best solution. The position and dimensions of _what_ would be out of sync? This is a very different rationale to Kenneth's which said this was to accommodate low end CPU's. Why do you think you need all of this suspension for scaling and rotation?

(In reply to comment #21) > (In reply to comment #20) > > Created an attachment (id=122239) [details] [details] > > Patch > > Updated comments. > > I have removed painting suspension again. I am not entirely sure it is even necessary, Since we don't suspend webcore timers in general, items, for example animated gifs can trigger updates. While the document is in background, those updates should not end up as painting operations.

(In reply to comment #22) > (In reply to comment #14) > > (In reply to comment #13) > > > I think this is not a good way to solve this. If your scrolling/panning is in another process/thread, then why not just increase the priority of it relative to webkit/webcore? Then you can let the kernel/scheduler help you instead of manually disabling stuff on one thread so another thread runs smoother. > > > > > > Something I'm missing? > > > > The position and dimensions would still be out of sync between the UIProcess and the WebProcess. Suspending the WebProcess ensures the WebProcess is in sync since it is paused while the UIProcess is changing the values. > > > > I might find another solution for panning, but for scaling and rotation I still believe suspending is the best solution. > > The position and dimensions of _what_ would be out of sync? This is a very different rationale to Kenneth's which said this was to accommodate low end CPU's. Why do you think you need all of this suspension for scaling and rotation? The UIProcess and WebProcess runs in separate process and both have the capability of doing transformations on the viewport, and both have their own idea of where and how large the viewport is. When one of them moves or transform the viewport there will be delay before the other process has its value updated (or reversely there will be a delay between the other process is updated and the transformation actually happens). This leads to a lot of problems with animation and rendering glitches (not to mention DOM reading wrong values). The one thing I removed from the patch was suspending painting that doesn't move content or query the position of the viewport. This painting is safe from a consistency point-of-view, but might still be desirable to suspend on a low-end platform to ensure smooth animations. This will be part of a new bug-report and patch later if still needed.

(In reply to comment #23) > (In reply to comment #21) > > (In reply to comment #20) > > > Created an attachment (id=122239) [details] [details] [details] > > > Patch > > > > Updated comments. > > > > I have removed painting suspension again. I am not entirely sure it is even necessary, > Since we don't suspend webcore timers in general, items, for example animated gifs can trigger updates. While the document is in background, those updates should not end up as painting operations. That is good point, but it is not a viewport synchronization issue, so I would still like to deal with that separately in another patch.

(In reply to comment #25) > (In reply to comment #23) > > (In reply to comment #21) > > > (In reply to comment #20) > > > > Created an attachment (id=122239) [details] [details] [details] [details] > > > > Patch > > > > > > Updated comments. > > > > > > I have removed painting suspension again. I am not entirely sure it is even necessary, > > Since we don't suspend webcore timers in general, items, for example animated gifs can trigger updates. While the document is in background, those updates should not end up as painting operations. > > That is good point, but it is not a viewport synchronization issue, so I would still like to deal with that separately in another patch. sounds good.

I think that Sam and/or Anders should comment on this approach, too.

I don't think this is something we want to support as it will give the false impression that calling suspend() will actually cause the web content to suspend immediately and unnecessarily complicates the architecture. Additionally, from experience of using the existing WebKit2 API to implement the features you are discussing, it was not necessary to add these things.

> The UIProcess and WebProcess runs in separate process and both have the capability of doing transformations on the viewport, and both have their own idea of where and how large the viewport is. When one of them moves or transform the viewport there will be delay before the other process has its value updated (or reversely there will be a delay between the other process is updated and the transformation actually happens). This leads to a lot of problems with animation and rendering glitches (not to mention DOM reading wrong values). > > The one thing I removed from the patch was suspending painting that doesn't move content or query the position of the viewport. This painting is safe from a consistency point-of-view, but might still be desirable to suspend on a low-end platform to ensure smooth animations. This will be part of a new bug-report and patch later if still needed. Sure, you have two different threads with their own notion of what the current viewport is and they are not always in sync, but I'm not sure how this causes you a, "lot of problems with animation and rendering glitches (not to mention DOM reading wrong values)." Can you elaborate? I know that for BlackBerry port we don't put the panning/zooming in another process, but it is in another thread. Can I ask why you chose to put it in another *process* as opposed to just another thread? Finally, I think - not sure - that iOS gets around this by implementing ScrollViewMac which is the arbiter of what the current viewport is. ScrollViewFoo could be whatever it wanted to be including having a semaphore/mutex to coordinate syncing or just always report the UIProcess's value. Maybe Sam can say whether this is correct? Anyway, there exist ways to solve these problems without resorting to halting all operation in the webkit's thread/process.

(In reply to comment #28) >I don't think this is something we want to support as it will give the false impression that calling suspend() will actually cause the web content to suspend immediately and unnecessarily complicates the architecture. Additionally, from experience of using the existing WebKit2 API to implement the features you are discussing, it was not necessary to add these things. I don't find it that confusing. I expect the web content to be suspended after returning from the function, and from what I can see this is very little code, and not complicating at all. In fact right now it is just a few extra API functions that you can ignore if you don't want to use the feature. I would love to hear good alternatives though. If there is a smarter way of doing this I am all ears. (In reply to comment #29) > I know that for BlackBerry port we don't put the panning/zooming in another process, but it is in another thread. Can I ask why you chose to put it in another *process* as opposed to just another thread? > The UIProcess is another process by design of WebKit2, it is in fact pretty much the central defining charateristic of WebKit2. What we do is to keep the UI animations in the process that is closest to the platform level and does the final rendering, the UIProcess. At least for scrolling though, I like the idea of adding another thread to WebCore to scroll closer to the rendering model, but the ScrollCoordinator is still far from finished. If anyone is succesfully using a design like this, the code is not currently published in WebKit. > Finally, I think - not sure - that iOS gets around this by implementing ScrollViewMac which is the arbiter of what the current viewport is. ScrollViewFoo could be whatever it wanted to be including having a semaphore/mutex to coordinate syncing or just always report the UIProcess's value. Maybe Sam can say whether this is correct? > > Anyway, there exist ways to solve these problems without resorting to halting all operation in the webkit's thread/process. I have never used iOS devices that much, but I have been told the suspend feature is similar to what the browser in iPhones actual do during panning and pinch gestures. Also I don't think iOS uses the WebKit2 API, and sharing datastructures makes more sense between threads than between processes. It is certainly possible in shared memory between two processes (with some limitations), but it would be a new step in the WebKit2 API.

(In reply to comment #30) > (In reply to comment #28) > >I don't think this is something we want to support as it will give the false impression that calling suspend() will actually cause the web content to suspend immediately and unnecessarily complicates the architecture. Additionally, from experience of using the existing WebKit2 API to implement the features you are > discussing, it was not necessary to add these things. > > I don't find it that confusing. I expect the web content to be suspended after returning from the function, and from what I can see this is very little code, and not complicating at all. In fact right now it is just a few extra API functions that you can ignore if you don't want to use the feature. > > I would love to hear good alternatives though. If there is a smarter way of doing this I am all ears. > > > (In reply to comment #29) > > I know that for BlackBerry port we don't put the panning/zooming in another process, but it is in another thread. Can I ask why you chose to put it in another *process* as opposed to just another thread? > > > > The UIProcess is another process by design of WebKit2, it is in fact pretty much the central defining charateristic of WebKit2. Absolutely, no doubt. The thing I was questioning is why you are _panning/zooming_ in this process. > What we do is to keep the UI animations in the process that is closest to the platform level and does the final rendering, the UIProcess. At least for scrolling though, I like the idea of adding another thread to WebCore to scroll closer to the rendering model, but the ScrollCoordinator is still far from finished. If anyone is succesfully using a design like this, the code is not currently published in WebKit. We are in the middle of upstreaming our port which does the panning/zooming in another thread. It is definitely not all up yet, but we're working on it. Not sure what the ScrollCoordinator is. I will say that although we have this in our WebKit port that our backingstore will likely move to the platform level at some point. I don't think WebKit is the right place for it. The key though is that AFAIK it is possible to solve these problems without this kind of suspending of the WebCore thread. > I have never used iOS devices that much, but I have been told the suspend feature is similar to what the browser in iPhones actual do during panning and pinch gestures... News to me :) But how come we don't see this suspend/resume in the WebKit codebase?

> Absolutely, no doubt. The thing I was questioning is why you are _panning/zooming_ in this process. We [Qt] are not, all panning and zooming happens in the UI process, which uses a tiled backing store. > News to me :) But how come we don't see this suspend/resume in the WebKit codebase? The iOS port is not upstreamed and I believe based on an internal branch, so it would be pretty natural to not see code for these things in WebKit trunk. The same counts for Android, our N9 browser and even your (though you are in the process of upstreaming the code).

Created attachment 123293 [details] WebCore side of patch

Created attachment 123294 [details] WebKit2 side of patch

I have made a new patch, split in two parts. Compared to the old it doesn't disable javascripting in general, but only activedomobjects and animations. The new ReasonForSuspension is the same enum as in iOS branch of WebCore. Though the rest of the implementation is completely new since the implementation and use of Frame::timerspaused is oddly absent in the released WebCore code. The WebCore side of the patch also fixes a few bugs with suspendActiveDOMObjects by having the ScriptExecutionContext store the fact it is suspending and the reason why, so that new ActiveDOMObjects are also suspended (such as new DOMTimers). On the WebKit2 side, the suspend code now only suspends for zoomandpanning in itself. For going to background it is supposed to be followed by calling setIsInWindow.

Attachment 123293 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 123301 [details] WebCore side of patch

Comment on attachment 123294 [details] WebKit2 side of patch Attachment 123294 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11314104

(In reply to comment #38) > (From update of attachment 123294 [details]) > Attachment 123294 [details] did not pass gtk-ews (gtk): > Output: http://queues.webkit.org/results/11314104 Yes, it depends on the first patch. Without it, it should be able to build at all. I separated them because some reviewers only felt they could review one side of it. For the sake of the build-bots I will cat them together later.

Please amend this bug title to make it clearer what you're trying to do. Do these patches suspect JS timers as well?

(In reply to comment #40) > Please amend this bug title to make it clearer what you're trying to do. Do these patches suspect JS timers as well? Yes, DOMTimers are ActiveDOMObjects, so they are suspended by Document::suspendActiveDOMObjects.

I think you should make it so that suspend/resume calls can be nested.

(In reply to comment #42) > I think you should make it so that suspend/resume calls can be nested. I have made it so that the Frame::setActiveDomObjectsPaused calls can be nested. Nesting the suspend/resume ActiveDomObjects on Document is not that easy since they have several different suspend reasons which would require tracking which suspend is being resumed. I could add a two level nesting that maintains non-paused suspend reasons and resumes paused reasons when resuming a non-paused reason, but that is it.

Comment on attachment 123294 [details] WebKit2 side of patch View in context: https://bugs.webkit.org/attachment.cgi?id=123294&action=review This patch (WebKit2 side) looks good to me > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1623 > + // Suspend CSS animations. > + frame->animation()->suspendAnimations(); I dont think this comment gives any value, it is pretty obvious already

Comment on attachment 123301 [details] WebCore side of patch View in context: https://bugs.webkit.org/attachment.cgi?id=123301&action=review As we already discussed this patch offline, I'm only doing some nitpicking here and would like someone else to do the actual review. > Source/WebCore/dom/ScriptExecutionContext.cpp:270 > + // Ensure all ActiveDOMObjects are suspended also newly created ones nit: Add a dot at the end > Source/WebCore/page/Frame.cpp:201 > + // Pause future ActiveDOMObjects if this frame is created when page is in paused state. is being created > Source/WebCore/page/Frame.cpp:206 > + setActiveDOMObjectsPaused(true); > + > } unneeded newline > Source/WebCore/page/Frame.cpp:1040 > + if (activeDOMObjectsPaused()) { I think it would be more clear to call m_activeDOMObjectsPausedCount directly here

Created attachment 123541 [details] Patch New combined patch.

Comment on attachment 123541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123541&action=review In general what I see here is two patches. One is add WebKit2 API that there's not unanimous agreement we should have. That's what this bug represents. The other is a WebCore patch that fixes https://bugs.webkit.org/show_bug.cgi?id=53733. I know all you care about is the WebKit2 API, but I would really like to see you split off all the WebCore stuff that creates suspending DOM objects, etc. Get it reviewed in https://bugs.webkit.org/show_bug.cgi?id=53733. Do *not* include the new enum as that is truly part of supporting the WK2 API. That will make both separate patches easier to review and will also make your case that the API should be included much easier to make as the patch will be smaller and more focused on that goal. > Source/WebCore/ChangeLog:14 > + * dom/ActiveDOMObject.h: > + > + New ReasonForSuspension: DocumentWillBePaused. > + Identical to name in iOS branch of WebCore Comments for a change normally start on the line of the file where the change came from. * dome/ActiveDOMObject.h: New ReasonForSuspension... At the very least the description shouldn't have an empty line between the file and the description. Same for the whole ChangeLog > Source/WebCore/dom/ActiveDOMObject.h:66 > - DocumentWillBecomeInactive > + DocumentWillBecomeInactive, > + DocumentWillBePaused Naming! Pretend I don't know this code at all and I walk up to WebCore for the first time and look at these enums. What is the difference between Inactive and Paused? Also it would be "BecomePaused" to follow "BecomeInactive", but I still hate having both "paused" and "inactive" in the same set of enums.

Comment on attachment 123541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123541&action=review > Source/WebCore/dom/ScriptExecutionContext.h:106 > + bool isSuspendingActiveDOMObjects() const { return m_isSuspendingActiveDOMObjects; } I think this would be better as activeDOMObjectsAreSuspended() I find the paused/suspended/inactive mixture of terminology confusing. I think you might want to do a rename pass first, then do this patch. > Source/WebCore/dom/ScriptExecutionContext.h:212 > + bool m_isSuspendingActiveDOMObjects; Ditto. > Source/WebCore/page/Frame.cpp:204 > + setActiveDOMObjectsPaused(true); Why isn't this setActiveDOMObjectsSuspended()? > Source/WebCore/page/Frame.cpp:1023 > +void Frame::setActiveDOMObjectsPausedInternal(bool pause) Internal is a bit vague. Maybe "ForThisFrame"?

(In reply to comment #48) > (From update of attachment 123541 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123541&action=review > > > Source/WebCore/dom/ScriptExecutionContext.h:106 > > + bool isSuspendingActiveDOMObjects() const { return m_isSuspendingActiveDOMObjects; } > > I think this would be better as activeDOMObjectsAreSuspended() > I can do the rename, if you think that is better. I used isSuspending because it was a reminder to the ScriptExecutionContext that is suspending all new ActiveDOMObjects. > I find the paused/suspended/inactive mixture of terminology confusing. I think you might want to do a rename pass first, then do this patch. > > > Source/WebCore/page/Frame.cpp:204 > > + setActiveDOMObjectsPaused(true); > > Why isn't this setActiveDOMObjectsSuspended()? > I used different terminology because there can be several reasons for suspending activeDOMObjects, but only one of the reasons is that the page is paused. > > Source/WebCore/page/Frame.cpp:1023 > > +void Frame::setActiveDOMObjectsPausedInternal(bool pause) > > Internal is a bit vague. Maybe "ForThisFrame"? That would be more accurate, yes. Internal is only because it is an auxiliary and private function. Please note by the way, that I have been requested to submit the parts on ScriptExecutionContext as a separate patch against https://bugs.webkit.org/show_bug.cgi?id=53733

(In reply to comment #49) > (In reply to comment #48) > > (From update of attachment 123541 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=123541&action=review > > > > > Source/WebCore/dom/ScriptExecutionContext.h:106 > > > + bool isSuspendingActiveDOMObjects() const { return m_isSuspendingActiveDOMObjects; } > > > > I think this would be better as activeDOMObjectsAreSuspended() > > > I can do the rename, if you think that is better. I used isSuspending because it was a reminder to the ScriptExecutionContext that is suspending all new ActiveDOMObjects. "isSuspending" could be read to mean "I am in the act of making them suspended", rather than "I am in the state where they are suspended". You want the latter. > > I find the paused/suspended/inactive mixture of terminology confusing. I think you might want to do a rename pass first, then do this patch. > > > > > Source/WebCore/page/Frame.cpp:204 > > > + setActiveDOMObjectsPaused(true); > > > > Why isn't this setActiveDOMObjectsSuspended()? > > > I used different terminology because there can be several reasons for suspending activeDOMObjects, but only one of the reasons is that the page is paused. Maybe this should be setActiveDOMObjectsState(foo) then? > > > Source/WebCore/page/Frame.cpp:1023 > > > +void Frame::setActiveDOMObjectsPausedInternal(bool pause) > > > > Internal is a bit vague. Maybe "ForThisFrame"? > > That would be more accurate, yes. Internal is only because it is an auxiliary and private function. > > Please note by the way, that I have been requested to submit the parts on ScriptExecutionContext as a separate patch against https://bugs.webkit.org/show_bug.cgi?id=53733 Sounds good.

Created attachment 123748 [details] Patch Renamed functions as requested. After further debug, my patch does not affect bug #53733. During debugging that bug, I did find one corner-case more that where Suspendable Timers was started even though they had been correctly suspended. This is now solved.

Comment on attachment 123748 [details] Patch Updated patch pending

Comment on attachment 123294 [details] WebKit2 side of patch Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 123294 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Created attachment 129004 [details] Patch

Comment on attachment 129004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129004&action=review Great > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1678 > + // We need to repaint on resume to kickstart animated painting again. > + drawingArea()->setNeedsDisplay(bounds()); It this working correctly with our tiling? Remember we resume on pan end etc

(In reply to comment #55) > (From update of attachment 129004 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129004&action=review > > Great > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1678 > > + // We need to repaint on resume to kickstart animated painting again. > > + drawingArea()->setNeedsDisplay(bounds()); > > It this working correctly with our tiling? Remember we resume on pan end etc It is the same call resumePainting makes, but I guess we can restrict it to only asking for a repaint in the viewport.

(In reply to comment #56) > (In reply to comment #55) > > (From update of attachment 129004 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=129004&action=review > > > > Great > > > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1678 > > > + // We need to repaint on resume to kickstart animated painting again. > > > + drawingArea()->setNeedsDisplay(bounds()); > > > > It this working correctly with our tiling? Remember we resume on pan end etc > > It is the same call resumePainting makes, but I guess we can restrict it to only asking for a repaint in the viewport. We [Qt] already does this when we stop panning, so maybe we need to look into this again

Created attachment 129015 [details] Patch

(In reply to comment #57) > (In reply to comment #56) > > (In reply to comment #55) > > > (From update of attachment 129004 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=129004&action=review > > > > > > Great > > > > > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1678 > > > > + // We need to repaint on resume to kickstart animated painting again. > > > > + drawingArea()->setNeedsDisplay(bounds()); > > > > > > It this working correctly with our tiling? Remember we resume on pan end etc > > > > It is the same call resumePainting makes, but I guess we can restrict it to only asking for a repaint in the viewport. > > We [Qt] already does this when we stop panning, so maybe we need to look into this again We only repaint tiles that are dirty. The problem is when the animations are suspended the tiles are not marked dirty and thus never gets repainted though the animated images would paint something new the next time. This solutions works, but later it would probably be a good idea to look into just marking the images dirty instead of doing a full repaint.

Created attachment 129037 [details] Patch for landing Correct mistake in WebPage::suspend after API clean up. The call is now even cleaner.

Created attachment 129038 [details] Patch for landing Removed accidentally included change to which tests were run.

Comment on attachment 129038 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=129038&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.h:602 > + void suspend(); > + void resume(); I think these names are too generic at this level. "Suspend" can mean many things for a web page, and it's not clear what these do.

I agree that this patch still doesn't resolve one of the first comments that were given here - there are too many ways to "suspend", and little explanation of what each is doing.

Created attachment 129237 [details] Patch Continued the longer and more accurate function naming to WebKit2 level.

(In reply to comment #64) > Created an attachment (id=129237) [details] > Patch > > Continued the longer and more accurate function naming to WebKit2 level. Simon, Alexey, are you OK with the names now?

(In reply to comment #65) > (In reply to comment #64) > > Created an attachment (id=129237) [details] [details] > > Patch > > > > Continued the longer and more accurate function naming to WebKit2 level. > > Simon, Alexey, are you OK with the names now? There were no objections in the last 3 days, so I'm going to land this now.

Comment on attachment 129237 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129237&action=review Landed in: http://trac.webkit.org/changeset/109548 > Source/WebCore/page/Frame.h:264 > + I removed this new line. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1677 > + I removed this new line.

(In reply to comment #67) > (From update of attachment 129237 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129237&action=review > > Landed in: http://trac.webkit.org/changeset/109548 > > > Source/WebCore/page/Frame.h:264 > > + > > I removed this new line. > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1677 > > + > > I removed this new line. Great, you guys broke the Mac build. Could you fix it before Apple guys wake up? :)

(In reply to comment #68) > (In reply to comment #67) > > (From update of attachment 129237 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=129237&action=review > > > > Landed in: http://trac.webkit.org/changeset/109548 > > > > > Source/WebCore/page/Frame.h:264 > > > + > > > > I removed this new line. > > > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1677 > > > + > > > > I removed this new line. > > Great, you guys broke the Mac build. Could you fix it before Apple guys wake up? :) And the MAC EWS was red all along with exactly the same error :)

Created attachment 129882 [details] BuildFix Build fix for AppleWebKit

Comment on attachment 129882 [details] BuildFix Clearing flags on attachment: 129882 Committed r109558: <http://trac.webkit.org/changeset/109558>

All reviewed patches have been landed. Closing bug.

Reopen, because it broke Qt WK2 API tests. See https://bugs.webkit.org/show_bug.cgi?id=80247 for details.

> Simon, Alexey, are you OK with the names now? Of course not. The objection was about the word "suspend", and it's still called "suspend", just with some more words added afterwards. If anything, the name got worse. What does "activeDOMObjectsAndAnimationsSuspended" return if just one of these is suspended?

(In reply to comment #74) > > Simon, Alexey, are you OK with the names now? > > Of course not. The objection was about the word "suspend", and it's still called "suspend", just with some more words added afterwards. > I originally called it pagePaused, it was still a vague name, but I used it exactly to distinguish it from suspend. I changed it because confusion was expressed over using two different terms. I think calling it suspend does make sense, since both animations have suspend API and active DOM objects have suspend API, and this function ensures both are suspended. But I am not particular proud of the long name and it is only used due to the lack of a better one. If someone has a better name I would be happy to open a new bug and rename it.

Blocking bug 80247 closed.

Is this being tested somehow?

(In reply to comment #77) > Is this being tested somehow? It is only tested manually. We don't have much automatic testing for the UI-side of WebKit2. The individual functions on the WebCore side has different ways to be tested. However, since the suspend is triggered on all pan gestures. It is quite easy to test. You just touch the page and move your finger, the content is then suspended until you let go.