Bug 46219 – [chromium] invalidateRootLayerRect needs to schedule compositor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nat Duca]

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.

[Nat Duca]

Created attachment 68296 [details]
Patch

[James Robinson]

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.

[Nat Duca]

Good question.

In the current implementation, the root layer behaves differently than all other layers, and does not actually call notifySyncRequired.

[James Robinson]

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

[Nat Duca]

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.

[Nat Duca]

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

[Nat Duca]

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.

[Nat Duca]

Created attachment 68429 [details]
Add comments to LayerChromium::setNeedsDisplay

[Vangelis Kokkevis]

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

[Nat Duca]

Created attachment 68487 [details]
Minimally-controversial approach (I hope) to root layer damage tracking.

[James Robinson]

Looks OK to me - will r+ if Vangelis agrees.

[Nat Duca]

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.

[Vangelis Kokkevis]

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

[Nat Duca]

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.

[Nat Duca]

Created attachment 68760 [details]
Patch

[Nat Duca]

Vangelis - any feedback or are you good?

[Vangelis Kokkevis]

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

[WebKit Commit Bot]

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 [e-mail]...
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

[Nat Duca]

Created attachment 69116 [details]
Make it commitable.

[WebKit Commit Bot]

Comment on attachment 69116 [details]
Make it commitable.

Clearing flags on attachment: 69116

Committed r68604: <http://trac.webkit.org/changeset/68604>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.