Closed Bug 1204872 Opened 6 years ago Closed 6 years ago

Add gtest for AccessibleCaretManager

Categories

(Core :: DOM: Selection, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Currently, there's no unit test for the appearance of the two AccessibleCarets after being manipulated by AccessibleCaretManager. Moreover, the root cause of some Gaia issues like bug 1196176 is that gecko does not send the necessary CaretStateChanged events. We should have gtest to cover that.
Bug 1204872 - Add documentation and rename mCaretMode. r=roc

Rename mCaretMode to mLastUpdateCaretsMode to make it clear that this
variable track the caret mode since last update. This also avoid the
confusion that GetCaretMode() returns mCaretMode.
Attachment #8661268 - Flags: review?(roc)
Bug 1204872 - Make AccessibleCaret testable. r=roc

Remove the assert of the existence of PresShell in the constructor since
there's no PresShell in gtest. Also make AccessibleCaret inheritable.
Attachment #8661269 - Flags: review?(roc)
Bug 1204872 - Make AccessibleCaretManager testable. r=roc

We need to extract statements that touch PresShell or access utilities
into functions so that we could mock them in gtest.
Attachment #8661270 - Flags: review?(roc)
Bug 1204872 - Prettify enum class printing. r=roc

Provide operator<< functions so that gtest can use them to print enum
classes.
Attachment #8661271 - Flags: review?(roc)
Bug 1204872 - Add gtest for AccessibleCaretManager. r=roc

Add basic gtest to ensure AccessibleCaret and AccessibleCaretManager are
both testable. More tests to come.
Attachment #8661272 - Flags: review?(roc)
Comment on attachment 8661268 [details]
MozReview Request: Bug 1204872 - Add documentation and rename mCaretMode. r=roc

https://reviewboard.mozilla.org/r/19321/#review17335
Attachment #8661268 - Flags: review?(roc) → review+
Comment on attachment 8661269 [details]
MozReview Request: Bug 1204872 - Make AccessibleCaret testable. r=roc

https://reviewboard.mozilla.org/r/19323/#review17341
Attachment #8661269 - Flags: review?(roc) → review+
Comment on attachment 8661270 [details]
MozReview Request: Bug 1204872 - Make AccessibleCaretManager testable. r=roc

https://reviewboard.mozilla.org/r/19325/#review17343

It makes me sad we have to make functions virtual in release builds just for testing.

Please add comments for each such function saying that it's virtual for testing purposes.
Attachment #8661270 - Flags: review?(roc) → review+
https://reviewboard.mozilla.org/r/19325/#review17343

To avoid run time overhead by virtual functions in production, how about we move those function into a helper class, say AccessibleCaretUtility, and make a template parameter for AccessibleCaretManager to use it? In production we can construct AccessibleCaretManager<AccessibleCaretUtility>. And in testing, we can provide a MockAccessibleCaretUtility with the same signatures as AccessibleCaretUtility, and construct it by using AccessibleCaretManager<MockAccessibleCaretUtility>.

https://code.google.com/p/googlemock/wiki/CookBook#Mocking_Nonvirtual_Methods
Flags: needinfo?(roc)
That sounds good!
Flags: needinfo?(roc)
Comment on attachment 8661268 [details]
MozReview Request: Bug 1204872 - Add documentation and rename mCaretMode. r=roc

Bug 1204872 - Add documentation and rename mCaretMode. r=roc

Rename mCaretMode to mLastUpdateCaretsMode to make it clear that this
variable track the caret mode since last update. This also avoid the
confusion that GetCaretMode() returns mCaretMode.
Comment on attachment 8661269 [details]
MozReview Request: Bug 1204872 - Make AccessibleCaret testable. r=roc

Bug 1204872 - Make AccessibleCaret testable. r=roc

Remove the assert of the existence of PresShell in the constructor since
there's no PresShell in gtest. Also make AccessibleCaret inheritable.
Comment on attachment 8661270 [details]
MozReview Request: Bug 1204872 - Make AccessibleCaretManager testable. r=roc

Bug 1204872 - Make AccessibleCaretManager testable. r=roc

We need to extract statements that touch PresShell, access frame trees,
or call other utility functions into functions so that we could mock or
stub them in gtest.

Inline GetFocusedContent since it's only used once.
Comment on attachment 8661271 [details]
MozReview Request: Bug 1204872 - Prettify enum class printing. r=roc

Bug 1204872 - Prettify enum class printing. r=roc

Provide operator<< functions so that gtest can use them to print enum
classes.
Comment on attachment 8661272 [details]
MozReview Request: Bug 1204872 - Add gtest for AccessibleCaretManager. r=roc

Bug 1204872 - Add gtest for AccessibleCaretManager. r=roc

Add basic gtest to ensure AccessibleCaret and AccessibleCaretManager are
both testable. More tests to come.
Re comment #8 and comment #11:

> Please add comments for each such function saying that it's virtual for
> testing purposes.

On second thought, I think it might not be worth adding a class template parameter for gtest since it will touch too many code. The code for AccessibleCaret isn't that hot. Virtual function calls should be bearable. I've upload a new patch set to group those virtual functions together on the header, and add comments saying that they're virtual for testing.
Comment on attachment 8661268 [details]
MozReview Request: Bug 1204872 - Add documentation and rename mCaretMode. r=roc

Bug 1204872 - Add documentation and rename mCaretMode. r=roc

Rename mCaretMode to mLastUpdateCaretsMode to make it clear that this
variable track the caret mode since last update. This also avoid the
confusion that GetCaretMode() returns mCaretMode.
Comment on attachment 8661269 [details]
MozReview Request: Bug 1204872 - Make AccessibleCaret testable. r=roc

Bug 1204872 - Make AccessibleCaret testable. r=roc

Remove the assert of the existence of PresShell in the constructor since
there's no PresShell in gtest. Also make AccessibleCaret inheritable.
Comment on attachment 8661270 [details]
MozReview Request: Bug 1204872 - Make AccessibleCaretManager testable. r=roc

Bug 1204872 - Make AccessibleCaretManager testable. r=roc

We need to extract statements that touch PresShell, access frame trees,
or call other utility functions into functions so that we could mock or
stub them in gtest.

Inline GetFocusedContent since it's only used once.
Comment on attachment 8661271 [details]
MozReview Request: Bug 1204872 - Prettify enum class printing. r=roc

Bug 1204872 - Prettify enum class printing. r=roc

Provide operator<< functions so that gtest can use them to print enum
classes.
Comment on attachment 8661272 [details]
MozReview Request: Bug 1204872 - Add gtest for AccessibleCaretManager. r=roc

Bug 1204872 - Add gtest for AccessibleCaretManager. r=roc

Add basic gtest to ensure AccessibleCaret and AccessibleCaretManager are
both testable. More tests to come.

Allow -Winconsistent-missing-override warning in gtest since MOCK_METHOD
does not have 'override' keyword. See bug 1169974.
You need to log in before you can comment on or make changes to this bug.