To support text selection mode in FxOS, we have to add a selection state machine in framescript 1. Enter selection mode by long press event. 2. Leave selection mode by a move down event out of selection area 3. Change selection range by mouse move event.

To ensure text selection feature matching 1.4 milestone, I separate bug 924692 into two bugs. This one need to be done before we can say text selection complete. Label as 1.4?

To clarify the implementation, can I say the bug 924692 is for cursor movement and this one is for text selection? We've treated the bug 924692 as the implementation work for cursor movement. Since cursor movement is the committed feature, the bug 924692 is 1.4+. For the text selection, it is the targeted feature so that this one will not be the blocker of v1.4. But we definitely have to keep working on this implementation for the text selection feature in v1.4. I mark this bug as "---" and link it to user story bug 921965

Per discussion with CJ, this bug has to be done for the cursor to be correctly shown up after the selection movement. Thus, plus this bug for v1.4. Also change dependency to block cursor movement user story bug 921964.

https://bugzilla.mozilla.org/show_bug.cgi?id=921964 has a frame script that already listens for this.

Rename to Touch Caret. In this bug, we should only focus on Touch Caret(single touch indicator) for 1.4 committed feature. Text Selection(double touch indicators) should be done for later milestone

(In reply to Jan Jongboom [:janjongboom] from comment #4) > https://bugzilla.mozilla.org/show_bug.cgi?id=921964 has a frame script that > already listens for this. Jan, Thank you for the help on this. I think Vincent will base on your iframe script as the reference to implement this. Please help him clarify questions if there is any.

WIP It depends on the WIP in Bug 924692. The first idea is to implement a HandleTouchCaret() member function in nsFrame. But the TouchCaret frame implemented in Bug 924692 seems not able to receive event from nsPresShell. We have to check some conditions when any frame's instance got a event. If TouchCaret is visible and touch point is in the region of TouchCaret's region, I gonna get caret's frame and manipulate its FrameSelection with touch event's information.

Comment on attachment 8367159 [details] [diff] [review] bug955987.patch Review of attachment 8367159 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall, although it's hard to understand this patch completely without bug 924692. ::: layout/generic/nsFrame.cpp @@ +89,4 @@ > #include "mozilla/css/ImageLoader.h" > #include "mozilla/gfx/Tools.h" > > +#include "nsTouchCaret.h" Nit: please move this one line up to put it next to the other #include statements. @@ +2969,5 @@ > } > > +NS_IMETHODIMP nsFrame::HandleTouchCaret(nsPresContext* aPresContext, > + WidgetGUIEvent* aEvent, > + nsEventStatus* aEventStatus) Nit: indentation and trailing spaces here and elsewhere. @@ +2972,5 @@ > + WidgetGUIEvent* aEvent, > + nsEventStatus* aEventStatus) > +{ > + nsIPresShell *presShell = aPresContext->PresShell(); > + nsRefPtr<nsCaret> caret = presShell->GetCaret(); So it seems like the caret variable is not used here, and I'd like to see a patch to bug 924692 first but tying the touch caret to nsCaret is a mistake, since we want to use most of the code for the touch caret in the future for non-collapsed selection as well.

The last mile of this bug is about to update Selection of caret. Bad news is the current implementation of nsFrame doesn't handle "touchmove" event. In Fennec while receiving "touchmove" event, SelectionHandler.js will call sendMouseEvent() DOM API to dispatch "mousedown" and "mouseup" events which can be handled by nsFrame. I'm gonna try to do the same way in TabChild.

WIP. (Must work with the WIP of Bug 924692) I think this WIP is quite close to the final solution. The concept is quite similar to FrameScript designed for Fennec, but the WIP I implemented here is in TabChild the native code. Before make it being reviewed, I would like to have more test with the WIP of Bug 924692, discussing with UX team and look forward to your feedback. As TabChild received a "touchmove" in content process, basically it's gonna to 1. Get the pressed point of the touch event. 2. Check if pressed point is within the region of touchCaret. 3. if touchCaret is hit, both "mousedown" and "mouseup" events will be dispatched to the position of caret[1]. Along the way, we make a boundary about where the events are dispatched to with reference to text range. Dragging mode flag(mTouchCaretDragging) makes a binary switchable state machine to force touchCaret draggable. [1] The current design of nsFrame doesn't handle "touchmove" event. Instead of implement a handler for it, I would prefer this way which is simpler and just like FrameScript.

Comment on attachment 8371254 [details] [diff] [review] bug955987.patch Review of attachment 8371254 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabChild.cpp @@ +4,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +#include "android/log.h" > + Remove this line in final patch @@ +86,5 @@ > > +#define LOG(args...) __android_log_print(ANDROID_LOG_INFO, "TabChild" , ## args) > +#define LOGW(args...) __android_log_print(ANDROID_LOG_WARN, "TabChild", ## args) > +#define LOGE(args...) __android_log_print(ANDROID_LOG_ERROR, "TabChild", ## args) > + Remove these lines in final patch @@ +1937,5 @@ > + nsRect aResult; > + float left, top, width, height; > + nsIntPoint caretPoint; > + nsCOMPtr<nsIDOMNode> mTargetElement; > + int32_t mTargetOffset; Remove m prefix sine those are local variables. @@ +1955,5 @@ > + //Target Node Cache > + caretSelection->GetFocusNode(getter_AddRefs(mTargetElement)); > + if (mTargetElement) > + caretSelection->GetFocusOffset(&mTargetOffset); > + if (!mTargetElement) return RecvRealTouchEvent(aEvent, aGuid); Since mTargetOffset is for debugging purpose, enclose code in #ifdef #ifdef DEBUG caretSelection->GetFocusOffset(&mTargetOffset); #endif @@ +1956,5 @@ > + caretSelection->GetFocusNode(getter_AddRefs(mTargetElement)); > + if (mTargetElement) > + caretSelection->GetFocusOffset(&mTargetOffset); > + > + if (mTouchCaretVisible) { 1. Touch Caret hit test should move to RecvRealTouchDownEvent, instead of keep do TouchCaret hit test in TouchMove event callback. 2. if (mTouchCaretDragging) @@ +1966,5 @@ > + if (!touch) { > + continue; > + } > + // The point being pressed by user. > + adjustedPoint = pressedPoint = LayoutDevicePoint(touch->mRefPoint.x, touch->mRefPoint.y); pressedPoint = LayoutDevicePoint(touch->mRefPoint.x, touch->mRefPoint.y); Declare adjustedPointer in #1975 scope. @@ +1969,5 @@ > + // The point being pressed by user. > + adjustedPoint = pressedPoint = LayoutDevicePoint(touch->mRefPoint.x, touch->mRefPoint.y); > + } > + > + // adjust the position where mouse events go to. // Depend on geometry of input element and text range, adjust the position of mouse event dispatching. @@ +1971,5 @@ > + } > + > + // adjust the position where mouse events go to. > + nsRefPtr<mozilla::dom::DOMRect> rect; > + { LayoutDevicePoint adjustedPoint = pressedPoint; @@ +1983,5 @@ > + > + rect = textRange->GetBoundingClientRect(); > + rect->SetRect(rect->Left()*scale, rect->Top()*scale, rect->Width()*scale, rect->Height()*scale); > + > + // Clamp vertically // Mouse Event is 1-base, TextRange rect is zero-base @@ +2007,5 @@ > + left = aResult.x; > + top = aResult.y; > + width = aResult.width; > + height = aResult.height; > + Move hit test logic into TouchCaret. Code here should be very simple mTouchCaretDragging = touchCaret->HitTest(pressedPoint); @@ +2017,5 @@ > + DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_UP, time, adjustedPoint); > + } else > + LOG("caret(%d, %d) touchCaret(%f, %f, %f, %f) visible, but pressedPoint(%f, %f) and adjustedPoint(%f, %f) outside the region ! textRange(%f, %f, %f, %f), mTargetOffset(%d), scale(%f)", caretPoint.x, caretPoint.y, left, top, width, height, > + pressedPoint.x, pressedPoint.y, adjustedPoint.x, adjustedPoint.y, rect->Left(), rect->Top(), rect->Width(), rect->Height(), mTargetOffset, mWidget->GetDefaultScale().scale); > + Remove #2019 ~ 2021 in final patch ::: dom/ipc/TabChild.h @@ +500,4 @@ > bool mUpdateHitRegion; > bool mContextMenuHandled; > bool mWaitingTouchListeners; > + bool mTouchCaretVisible; This member should be a local variable.

Comment on attachment 8371254 [details] [diff] [review] bug955987.patch Review of attachment 8371254 [details] [diff] [review]: ----------------------------------------------------------------- Please see the comments below on the architecture of the patch. But I would really really like to see a close to finished patch for bug 924692 before I can provide any valuable feedback here. It seems that this patch is inheriting some architecture mistakes that are coming from bug 924692, so I would prefer if we focus our efforts to finish up the work there first, because otherwise the work in this bug may need to get redone as a result of the changes in bug 924692, which may mean wasted effort for you, and I'd hate for that to happen! :-) ::: dom/ipc/TabChild.cpp @@ +4,3 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > Nit: please generate patches with 8 lines of context. That really helps a lot with reading the patch. @@ +1944,5 @@ > + nsCOMPtr<nsIDocShell> docShell = do_GetInterface(WebNavigation()); > + nsIDocument* doc; > + > + if (nsIPresShell* presShell = docShell->GetPresShell()) { > + touchCaret = presShell->GetTouchCaret(); So, I think that we need to have one touch caret per selection controller (and eventually when we start to handle non-collapsed selections, two touch carets per selection controller, one for each side of a non-collapsed selection.) Usually the pres shell has its own selection controller which is in charge of the main document's selection object. But things such as <input type=text> and <textarea> have a completely independent selection of their own, and they also have their own selection controllers (see <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp#200>). That's why I think tying this logic to the tab child is incorrect. Also, as I mentioned before, I don't think that the caret object should be involved here. Once we finish this work, we don't even want the caret object to show up. nsCaret is responsible for displaying the caret on non-touch platforms, and it doesn't really buy us much to tie this into nsCaret. Doing that means that we would need to redo all of this work when we start supporting touch drag end points for non-collapsed selections. (In addition to that problem, tying things to the TabChild means that we cannot use this code for the cases where we don't have a TabChild, such as the metro browser.) ::: dom/ipc/TabChild.h @@ +500,5 @@ > bool mUpdateHitRegion; > bool mContextMenuHandled; > bool mWaitingTouchListeners; > + bool mTouchCaretVisible; > + bool mTouchCaretDragging; There will be more than one touch caret per tab child potentially. We cannot store these members here.

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailacopolypse) from comment #12) > @@ +1944,5 @@ > > + nsCOMPtr<nsIDocShell> docShell = do_GetInterface(WebNavigation()); > > + nsIDocument* doc; > > + > > + if (nsIPresShell* presShell = docShell->GetPresShell()) { > > + touchCaret = presShell->GetTouchCaret(); > > So, I think that we need to have one touch caret per selection controller > (and eventually when we start to handle non-collapsed selections, two touch > carets per selection controller, one for each side of a non-collapsed > selection.) > > Usually the pres shell has its own selection controller which is in charge > of the main document's selection object. But things such as <input > type=text> and <textarea> have a completely independent selection of their > own, and they also have their own selection controllers (see > <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/ > nsTextEditorState.cpp#200>). That's why I think tying this logic to the tab > child is incorrect. my implementation in bug 924692 is to create the touch caret frame/element per presshell, and make the touch caret a listener to all the selection controllers in that document, including <input type=text> and <textarea>, and will update its position due to selection change. And for selection mode, I would like to create two other dedicated touch selection frame/element to handle that in the future. I do face some problem in this approach such as the touch caret wont scroll with the <textarea> since the touch caret frame isn't a child of <input> though. But I don't understand the must have one touch caret per selection controller logic concern is. Having one touch caret per selection controller sounds reasonable but I do face some implementation difficulty such as where and how to append an anonymous touch caret when each selection controller is instantiate.(especially for presshell case) > Also, as I mentioned before, I don't think that the caret object should be > involved here. Once we finish this work, we don't even want the caret > object to show up. nsCaret is responsible for displaying the caret on > non-touch platforms, and it doesn't really buy us much to tie this into > nsCaret. Doing that means that we would need to redo all of this work when > we start supporting touch drag end points for non-collapsed selections. So do you mean that we should handle all the caret show up in nsCaret by nsTouchCaret instead?

(In reply to Phoebe Chang [:phoebe] from comment #13) > (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, > emailacopolypse) from comment #12) > > So, I think that we need to have one touch caret per selection controller > > (and eventually when we start to handle non-collapsed selections, two touch > > carets per selection controller, one for each side of a non-collapsed > > selection.) > > > > Usually the pres shell has its own selection controller which is in charge > > of the main document's selection object. But things such as <input > > type=text> and <textarea> have a completely independent selection of their > > own, and they also have their own selection controllers (see > > <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/ > > nsTextEditorState.cpp#200>). That's why I think tying this logic to the tab > > child is incorrect. Hi Ehsan, By Phoebe's implementation, one PresSehll carries one TouchCaret. That single TouchCaret will be a listener of _all_ Selection objects, instead of only for the selection of PresShell. I think this design should be right. In patch of bug 924692 layout/base/nsTouchCaret.cpp privateSelection->AddSelectionListener(this); << Add PresShell::TouchCaret as listener of PresShell::Selection content/html/content/src/nsTextEditorState.cpp selPriv->AddSelectionListener(static_cast<nsISelectionListener*>(touchCaret)); << Add PresShell::TouchCaret as a listener of input or textarea's Selection

Feature work is not the blocker for the release. Reset blocking-b2g flag. This is just to change the way we tag feature work bugs. We still follow up our sprint planning for the FC

Update WIP.

I do some re-factory. Please reuse this class in both tabclient(Content process) and appshell(Chrome process)

Comment on attachment 8378152 [details] [diff] [review] touchCaret.patch Review of attachment 8378152 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabChild.h @@ +461,5 @@ > // Get the Document for the top-level window in this tab. > already_AddRefed<nsIDocument> GetDocument(); > > + bool TouchCaretHitTest(const LayoutDeviceIntPoint& pressedPoint, nsRect *dstRect); > + Can be a free function.

(Sorry for the late reply, please use the needinfo? flag for almost guaranteed 24-hour turn around time. :-) (In reply to comment #13) > (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, > emailacopolypse) from comment #12) > > > @@ +1944,5 @@ > > > + nsCOMPtr<nsIDocShell> docShell = do_GetInterface(WebNavigation()); > > > + nsIDocument* doc; > > > + > > > + if (nsIPresShell* presShell = docShell->GetPresShell()) { > > > + touchCaret = presShell->GetTouchCaret(); > > > > So, I think that we need to have one touch caret per selection controller > > (and eventually when we start to handle non-collapsed selections, two touch > > carets per selection controller, one for each side of a non-collapsed > > selection.) > > > > Usually the pres shell has its own selection controller which is in charge > > of the main document's selection object. But things such as <input > > type=text> and <textarea> have a completely independent selection of their > > own, and they also have their own selection controllers (see > > <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/ > > nsTextEditorState.cpp#200>). That's why I think tying this logic to the tab > > child is incorrect. > my implementation in bug 924692 is to create the touch caret frame/element per > presshell, and make the touch caret a listener to all the selection controllers > in that document, including <input type=text> and <textarea>, and will update > its position due to selection change. And for selection mode, I would like to > create two other dedicated touch selection frame/element to handle that in the > future. I do face some problem in this approach such as the touch caret wont > scroll with the <textarea> since the touch caret frame isn't a child of <input> > though. But I don't understand the must have one touch caret per selection > controller logic concern is. Having one touch caret per selection controller > sounds reasonable but I do face some implementation difficulty such as where > and how to append an anonymous touch caret when each selection controller is > instantiate.(especially for presshell case) If that ends up being simpler to implement, I have no problem with that strategy. I just imagined that my proposal would be simpler (but I'm not 100% sure.) > > Also, as I mentioned before, I don't think that the caret object should be > > involved here. Once we finish this work, we don't even want the caret > > object to show up. nsCaret is responsible for displaying the caret on > > non-touch platforms, and it doesn't really buy us much to tie this into > > nsCaret. Doing that means that we would need to redo all of this work when > > we start supporting touch drag end points for non-collapsed selections. > So do you mean that we should handle all the caret show up in nsCaret by > nsTouchCaret instead? No, I meant lets just ignore nsCaret completely, I don't think it provides any functionality which is helpful here. In the end we can disable nsCaret painting if the touch caret pref is set to true.

Update WIP~ Need further test

WIP~ For GestureDetector GestureDetector.cpp GestureDetector.h TabChild.cpp nsWindow.cpp For Key Event ~ nsPresShell.cpp For Timer and GestureDetector nsTouchCaret.cpp nsTouchCaret.h

Comment on attachment 8380448 [details] [diff] [review] TouchCaretWIP0224.patch Review of attachment 8380448 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/GestureDetector.cpp @@ +11,5 @@ > +using namespace mozilla; > + > +GestureDetector::GestureDetector() > + : mDraggingMode(false) > + , mInputBoundary(0,0,0,0) mInputBoundary(0,0,0,0) is not needed Add mWidget(nullptr) @@ +25,5 @@ > + > +nsEventStatus > +GestureDetector::HandleEvent(const WidgetTouchEvent& aTouchEvent, bool aUseAttachedEvents, nsIWidget* aWidget) > +{ > + MOZ_ASSERT(NS_IsMainThread()); @@ +38,5 @@ > + // Handle only down event in none-dragging mode. > + if (aTouchEvent.message == NS_TOUCH_START) { > + HandleTouchDown(aTouchEvent, status); > + if (status == nsEventStatus_eConsumeNoDefault) > + mWidget = aWidget; Whether assign mWidget to aWidget is depend on HitTest result. status code is depend on HitTest result as well. Please use the result of HitTest instead of status @@ +57,5 @@ > +} > + > +nsEventStatus > +GestureDetector::HandleEvent(const WidgetGUIEvent* aEvent, bool aUseAttachedEvents, nsIWidget* aWidget) > +{ MOZ_ASSERT(NS_IsMainThread()); @@ +65,5 @@ > + touchEvent = *(aEvent->AsTouchEvent()); > + else > + return nsEventStatus_eIgnore; > + > + return HandleEvent(touchEvent, aUseAttachedEvents, aWidget); if (aEvent->eventStructType == NS_TOUCH_EVENT) { return HandleEvent(WidgetTouchEvent(*(aEvent->AsTouchEvent())), aUseAttachedEvents, aWidget); } else { return nsEventStatus_eIgnore; } @@ +73,5 @@ > +nsEventStatus > +GestureDetector::DispatchWidgetEvent(WidgetGUIEvent& event) > +{ > + NS_ENSURE_TRUE(mWidget, nsEventStatus_eConsumeNoDefault); > + Should be an assertion instead. GestureDetector::DispatchWidgetEvent should be called with mWidget is valid. MOZ_ASSERT(mWidget); @@ +149,5 @@ > +} > + > +void > +GestureDetector::HandleTouchUp(const WidgetTouchEvent& aEvent, nsEventStatus &aStatus) > +{ mDraggingMode should be true unless we have logic error in HandleEvent MOZ_ASSERT(mDraggingMode); @@ +152,5 @@ > +GestureDetector::HandleTouchUp(const WidgetTouchEvent& aEvent, nsEventStatus &aStatus) > +{ > + nsRefPtr<nsTouchCaret> touchCaret = PresShell::GetActiveTouchCaret(); > + NS_ENSURE_TRUE_VOID(touchCaret); > + What's aStatus value if return at #155 @@ +170,5 @@ > + > + bool visible; > + touchCaret->GetTouchCaretVisible(&visible); > + NS_ENSURE_TRUE_VOID(visible); > + Please comment why you need to do this(#171~#173). Don't use NS_ENSURE_TRUE_VOID since you will dump many warning log in debug build, which is not you intend to do @@ +174,5 @@ > + > + dom::Touch* touch = aEvent.touches[0]; > + // In fact, I think we should have an assertion here since it's abnormal. > + NS_ENSURE_TRUE_VOID(touch); > + You are right, just do it

Vincent, this patch is kind of dirty, please remove useless code and rebase base on Phoebe's patch

Comment on attachment 8380448 [details] [diff] [review] TouchCaretWIP0224.patch Review of attachment 8380448 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/nsWindow.cpp @@ +273,5 @@ > nsEventStatus status; > + nsRefPtr<nsTouchCaret> touchCaret = PresShell::GetActiveTouchCaret(); > + GestureDetector* gd; > + if (touchCaret) > + gd = touchCaret->getGestureDetector(); touchCaret->getGestureDetector() should never return nullptr, make an assertion in getGestureDetector()

Comment on attachment 8380448 [details] [diff] [review] TouchCaretWIP0224.patch Review of attachment 8380448 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/GestureDetector.cpp @@ +14,5 @@ > + : mDraggingMode(false) > + , mInputBoundary(0,0,0,0) > +{ > + > +} Remove all PresShell::GetActiveTouchCaret() code in this unit ::: layout/base/nsTouchCaret.h @@ +19,5 @@ > +{ > +public: > + nsTouchCaret(nsIPresShell *aPresShell); > + virtual ~nsTouchCaret(); > + void Init() { mGestureDetect.Init(this); } void GestureDetect::Init(nsTouchCaret *aHost) { mHost = aHost; } Use mHost in GestureDetector instead of keep call PresShell::GetActiveTouchCaret in GestureDetector @@ +36,5 @@ > + void LaunchExpirationTimer(); > + void CancelExpirationTimer(); > + > + GestureDetector* getGestureDetector() {return mGestureDetector;}; > + GestureDetector& GetGestureDetector() const {return mGestureDetector; } @@ +52,5 @@ > + nsCOMPtr<nsIDOMNode> mFocusNode; > + nsIFrame* mFocusFrame; > + nsCOMPtr<nsITimer> mTouchCaretExpirationTimer; > + bool mTouchCaretTimerIsActive; > + GestureDetector* mGestureDetector; GestureDetector mGestureDetector;

Comment on attachment 8380448 [details] [diff] [review] TouchCaretWIP0224.patch Review of attachment 8380448 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/nsWindow.cpp @@ +274,5 @@ > + nsRefPtr<nsTouchCaret> touchCaret = PresShell::GetActiveTouchCaret(); > + GestureDetector* gd; > + if (touchCaret) > + gd = touchCaret->getGestureDetector(); > + if (gd) Remove getGestureDetector() Add nsTouchCaret::HandleEvent function, which wrap nsGestureDetector::HandleEvent. Hide nsGestureDetector in nsTouchCaret

Vincent/ Phoebe, 1. Most GestureDetector class definition and member function definition into nsTouchCaret.cpp 2. declare GestureDetector *mDetector in nsTouchCaret 3. Add nsTouchCaret::HandleEvent which call GectureDetector::HandleEvent 4. nsTouchCaret hosts GestureDetector, 1-to-1 relation. 5. Only nsTouchCaret knows GestureDetect. Put all things back to bug 924692 and close this one

(In reply to C.J. Ku[:CJKu] from comment #27) > Vincent/ Phoebe, > 1. Most GestureDetector class definition and member function definition into > nsTouchCaret.cpp typo: move > 2. declare GestureDetector *mDetector in nsTouchCaret > 3. Add nsTouchCaret::HandleEvent which call GectureDetector::HandleEvent > 4. nsTouchCaret hosts GestureDetector, 1-to-1 relation. > 5. Only nsTouchCaret knows GestureDetect. > > Put all things back to bug 924692 and close this one

GestureDetector(.cpp) was merged to nsTouchCaret(.cpp). After feedback+, as CJ's comment, I will dup this Bug to Bug 924692 and create a new Bug for the caller(nsWindow, TabChild). Let's make Bug 924692 only for the feature to be reviewed.

Comment on attachment 8381169 [details] [diff] [review] TouchCaretWIP0225.patch Review of attachment 8381169 [details] [diff] [review]: ----------------------------------------------------------------- feedback+ after you move DestureDetect class definition into cpp ::: layout/base/nsTouchCaret.h @@ +30,5 @@ > + static void SetActiveTouchCaret(nsTouchCaret* aTouchCaret) { sActiveTouchCaret = aTouchCaret; }; > + static nsTouchCaret* GetActiveTouchCaret() { return sActiveTouchCaret; }; > + > + nsEventStatus HandleEvent(const mozilla::WidgetTouchEvent& aTouchEvent, nsIWidget* aWidget); > + nsEventStatus HandleEvent(const mozilla::WidgetGUIEvent* aEvent, nsIWidget* aWidget); Only several public functions need. SetActiveTouchCaret GetActiveTouchCaret HandleEvent GetTouchCaretVisible SetTouchCaretVisible Declare all the others in private section and declare GestureDetect as friend class @@ +60,5 @@ > + bool mTouchCaretTimerIsActive; > + static nsTouchCaret* sActiveTouchCaret; > +}; > + > +class GestureDetector Move class definition into cpp

Comment on attachment 8381272 [details] [diff] [review] TouchCaretWIP0225.patch Review of attachment 8381272 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Vincent, please handle over to Phoebe

Merge this Bug and the patches to Bug 924692.