Bug 131732

Created attachment 229443 [details] Patch ChangeLog has some wrong entries generated by prepare-ChangeLog script. I'll report a bug related to that pointing to this patch as an example. Of course, before landing this should be fixed even manually if the issue is not fixed before.

Created attachment 229444 [details] Patch Fix Mac build.

Created attachment 229973 [details] Patch

Comment on attachment 229973 [details] Patch Attachment 229973 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6170024674852864 New failing tests: media/W3C/video/networkState/networkState_during_loadstart.html

Created attachment 229980 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Created attachment 230109 [details] Patch Update ChangeLog once bug #131733 has been fixed.

Ping reviewers. Thanks.

Hum, using integers for positioning is suspicious. Maybe Zalan will have an opinion about this.

(In reply to comment #8) > Hum, using integers for positioning is suspicious. Maybe Zalan will have an opinion about this. These look to be grid area indexes and not pixel positions. (similar to table row/column indexes)

(In reply to comment #9) > (In reply to comment #8) > > Hum, using integers for positioning is suspicious. Maybe Zalan will have an opinion about this. > These look to be grid area indexes and not pixel positions. (similar to table row/column indexes) RenderGrid has an internal structure m_grid: typedef Vector<RenderBox*, 1> GridCell; typedef Vector<Vector<GridCell> > GridRepresentation; GridRepresentation m_grid; These integers are positions inside that structure. You can check the new class GridResolvedPosition comment: // This class represents an index into one of the dimensions of the grid array. // Wraps a size_t integer just for the purpose of knowing what we manipulate in the grid code.

Created attachment 232067 [details] Patch Rebased patch.

Ping reviewers. Thanks.

Created attachment 232944 [details] Patch Rebased patch.

Comment on attachment 232944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232944&action=review It's nice to slim down the size of the renderer, I was going to r+ but then I wondered what would be the impact in terms of memory usage. Could you try to run the performance tests (that use huge grids) and paste here the results? > Source/WebCore/rendering/style/GridResolvedPosition.cpp:50 > + Not sure it pays off to have these two as they're only used in explicitGridSizeForSide() > Source/WebCore/rendering/style/GridResolvedPosition.h:144 > + static size_t explicitGridSizeForSide(const RenderStyle& gridContainerStyle, GridPositionSide); This does not make any sense to me. Let's just have the static method in the .cpp

Comment on attachment 232944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232944&action=review I've run the perftests several times in trunk and with my patch and the numbers are pretty similar: * auto-grid-lots-of-data: * Time: * Runs trunk: 360.17 runs/s vs Runs patch: 360.10 runs/s * Memory: * JSHeap trunk: 239.05 KB vs JSHeap patch: 239.05 KB * Malloc trunk: 4504.09 KB vs Malloc patch: 4513.19 KB * fixed-grid-lots-of-data: * Time: * Runs trunk: 356.52 runs/s vs Runs patch: 358.62 runs/s * Memory: * JSHeap trunk: 239.05 KB vs JSHeap patch: 239.05 KB * Malloc trunk: 4507.94 KB vs Malloc patch: 4506.38 KB So it seems, the patch is not regressing. I'm uploading a new patch with the suggested changes. >> Source/WebCore/rendering/style/GridResolvedPosition.cpp:50 >> + > > Not sure it pays off to have these two as they're only used in explicitGridSizeForSide() And in RenderGrid::populateExplicitGridAndOrderIterator(). >> Source/WebCore/rendering/style/GridResolvedPosition.h:144 >> + static size_t explicitGridSizeForSide(const RenderStyle& gridContainerStyle, GridPositionSide); > > This does not make any sense to me. Let's just have the static method in the .cpp Yeah, not sure why it is here :-). Removed from the .h.

Created attachment 232989 [details] Patch

Comment on attachment 232989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232989&action=review OK good figures. Let's get this in. I have a couple of nits that should be fixed before landing. Also check the EFL build failure. > Source/WebCore/rendering/style/GridResolvedPosition.cpp:50 > + Let's remove these two, they don't have anything to do with a resolved position, they just access the style. > Source/WebCore/rendering/style/GridResolvedPosition.h:87 > + Remove the resolvedOppositePosition argument name in these four. I guess it's obvious from the function name.

Created attachment 233041 [details] Patch for landing.

Comment on attachment 233041 [details] Patch for landing. Clearing flags on attachment: 233041 Committed r169934: <http://trac.webkit.org/changeset/169934>

All reviewed patches have been landed. Closing bug.