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

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 affected, firefox50 fixed)

Details

Attachments

(14 attachments)

58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
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?
Flags: needinfo?(bugs)
> Should eFind be eFindResult or eFound?

If it should be eFound, then, is eWrongWord better than eSpellCheck?
I would assume the current constants are used by quite a few addons. 
So think we shouldn't change nsISelectionController.idl
Flags: needinfo?(bugs)
(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).
Flags: needinfo?(bugs)
I see. Yeah, if that makes the code elsewhere simpler or easier to maintain, then sounds good.
Flags: needinfo?(bugs)
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?
Oops, please check the previous comment.
Flags: needinfo?(bugs)
I'd use eAccessibility and eFind and eSpellCheck... so trying to map the enum names to constants as much as possible.
Flags: needinfo?(bugs)
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/
Attachment #8761627 - Flags: review?(bugs)
Attachment #8761628 - Flags: review?(bugs)
Attachment #8761629 - Flags: review?(bugs)
Attachment #8761630 - Flags: review?(bugs)
Attachment #8761631 - Flags: review?(bugs)
Attachment #8761632 - Flags: review?(bugs)
Attachment #8761633 - Flags: review?(bugs)
Attachment #8761634 - Flags: review?(bugs)
Attachment #8761635 - Flags: review?(bugs)
Attachment #8761636 - Flags: review?(bugs)
Attachment #8761637 - Flags: review?(bugs)
Attachment #8761638 - Flags: review?(bugs)
Attachment #8761639 - Flags: review?(bugs)
Attachment #8761640 - Flags: review?(bugs)
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/
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/
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 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.
Attachment #8761627 - Flags: review?(bugs) → review+
Attachment #8761628 - Flags: review?(bugs) → review-
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 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?
Attachment #8761629 - Flags: review?(bugs) → review+
Attachment #8761630 - Flags: review?(bugs) → review+
Comment on attachment 8761630 [details]
Bug 1278014 part.4 Rename SelectionType::SELECTION_NONE to SelectionType::eNone

https://reviewboard.mozilla.org/r/58738/#review55978
Attachment #8761631 - Flags: review?(bugs) → review+
Comment on attachment 8761631 [details]
Bug 1278014 part.5 Rename SelectionType::SELECTION_NORMAL to SelectionType::eNormal

https://reviewboard.mozilla.org/r/58740/#review55980
Comment on attachment 8761632 [details]
Bug 1278014 part.6 Rename SelectionType::SELECTION_SPELLCHECK to SelectionType::eSpellCheck

https://reviewboard.mozilla.org/r/58742/#review55984
Attachment #8761632 - Flags: review?(bugs) → review+
Attachment #8761633 - Flags: review?(bugs) → review+
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 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.
Attachment #8761634 - Flags: review?(bugs) → review+
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
Attachment #8761635 - Flags: review?(bugs) → review+
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
Attachment #8761636 - Flags: review?(bugs) → review+
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.
Attachment #8761637 - Flags: review?(bugs)
Attachment #8761637 - Flags: review?(bugs)
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.
Attachment #8761637 - Flags: review?(bugs) → review+
Comment on attachment 8761637 [details]
Bug 1278014 part.11 Rename SelectionType::SELECTION_ACCESSIBILITY to SelectionType::eAccessibility

https://reviewboard.mozilla.org/r/58752/#review56006
Comment on attachment 8761638 [details]
Bug 1278014 part.12 Rename SelectionType::SELECTION_FIND to SelectionType::eFind

https://reviewboard.mozilla.org/r/58754/#review56008
Attachment #8761638 - Flags: review?(bugs) → review+
Comment on attachment 8761639 [details]
Bug 1278014 part.13 Rename SelectionType::SELECTION_URLSECONDARY to SelectionType::eURLSecondary

https://reviewboard.mozilla.org/r/58756/#review56010
Attachment #8761639 - Flags: review?(bugs) → review+
Comment on attachment 8761640 [details]
Bug 1278014 part.14 Rename SelectionType::SELECTION_URLSTRIKEOUT to SelectionType::eURLStrikeout

https://reviewboard.mozilla.org/r/58758/#review56012
Attachment #8761640 - Flags: review?(bugs) → review+
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?
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.
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.
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.
https://reviewboard.mozilla.org/r/58750/#review55996

> similar concern also here. But eIMESelectedClause is fine too

So, I'll keep using the new names.
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 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/
Attachment #8761628 - Flags: review- → review?(bugs)
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 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 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 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 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 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 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 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 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 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 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 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 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/
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).
Attachment #8761628 - Flags: review?(bugs) → review+
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
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.
Pushed by masayuki@d-toybox.com:
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
Depends on: 1280181
No longer depends on: 1280181
You need to log in before you can comment on or make changes to this bug.