Morris Tseng [:mtseng] [:Morris] (Inactive) Reporter
	Description • 10 years ago

Currently, we maintain logic of touch and selection carets in different files. But they have many common functions and we cannot easily add more logic which is related to those carets. Which means our carets are fragile.

So we have a plan to refactor those carets to let them become more maintainable. We'll also add gtest for those codes.

Current wip is at [1]. I'll upload to here once it is reviewable.

[1]: https://github.com/mephisto41/gecko-dev/tree/copypaste-refactor

	Peter Chang[:pchang]
	Comment 1 • 9 years ago

This bug will be landed at V3, set as p4 now.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 3 • 9 years ago

ClampRectToScrollFrames generalizes IsRectVisibleInScrollFrames by
returning the clamped rect in scroll frames. IsRectVisibleInScrollFrames
could be implemented by checking whether the clamped rect is empty or
not.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 6 • 9 years ago

The necessary modifications are the same as SelectionCarets. For
convenience, Touch/SelectionCarets will be disabled whenever
AccessibleCaret preference is enabled.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 7 • 9 years ago

The new class should behave like the old TouchCaret and SelectionCarets.
I simply refactor the setUp() to support both the old and new
preferences.

_test_handle_tilt_when_carets_overlap_to_each_other() is modified
because AccessibleCaret does not inflate the caret hit rectangle as
TouchCaret/SelectionCarets did. The point for tilt caret edges need to
shrink a bit.

	Morris Tseng [:mtseng] [:Morris] (Inactive) Reporter
	Comment 8 • 9 years ago

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #4)
> Created attachment 8592091 [details] [diff] [review]
> Part 2 - Unify TouchCaret and SelectionCarets
> 
> We added new classes CopyPasteEventHub, CopyPasteManager, and
> AccessibleCaret to unify the logic in TouchCaret and SelectionCarets.
> See each header file for the description about what the class does.
> 
> Note: The logic to dispatch information to Gaia will be implemented in
> a follow-up bug.
> 
> Some notable technical difference between AccessibleCaret and the
> Touch/SelectionCarets.
> * The anonymous dom element containing a caret image will be created by
>   AccessibleCaret by using the API landed in bug 1020244 instead of
>   being created by nsCanvasFrame.
> * CopyPasteManager uses two AccessibleCarets to unify the functionality
>   provided by TouchCaret and SelectionCarets. It has "cursor" mode and
>   "selection" mode, which corresponds to TouchCaret and SelectionCarets,
>   respectively.
> * It's hard to exchange information about the status of event handling
>   between TouchCaret and SelectionCarets. Now CopyPasteEventHub is the
>   entry point for all events and callbacks.
> * When developing SelectionCarets, we encounter performance issues
>   because SelectionCarets might dispatch unnecessary events to Gaia
>   driven by the selection changed events. SelectionCarets did not have
>   clear internal states to avoid that. To solve that, CopyPasteEventHub
>   implements state classes, and rely on the current states to call the
>   CopyPasteManager's handler only when it's needed. For example, when
>   dragging a caret, we do not interest in NotifySelectionChanged() for
>   updating the carets. Since we know a caret is being dragging, we can
>   call UpdateCarets() directly. Hence DragCaretState does override
>   OnSelectionChanged().

I suggest attach a state machine diagram for better understanding this patch.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 9 • 9 years ago

Comment on attachment 8592091 [details] [diff] [review]
Part 2 - Unify TouchCaret and SelectionCarets (v1)

(In reply to Morris Tseng [:mtseng] from comment #8)
> I suggest attach a state machine diagram for better understanding this patch.

Will do. Will also break part 2 into smaller patches.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 11 • 9 years ago

See AccessibleCaret.h for the class description.

Technical difference between AccessibleCaret and Touch/SelectionCarets:
The anonymous dom element containing a caret image will be created by
AccessibleCaret by using the API landed in bug 1020244 instead of being
created by nsCanvasFrame.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 12 • 9 years ago

See CopyPasteManager.h for the class description.

CopyPasteManager uses two AccessibleCarets to unify the functionality
provided by TouchCaret and SelectionCarets. It has "cursor" mode and
"selection" mode, which corresponds to TouchCaret and SelectionCarets,
respectively.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 13 • 9 years ago

See CopyPasteEventHub.h for the class description, and this link for the
state transition diagram.
https://wiki.mozilla.org/Copy_n_Paste#CopyPasteEventHub_State_Transition_Diagram

Both TouchCaret and SelectionCarets have their event handling mechanism,
which lead to a lot of code duplication. Now CopyPasteEventHub serves as
the single entry point for all events and callbacks.

We also encountered performance issues in SelectionCarets because many
unnecessary events might be dispatched to Gaia driven by the selection
changed events. SelectionCarets did not have clear internal states to
avoid this. To solve it, CopyPasteEventHub implements state classes, and
rely on the current states to call the CopyPasteManager's handler only
when it's needed.

For example, when dragging a caret, we do not interest in
NotifySelectionChanged() for updating the carets. Since we've known a
caret is being dragging, we can call UpdateCarets() directly. Hence
DragCaretState does not override OnSelectionChanged().

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 16 • 9 years ago

Comment on attachment 8592701 [details] [diff] [review]
Part 2.2 - Add AccessibleCaret (v1)

Review of attachment 8592701 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/AccessibleCaret.h
@@ +33,5 @@
> +// 1. Insert DOM Element as an anonymous conent which contains the caret image.
> +// 2. Provide functions to change the caret appearance and position.
> +//
> +// All the rect or point used are relative to root frame except being specified
> +// explicitly.

This doesn't really explain what an AccessibleCaret *is*, i.e., what its associated state means.

@@ +47,5 @@
> +    Normal,
> +    NormalNotShown,
> +    Left,
> +    Right
> +  };

Document these values.

@@ +49,5 @@
> +    Left,
> +    Right
> +  };
> +  bool IsVisuallyVisible() const;
> +  bool IsLogicallyVisible() const;

Describe these functions and why they're different.

@@ +52,5 @@
> +  bool IsVisuallyVisible() const;
> +  bool IsLogicallyVisible() const;
> +  Appearance GetAppearance() const;
> +  void SetAppearance(Appearance aAppearance);
> +  void SetBarEnabled(bool aEnabled);

Document this

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 17 • 9 years ago

Comment on attachment 8592702 [details] [diff] [review]
Part 2.3 - Add CopyPasteManager (v1)

Review of attachment 8592702 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/CopyPasteManager.h
@@ +32,5 @@
> +class CopyPasteEventHub;
> +
> +// -----------------------------------------------------------------------------
> +// CopyPasteManager handles events and callbacks from CopyPasteEventHub, and
> +// perform the real work to manipulate selection object and AccessibleCaret.

Explain how many of these there are (one per PresShell?)

@@ +75,5 @@
> +  bool ChangeFocus(nsIFrame* aFrame) const;
> +  nsresult SelectWord(nsIFrame* aFrame, const nsPoint& aPoint) const;
> +  void SetSelectionDragState(bool aState) const;
> +  void SetSelectionDirection(nsDirection aDir) const;
> +  nsIFrame* FindFirstNodeWithFrame(bool aBackward, int32_t* aOutOffset) const;

Document this method

@@ +98,5 @@
> +  // Member variables
> +  nscoord mOffsetYToCaretLogicalPosition = NS_UNCONSTRAINEDSIZE;
> +  nsIPresShell* mPresShell = nullptr;
> +  UniquePtr<AccessibleCaret> mFirstCaret;
> +  UniquePtr<AccessibleCaret> mSecondCaret;

Explain what "first" and "second" mean here

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 18 • 9 years ago

Comment on attachment 8592703 [details] [diff] [review]
Part 2.4 - Add CopyPasteEventHub (v1)

Review of attachment 8592703 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/CopyPasteEventHub.cpp
@@ +355,5 @@
> +  aContext->SetState(aContext->NoActionState());
> +
> +  // We should always consume the event since we've pressed on the caret.
> +  return nsEventStatus_eConsumeNoDefault;
> +}

Easier to read if you move these method definitions up into the class declaration.

::: layout/base/CopyPasteEventHub.h
@@ +38,5 @@
> +// concrete events. CopyPasteEventHub also synthesizes fake events such as
> +// long-tap or scroll-end if APZ is not in use.
> +//
> +// See this link for state transition diagram on Mozilla wiki.
> +// https://wiki.mozilla.org/Copy_n_Paste#CopyPasteEventHub_State_Transition_Diagram

Good diagram, but I'd prefer it to be in the tree, either as ASCII art here or an image checked in and referenced here.

Can you explain why CopyPasteEventHub is separate from CopyPasteManager?

Are these really the right names? Would it make more sense to call these TouchCaretManager or something like that? These aren't involved in regular copy and paste so these names might confuse people.

@@ +147,5 @@
> +
> +// -----------------------------------------------------------------------------
> +// Base class for state. A concrete state should inherit this class, and
> +// override the methods for handling the events or callbacks. A concrete state
> +// is also responsible for transforming to the next concrete state.

Can you explain why you chose this approach instead of, say, having a simple state enum value, where the On... methods are on CopyPasteEventHub and switch on the current state? Maybe that would be simpler than having separate state classes with virtual methods?

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 19 • 9 years ago

Re comment 18:

One quick question about class naming. You're right that CopyPasteEventHub and CopyPasteManager might confuse people. How about call them AccessibleCaretEventHub and AccessibleCaretManager? CaretEventHub and CaretManager would be shorter, but people might think they're related to nsCaret. Which ones do you prefer?

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 20 • 9 years ago

Re comment 18:

Q: Why CopyPasteEventHub is separate from CopyPasteManager?
A: We'd like the state transitions in CopyPasteEventHub to be testable by gtest, so we put operations involving AcceesibleCaret, PresShell, Selection, etc. in CopyPasteManager. In this way, we can mock  CopyPasteManager, and test the correctness of the state transition given various events, callbacks, and the returns values of mocked methods of CopyPasteManager.

Q: Why implementing state classes with virtual methods rather than simple enum and switch case?
A: When the idea of state transition emerged, I indeed modelled them by using enum and switch case. From the SelectionCarets experience, we already know all the events and callbacks need to be handled. But the states required to model the system were not so clearly understood. Then I found it's hard to add a new state into the system. The handling functions become large, the size of nested switch case grow, and some common operation are hard to extract.

Then I start from scratch and try the states class design. I feel it's easier to add new state and focused on choosing which events or callbacks to handle by overriding them in a specific state. State::Enter() is convenient that I can put common code in it when multiple events lead the transition into this state. So is State::Leave(). All those State::OnXXX methods should be short and simple. And I hopes they're easier to read than a large nested switch case :-)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 21 • 9 years ago

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #19)
> One quick question about class naming. You're right that CopyPasteEventHub
> and CopyPasteManager might confuse people. How about call them
> AccessibleCaretEventHub and AccessibleCaretManager? CaretEventHub and
> CaretManager would be shorter, but people might think they're related to
> nsCaret. Which ones do you prefer?

AccessibleCaretEventHub/AccessibleCaretManager sounds good.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 22 • 9 years ago

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #20)
> Q: Why CopyPasteEventHub is separate from CopyPasteManager?
> A: We'd like the state transitions in CopyPasteEventHub to be testable by
> gtest, so we put operations involving AcceesibleCaret, PresShell, Selection,
> etc. in CopyPasteManager. In this way, we can mock  CopyPasteManager, and
> test the correctness of the state transition given various events,
> callbacks, and the returns values of mocked methods of CopyPasteManager.

OK. Please document that in the code and make sure it's clear to maintainers which functionality belongs in which class.

> Q: Why implementing state classes with virtual methods rather than simple
> enum and switch case?
> A: When the idea of state transition emerged, I indeed modelled them by
> using enum and switch case. From the SelectionCarets experience, we already
> know all the events and callbacks need to be handled. But the states
> required to model the system were not so clearly understood. Then I found
> it's hard to add a new state into the system. The handling functions become
> large, the size of nested switch case grow, and some common operation are
> hard to extract.
> 
> Then I start from scratch and try the states class design. I feel it's
> easier to add new state and focused on choosing which events or callbacks to
> handle by overriding them in a specific state. State::Enter() is convenient
> that I can put common code in it when multiple events lead the transition
> into this state. So is State::Leave(). All those State::OnXXX methods should
> be short and simple. And I hopes they're easier to read than a large nested
> switch case :-)

OK.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 23 • 9 years ago

Rename CopyPaste -> AccessibleCaret

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 24 • 9 years ago

Document enum Appearance and other methods suggested in comment 16.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 25 • 9 years ago

See AccessibleCaretManager.h for the class description.

AccessibleCaretManager uses two AccessibleCarets to unify the
functionality provided by TouchCaret and SelectionCarets. It has
"cursor" mode and "selection" mode, which corresponds to TouchCaret and
SelectionCarets, respectively.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 26 • 9 years ago

Addressed comment 18.
* Rename CopyPasteEventHub -> AccessibleCaretEventHub
* Move State method definitions into class declaration
* Check-in image and source code of the state transition diagram into layout/base/doc
* Improve documentation of AccessibleCaretEventHub

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 27 • 9 years ago

Rename CopyPaste -> AccessibleCaret

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 28 • 9 years ago

* Rename TestCopyPasteEventHub -> TestAccessibleCaretEventHub
* Style clean-up

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 29 • 9 years ago

Rename CopyPaste -> AccessibleCaret

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 30 • 9 years ago

Addressed comment 17.
* Rewrite documentation of AccessibleCaretManager
* Document FindFirstNodeWithFrame, mFirstCaret, mSecondCaret, and other methods.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 31 • 9 years ago

Remove the gray background color of AccessibleCaret for debugging in ua.css.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 32 • 9 years ago

Diff between v1 and v4
* Document enum Appearance and other methods suggested in comment 16.
* Remove the gray background color of AccessibleCaret for debugging in ua.css.
* Rename some functions.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 33 • 9 years ago

Diff between v1 and v3
* Addressed comment 17 by rewrite documentation of AccessibleCaretManager.
* Document FindFirstNodeWithFrame, mFirstCaret, mSecondCaret, and other methods.
* Function name changes in AccessibleCaret.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 34 • 9 years ago

When nsDisplayListBuilder doesn't build caret, we need to skip
AccessibleCaret frame by checking that the content of the frame is in
native anonymous subtree and has the "moz-accessiblecaret" class.

Thank :mtseng for providing this approach.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 35 • 9 years ago

Ports the patch for Touch/SelectionCarets in bug 1021499.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 36 • 9 years ago

Comment on attachment 8597916 [details] [diff] [review]
Part 2.2 - Add AccessibleCaret (v4)

Review of attachment 8597916 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly looks good.

::: layout/base/AccessibleCaret.cpp
@@ +63,5 @@
> +
> +AccessibleCaret::Appearance
> +AccessibleCaret::GetAppearance() const
> +{
> +  return mAppearance;

A lot of these short functions should be defined in the header file for conciseness.

@@ +77,5 @@
> +  ErrorResult rv;
> +  CaretElement()->ClassList()->Remove(AppearanceString(mAppearance), rv);
> +  MOZ_ASSERT(!rv.Failed(), "Remove old appearance failed!");
> +
> +  CaretElement()->ClassList()->Add(AppearanceString(aAppearance), rv);

Instead of adding a "none" class, it would be slightly better to simply not have a class set at all. So you would only remove the old mAppearance if it's not None/NormalNotShown, and you would only add the new appearance if it's not None/NormalNotShown.

@@ +142,5 @@
> +
> +Element*
> +AccessibleCaret::SelectionBarElement() const
> +{
> +  return CaretElement()->GetLastElementChild();

More functions that could just go in the header

@@ +288,5 @@
> +
> +void
> +AccessibleCaret::SetCaretElementPosition(nsIFrame* aFrame, const nsRect& aRect)
> +{
> +  // Transform position so that it relatives to containerFrame.

"So that it's relative to containerFrame"

::: layout/base/AccessibleCaret.h
@@ +33,5 @@
> +// anonymous content containing the caret image. The caret appearance and
> +// position can be controlled by SetAppearance() and SetPosition().
> +//
> +// All the rect or point used are relative to root frame except being specified
> +// explicitly.

Document that none of the methods of this class flush layout or style (if that's true!).

@@ +97,5 @@
> +  // Does two AccessibleCarets overlap?
> +  bool Intersects(const AccessibleCaret& rhs) const;
> +
> +  // Is the position within the caret's rect?
> +  bool Contains(const nsPoint& aPosition) const;

Document what this point is relative to

@@ +146,5 @@
> +
> +  // Member variables
> +  Appearance mAppearance = Appearance::None;
> +  bool mBarEnabled = false;
> +  nsIPresShell* mPresShell = nullptr;

Is this a weak reference? How does it get cleared when the presshell goes away? Please document this.

@@ +148,5 @@
> +  Appearance mAppearance = Appearance::None;
> +  bool mBarEnabled = false;
> +  nsIPresShell* mPresShell = nullptr;
> +  nsRefPtr<dom::AnonymousContent> mCaretElementHolder;
> +  nsRect mImaginaryCaretRect;

Document what this is relative to.

@@ +152,5 @@
> +  nsRect mImaginaryCaretRect;
> +
> +  // A no-op touch-start listener which prevents APZ from panning when dragging
> +  // the caret.
> +  nsRefPtr<DummyTouchListener> mDummyTouchListener{new DummyTouchListener()};

Initializing these members at declaration is a new C++ feature. Might be better for now to keep doing these in the constructor like other classes do.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 37 • 9 years ago

Comment on attachment 8597921 [details] [diff] [review]
Part 2.3 - Add AccessibleCaretManager (v3)

Review of attachment 8597921 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly looks good.

::: layout/base/AccessibleCaretManager.cpp
@@ +41,5 @@
> +  : mPresShell(aPresShell)
> +{
> +  if (mPresShell) {
> +    mFirstCaret = MakeUnique<AccessibleCaret>(mPresShell);
> +    mSecondCaret = MakeUnique<AccessibleCaret>(mPresShell);

Any reason to not just make mFirstCaret/mSecondCaret direct members of this object, instead of pointers?

@@ +287,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +AccessibleCaretManager::TapCaret(const nsPoint& aPoint)

Why doesn't this function do anything?

@@ +301,5 @@
> +  return rv;
> +}
> +
> +nsresult
> +AccessibleCaretManager::SelectWordOrShortcut(const nsPoint& aPoint)

Please document which functions flush layout and which don't. It's a bit unclear that callers of this function will want to flush layout first.

::: layout/base/AccessibleCaretManager.h
@@ +47,5 @@
> +
> +  virtual nsresult PressCaret(const nsPoint& aPoint);
> +  virtual nsresult DragCaret(const nsPoint& aPoint);
> +  virtual nsresult ReleaseCaret();
> +  virtual nsresult TapCaret(const nsPoint& aPoint);

Document the difference between PressCaret and TpaCaret

@@ +48,5 @@
> +  virtual nsresult PressCaret(const nsPoint& aPoint);
> +  virtual nsresult DragCaret(const nsPoint& aPoint);
> +  virtual nsresult ReleaseCaret();
> +  virtual nsresult TapCaret(const nsPoint& aPoint);
> +  virtual nsresult SelectWordOrShortcut(const nsPoint& aPoint);

Document what these points are relative to.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 38 • 9 years ago

Comment on attachment 8597139 [details] [diff] [review]
Part 2.4 - Add AccessibleCaretEventHub (v2)

Review of attachment 8597139 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

::: layout/base/AccessibleCaretEventHub.cpp
@@ +394,5 @@
> +      !aPresShell->GetCanvasFrame()->GetCustomContentContainer()) {
> +    return;
> +  }
> +
> +  nsAutoScriptBlocker scriptBlocker;

Why do you need a script blocker here?

::: layout/base/AccessibleCaretEventHub.h
@@ +48,5 @@
> +// transitions by giving events, callbacks, and the return values by mocked
> +// methods of AccessibleCaretEventHub. See TestAccessibleCaretEventHub.cpp.
> +//
> +// Besides dealing with real events, AccessibleCaretEventHub also synthesizes
> +// fake events such as scroll-end or long-tap providing APZ is not in use.

Why do we need to do this? Won't APZ be in use in all the cases we care about?

@@ +114,5 @@
> +  nsEventStatus HandleKeyboardEvent(WidgetKeyboardEvent* aEvent);
> +
> +  virtual nsPoint GetTouchEventPosition(WidgetTouchEvent* aEvent,
> +                                        int32_t aIdentifier) const;
> +  virtual nsPoint GetMouseEventPosition(WidgetMouseEvent* aEvent) const;

Why are these virtual?

::: layout/base/doc/AccessibleCaretEventHubStates.dot
@@ +1,3 @@
> +// Steps to generate AccessibleCaretEventHubStates.png
> +// 1. Install Graphviz
> +// 2. dot -T png -o AccessibleCaretEventHubStates.png AccessibleCaretEventHubStates.dot

Very nice :-)

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 39 • 9 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> Initializing these members at declaration is a new C++ feature. Might be
> better for now to keep doing these in the constructor like other classes do.

According to [1], we support "member initializers" on all compilers now. Any specific risk not to used this feature?

[1] https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 40 • 9 years ago

Comment on attachment 8598469 [details] [diff] [review]
Part 6 - Skip AccessibleCaret frame if nsDisplayListBuilder doesn't build caret (v1)

Review of attachment 8598469 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +2237,5 @@
>    if (aBuilder->IsBackgroundOnly())
>      return;
>  
> +  // Skip AccessibleCaret frame if the builder does not build caret.
> +  if (!aBuilder->IsBuildingCaret()) {

Let's not add this here. This is a very hot code.

I think we could fix this by modifying nsCanvasFrame instead, to walk the frame children of the custom content container in nsCanvasFrame and call BuildDisplayList on them directly, skipping the caret elements when !IsBuildingCaret.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 41 • 9 years ago

Addressed comment 36.

Diff between v4 and v5:
* Move one-line functions from cpp to header.
* Document the coordinate of Contains() and mImaginaryCaretRect
* Document mPresShell, and annotate it with MOZ_NON_OWNING_REF
* Document AccessibleCaret does not flush layout or style.

Comments not adderssed:
1. About the "none" class when setting appearance.
   I use the "none" class to add "display: none" (in ua.css) to hide all the elements subtree of AccessibleCaret. If the "none" class is not used, we'll need to set "display: none" to the default style, and add "display: block" to all the Normal/Left/Right css.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 42 • 9 years ago

Address comment 37.
* Document all public methods
* Document AccessibleCaretManager does not flush layout in all public methods prior to performing tasks.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 43 • 9 years ago

Re comment 37.

Q: Why making mFirstCaret/mSecondCaret unique pointers instead of direct members?
A: We do not want to allocate mFirstCaret and mSecondCaret in gtest since mPresShell is a nullptr there.

Q: Why doesn't AccessibleCaretManager::TapCaret() do anything?
A: Will implement in bug 1155493.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 44 • 9 years ago

* Document mPresShell
* Document why need a script blocker in AccessibleCaretEventHub::Init()

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 45 • 9 years ago

Re comment 38

Q: Doesn't APZ be in use in all the cases we care about?
A: Currently, APZ does not in use on B2G desktop (for Gaia test) and desktop browser. We still need to synthesize scroll-end and long-tap for these platforms.

Q: Why are GetTouchEventPosition() and GetMouseEventPosition() virtual?
A: To override them in gtest. They will convert the position to be relative to root frame. We need to override them where mPresShell is nullptr in gtest.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 46 • 9 years ago

Need info about comment 39. Thanks.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 47 • 9 years ago

This part becomes simpler due to new part 2.2 (v5).

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 48 • 9 years ago

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #39)
> According to [1], we support "member initializers" on all compilers now. Any
> specific risk not to used this feature?

I've asked on dev-platform whether people think we should use them indiscriminately. Let's see what people say.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 49 • 9 years ago

Comment on attachment 8599731 [details] [diff] [review]
Part 2.2 - Add AccessibleCaret  (v5)

Review of attachment 8599731 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/AccessibleCaret.h
@@ +33,5 @@
> +// All the rect or point are relative to root frame except being specified
> +// explicitly.
> +//
> +// None of the methods in AccessibleCaret will flush layout or style. To ensure
> +// that SetPostion() works correctly, the caller must make sure the layout is up

SetPosition

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 50 • 9 years ago

Comment on attachment 8599738 [details] [diff] [review]
Part 2.4 - Add AccessibleCaretEventHub (v3)

Review of attachment 8599738 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/AccessibleCaretEventHub.cpp
@@ +395,5 @@
> +    return;
> +  }
> +
> +  // Without this, root frame might become nullptr in AccessibleCaretManager's
> +  // constructor after mFirstCaret is constructed.

Which root frame? Can you explain this in more detail?

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 51 • 9 years ago

Addressed comment 50 by providing more detail about why needs nsAutoScriptBlocker.

Full debug log by using rr and reverse-continue to find the cause. Thank you roc!
Without using nsAutoScriptBlocker, the frame tree got destroyed after mFirstCaret's InjectCaretElement() was called.
https://pastebin.mozilla.org/8832301

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 52 • 9 years ago

Comment on attachment 8600769 [details] [diff] [review]
Part 2.4 - Add AccessibleCaretEventHub (v4)

Review of attachment 8600769 [details] [diff] [review]:
-----------------------------------------------------------------

It looks to me like the 897852.html testcase reveals a bug that we should fix in a different way --- content mutation event listeners should not fire on changes to the nsCanvasFrame's anonymous content. I'll r+ this so we can get this landed, but please file a new bug about this --- and fix it :-). It's probably just a matter of marking some content native-anonymous I guess...

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 53 • 9 years ago

Re comment 52: Open a follow-up bug for 897852.html testcase in bug 1161384.

Move part 6 to bug 1161389 for further invesigation, and move part 7 to bug 1161392 for dependency.

roc, thank you for reviewing these huge patches :-) Let's land part 1 ~ 5.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 54 • 9 years ago

Addressed comment 49. Fix a typo.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 55 • 9 years ago

Rebased.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 56 • 9 years ago
try

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

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 57 • 9 years ago

Fix test failure on try, and refine test cases.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 58 • 9 years ago

Forgot to modify layout/base/tests/marionette/manifest.ini in v2. Now fixed.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 59 • 9 years ago

Marionette tests are all green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5cacbb336a2

	Carsten Book [:Tomcat]
	Comment 60 • 9 years ago

part 3 failed to apply :

(eg '1-3,5', or 's' to toggle the sort order between id & patch description) 8
adding 1110039 to series file
renamed 1110039 -> Bug-1110039-Part-3---Add-gtest-for-AccessibleCaret.patch
applying Bug-1110039-Part-3---Add-gtest-for-AccessibleCaret.patch
patching file layout/base/moz.build
Hunk #1 FAILED at 144
1 out of 1 hunks FAILED -- saving rejects to file layout/base/moz.build.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh Bug-1110039-Part-3---Add-gtest-for-AccessibleCaret.patch

could you take a loo, thanks!

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 61 • 9 years ago

Rebased.

	Pulsebot
	Comment 62 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e439532cdc82
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae5c8904cae7
https://hg.mozilla.org/integration/mozilla-inbound/rev/2228e70314f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e426254bcc6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c83422ef7f41
https://hg.mozilla.org/integration/mozilla-inbound/rev/31ad49353c23
https://hg.mozilla.org/integration/mozilla-inbound/rev/11e428dac23c
https://hg.mozilla.org/integration/mozilla-inbound/rev/88a289fcb214
https://hg.mozilla.org/integration/mozilla-inbound/rev/48b7fceaeea9

	Carsten Book [:Tomcat]
	Comment 63 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e439532cdc82
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae5c8904cae7
https://hg.mozilla.org/integration/mozilla-inbound/rev/2228e70314f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e426254bcc6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c83422ef7f41
https://hg.mozilla.org/integration/mozilla-inbound/rev/31ad49353c23
https://hg.mozilla.org/integration/mozilla-inbound/rev/11e428dac23c
https://hg.mozilla.org/integration/mozilla-inbound/rev/88a289fcb214
https://hg.mozilla.org/integration/mozilla-inbound/rev/48b7fceaeea9

	Carsten Book [:Tomcat]
	Comment 64 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/e439532cdc82
https://hg.mozilla.org/mozilla-central/rev/ae5c8904cae7
https://hg.mozilla.org/mozilla-central/rev/2228e70314f8
https://hg.mozilla.org/mozilla-central/rev/e426254bcc6b
https://hg.mozilla.org/mozilla-central/rev/c83422ef7f41
https://hg.mozilla.org/mozilla-central/rev/31ad49353c23
https://hg.mozilla.org/mozilla-central/rev/11e428dac23c
https://hg.mozilla.org/mozilla-central/rev/88a289fcb214
https://hg.mozilla.org/mozilla-central/rev/48b7fceaeea9

	Daniel Holbert [:dholbert]
	Comment 65 • 9 years ago

This was missing an 'override' annotation on the Name() macro in NS_IMPL_STATE_UTILITIES, which causes clang 3.6 & newer to spam a "-Winconsistent-missing-override" build warning, which busts --enable-warnings-as-errors builds (with clang 3.6+).

I pushed a followup to add that keyword, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71935b9669d9

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 66 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/71935b9669d9