Bug 54620 – SelectionController.cpp could be simplified with ternary ifs and boolean assignment

• Status: RESOLVED
• Resolution: WONTFIX
• Component: HTML Editing

[Benjamin (Ben) Kalman]

There is a lot of code in SelectionController.cpp which looks like

if (cond)
    pos = foo();
else
    pos = bar();

e.g. (picking random example from the file, line ~550)

if (isEditablePosition(pos.deepEquivalent()))
    pos = endOfEditableContent(pos);
else
    pos = endOfDocument(pos);

This would be more cleanly expressed with ternary notation, in my opinion.  Ternary notation is appropriate when the semantics are functional, which in these cases they are; "assign a value to pos, the value being dependent on whether the position is editable or not".

pos = isEditablePosition(pos.deepEquivalent()) ? endOfEditableContent(pos) : endOfDocument(pos);

Same sort of thing with RTL/LTR assignment decisions, the other common case for ternary ifs.

---

Likewise, there are quite a few examples of

if (cond)
    pos = true;
else
    pos = false;

which obviously is equivalent to

pos = cond;

e.g. (another random position in file, this time line ~350)

if (directionOfEnclosingBlock() == LTR)
    baseIsStart = false;
else
    baseIsStart = true;

This is less of a controversial issue than the ternary thing.

---

I'm filing this bug without a patch because I don't want to go through the effort of "cleaning this up" if the general consensus is that it's actually not a cleanup.  In my opinion, obviously, it is (where appropriate of course).  I'm happy to make the changes, it will probably be a 50-200 line patch (have only scrolled quickly through it).

[Benjamin (Ben) Kalman]

So I guess this is more a question of webkit style.

[Alexey Proskuryakov]

My 2c: the latter looks like a definitive improvement. For ternary operator, it's a case by case call. With your example at line 550, I'd probably say that it's better as is.

[Ojan Vafai]

I'm pro using ternaries where the end result is actually more readable. The first example on line 550 is maybe not as readable, but I'd be fine with it either way.

Certainly the cases of the below should be fixed though:
if (cond)
    pos = true;
else
    pos = false;

That's just silly. :)

[Benjamin (Ben) Kalman]

It's definitely a case-by case basis.  Possibly the case I mentioned isn't the most compelling, but (and this is what prompted me to file this bug actually)

if (directionOfEnclosingBlock() == LTR)
    pos = previousWordPosition(pos);
else
    pos = nextWordPosition(pos);

makes sense to be ternary.  There are lots of examples of this.

pos = directionOfEnclosingBlock() == LTR ? previousWordPosition(pos) : nextWordPosition(pos)

and even more concise (just for fun -- though I have seen examples of this in existing code -- and it *is* stylistically more functional):

pos = (directionOfEnclosingBlock() == LTR ? previousWordPosition : nextWordPosition)(pos);

[Alexey Proskuryakov]

> pos = directionOfEnclosingBlock() == LTR ? previousWordPosition(pos) : nextWordPosition(pos)

FWIW, I like to add braces around the condition, to make it look more like a condition.

[Parag Radke]

Created attachment 142690 [details]
Proposed patch

Code clean up. Replacing if/else with ternary ifs and boolean assignment.

[WebKit Review Bot]

Attachment 142690 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Parag Radke]

Created attachment 142693 [details]
Updated patch

Code clean up. Replacing if/else with ternary ifs and boolean assignment.

[Rafael Brandao]

Comment on attachment 142693 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142693&action=review

Informal review.

> Source/WebCore/editing/FrameSelection.cpp:498
> +            baseIsStart = (directionOfSelection() == LTR) ? true : false;

In this particular case, why you just don't do: baseIsStart = (directionOfSelection() == LTR)?

> Source/WebCore/editing/FrameSelection.cpp:504
> +            baseIsStart = (directionOfSelection() == LTR) ? false : true;

In a similar way, I believe you could just compare here.

> Source/WebCore/editing/FrameSelection.cpp:770
> +                VisiblePosition(m_selection.end(), m_selection.affinity());

Not sure which is the right way to ident, but here you use 4 spaces...

> Source/WebCore/editing/FrameSelection.cpp:803
> +                VisiblePosition(m_selection.extent(), m_selection.affinity()).previous(CannotCrossEditingBoundary);

and here you use 8. One of them is wrong.

> Source/WebCore/editing/FrameSelection.cpp:1067
> +    m_selection.isBaseFirst() ? setBase(pos, trigger) : setExtent(pos, trigger);

Now this looks odd. I don't think it increases readability. I think the problem here is that we don't have an assignment like the previous changes.

> Source/WebCore/editing/FrameSelection.cpp:1072
> +    m_selection.isBaseFirst() ? setExtent(pos, trigger) : setBase(pos, trigger);

Same here.

> Source/WebCore/editing/FrameSelection.cpp:1598
> +    enable ? enableSecureTextInput() : disableSecureTextInput();

Same here.

[Ryosuke Niwa]

Comment on attachment 142693 [details]
Updated patch

I don't think this is necessarily an improvement.

[Ryosuke Niwa]

This comes up every now and then, but it's just a matter of preferences as far as I'm concerned, and it's better to maintain the svn history at this point.