[Webkit-unassigned] [Bug 129867] Cut and copy functions shoud be refactored as suggested in FIXME line

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #4 from Darin Adler <darin at apple.com>  2014-03-15 17:38:14 PST ---
(From update of attachment 226109)
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.

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