Bug 132332

Created attachment 230366 [details] Patch

Created attachment 232075 [details] Patch Rebased patch.

Created attachment 232095 [details] Patch Fix build.

Created attachment 232421 [details] Patch Upload rebased patch after r169385.

Comment on attachment 232421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232421&action=review > Source/WebCore/rendering/RenderGrid.cpp:1201 > + // We could avoid to dirty the grid if the new child has definite positions and grid auto-flow property is none. The original patch from blink contained some additional logic here, it didn't always just dirty the grid. Is there a reason you removed that? > Source/WebCore/rendering/RenderGrid.cpp:1219 > + return nextSibling; I think it would be clearer and better for future modifications to have a single return point. E.g.: RenderObject* nextSibling = RenderBlock::removeChild(child); if (!girdIsDirty()) dirtyGrid(); return nextSibling;

Comment on attachment 232421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232421&action=review I'm uploading a new patch changing RenderGrid::removeChild() as suggested. >> Source/WebCore/rendering/RenderGrid.cpp:1201 >> + // We could avoid to dirty the grid if the new child has definite positions and grid auto-flow property is none. > > The original patch from blink contained some additional logic here, it didn't always just dirty the grid. Is there a reason you removed that? Basically the patch in Blink was not right, as it was introducing some bugs some of them due to the extra logic added here. I've plans to avoid marking the grid as dirty always, but that will be done in a later step. >> Source/WebCore/rendering/RenderGrid.cpp:1219 >> + return nextSibling; > > I think it would be clearer and better for future modifications to have a single return point. E.g.: > > RenderObject* nextSibling = RenderBlock::removeChild(child); > if (!girdIsDirty()) > dirtyGrid(); > > return nextSibling; Nice suggestion, I've modified it.

Created attachment 232436 [details] Patch

Comment on attachment 232421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232421&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:1201 >>> + // We could avoid to dirty the grid if the new child has definite positions and grid auto-flow property is none. >> >> The original patch from blink contained some additional logic here, it didn't always just dirty the grid. Is there a reason you removed that? > > Basically the patch in Blink was not right, as it was introducing some bugs some of them due to the extra logic added here. > > I've plans to avoid marking the grid as dirty always, but that will be done in a later step. Ok, sounds good.

Created attachment 233300 [details] Patch New version of the patch including a new change from Blink (https://codereview.chromium.org/302083005).

Comment on attachment 233300 [details] Patch This is causing some security issues in Blink, so until it gets stabilized we won't integrate it on WebKit. Probably a new rebase would be needed but that time.