Summary: | [CoordinatedGraphics] Use unsigned integers for CoordinatedTile IDs | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | jturcotte, kenneth, noam, webkit.review.bot, zeno | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 104654 | ||||||||
Attachments: |
|
Description
Chris Dumez
2012-12-01 10:53:27 PST
Created attachment 177110 [details]
Patch
Comment on attachment 177110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177110&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeCoordinatorProxy.h:27 > +#include "CoordinatedTile.h" For layering protection _I think_ that we try to avoid WebKit2/UIProcess files including WebKit2/WebProcess files. This is the purpose of WebKit2/Shared. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedTile.cpp:109 > - return !!m_ID; > + return m_ID != InvalidCoordinatedTileID; To be honest I'm not totaly for this change because: - If a page creates 100 layers per frame per second, it would take 926 years to reach INT_MAX - This will still have a bad behavior once we wrap around to 0, so it's not 100% correct either So overall I think it adds unecessary complexity. That said, I'm ok if it gets in, the change itself looks fine. (In reply to comment #2) > - If a page creates 100 layers per frame per second, it would take 926 years to reach INT_MAX Strike that, I forgot an important division, it would rather be 248 days for 100 layers created per frame per seconds. It get almost possible at this point. Comment on attachment 177110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177110&action=review >> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedTile.cpp:109 >> + return m_ID != InvalidCoordinatedTileID; > > To be honest I'm not totaly for this change because: > - If a page creates 100 layers per frame per second, it would take 926 years to reach INT_MAX > - This will still have a bad behavior once we wrap around to 0, so it's not 100% correct either > So overall I think it adds unecessary complexity. > > That said, I'm ok if it gets in, the change itself looks fine. Why are you saying there is still bad behavior once we wrap? I updated the code in CoordinatedTile::updateBackBuffer() to handle the case where it wraps. To my knowledge, there is no bad behavior on wrapping anymore. As you said, it is going to take a while to reach INT_MAX but I think it is cleaner and safer to use unsigned IDs still. To my knowledge, others IDs in WebKit are also unsigned, right? You said it brings unnecessary complexity, but I'm not sure I follow. The only code I added if an extra if case in updateBackBuffer() to handle the wrapping case (make sure we don't reuse 0 as ID for a tile). The rest of the changes are purely about using an enum for the ID type instead of the actual integer type. In my opinion, this improves readability but of course it is optional. Comment on attachment 177110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177110&action=review >>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedTile.cpp:109 >>> + return m_ID != InvalidCoordinatedTileID; >> >> To be honest I'm not totaly for this change because: >> - If a page creates 100 layers per frame per second, it would take 926 years to reach INT_MAX >> - This will still have a bad behavior once we wrap around to 0, so it's not 100% correct either >> So overall I think it adds unecessary complexity. >> >> That said, I'm ok if it gets in, the change itself looks fine. > > Why are you saying there is still bad behavior once we wrap? I updated the code in CoordinatedTile::updateBackBuffer() to handle the case where it wraps. To my knowledge, there is no bad behavior on wrapping anymore. > As you said, it is going to take a while to reach INT_MAX but I think it is cleaner and safer to use unsigned IDs still. To my knowledge, others IDs in WebKit are also unsigned, right? > > You said it brings unnecessary complexity, but I'm not sure I follow. The only code I added if an extra if case in updateBackBuffer() to handle the wrapping case (make sure we don't reuse 0 as ID for a tile). The rest of the changes are purely about using an enum for the ID type instead of the actual integer type. In my opinion, this improves readability but of course it is optional. Yes you're right, the overflow handling code that you added seems right (I didn't see it). The unecessary complexity I see is the header cross-include that is required for the typedef + the overflow handling code that is impossible to test and that no living user will likely ever run. But again, this is not a strong opposition, just a personal taste. Other parts of WebKit2 are using uint64_t actually, without typedef for types not defined in WebCore. How would that sound to you? (In reply to comment #5) > (From update of attachment 177110 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177110&action=review > > >>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedTile.cpp:109 > >>> + return m_ID != InvalidCoordinatedTileID; > >> > >> To be honest I'm not totaly for this change because: > >> - If a page creates 100 layers per frame per second, it would take 926 years to reach INT_MAX > >> - This will still have a bad behavior once we wrap around to 0, so it's not 100% correct either > >> So overall I think it adds unecessary complexity. > >> > >> That said, I'm ok if it gets in, the change itself looks fine. > > > > Why are you saying there is still bad behavior once we wrap? I updated the code in CoordinatedTile::updateBackBuffer() to handle the case where it wraps. To my knowledge, there is no bad behavior on wrapping anymore. > > As you said, it is going to take a while to reach INT_MAX but I think it is cleaner and safer to use unsigned IDs still. To my knowledge, others IDs in WebKit are also unsigned, right? > > > > You said it brings unnecessary complexity, but I'm not sure I follow. The only code I added if an extra if case in updateBackBuffer() to handle the wrapping case (make sure we don't reuse 0 as ID for a tile). The rest of the changes are purely about using an enum for the ID type instead of the actual integer type. In my opinion, this improves readability but of course it is optional. > > Yes you're right, the overflow handling code that you added seems right (I didn't see it). > > The unecessary complexity I see is the header cross-include that is required for the typedef + the overflow handling code that is impossible to test and that no living user will likely ever run. But again, this is not a strong opposition, just a personal taste. > Other parts of WebKit2 are using uint64_t actually, without typedef for types not defined in WebCore. How would that sound to you? Yes, I definitely need to fix that. Including the WebProcess headers like I did is not good. Created attachment 177260 [details]
Patch
Take Jocelyn's feedback into consideration.
Comment on attachment 177260 [details] Patch Clearing flags on attachment: 177260 Committed r136658: <http://trac.webkit.org/changeset/136658> All reviewed patches have been landed. Closing bug. |