Change caret to show language direction

Change caret to show language direction

BUG=4572

[h.joshi, 2014-05-08 09:45:57]

PTAL

[Peter Kasting, 2014-05-08 17:44:03]

I'm not a good reviewer, so removing myself.

In general, your change has style problems.

(1) Make your comments complete sentences with capitalization and punctuation. 
Ideally, they're good, readable English as well -- if English is not a strong
language for you, try and find someone to help clean up.
(2) Check your indenting (some lines weren't indented).
(3) No {} on conditionals when all arms are one line.

[eseidel, 2014-05-10 09:55:12]

Do any browsers still have this feature?

[h.joshi, 2014-05-11 14:20:31]

Firefox supports this with "bidi.browser.ui" flag, by defaults is OFF. Its
showing the same way as mentioned in the Bug.
Pls suggest, should we have this kind of behavior inside some setting similar to
Firefox. 

On 2014/05/10 09:55:12, eseidel wrote:
> Do any browsers still have this feature?

[progame, 2014-05-12 13:33:11]

On 2014/05/11 14:20:31, h.joshi wrote:
> Firefox supports this with "bidi.browser.ui" flag, by defaults is OFF. Its
> showing the same way as mentioned in the Bug.
> Pls suggest, should we have this kind of behavior inside some setting similar
to
> Firefox. 
> 
> On 2014/05/10 09:55:12, eseidel wrote:
> > Do any browsers still have this feature?

IE has it.
Eric, How about turn it on in the same condition where we enable Ctrl+Shift
combination to enable RTL<>LTR changes?

[jeremy, 2014-05-13 09:21:06]

On 2014/05/12 13:33:11, progame wrote:
> On 2014/05/11 14:20:31, h.joshi wrote:
> > Firefox supports this with "bidi.browser.ui" flag, by defaults is OFF. Its
> > showing the same way as mentioned in the Bug.
> > Pls suggest, should we have this kind of behavior inside some setting
similar
> to
> > Firefox. 
> > 
> > On 2014/05/10 09:55:12, eseidel wrote:
> > > Do any browsers still have this feature?
> 
> IE has it.
> Eric, How about turn it on in the same condition where we enable Ctrl+Shift
> combination to enable RTL<>LTR changes?

It's worth noting that this behaves differently on Mac (System
Preferences->Keyboard->[Hebrew Keyboard layout]->Use split cursor) and we
probably want to match the platform conventions.
Apart from Peter's remarks about English in comments, this also needs tests and
a screenshot of how this looks would also be helpful.

[h.joshi, 2014-05-16 06:35:02]

We have make this under some flag same as Firefox has done, Erik also suggested
the same in bug.

[h.joshi, 2014-05-16 06:41:52]

Sorry for the typo,
We can make this under some flag same as Firefox has done, Erik also suggested
the same in bug.

[Inactive, 2014-05-22 14:15:04]

This is missing a layout test.

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cpp
File Source/core/editing/Caret.cpp (right):

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:193: // as extra rect are draw so need to refresh
more area
Comments should start with a capital letter and end with a '.'.

also "are draw" -> "are drawn"

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:194: TextDirection containerDirection =
toRenderBoxModelObject(caretRenderer(node))->style()->direction();
You likely want to use caretPainter variable here.

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:233: // as extra rect are draw so need to refresh
more area
Ditto.

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:235: TextDirection containerDirection =
toRenderBoxModelObject(caretRenderer(node))->style()->direction();
Code duplication, you should probably move the code to a utility function to
avoid this.

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:260: LayoutRect ptrCaret = caret;
please no not use abbreviations in variable names. Also ptr is a bit confusing 
as it is not technically a pointer.

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:263: if (containerDirection == LTR) {
No {} for one-liners.

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:264: ptrCaret.setX(ptrCaret.x() + 1);
identation problem

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:265: } else {
ditto

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:266: ptrCaret.setX(ptrCaret.x() - 2);
ditto.

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:285: // Update ptrCaret points and lets draw one
more small rect to match IE
end with a '.'

[h.joshi, 2014-05-23 13:26:31]

Chris, new patch added with comment fix. PTAL

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cpp
File Source/core/editing/Caret.cpp (right):

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:193: // as extra rect are draw so need to refresh
more area
Okey, will fix this

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:194: TextDirection containerDirection =
toRenderBoxModelObject(caretRenderer(node))->style()->direction();
Okey, will fix this

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:235: TextDirection containerDirection =
toRenderBoxModelObject(caretRenderer(node))->style()->direction();
Okey, will make new utility method

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:263: if (containerDirection == LTR) {
Okey, will update condition

https://codereview.chromium.org/275563003/diff/1/Source/core/editing/Caret.cp...
Source/core/editing/Caret.cpp:285: // Update ptrCaret points and lets draw one
more small rect to match IE
Okey, will update comment

[Inactive, 2014-05-23 19:57:00]

A couple more comments. I am not familiar with this area of the code so someone
else will need to take over the review.

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
File Source/core/editing/Caret.cpp (right):

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:182: void CaretBase::updateDirectionPointer(const
RenderObject* caretPainter , LayoutRect& rect)
nit: extra space before comma

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:185: TextDirection containerDirection =
toRenderBoxModelObject(caretPainter)->style()->direction();
Would be nicer if this function took  a RenderBoxModelObject then and if the
toRenderBoxModelObject() call were in the callers. This looks unsafe now.

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:202:
CaretBase::updateDirectionPointer(caretPainter, inflatedRect);
nit: "CaretBase::" should not be needed.

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:238:
CaretBase::updateDirectionPointer(caretRenderer(node), drawingRect);
"CaretBase::" should not be needed.

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:258: TextDirection containerDirection =
toRenderBoxModelObject(renderer)->style()->direction();
It is not clear to me why toRenderBoxModelObject() is safe here but it may be
right, I am not familiar with this code. I would just like to bring this up.
Above it does a isBox() check before casting to RenderBox.

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Caret.h
File Source/core/editing/Caret.h (right):

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
Source/core/editing/Caret.h:67: static void updateDirectionPointer(const
RenderObject* caretPainter , LayoutRect&);
nit: Extra space before comma

[h.joshi, 2014-05-24 02:18:20]

Will make suggested changes and upload new patch.

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
File Source/core/editing/Caret.cpp (right):

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:182: void CaretBase::updateDirectionPointer(const
RenderObject* caretPainter , LayoutRect& rect)
Will remove extra space.

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:185: TextDirection containerDirection =
toRenderBoxModelObject(caretPainter)->style()->direction();
Okey, will make required changes. I have a question, why its unsafe?

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:202:
CaretBase::updateDirectionPointer(caretPainter, inflatedRect);
Added for clarity, will remove.

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:238:
CaretBase::updateDirectionPointer(caretRenderer(node), drawingRect);
ditto

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:258: TextDirection containerDirection =
toRenderBoxModelObject(renderer)->style()->direction();
I am also new to this code so need to check, once all shared comments are fixed,
will start looking into this.

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Caret.h
File Source/core/editing/Caret.h (right):

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
Source/core/editing/Caret.h:67: static void updateDirectionPointer(const
RenderObject* caretPainter , LayoutRect&);
Okey, will remove extra space.

[Inactive, 2014-05-24 02:22:30]

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
File Source/core/editing/Caret.cpp (right):

https://codereview.chromium.org/275563003/diff/20001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:185: TextDirection containerDirection =
toRenderBoxModelObject(caretPainter)->style()->direction();
On 2014/05/24 02:18:20, h.joshi wrote:
> Okey, will make required changes. I have a question, why its unsafe?

It looks unsafe because the method takes a RenderObject* and you are casting it
to a RenderBoxModelObject* without checking its type. Therefore, it will crash
if the caller passes something else than a RenderBoxModelObject renderer to your
method.

If you let the caller of this method do the casting (if necessary), it will be
more obvious in that context if it is safe to cast to a RenderBoxModelObject* or
not.

[h.joshi, 2014-05-26 06:37:04]

@Chris: New patch with suggested changes, pls review

[Inactive, 2014-05-27 13:10:44]

I believe you'll need to add your new tests to TestExpectations so that baseline
are generated for all platforms (you only generated the Linux ones). Should look
like so:
crbug.com/4572 editing/caret/caret-pointer-direction-LTR.html [ NeedsRebaseline
]
crbug.com/4572 editing/caret/caret-pointer-direction-RTL.html [ NeedsRebaseline
]

https://codereview.chromium.org/275563003/diff/40001/Source/core/editing/Care...
File Source/core/editing/Caret.cpp (right):

https://codereview.chromium.org/275563003/diff/40001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:185: TextDirection containerDirection =
renderBoxModelObject->style()->direction();
Ok, so I looked this up a bit more. Why do you need a RenderBoxModelObject here?
style() is on RenderObject so taking a RenderObject* in argument should be
sufficient.

https://codereview.chromium.org/275563003/diff/40001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:202:
updateDirectionPointer(toRenderBoxModelObject(caretPainter), inflatedRect);
This cast should not be needed. Also, I am still not convinced this was safe as
other places call isBox() before casting to RenderBox*.

https://codereview.chromium.org/275563003/diff/40001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:238:
updateDirectionPointer(toRenderBoxModelObject(caretRenderer(node)),
drawingRect);
Ditto. Cast should not be needed.
Also, what if caredRenderer() returns null? other places have null checks but
you don't have one.

https://codereview.chromium.org/275563003/diff/40001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:258: TextDirection containerDirection =
toRenderBoxModelObject(renderer)->style()->direction();
Cast should not be needed.

https://codereview.chromium.org/275563003/diff/40001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:261: // Drawing different Caret which shows
editing style similar to IE
nit: needs to end with a '.'

[h.joshi, 2014-05-28 03:02:53]

Will make new patch with suggested changes.
(One WIP patch is shared which is not for review, need to fix my workspace)

https://codereview.chromium.org/275563003/diff/40001/Source/core/editing/Care...
File Source/core/editing/Caret.cpp (right):

https://codereview.chromium.org/275563003/diff/40001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:185: TextDirection containerDirection =
renderBoxModelObject->style()->direction();
Yes, RenderObject will be enough here to get style(), checking the same and will
update patch.

https://codereview.chromium.org/275563003/diff/40001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:202:
updateDirectionPointer(toRenderBoxModelObject(caretPainter), inflatedRect);
Yes, will remove after changing method usage.

https://codereview.chromium.org/275563003/diff/40001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:238:
updateDirectionPointer(toRenderBoxModelObject(caretRenderer(node)),
drawingRect);
Ditto

https://codereview.chromium.org/275563003/diff/40001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:258: TextDirection containerDirection =
toRenderBoxModelObject(renderer)->style()->direction();
Yes, will make changes

https://codereview.chromium.org/275563003/diff/40001/Source/core/editing/Care...
Source/core/editing/Caret.cpp:261: // Drawing different Caret which shows
editing style similar to IE
Will make changes.