Assertion failure (!needsLayout()) loading inkedmag.com
Created attachment 252241 [details] Patch
Created attachment 252242 [details] Patch
rdar://problem/20788681
Comment on attachment 252242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252242&action=review GTK and EFL build errors are both: Source/WebCore/rendering/RenderReplaced.cpp:143:5: error: 'SetLayoutNeededForbiddenScope' is not a member of 'WebCore::RenderObject' r=me assuming you fix the release build, but please consider my comments too > Source/WebCore/platform/graphics/BitmapImage.cpp:91 > +void BitmapImage::startTimer(double delay) In new code, we should be using standard library types, not doubles, for time and duration values. I think std::chrono::milliseconds might be what we want here and it’s probably worth considering rewriting the code to use it. > Source/WebCore/platform/graphics/BitmapImage.cpp:94 > + m_frameTimer = std::make_unique<Timer>(*this, &BitmapImage::advanceAnimation); > + m_frameTimer->startOneShot(delay); Should we assert there is no existing timer? If there is, then destroying it is a side effect that doesn’t seem like part of the function name “start timer”. Surprisingly low level graphics class to have a timer in it. I realize you were just refactoring, but I am still surprised. In modern code we would use a lambda instead of a pointer to member function for the timer. > Source/WebCore/platform/graphics/BitmapImage.h:339 > + bool m_pendingNotifyAfterCaughtUpToLastFrame : 1; I find this name confusing. The noun here seems to be “notify” as in “this is a notify”, which doesn’t make sense. How would you describe this boolean flag in colloquial English? Maybe that would point to a better name for it. > Source/WebCore/rendering/RenderReplaced.cpp:143 > + RenderObject::SetLayoutNeededForbiddenScope scope(this); Looks like this class is defined for builds without NDEBUG only. Would be better if it was a harmless inlined no-op in those builds, but otherwise you need to put #ifndef NDEBUG around this. Also, it should take a RenderObject&, not a RenderObject*. Shouldn’t need the RenderObject:: prefix here. This is a member function of a class derived from RenderObject so that should already be in scope without an explicit qualifier.
(In reply to comment #4) > Comment on attachment 252242 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252242&action=review > > GTK and EFL build errors are both: > Source/WebCore/rendering/RenderReplaced.cpp:143:5: error: > 'SetLayoutNeededForbiddenScope' is not a member of 'WebCore::RenderObject' > > r=me assuming you fix the release build, but please consider my comments too > > > Source/WebCore/platform/graphics/BitmapImage.cpp:91 > > +void BitmapImage::startTimer(double delay) > > In new code, we should be using standard library types, not doubles, for > time and duration values. I think std::chrono::milliseconds might be what we > want here and it’s probably worth considering rewriting the code to use it. I'd prefer to switch the entire class over to std::chrono later. > > Source/WebCore/platform/graphics/BitmapImage.cpp:94 > > + m_frameTimer = std::make_unique<Timer>(*this, &BitmapImage::advanceAnimation); > > + m_frameTimer->startOneShot(delay); > > Should we assert there is no existing timer? If there is, then destroying it > is a side effect that doesn’t seem like part of the function name “start > timer”. Added assert. > Surprisingly low level graphics class to have a timer in it. I realize you > were just refactoring, but I am still surprised. In modern code we would use > a lambda instead of a pointer to member function for the timer. Agreed; timer cleanup can come in a separate patch. > > Source/WebCore/platform/graphics/BitmapImage.h:339 > > + bool m_pendingNotifyAfterCaughtUpToLastFrame : 1; > > I find this name confusing. The noun here seems to be “notify” as in “this > is a notify”, which doesn’t make sense. How would you describe this boolean > flag in colloquial English? Maybe that would point to a better name for it. Renamed. > > Source/WebCore/rendering/RenderReplaced.cpp:143 > > + RenderObject::SetLayoutNeededForbiddenScope scope(this); > > Looks like this class is defined for builds without NDEBUG only. Would be > better if it was a harmless inlined no-op in those builds, but otherwise you > need to put #ifndef NDEBUG around this. Also, it should take a > RenderObject&, not a RenderObject*. Wrapping in #ifndef NDEBUG for now, will clean up in a separate patch. > Shouldn’t need the RenderObject:: prefix here. This is a member function of > a class derived from RenderObject so that should already be in scope without > an explicit qualifier. Fixed.
Created attachment 252245 [details] For EWS
Comment on attachment 252245 [details] For EWS Bring Darin's review forward.
http://trac.webkit.org/changeset/183732
This broke a test on Windows: https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fimages%2Fanimated-gif-body-outside-viewport.html Should we roll out?
No, I'll take a look when I get in soon.
Windows test skipped in https://trac.webkit.org/r183756.