Bug 37909 - Call sites of TextIterator constructor are difficult to read
Summary: Call sites of TextIterator constructor are difficult to read
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Shinichiro Hamaji
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-20 22:59 PDT by Shinichiro Hamaji
Modified: 2010-04-21 23:32 PDT (History)
1 user (show)

See Also:


Attachments
Patch v1 (17.86 KB, patch)
2010-04-20 23:02 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v2 (18.12 KB, patch)
2010-04-20 23:06 PDT, Shinichiro Hamaji
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinichiro Hamaji 2010-04-20 22:59:29 PDT
As suggested in Bug 36419, TextIterator and CharacterIterator are instantiated with confusing boolean parameters. Let's use enum version of their constructor and eliminate the old constructors.

I'm planning to change the names of boolean members of these classes (e.g., m_emitCharactersBetweenAllVisiblePositions). They should be third person singular.
Comment 1 Shinichiro Hamaji 2010-04-20 23:02:32 PDT
Created attachment 53918 [details]
Patch v1
Comment 2 Shinichiro Hamaji 2010-04-20 23:06:58 PDT
Created attachment 53919 [details]
Patch v2
Comment 3 Shinichiro Hamaji 2010-04-20 23:08:17 PDT
(In reply to comment #2)
> Created an attachment (id=53919) [details]
> Patch v2

I just modified the ChangeLog.
Comment 4 Darin Adler 2010-04-21 09:34:40 PDT
Comment on attachment 53919 [details]
Patch v2

>  enum TextIteratorBehavior {
>      TextIteratorBehaviorDefault = 0,
> -    TextIteratorBehaviorEmitCharactersBetweenAllVisiblePositions = 1 << 0,
> -    TextIteratorBehaviorEnterTextControls = 1 << 1,
> +    TextIteratorBehaviorEmitsCharactersBetweenAllVisiblePositions = 1 << 0,
> +    TextIteratorBehaviorEntersTextControls = 1 << 1,
>      TextIteratorBehaviorEmitsTextsWithoutTranscoding = 1 << 2,
>  };

I think these repeat the word "Behavior" too much. I would call these:

    TextIteratorDefaultBehavior = 0,
    TextIteratorEmitsCharactersBetweenAllVisiblePositions = 1 << 0,
    TextIteratorEntersTextControls = 1 << 1,
    TextIteratorEmitsTextWithoutTranscoding = 1 << 2,

r=me, whether or not you change the names
Comment 5 Eric Seidel (no email) 2010-04-21 18:15:51 PDT
Attachment 53919 [details] was posted by a committer and has review+, assigning to Shinichiro Hamaji for commit.
Comment 6 Shinichiro Hamaji 2010-04-21 23:32:57 PDT
Landed in http://trac.webkit.org/changeset/58040

Thanks Darin for your review. I changed the name of enum values as you suggested.