[webkit-reviews] review denied: [Bug 104191] Implement matching cue by the class name with ::cue pseudo element : [Attachment 178723] Proposed fix 0.3

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Dima Gorbik <dgorbik at apple.com>'s
request for review:
Bug 104191: Implement matching cue by the class name with ::cue pseudo element
https://bugs.webkit.org/show_bug.cgi?id=104191

Attachment 178723: Proposed fix 0.3
https://bugs.webkit.org/attachment.cgi?id=178723&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=178723&action=review


r- for unsafe cast and many style errors. Please check
http://www.webkit.org/coding/coding-style.html

> Source/WebCore/dom/Element.h:432
> +    virtual bool isWebVTTNode() const;

This doesn't need to be virtual since you are putting the bit to the rare data.


> Source/WebCore/html/track/TextTrackCue.cpp:490
> +void TextTrackCue::markNodesAsWebVTTNodes(Node *object)

Please use meaningful variable names. 'object' is as generic as it gets.

> Source/WebCore/html/track/TextTrackCue.cpp:492
> +    for (Node *child = object->firstChild(); child; child =
child->nextSibling()) {

You should use NodeTraversal::next(current, root) instead of recursing.

* goes next to the type, Node* current

> Source/WebCore/html/track/TextTrackCue.cpp:493
> +	   ((Element *) child)->setIsWebVTTNode(true);

You need to check with current->isElementNode() before casting.
Use C++ style static_cast
Remove unnecessary spaces, Element*

> Source/WebCore/html/track/TextTrackCue.cpp:494
> +	   markNodesAsWebVTTNodes(child);

Use NodeTraversal  instead of recursing.

> Source/WebCore/html/track/TextTrackCue.h:128
> +    void markNodesAsWebVTTNodes(Node *);

* placement