Bug 129867 – Cut and copy functions should be refactored as suggested in FIXME line

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Lukasz Bialek]

Most of code in Cut and Copy functions in Editor.cpp is identical (or analogical). There is a FIXME line suggesting to refactor this part of the code to start sharing code.

[Lukasz Bialek]

Created attachment 226093 [details]
Patch

[Lukasz Bialek]

Created attachment 226100 [details]
There was small error in first version. Fixed.

[Lukasz Bialek]

Created attachment 226109 [details]
Patch

copy() and cut() functions have to be kept (calling newly refactored function) to avoid changing function names in all ports (which is not a good idea...)

[Darin Adler]

Comment on attachment 226109 [details]
Patch

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.

[Lukasz Bialek]

Created attachment 228033 [details]
Patch

All comments were very accurate and has been addressed.

[Lukasz Bialek]

> I’m going to say review-, but please don’t let that discourage you!

It's not so easy to discourage me :) Especially when comments are good and accurate. Fixed and ready for next review.

[WebKit Commit Bot]

Comment on attachment 228033 [details]
Patch

Clearing flags on attachment: 228033

Committed r166445: <http://trac.webkit.org/changeset/166445>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.