Bug 86919 - Don't force rendering in finishAllRendering
Summary: Don't force rendering in finishAllRendering
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Labour
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-18 16:10 PDT by Antoine Labour
Modified: 2012-05-21 15:57 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.85 KB, patch)
2012-05-18 16:19 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (7.44 KB, patch)
2012-05-18 18:18 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Labour 2012-05-18 16:10:40 PDT
Don't force rendering in finishAllRendering
Comment 1 Antoine Labour 2012-05-18 16:19:07 PDT
Created attachment 142816 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-18 16:30:35 PDT
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.
Comment 3 Dana Jansens 2012-05-18 16:32:46 PDT
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?
Comment 4 James Robinson 2012-05-18 17:38:46 PDT
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.
Comment 5 Antoine Labour 2012-05-18 18:18:01 PDT
Created attachment 142840 [details]
Patch
Comment 6 Nat Duca 2012-05-20 12:12:07 PDT
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.
Comment 7 Antoine Labour 2012-05-21 10:20:08 PDT
+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)
Comment 8 Adrienne Walker 2012-05-21 11:45:46 PDT
(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.
Comment 9 Nat Duca 2012-05-21 14:55:41 PDT
Thanks Antoine for studying this. I know it was a pain to do. This seems sane to me, given the constraints.
Comment 10 James Robinson 2012-05-21 15:01:48 PDT
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.
Comment 11 WebKit Review Bot 2012-05-21 15:57:05 PDT
Comment on attachment 142840 [details]
Patch

Clearing flags on attachment: 142840

Committed r117825: <http://trac.webkit.org/changeset/117825>
Comment 12 WebKit Review Bot 2012-05-21 15:57:11 PDT
All reviewed patches have been landed.  Closing bug.