When a root layer invalidation occurs, we need to schedule a compositor run. Without this, selecting text on the root layer appears to be broken.
Created attachment 68296 [details] Patch
Why do we need this? LayerChromium::setNeedsDisplay() should call setNeedsCommit() which should call notifySyncRequired() which do the same thing. In fact there's this comment in setNeedsCommit(): // Call notifySyncRequired(), which in this implementation plumbs through to // call setRootLayerNeedsDisplay() on the WebView, which will cause LayerRendererChromium // to render a frame.
Good question. In the current implementation, the root layer behaves differently than all other layers, and does not actually call notifySyncRequired.
(In reply to comment #3) > Good question. > > In the current implementation, the root layer behaves differently than all other layers, and does not actually call notifySyncRequired. Isn't that the actual bug, then?
Yes and no. The current compositor architecture intentionally treats the root layer the same as non-root layers. I have been making incremental changes to get the root layer more-and-more similar to the other layers, but we're not to the point of unification yet. As things stand right now, the root layer is broken when you for example highlight text on it. So, yes, the current architecture is flawed. But, fixing the flaw is something we might want to hit in another changelist.
Erm, mistake in typing --- "The current compositor architecture intentionally treats the root layer the same as non-root layers" should read "The current compositor architecture intentionally treats the root layer ***differently*** from non-root layers" Hahah
As far as I can tell, that is by design of the overall compositor. E.g. the code shared with Safari Windows, treats root & child layers differently as well. If you want me to update the comment you saw to say something like "except on root layer," that would be fine, since the comment is a lie.
Created attachment 68429 [details] Add comments to LayerChromium::setNeedsDisplay
(In reply to comment #4) > (In reply to comment #3) > > Good question. > > > > In the current implementation, the root layer behaves differently than all other layers, and does not actually call notifySyncRequired. > > Isn't that the actual bug, then? I think Nat's solution is correct here. The root layer does behave differently from the rest of the layers so it isn't that surprising that it needs to notify the window to repaint somewhat differently from other layers.
Created attachment 68487 [details] Minimally-controversial approach (I hope) to root layer damage tracking.
Looks OK to me - will r+ if Vangelis agrees.
I think James has brought up a good point, though, in his reviews, which is essentially that our current use of the LayerChromium is extremely hacky. The reason for this is because root vs non-root layers behave differently in WebKit. What I have done in the new patch is to stop the root layer damage logic from piggybacking on the LayerChromium code. By pulling it into the WebView, we keep our code clear. My guess is that iframe handling will end up driving us to resolve root vs non-root issues. A bug for this exists at http://code.google.com/p/chromium/issues/detail?id=54758. I think that bug will ensure that the root-non-root concerns brought up in this discussion will not get swept under the rug.
Comment on attachment 68487 [details] Minimally-controversial approach (I hope) to root layer damage tracking. View in context: https://bugs.webkit.org/attachment.cgi?id=68487&action=review > WebKit/chromium/src/WebViewImpl.cpp:2290 > + IntRect contentRect = view->windowToContents(rect); Is this transformation necessary? It seems to me that both regular and scroll damage rects are in viewport space. > WebKit/chromium/src/WebViewImpl.cpp:2412 > + m_rootLayerDirtyRect = WebRect(); WebRect() -> IntRect() ?
Use of content space is still necessary to deal with scrolls mixed with dirty rects; otherwise, I'd have to scroll the content dirty rect when we scroll. However, I'd rather not touch this as I have to rip this all out and write yet another version to fix the scrollbars issue. Will upload fix for the webrect now.
Created attachment 68760 [details] Patch
Vangelis - any feedback or are you good?
(In reply to comment #16) > Vangelis - any feedback or are you good? Oops, sorry for not responding earlier. I like the latest patch. No issues here.
Comment on attachment 68760 [details] Patch Rejecting patch 68760 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 68760]" exit_code: 2 Cleaning working directory Updating working directory Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=68760&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=46219&ctype=xml Processing 1 patch from 1 bug. Processing patch 68760 from bug 46219. Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'James Robinson', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/4144001
Created attachment 69116 [details] Make it commitable.
Comment on attachment 69116 [details] Make it commitable. Clearing flags on attachment: 69116 Committed r68604: <http://trac.webkit.org/changeset/68604>
All reviewed patches have been landed. Closing bug.