Bug 83229 - Virtualize createAnonymousBlockWithSameTypeAs and move to RenderBox
Summary: Virtualize createAnonymousBlockWithSameTypeAs and move to RenderBox
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Abhishek Arya
URL:
Keywords:
Depends on:
Blocks: 82630
  Show dependency treegraph
 
Reported: 2012-04-04 15:52 PDT by Abhishek Arya
Modified: 2012-04-06 14:33 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.64 KB, patch)
2012-04-04 16:29 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff
Patch (8.75 KB, patch)
2012-04-06 10:15 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff
Patch (8.88 KB, patch)
2012-04-06 14:27 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Abhishek Arya 2012-04-04 15:52:26 PDT
Part 2 of fixing https://bugs.webkit.org/show_bug.cgi?id=82630
Comment 1 Abhishek Arya 2012-04-04 16:29:21 PDT
Created attachment 135713 [details]
Patch
Comment 2 Julien Chaffraix 2012-04-06 09:59:21 PDT
Comment on attachment 135713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135713&action=review

> Source/WebCore/ChangeLog:9
> +        This helps to use the same function to create anonymous
> +        table parts as well.

This is also in preparation of adding more of them IIRC.

> Source/WebCore/rendering/RenderTable.h:215
> +    virtual RenderBox* createAnonymousBoxWithSameTypeAs(const RenderObject* parent) const

Does this function need to be public? Also let's to annotate the overridden functions with OVERRIDE. (comments not repeated on all the other table renderers)
Comment 3 Abhishek Arya 2012-04-06 10:01:47 PDT
(In reply to comment #2)
> (From update of attachment 135713 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135713&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        This helps to use the same function to create anonymous
> > +        table parts as well.
> 
> This is also in preparation of adding more of them IIRC.

Correcting the comment.

> 
> > Source/WebCore/rendering/RenderTable.h:215
> > +    virtual RenderBox* createAnonymousBoxWithSameTypeAs(const RenderObject* parent) const
> 
> Does this function need to be public? Also let's to annotate the overridden functions with OVERRIDE. (comments not repeated on all the other table renderers)

Doing the OVERRIDE now. It needs to be public since it will be called from the otherAnonymousBlock as otherAnonymousBlock->createAnonymousBoxWithSameTypeAs(parent).
Comment 4 Abhishek Arya 2012-04-06 10:15:47 PDT
Created attachment 136040 [details]
Patch
Comment 5 Julien Chaffraix 2012-04-06 14:14:07 PDT
Comment on attachment 136040 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136040&action=review

> Source/WebCore/ChangeLog:9
> +        table parts and in the future extend to more derived

Nit: "more derived" was like deeper class hierarchy in my first reading, I would just re-arrange the wording: in the future extend to more classes derived from RenderBox.

> Source/WebCore/ChangeLog:11
> +

Let's add a comment about why we did that: the current switch case situation was going to be messy as we will need to mix cases that were very dependent on the class so it made sense to add a virtual function.

> Source/WebCore/rendering/RenderBox.h:517
> +    virtual RenderBox* createAnonymousBoxWithSameTypeAs(const RenderObject*) const

If you are exposing this function in the derived class, we may as well expose this one. It's unfortunate that we have to but I see your point.
Comment 6 Abhishek Arya 2012-04-06 14:27:51 PDT
Created attachment 136066 [details]
Patch
Comment 7 Abhishek Arya 2012-04-06 14:33:01 PDT
Comment on attachment 136066 [details]
Patch

Clearing flags on attachment: 136066

Committed r113497: <http://trac.webkit.org/changeset/113497>
Comment 8 Abhishek Arya 2012-04-06 14:33:06 PDT
All reviewed patches have been landed.  Closing bug.