RenderListMarker::computePreferredLogicalWidth should not be public
Created attachment 182869 [details] Patch
Comment on attachment 182869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182869&action=review > Source/WebCore/ChangeLog:17 > + The isImage() codepath never calls computePreferredLogicalWidths, so we need to make > + sure the content and margins are updated. Why doesn't the isImage() codepath call computePreferredLogicalWidth? It looks like computePreferredLogicalWidth has an isImage() branch. > Source/WebCore/rendering/RenderListMarker.cpp:1371 > + return; font and fontMetrics could be moved into the isImage() branch. > Source/WebCore/rendering/RenderListMarker.cpp:1383 > + default: We normally enumerate all the enum values in a switch. I guess this is OK... > Source/WebCore/rendering/RenderListMarker.h:48 > + void updateMarginsAndContent() { updateContent(); updateMargins(); } I would move the implementation to the cpp file unless it needs to be inlined. If it needs to be inlined, then I would split it to multiple lines. > Source/WebCore/rendering/RenderListMarker.h:52 > + virtual void computePreferredLogicalWidths(); OVERRIDE
Comment on attachment 182869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182869&action=review >> Source/WebCore/ChangeLog:17 >> + sure the content and margins are updated. > > Why doesn't the isImage() codepath call computePreferredLogicalWidth? It looks like computePreferredLogicalWidth has an isImage() branch. It doesn't use the min or max preferred widths, so there's no need to. >> Source/WebCore/rendering/RenderListMarker.cpp:1383 >> + default: > > We normally enumerate all the enum values in a switch. I guess this is OK... Sure. It was a long list, so I went with this, but that's fine.
Created attachment 182893 [details] Patch
Comment on attachment 182893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182893&action=review > Source/WebCore/rendering/RenderListMarker.cpp:1365 > + // FIXME: This if-statement is just a performance optimization, but it's messy to use the preferredLogicalWidths dirty bit for this. > + // It's unclear if this is a premature optimization. > + if (!preferredLogicalWidthsDirty()) > + return; It seems like it would be possible for preferredLogicalWidthsDirty to be false and the content changed, but I guess that means we would have a bug where changing the content didn't set preferredLogicalWidthsDirty to true?
(In reply to comment #5) > (From update of attachment 182893 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=182893&action=review > > > Source/WebCore/rendering/RenderListMarker.cpp:1365 > > + // FIXME: This if-statement is just a performance optimization, but it's messy to use the preferredLogicalWidths dirty bit for this. > > + // It's unclear if this is a premature optimization. > > + if (!preferredLogicalWidthsDirty()) > > + return; > > It seems like it would be possible for preferredLogicalWidthsDirty to be false and the content changed, but I guess that means we would have a bug where changing the content didn't set preferredLogicalWidthsDirty to true? Right. That was my thinking. I went down the path of adding a bit to explicitly track this and it was a bit silly because I had to set the dirty bit everywhere that the preferredLogicalWidthsDirty bit was set. That, combined with the extra memory usage, made me decide on the current patch.
Comment on attachment 182893 [details] Patch Clearing flags on attachment: 182893 Committed r139891: <http://trac.webkit.org/changeset/139891>
All reviewed patches have been landed. Closing bug.