[Webkit-unassigned] [Bug 47815] First-letter style is applied to the letters that were once first but not any more due to style change

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=47815


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #94924|review?                     |review-
               Flag|                            |




--- Comment #17 from Darin Adler <darin at apple.com>  2012-10-29 12:37:47 PST ---
(From update of attachment 94924)
View in context: https://bugs.webkit.org/attachment.cgi?id=94924&action=review

Code looks generally OK, but there seem to be a few problems.

> Source/WebCore/rendering/RenderBlock.cpp:5299
> +    RenderObject* nextObj = remainingTextFragment->nextSibling();

Please don’t use abbreviations like “obj” in new code in WebKit. We prefer whole words.

> Source/WebCore/rendering/RenderBlock.h:195
> +    bool isFirstLetterCreated() const { return m_rareData ? m_rareData->m_isFirstLetterCreated : false; }

Naming here is not right.

When we have a boolean member function, we name it so that it works in a phrase like this “<class> <function>”, in this case “render block <function>”. But the phrase “render block is first letter created” is nonsense, showing us that it’s not a good name. One good name might be “has first letter block”, which would be written like this: hasFirstLetterBlock.

The code says this is used to avoid unnecessary searching, but

> Source/WebCore/rendering/RenderBlock.h:353
> +    void removeFirstLetter(RenderObject* firstLetter);

These functions that use the words “first letter” for the “first letter block” are really confusing. Such functions do not “remove the first letter”, so it’s confusing to call them “remove first letter”.

> Source/WebCore/rendering/RenderBlock.h:355
> +    void removeFirstLetter(RenderObject* firstLetter);
> +    bool removeStaleFirstLetter(RenderObject* container);
> +    void removeStaleFirstLetterForFirstLetterBlock(RenderObject* firstLetterBlock);

All three of these functions seem like they should be static member functions. None of them seem to use the “this” pointer.

> Source/WebCore/rendering/RenderTextFragment.cpp:52
> +PassRefPtr<StringImpl> RenderTextFragment::fullOriginalText() const

It is incorrect for this function to return a PassRefPtr. It does not produce a new object.

> Source/WebCore/rendering/RenderTextFragment.cpp:54
> +    Node* domNode = node();

WebKit coding style in cases like this is to do this:

    Node* node = this->node();

That way we don’t end up with awkward names like domNode.

> Source/WebCore/rendering/RenderTextFragment.cpp:55
> +    return ((domNode && domNode->isTextNode()) ? static_cast<Text*>(domNode)->dataImpl() : contentString());

We normally would not use those parentheses in WebKit coding style.

> Source/WebCore/rendering/RenderTextFragment.cpp:66
> +RenderText* RenderTextFragment::reproduceRenderText(RenderArena* renderArena)

The verb “reproduce” is not really an apt verb to use here. In cases like this we sometimes use the verb “clone”, but I think the function name ideally needs to be considerably clearer on what kind of RenderText this creates. I’m also not clear that it makes sense to have this function be a member function of RenderTextFragment. Perhaps it could be a RenderText static member function that takes a RenderTextFragment* argument instead.

> Source/WebCore/rendering/RenderTextFragment.cpp:110
> +        Node* domNode = node();
> +        StringImpl* original = ((domNode && domNode->isTextNode()) ? static_cast<Text*>(domNode)->dataImpl() : contentString());

This is a copy of the new fullOriginalText function. We do not want two copies of this code.

> Source/WebCore/rendering/RenderTextFragment.h:51
> +    PassRefPtr<StringImpl> fullOriginalText() const;

Why does this need to be a public function? It’s not used outside the class at this time.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.