Enhance testSelectionHandler tests

RESOLVED FIXED in Firefox 38

Status

()

Firefox for Android
Text Selection
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: capella, Assigned: capella)

Tracking

unspecified
Firefox 38
ARM
Android
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 8543662 [details] [diff] [review]
bugEnhanceSelectionHandlerTests.diff

Basic enhancements to:

   ) Add tests for failing startSelection() actions
   ) Add test for successful attachCaret() action
   ) Add tests failing attachCaret() actions

Additional failure reporting aids in developing new testSelectionFoo() cases.
Attachment #8543662 - Flags: review?(margaret.leibovic)

Comment 1

4 years ago
Comment on attachment 8543662 [details] [diff] [review]
bugEnhanceSelectionHandlerTests.diff

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

This is looking good, thanks for taking the time to improve these tests. I just have a few small pieces of feedback, but r+ with comments addressed.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +19,5 @@
> +  START_ERROR_NO_SELECTION: "Selection performed, but nothing resulted.",
> +  START_ERROR_PROXIMITY: "Selection target and result seem unrelated.",
> +
> +  // Error codes returned during attachCaret()
> +  ATTACH_ERROR_NONE: "",

Maybe we can combine this and START_ERROR_NONE to just create a single ERROR_NONE case? That might make it easier for consumers to know what to return value to check.

Overall, though, these return values seem like a good improvement to make it easier to know what's going on, rather than just throwing around booleans.

::: mobile/android/chrome/content/browser.js
@@ +2512,5 @@
>          // If textSelection WORD is successful,
>          // consume / preventDefault the context menu event.
>          if (SelectionHandler.startSelection(this._target,
> +          { mode: SelectionHandler.SELECT_AT_POINT, x: event.clientX, y: event.clientY }) ===
> +          SelectionHandler.START_ERROR_NONE) {

The formatting here is confusing. Could you make the left side of this identity comparison into a local variable?

@@ +2521,4 @@
>          // If textSelection caret-attachment is successful,
>          // consume / preventDefault the context menu event.
> +        if (SelectionHandler.attachCaret(this._target) ===
> +          SelectionHandler.ATTACH_ERROR_NONE) {

Nit: No line break to the right of the identity operator.
Attachment #8543662 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 3

4 years ago
Created attachment 8546324 [details] [diff] [review]
bug1117542.diff

That didn't end well. I tweaked the test involved thinking I had an edge-case in regards to specifying screen points(,). While again successful on my phone and tablet test devices, I still wind up with random oranges on the try-server [1].

It looks like the predicted failure testcase does fail, but with one of two possible reasons, either |Selection performed, but nothing resulted|, or |Word selection at point failed|.

I'm assuming it's a device/OS/screen resolution combination thing, but for now would like to pull that particular test, and open a followup exploratory bug.

I've re-pushed the patch without that test, and we seem good-to-go again. [2]

[1] https://tbpl.mozilla.org/?tree=Try&rev=52d7d99a55af
[2] https://tbpl.mozilla.org/?tree=Try&rev=d6107875ad23
Attachment #8543662 - Attachment is obsolete: true
Attachment #8546324 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 4

4 years ago
Followup: Bug 1119625 - Debug and correct random orange test-case intended for testSelectionHandler

Comment 5

4 years ago
Comment on attachment 8546324 [details] [diff] [review]
bug1117542.diff

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

Thanks for your patience. Green try run == land it!
Attachment #8546324 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/a914814d2054
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.