1278014 - Create mozilla::SelectionType as an enum class for type safer and shorter constants

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

nsISelectionController::SELECTION_* is too long for our coding rules and it's just |short|. So, can be included an invalid value and it's hard to understand which value is which selection in debugger (although, watching such value is rare...).

Especially for the former, I'd like to create an enum class SelectionType in nsISelectionController.h. Before that, we should rename current |SelectionType| which is alias of |short| to |RawSelectionType|.

I'm thinking about the enum class is:

namespace mozilla {

typedef short RawSelectionType;
enum class SelectionType : RawSelectionType
{
  eNone = nsISelectionController::SELECTION_NONE,
  eNormal = nsISelectionController::SELECTION_NORMAL,
  eSpellCheck = nsISelectionController::SELECTION_SPELLCHECK,
  eIMERawClause = nsISelectionController::SELECTION_IME_RAWINPUT=4,
  eIMESelectedRawClause =
    nsISelectionController::SELECTION_IME_SELECTEDRAWTEXT,
  eIMEConvertedClause =
    nsISelectionController::SELECTION_IME_CONVERTEDTEXT,
  eIMESelectedClause =
    nsISelectionController::SELECTION_IME_SELECTEDCONVERTEDTEXT,
  eAccessibility =
    nsISelectionController::SELECTION_ACCESSIBILITY,
  eFind = nsISelectionController::SELECTION_FIND,
  eURLSecondary = nsISelectionController::SELECTION_URLSECONDARY,
  eURLStrikeout = nsISelectionController::SELECTION_URLSTRIKEOUT,
};

Should eAccessibility be eA11y? Should eFind be eFindResult or eFound?

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Should eFind be eFindResult or eFound?

If it should be eFound, then, is eWrongWord better than eSpellCheck?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I would assume the current constants are used by quite a few addons. 
So think we shouldn't change nsISelectionController.idl

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #2)
> I would assume the current constants are used by quite a few addons. 
> So think we shouldn't change nsISelectionController.idl

Yeah, I do NOT try to change nsISelectionController "interface". I'd like to add enum class which is defined with nsISelectionController::SELECTION_* for internal native code. I.e., adding new enum class definition bottom of the .idl file with |%{C++| block (Although, I can do that actually, if it's impossible I should do that with globally exported header file).

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I see. Yeah, if that makes the code elsewhere simpler or easier to maintain, then sounds good.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks. But how do you think about the naming issue mentioned in comment 0 and comment 1?

> Should eAccessibility be eA11y? Should eFind be eFindResult or eFound?

> If it should be eFound, then, is eWrongWord better than eSpellCheck?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, please check the previous comment.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I'd use eAccessibility and eFind and eSpellCheck... so trying to map the enum names to constants as much as possible.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eba7edc5e596

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae8b5366c769

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c3e6bc71697

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9627a95e4fe

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23a204734754

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1278014 part.1 Rename SelectionType in nsISelectionController.idl to mozilla::RawSelectionType

mozilla::SelectionType will be an enum class. Therefore, we need to rename SelectionType with a word "raw" since it's a type for raw nsISelectionController::SELECTION_*.

Review commit: https://reviewboard.mozilla.org/r/58732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58732/

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1278014 part.2 Define mozilla::SelectionType as an enum class and use it instead of RawSelectionType as far as possible

This patch defines mozilla::SelectionType as an enum class.  This is safer than nsISelectionController::SELECTION_* since setting illegal value to its variable is checked at build time.  So, as far as possible, this should be used everywhere (but of course, this isn't available in scriptable interfaces).

And also this implements some useful methods for managing SelectionType and RawSelectionType which are implemented in layout/nsSelection.cpp because nsISelectionController is implemented by both PresShell and nsTextEditorState.  Therefore, implementing one of them may make hard to find them.  On the other hand, nsSelection.cpp is a better file name to look for them.

Note that this patch creates mozilla::Selection::RawType() for binding.  Native code should keep using Selection::Type() but the binding code needs to use RawType() due to impossible to convert from SelectionType to RawSelectionType without explicit cast.

Review commit: https://reviewboard.mozilla.org/r/58734/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58734/

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1278014 part.3 Define 2 constants for number of SelectionType and number of SelectionType except "none"

This patch defines two constants kSelectionTypeCount and kPresentSelectionTypeCount.  The former is same as nsISelectionController::NUM_SELECTIONTYPES.  The latter is kSelectionTypeCount - 1 for excluding SELECTION_NONE.  The latter is useful in some loops which handle all selection types except SELECTION_NONE.

Note that this patch fixes a bug of nsFrameSelection.  That doesn't treat SELECTION_NONE as a selection (see the definition of index), however, it defines redundant item and doesn't use it actually.  Additionally, it computes invalid selection type in each loop.  Therefore, without this patch, debug build hits MOZ_ASSERT() in ToSelectionType(RawSelectionType).

Note that these constants are defined as anonymous enum because we cannot define as const (or static) even with extern.  If we'd try to do it, it caused link error or not available in nsFrameSelection.cpp as constant value since they were not initialized if they were initialized in nsSelection.cpp.  Therefore, these constants are defined as enum items but using "k" prefix.

Review commit: https://reviewboard.mozilla.org/r/58736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58736/

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1278014 part.4 Rename SelectionType::SELECTION_NONE to SelectionType::eNone

Review commit: https://reviewboard.mozilla.org/r/58738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58738/

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1278014 part.5 Rename SelectionType::SELECTION_NORMAL to SelectionType::eNormal

Review commit: https://reviewboard.mozilla.org/r/58740/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58740/

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1278014 part.6 Rename SelectionType::SELECTION_SPELLCHECK to SelectionType::eSpellCheck

Review commit: https://reviewboard.mozilla.org/r/58742/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58742/

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1278014 part.7 Rename SelectionType::SELECTION_IME_RAWINPUT to SelectionType::eIMERawClause

Review commit: https://reviewboard.mozilla.org/r/58744/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58744/

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1278014 part.8 Rename SelectionType::SELECTION_IME_SELECTEDRAWTEXT to SelectionType::eIMESelectedRawClause

Review commit: https://reviewboard.mozilla.org/r/58746/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58746/

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1278014 part.9 Rename SelectionType::SELECTION_IME_CONVERTEDTEXT to SelectionType::eIMEConvertedClause

Review commit: https://reviewboard.mozilla.org/r/58748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58748/

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1278014 part.10 Rename SelectionType::SELECTION_IME_SELECTEDCONVERTEDTEXT to SelectionType::eIMESelectedClause

Review commit: https://reviewboard.mozilla.org/r/58750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58750/

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1278014 part.11 Rename SelectionType::SELECTION_ACCESSIBILITY to SelectionType::eAccessibility

Review commit: https://reviewboard.mozilla.org/r/58752/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58752/

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1278014 part.12 Rename SelectionType::SELECTION_FIND to SelectionType::eFind

Review commit: https://reviewboard.mozilla.org/r/58754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58754/

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1278014 part.13 Rename SelectionType::SELECTION_URLSECONDARY to SelectionType::eURLSecondary

Review commit: https://reviewboard.mozilla.org/r/58756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58756/

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1278014 part.14 Rename SelectionType::SELECTION_URLSTRIKEOUT to SelectionType::eURLStrikeout

Review commit: https://reviewboard.mozilla.org/r/58758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58758/

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: The patches basically do NOT change any behavior intentionally but except part.3, it needs to change the behavior of nsFrameSelection because current buggy code hits new assertion in ToSelectionType(RawSelectionType).

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761627 [details]
Bug 1278014 part.1 Rename SelectionType in nsISelectionController.idl to mozilla::RawSelectionType

https://reviewboard.mozilla.org/r/58732/#review55874

I assume the latter patches make C++ code to use some new enum values, so this is just intermediate step.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761628 [details]
Bug 1278014 part.2 Define mozilla::SelectionType as an enum class and use it instead of RawSelectionType as far as possible

https://reviewboard.mozilla.org/r/58734/#review55876

So I think I'd like to see a new patch fixing especially invalid raw value handling. And explanation or fix for the operator&

::: dom/bindings/Bindings.conf:981
(Diff revision 1)
>  'Screen': {
>      'nativeType': 'nsScreen',
>  },
>  
> +'Selection': {
> +    'binaryNames': {

You can and should define this in Selection.webidl
using [BinaryName].
Better to try to avoid putting stuff to Bindings.conf whenever possible.

::: dom/html/nsTextEditorState.cpp:347
(Diff revision 1)
> +                                       nsISelection** aSelection)
>  {
>    if (!mFrameSelection)
>      return NS_ERROR_NULL_POINTER;
> -    
> -  *_retval = mFrameSelection->GetSelection(type);
> +
> +  if (!IsValidSelectionType(aRawSelectionType)) {

I wonder if it would make code simpler if we just had a SelectionType for invalid values.
GetSelection() would just return null for such.

I should at least make the code less error prone, since now everyone needs to remember to check validity of selection type.

::: layout/generic/nsSelection.cpp:165
(Diff revision 1)
> +      return false;
> +  }
> +}
> +
> +SelectionType
> +ToSelectionType(RawSelectionType aRawSelectionType)

So I would make this method to check whether the raw type is valid, and if not, return some
SelectionType::SELECTION_INVALID_TYPE

::: layout/generic/nsSelection.cpp:177
(Diff revision 1)
> +ToRawSelectionType(SelectionType aSelectionType)
> +{
> +  return static_cast<RawSelectionType>(aSelectionType);
> +}
> +
> +bool operator &(SelectionType aSelectionType,

It is unclear to me why we want to have this kind of operator. Could you explain

::: layout/generic/nsTextFrame.cpp:6206
(Diff revision 1)
>    // general principal that lower-numbered selections are higher priority.
> -  allRawSelectionTypes &= RawSelectionTypesWithDecorations;
> +  allRawSelectionTypes &= kRawSelectionTypesWithDecorations;
>    for (int32_t i = nsISelectionController::NUM_SELECTIONTYPES - 1;
>         i >= 1; --i) {
> -    RawSelectionType rawSelectionType = 1 << (i - 1);
> -    if (allRawSelectionTypes & rawSelectionType) {
> +    SelectionType selectionType = ToSelectionType(1 << (i - 1));
> +    if (selectionType & allRawSelectionTypes) {

aha, so the operator is used here. But do we really want that? Why can't we play with selection types here, not with raw type

::: layout/generic/nsTextFrame.cpp:7157
(Diff revision 1)
>    nsPresContext* presContext = PresContext();
>    while (f && f->GetContentOffset() < int32_t(aEnd)) {
>      // We may need to reflow to recompute the overflow area for
>      // spellchecking or IME underline if their underline is thicker than
>      // the normal decoration line.
> -    if (aRawSelectionType & RawSelectionTypesWithDecorations) {
> +    if (aSelectionType & kRawSelectionTypesWithDecorations) {

ah used also here. Looks odd.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761629 [details]
Bug 1278014 part.3 Define 2 constants for number of SelectionType and number of SelectionType except "none"

https://reviewboard.mozilla.org/r/58736/#review55974

::: dom/base/nsISelectionController.idl:306
(Diff revision 1)
>    SELECTION_URLSTRIKEOUT = nsISelectionController::SELECTION_URLSTRIKEOUT,
>  };
>  
> +// Using anonymous enum to define constants because these constants may be
> +// used at defining fixed size array in some header files (e.g.,
> +// nsFrameSelection.h).  So, the values needs to be defined here but in such

Something odd with the sentence. "but in such cause..."
Perhaps just "...here, but we cannot use..."

::: dom/base/nsISelectionController.idl:314
(Diff revision 1)
> +enum : size_t
> +{
> +  // kSelectionTypeCount is number of SelectionType.
> +  kSelectionTypeCount = nsISelectionController::NUM_SELECTIONTYPES,
> +  // kPresentSelectionTypeCount is number of SelectionType except "none".
> +  kPresentSelectionTypeCount = kSelectionTypeCount - 1

'present' sounds a bit odd. Right now I don't have a good suggestion though.

::: layout/generic/nsTextFrame.cpp:6203
(Diff revision 1)
>    // Iterate through just the selection rawSelectionTypes that paint decorations
>    // and paint decorations for any that actually occur in this frame. Paint
>    // higher-numbered selection rawSelectionTypes below lower-numered ones on the
>    // general principal that lower-numbered selections are higher priority.
>    allRawSelectionTypes &= kRawSelectionTypesWithDecorations;
> -  for (int32_t i = nsISelectionController::NUM_SELECTIONTYPES - 1;
> +  for (size_t i = kSelectionTypeCount - 1; i >= 1; --i) {

So why we don't use kPresentSelectionTypeCount here?

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761630 [details]
Bug 1278014 part.4 Rename SelectionType::SELECTION_NONE to SelectionType::eNone

https://reviewboard.mozilla.org/r/58738/#review55978

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761631 [details]
Bug 1278014 part.5 Rename SelectionType::SELECTION_NORMAL to SelectionType::eNormal

https://reviewboard.mozilla.org/r/58740/#review55980

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761632 [details]
Bug 1278014 part.6 Rename SelectionType::SELECTION_SPELLCHECK to SelectionType::eSpellCheck

https://reviewboard.mozilla.org/r/58742/#review55984

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761633 [details]
Bug 1278014 part.7 Rename SelectionType::SELECTION_IME_RAWINPUT to SelectionType::eIMERawClause

https://reviewboard.mozilla.org/r/58744/#review55986

::: dom/base/nsISelectionController.idl:290
(Diff revision 1)
>  enum class SelectionType : RawSelectionType
>  {
>    eNone = nsISelectionController::SELECTION_NONE,
>    eNormal = nsISelectionController::SELECTION_NORMAL,
>    eSpellCheck = nsISelectionController::SELECTION_SPELLCHECK,
> -  SELECTION_IME_RAWINPUT = nsISelectionController::SELECTION_IME_RAWINPUT,
> +  eIMERawClause = nsISelectionController::SELECTION_IME_RAWINPUT,

A bit annoying to have so different names for the enum vs. the constant.
Would eIMERawInput be really that bad?
But up to you.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761634 [details]
Bug 1278014 part.8 Rename SelectionType::SELECTION_IME_SELECTEDRAWTEXT to SelectionType::eIMESelectedRawClause

https://reviewboard.mozilla.org/r/58746/#review55990

::: dom/base/nsISelectionController.idl:291
(Diff revision 1)
>  {
>    eNone = nsISelectionController::SELECTION_NONE,
>    eNormal = nsISelectionController::SELECTION_NORMAL,
>    eSpellCheck = nsISelectionController::SELECTION_SPELLCHECK,
>    eIMERawClause = nsISelectionController::SELECTION_IME_RAWINPUT,
> -  SELECTION_IME_SELECTEDRAWTEXT =
> +  eIMESelectedRawClause = nsISelectionController::SELECTION_IME_SELECTEDRAWTEXT,

Same question here.
Would eIMESelectedRawText be really that bad?
But I can live with eIMESelectedRawClause too.
Up to you.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761635 [details]
Bug 1278014 part.9 Rename SelectionType::SELECTION_IME_CONVERTEDTEXT to SelectionType::eIMEConvertedClause

https://reviewboard.mozilla.org/r/58748/#review55992

::: dom/base/nsISelectionController.idl:292
(Diff revision 1)
>    eNone = nsISelectionController::SELECTION_NONE,
>    eNormal = nsISelectionController::SELECTION_NORMAL,
>    eSpellCheck = nsISelectionController::SELECTION_SPELLCHECK,
>    eIMERawClause = nsISelectionController::SELECTION_IME_RAWINPUT,
>    eIMESelectedRawClause = nsISelectionController::SELECTION_IME_SELECTEDRAWTEXT,
> -  SELECTION_IME_CONVERTEDTEXT =
> +  eIMEConvertedClause = nsISelectionController::SELECTION_IME_CONVERTEDTEXT,

and same also here.
eIMEConvertedText would keep the name consistent with the constant value.
But I can live with eIMEConvertedClause too

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761636 [details]
Bug 1278014 part.10 Rename SelectionType::SELECTION_IME_SELECTEDCONVERTEDTEXT to SelectionType::eIMESelectedClause

https://reviewboard.mozilla.org/r/58750/#review55996

::: dom/base/nsISelectionController.idl:293
(Diff revision 1)
>    eNormal = nsISelectionController::SELECTION_NORMAL,
>    eSpellCheck = nsISelectionController::SELECTION_SPELLCHECK,
>    eIMERawClause = nsISelectionController::SELECTION_IME_RAWINPUT,
>    eIMESelectedRawClause = nsISelectionController::SELECTION_IME_SELECTEDRAWTEXT,
>    eIMEConvertedClause = nsISelectionController::SELECTION_IME_CONVERTEDTEXT,
> -  SELECTION_IME_SELECTEDCONVERTEDTEXT =
> +  eIMESelectedClause =

similar concern also here. But eIMESelectedClause is fine too

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761637 [details]
Bug 1278014 part.11 Rename SelectionType::SELECTION_ACCESSIBILITY to SelectionType::eAccessibility

https://reviewboard.mozilla.org/r/58752/#review56000

mozreview gives some errors with this. Can't review.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761637 [details]
Bug 1278014 part.11 Rename SelectionType::SELECTION_ACCESSIBILITY to SelectionType::eAccessibility

https://reviewboard.mozilla.org/r/58752/#review56002

oh, maybe it is some mozreview bustage.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761637 [details]
Bug 1278014 part.11 Rename SelectionType::SELECTION_ACCESSIBILITY to SelectionType::eAccessibility

https://reviewboard.mozilla.org/r/58752/#review56006

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761638 [details]
Bug 1278014 part.12 Rename SelectionType::SELECTION_FIND to SelectionType::eFind

https://reviewboard.mozilla.org/r/58754/#review56008

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761639 [details]
Bug 1278014 part.13 Rename SelectionType::SELECTION_URLSECONDARY to SelectionType::eURLSecondary

https://reviewboard.mozilla.org/r/58756/#review56010

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761640 [details]
Bug 1278014 part.14 Rename SelectionType::SELECTION_URLSTRIKEOUT to SelectionType::eURLStrikeout

https://reviewboard.mozilla.org/r/58758/#review56012

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://reviewboard.mozilla.org/r/58734/#review55876

> I wonder if it would make code simpler if we just had a SelectionType for invalid values.
> GetSelection() would just return null for such.
> 
> I should at least make the code less error prone, since now everyone needs to remember to check validity of selection type.

Oh, that's a good idea. Okay, I'll check it to so.

> aha, so the operator is used here. But do we really want that? Why can't we play with selection types here, not with raw type

SelectionType is not a simple integer values. Named enum and enum class are not computed as integer. Therefore, only with SelectionType, we cannot represent multiple values with a single variable. Of course, like &, we can write operatar definition for enum and make SelectionType computable. But I don't like to do that because impossible to compute values is a good point of enum class. That guarantees to avoid invalid value with simple mistake.

Therefore, my conclusion is adding a compair operator & here. How do you think?

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://reviewboard.mozilla.org/r/58744/#review55986

> A bit annoying to have so different names for the enum vs. the constant.
> Would eIMERawInput be really that bad?
> But up to you.

The reason why I defined different names at defining TextRangeType is nsISelectionController::SELECTION_IME_* are not resonable names. E.g., INPUT vs. TEXT, and too long names.  And these names are similar to TextRangeType whose values are mapped with SelectionType. So, different names from nsISelectionController::SELECTION_IME_* causes better consistency with TextRangeType.  At least in C++ code, we should use SelectionType as far as possible.  Therefore, it shouldn't be problem to use different names from RawSelectionType.

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://reviewboard.mozilla.org/r/58746/#review55990

> Same question here.
> Would eIMESelectedRawText be really that bad?
> But I can live with eIMESelectedRawClause too.
> Up to you.

As far as I said, this name is similar to TextRangeType.  That means that it's easier to understand for developers who know native IME APIs.

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://reviewboard.mozilla.org/r/58748/#review55992

> and same also here.
> eIMEConvertedText would keep the name consistent with the constant value.
> But I can live with eIMEConvertedClause too

Same as I said. I like better to use similar name to TextRangeType.

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://reviewboard.mozilla.org/r/58750/#review55996

> similar concern also here. But eIMESelectedClause is fine too

So, I'll keep using the new names.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://reviewboard.mozilla.org/r/58736/#review55974

> 'present' sounds a bit odd. Right now I don't have a good suggestion though.

Yeah, I didn't get some good idea to represent "non-noselection". For now, I'll keep using this name, but if you find a better name, feel free to suggest me even if I'll land them since renaming constant is very easy.

> So why we don't use kPresentSelectionTypeCount here?

Indeed, kPresentSelectionTypeCount - 1 is same as kSelectionTypeCount. However, the meaning are different.

In this for loop, it treats every SelectionType from the last item to the first item. Therefore, I believe that using kSelectionTypeCount - 1 is better than kPresentSelectionTypeCount for easier to understand.  How about you?

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0572042d17a

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8761627 [details]
Bug 1278014 part.1 Rename SelectionType in nsISelectionController.idl to mozilla::RawSelectionType

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58732/diff/1-2/

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8761628 [details]
Bug 1278014 part.2 Define mozilla::SelectionType as an enum class and use it instead of RawSelectionType as far as possible

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58734/diff/1-2/

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8761629 [details]
Bug 1278014 part.3 Define 2 constants for number of SelectionType and number of SelectionType except "none"

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58736/diff/1-2/

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8761630 [details]
Bug 1278014 part.4 Rename SelectionType::SELECTION_NONE to SelectionType::eNone

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58738/diff/1-2/

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8761631 [details]
Bug 1278014 part.5 Rename SelectionType::SELECTION_NORMAL to SelectionType::eNormal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58740/diff/1-2/

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8761632 [details]
Bug 1278014 part.6 Rename SelectionType::SELECTION_SPELLCHECK to SelectionType::eSpellCheck

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58742/diff/1-2/

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8761633 [details]
Bug 1278014 part.7 Rename SelectionType::SELECTION_IME_RAWINPUT to SelectionType::eIMERawClause

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58744/diff/1-2/

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8761634 [details]
Bug 1278014 part.8 Rename SelectionType::SELECTION_IME_SELECTEDRAWTEXT to SelectionType::eIMESelectedRawClause

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58746/diff/1-2/

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8761635 [details]
Bug 1278014 part.9 Rename SelectionType::SELECTION_IME_CONVERTEDTEXT to SelectionType::eIMEConvertedClause

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58748/diff/1-2/

Comment 60

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8761636 [details]
Bug 1278014 part.10 Rename SelectionType::SELECTION_IME_SELECTEDCONVERTEDTEXT to SelectionType::eIMESelectedClause

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58750/diff/1-2/

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8761637 [details]
Bug 1278014 part.11 Rename SelectionType::SELECTION_ACCESSIBILITY to SelectionType::eAccessibility

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58752/diff/1-2/

Comment 62

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8761638 [details]
Bug 1278014 part.12 Rename SelectionType::SELECTION_FIND to SelectionType::eFind

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58754/diff/1-2/

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8761639 [details]
Bug 1278014 part.13 Rename SelectionType::SELECTION_URLSECONDARY to SelectionType::eURLSecondary

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58756/diff/1-2/

Comment 64

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8761640 [details]
Bug 1278014 part.14 Rename SelectionType::SELECTION_URLSTRIKEOUT to SelectionType::eURLStrikeout

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58758/diff/1-2/

Comment 65

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://reviewboard.mozilla.org/r/58734/#review55876

> So I would make this method to check whether the raw type is valid, and if not, return some
> SelectionType::SELECTION_INVALID_TYPE

SelectionType::eInvalid is created (-1).

Comment 66

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8761628 [details]
Bug 1278014 part.2 Define mozilla::SelectionType as an enum class and use it instead of RawSelectionType as far as possible

https://reviewboard.mozilla.org/r/58734/#review56094

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you for your quick review! If you have no objections about above my answers, I'll land them tomorrow morning. Anyway, I'd like to put off some changes to another bug since another bug fix is in my queue already and it depends on these patches.

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc4b075bd57b
part.1 Rename SelectionType in nsISelectionController.idl to mozilla::RawSelectionType r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b64f33cd6b54
part.2 Define mozilla::SelectionType as an enum class and use it instead of RawSelectionType as far as possible r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f3b2e389408
part.3 Define 2 constants for number of SelectionType and number of SelectionType except "none" r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9c5aa89d70
part.4 Rename SelectionType::SELECTION_NONE to SelectionType::eNone r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a29531874b5
part.5 Rename SelectionType::SELECTION_NORMAL to SelectionType::eNormal r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8afbf2c85ffd
part.6 Rename SelectionType::SELECTION_SPELLCHECK to SelectionType::eSpellCheck r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d220f0bab515
part.7 Rename SelectionType::SELECTION_IME_RAWINPUT to SelectionType::eIMERawClause r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/02b6522d9795
part.8 Rename SelectionType::SELECTION_IME_SELECTEDRAWTEXT to SelectionType::eIMESelectedRawClause r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/58847e4645ad
part.9 Rename SelectionType::SELECTION_IME_CONVERTEDTEXT to SelectionType::eIMEConvertedClause r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e5f5c83523f
part.10 Rename SelectionType::SELECTION_IME_SELECTEDCONVERTEDTEXT to SelectionType::eIMESelectedClause r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/601d00411494
part.11 Rename SelectionType::SELECTION_ACCESSIBILITY to SelectionType::eAccessibility r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/524e2f692ac2
part.12 Rename SelectionType::SELECTION_FIND to SelectionType::eFind r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b68f6eea63
part.13 Rename SelectionType::SELECTION_URLSECONDARY to SelectionType::eURLSecondary r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a0742ffefdc
part.14 Rename SelectionType::SELECTION_URLSTRIKEOUT to SelectionType::eURLStrikeout r=smaug

Comment 69

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc4b075bd57b
https://hg.mozilla.org/mozilla-central/rev/b64f33cd6b54
https://hg.mozilla.org/mozilla-central/rev/5f3b2e389408
https://hg.mozilla.org/mozilla-central/rev/0e9c5aa89d70
https://hg.mozilla.org/mozilla-central/rev/5a29531874b5
https://hg.mozilla.org/mozilla-central/rev/8afbf2c85ffd
https://hg.mozilla.org/mozilla-central/rev/d220f0bab515
https://hg.mozilla.org/mozilla-central/rev/02b6522d9795
https://hg.mozilla.org/mozilla-central/rev/58847e4645ad
https://hg.mozilla.org/mozilla-central/rev/6e5f5c83523f
https://hg.mozilla.org/mozilla-central/rev/601d00411494
https://hg.mozilla.org/mozilla-central/rev/524e2f692ac2
https://hg.mozilla.org/mozilla-central/rev/a8b68f6eea63
https://hg.mozilla.org/mozilla-central/rev/6a0742ffefdc