Bug 100671 - CoordinatedGraphics: Unrelease adopted images
Summary: CoordinatedGraphics: Unrelease adopted images
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: Jocelyn Turcotte
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-29 07:01 PDT by Jocelyn Turcotte
Modified: 2012-10-30 02:15 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.28 KB, patch)
2012-10-29 07:05 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (2.96 KB, patch)
2012-10-29 09:42 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2012-10-29 07:01:08 PDT
CoordinatedGraphics: Unrelease adopted images
Comment 1 Jocelyn Turcotte 2012-10-29 07:05:17 PDT
Created attachment 171242 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-10-29 07:14:36 PDT
Comment on attachment 171242 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:516
> +    // Check if we were going to release this image on the next flush.

during next flush.
check whether

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:517
> +    size_t releasedPos = m_releasedDirectlyCompositedImages.find(key);

Pos meaning what? position?
Comment 3 Jocelyn Turcotte 2012-10-29 07:37:36 PDT
Comment on attachment 171242 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:516
>> +    // Check if we were going to release this image on the next flush.
> 
> during next flush.
> check whether

I don't understand what's wrong with "Check if" and "on the next".

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:517
>> +    size_t releasedPos = m_releasedDirectlyCompositedImages.find(key);
> 
> Pos meaning what? position?

Yep that one is misleading, "index" would be clearer. I'll change it.
Comment 4 Kenneth Rohde Christiansen 2012-10-29 07:40:30 PDT
Isnt it called "during a flush", you dont say on a flush, at least it sounds weird to me.
Comment 5 Jocelyn Turcotte 2012-10-29 07:59:18 PDT
(In reply to comment #4)
> Isnt it called "during a flush", you dont say on a flush, at least it sounds weird to me.

Ok, according to the Internet "on" should only be used for specific days.
Comment 6 Noam Rosenthal 2012-10-29 08:02:05 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Isnt it called "during a flush", you dont say on a flush, at least it sounds weird to me.
> 
> Ok, according to the Internet "on" should only be used for specific days.

Usually "on" is for instantaneous reactions or events, and "during" is for operations that take time; e.g. "on the next timer event" or "during the next render phase".

"if" makes a sentence into a conditional statement. In this case the statement is not conditional. 
</grammar nit>
Comment 7 Jocelyn Turcotte 2012-10-29 09:42:29 PDT
Created attachment 171264 [details]
Patch

Reuploading since I just figured out that the ShareableBitmap should also happen only if it wasn't in the release vector.
I checked and 'if' should be fine here, just less formal. 'If' have the same meaning as 'whether' as long as it's not followed by a second alternative or an infinitive.
Comment 8 Kenneth Rohde Christiansen 2012-10-30 02:09:06 PDT
Comment on attachment 171264 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        Coordinated Graphics: Unrelease adopted images

unrelease? you really mean 'do not release'? Wouldn't it be cleared to use that wording, anyway nit.
Comment 9 WebKit Review Bot 2012-10-30 02:15:47 PDT
Comment on attachment 171264 [details]
Patch

Clearing flags on attachment: 171264

Committed r132883: <http://trac.webkit.org/changeset/132883>
Comment 10 WebKit Review Bot 2012-10-30 02:15:51 PDT
All reviewed patches have been landed.  Closing bug.