Bug 86919 – Don't force rendering in finishAllRendering

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Labour]

Don't force rendering in finishAllRendering

[Antoine Labour]

Created attachment 142816 [details]
Patch

[WebKit Review Bot]

Attachment 142816 [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:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dana Jansens]

Comment on attachment 142816 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

Can you explain why here?

[James Robinson]

Comment on attachment 142816 [details]
Patch

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

Yes!

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]

You'll have to do this - might be simple enough to write a CCLayerTreeHostTest that hits this case.

[Antoine Labour]

Created attachment 142840 [details]
Patch

[Nat Duca]

Comment on attachment 142840 [details]
Patch

We need to reconcile this with why this existed in the first place before we just go fixing the bug. This was added way back very specifically to prevent flash prevention when switching modes on Windows and OSX. You should check with @jbates to see if this will regress us in some way. EVen better, you could talk with John and figure out how to remove this flow entirely, since the only reason it exists is for flashes. I'm sure updateRect unification fixes this, as does a lot of the recent transport stuff. Let me know, but I'd like to understand the plan before we just remove this.

[Antoine Labour]

+jbates.

As discussed, this only affects the thread, which we're not shipping on any platform where we don't have forced compositing, so this will not introduce any flash on any platform we ship.

I'm happy to remove the finish altogether if we can, but otherwise we need a way to fix this correctness issue (using a texture after it's been destroyed)

[Adrienne Walker]

(In reply to comment #7)
> +jbates.
> 
> As discussed, this only affects the thread, which we're not shipping on any platform where we don't have forced compositing, so this will not introduce any flash on any platform we ship.

If you do land this, please add a blocking bug for http://crbug.com/128385 to fix the flash so that threaded compositing is not turned on for other platforms that may not have force compositing mode on.

[Nat Duca]

Thanks Antoine for studying this. I know it was a pain to do. This seems sane to me, given the constraints.

[James Robinson]

Comment on attachment 142840 [details]
Patch

Yeah, I'm not sure whether we really have a flash in threaded mode or not but I think it's safer to assume that we don't until proven otherwise.  The finishAllRendering() call was added well before we got threaded mode up and running.

[WebKit Review Bot]

Comment on attachment 142840 [details]
Patch

Clearing flags on attachment: 142840

Committed r117825: <http://trac.webkit.org/changeset/117825>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.