Bug 106956 - RenderListMarker::computePreferredLogicalWidth should not be public
Summary: RenderListMarker::computePreferredLogicalWidth should not be public
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-15 16:11 PST by Ojan Vafai
Modified: 2013-01-16 10:04 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.95 KB, patch)
2013-01-15 16:22 PST, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (9.03 KB, patch)
2013-01-15 18:25 PST, Ojan Vafai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2013-01-15 16:11:21 PST
RenderListMarker::computePreferredLogicalWidth should not be public
Comment 1 Ojan Vafai 2013-01-15 16:22:25 PST
Created attachment 182869 [details]
Patch
Comment 2 Tony Chang 2013-01-15 17:02:38 PST
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 3 Ojan Vafai 2013-01-15 18:24:51 PST
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.
Comment 4 Ojan Vafai 2013-01-15 18:25:36 PST
Created attachment 182893 [details]
Patch
Comment 5 Tony Chang 2013-01-16 09:48:06 PST
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?
Comment 6 Ojan Vafai 2013-01-16 09:58:46 PST
(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 7 WebKit Review Bot 2013-01-16 10:04:13 PST
Comment on attachment 182893 [details]
Patch

Clearing flags on attachment: 182893

Committed r139891: <http://trac.webkit.org/changeset/139891>
Comment 8 WebKit Review Bot 2013-01-16 10:04:18 PST
All reviewed patches have been landed.  Closing bug.