CoordinatedTile currently uses *signed* integer type for its identifier. Due to the way we generate those IDs, it would be safer to use *unsigned* integers. This is because the generated ID will overflow at some point and the C and C++ language standards say that overflow of a signed value is undefined behaviour. In the C99 standard this is in section 6.5. In the C++98 standard it is in section 5 [expr], paragraph 5. "This means that a correct C/C++ program must never generate signed overflow when computing an expression. It also means that a compiler may assume that a program will never generated signed overflow". Note that gcc has -fwrapv flag to assume that signed arithmetic overflow of addition, subtraction and multiplication wraps around using twos-complement representation. However, I still believe it is safer and more consistent to use unsigned integers here. Note that some other classes in CoordinatedGraphics uses signed integers for those IDs but I'm planning to update those in separate patches (in this one gets accepted).
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.