Bug 106956 – RenderListMarker::computePreferredLogicalWidth should not be public

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

RenderListMarker::computePreferredLogicalWidth should not be public

[Ojan Vafai]

Created attachment 182869 [details]
Patch

[Tony Chang]

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

[Ojan Vafai]

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.

[Ojan Vafai]

Created attachment 182893 [details]
Patch

[Tony Chang]

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?

[Ojan Vafai]

(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.

[WebKit Review Bot]

Comment on attachment 182893 [details]
Patch

Clearing flags on attachment: 182893

Committed r139891: <http://trac.webkit.org/changeset/139891>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.