[webkit-reviews] review denied: [Bug 129867] Cut and copy functions shoud be refactored as suggested in FIXME line : [Attachment 226109] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Lukasz Bialek <l.bialek at samsung.com>'s
request for review:
Bug 129867: Cut and copy functions shoud be refactored as suggested in FIXME
line
https://bugs.webkit.org/show_bug.cgi?id=129867

Attachment 226109: Patch
https://bugs.webkit.org/attachment.cgi?id=226109&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=226109&action=review


This patch looks OK, but I don’t want to say review+ because I don’t want to
add a new enum and a new public function to Editor just to restructure the code
internals. I’m going to say review-, but please don’t let that discourage you!

> Source/WebCore/editing/Editor.cpp:1298
> +    bool isDoneByDHTML = (action == CutAction) ? tryDHTMLCut() :
tryDHTMLCopy();
> +    if (isDoneByDHTML)
>	   return; // DHTML did the whole operation
> -    if (!canCut()) {
> +
> +    bool canPerformAction = (action == CutAction) ? canCut() : canCopy();
> +    if (!canPerformAction) {
>	   systemBeep();
>	   return;
>      }

Not sure that putting this part of the function into shared code is worthwhile.
I think cut and copy should just do these and then they could share a helper
that does the rest.

> Source/WebCore/editing/Editor.h:90
> +enum EditorActionSpecifier { CutAction, CopyAction };

This enum should be private to the Editor class rather than at the top level.
The other enums are at the namespace level for convenient use outside the
class, but this one does not need to be used outside the class.

> Source/WebCore/editing/Editor.h:130
> +    void performCutOrCopy(EditorActionSpecifier);

This function should be private.