Closed Bug 1125527 Opened 6 years ago Closed 6 years ago

Create common Utilities script for testSelectionHandler, testTextareaSelections and testInputSelections

Categories

(Firefox for Android Graveyard :: Text Selection, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox38 fixed)

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: capella, Assigned: kuoe0.tw, Mentored)

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 3 obsolete files)

The three SelectionHandler tests use a (mostly) common set of utility functions [1] hardcoded / duplicated in three places.

Let's consolidate them into a single script (The version in testInputSelections.html is the most complete), and include them like we do EventUtils [2].

[1] http://mxr.mozilla.org/mozilla-central/search?string=%3D%3D%3D%3D%3D+Utility+functions+%3D%3D%3D%3D%3D%3D&case=on&find=mobile.*.html%24&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/roboextender/testTextareaSelections.html?rev=c837afdde124&force=1&mark=5-6
Mentor: markcapella
Whiteboard: [lang=js]
I would like to try this!
Assignee: nobody → kuoe0
Status: NEW → ASSIGNED
Attached patch Bug-1125527.patch (obsolete) — Splinter Review
I merge the common code of tests into common.js at the same folder of testing programs.
TryServer result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39432a81ef56

By the way, is there any suggestion for the filename of the common utilities?
Comment on attachment 8557727 [details] [diff] [review]
Bug-1125527.patch

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

In future versions, can you fixup your commit message? A simple example is
https://bugzilla.mozilla.org/attachment.cgi?id=8558275&action=edit

In that example, I'd used r=bnicholson on the commit message, but for yours, please use r=margaret, and
remove all the unneccessary comment lines such as
mobile/android/base/tests/roboextender/common.js   | 126 ++++

I'm going to provide you enough feedback to get you to where you can eventually ask her for ?review, so she needs to be listed in your commit message.

For now, when you repost the patch, just flag me as ?feedback again.

Let's have you use SelectionUtils for the filename of the common utilities.

FYI, there are other devs working in this code and some of their changes might get approved ahead of yours causing you to have to rebase your patch locally as we go forward. I'll try to let you know if conflicts develop, but if you keep your local repo in-sync with m-c like daily, you should notice for yourself.

Also FYI, we hope you'll be helping with further patches in the future, so in general, you might like to review: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

::: mobile/android/base/tests/roboextender/common.js
@@ +28,5 @@
> +    type: typeName,
> +         todo: result,
> +         msg: msg
> +  });
> +}

Please preserve the original indentations/styles ... for example here you'd use:
function todo(result, msg) {
  return Messaging.sendRequestForResult({
    type: typeName,
    todo: result,
    msg: msg
  });
}

I wonder if your editor is inserting TABS instead of spaces? Please make sure to use space characters.

::: mobile/android/base/tests/roboextender/testInputSelections.html
@@ +1,5 @@
>  <html>
>    <head>
>      <title>Automated RTL/LTR Text Selection tests for Input elements</title>
>      <meta name="viewport" content="initial-scale=1.0"/>
> +    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>

Avoid reformating lines throughout not directly needed for your patch. This line for example is
formatted to conform to Moz standard, where JS line length is kept under 100. No change
here is required.

@@ +7,3 @@
>      <script type="application/javascript">
>  
> +      const typeName = "Robocop:testInputSelections";

nit-picky: add a blank line after here.

@@ +8,5 @@
>  
> +      const typeName = "Robocop:testInputSelections";
> +      // Used to create handle movement events for SelectionHandler.
> +      const ANCHOR = "ANCHOR";
> +      const FOCUS = "FOCUS";

I don't know if you modified all these lines yourself, or if your editor auto-generated styles,
but it needs to corrected throughout. As mentioned above, avoid changing anything not required
for the patch.

::: mobile/android/base/tests/roboextender/testSelectionHandler.html
@@ +2,5 @@
>    <head>
>      <title>Automated Text Selection tests for Mobile</title>
>      <meta name="viewport" content="initial-scale=1.0"/>
> +    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>
> +    <script type="application/javascript" src="common.js"></script>

Same comments from testInputSelections ... avoid irrelevant changes.

@@ +9,2 @@
>  
> +      const typeName = "Robocop:testSelectionHandler";

nit-picky: add a blank line after here.

@@ +11,5 @@
> +      const DIV_POINT_TEXT = "Under";
> +      const INPUT_TEXT = "Text for select all in an <input>";
> +      const TEXTAREA_TEXT = "Text for select all in a <textarea>";
> +      const READONLY_INPUT_TEXT = "readOnly text";
> +

Same comments from testInputSelections ... avoid irrelevant changes here and throughout.

::: mobile/android/base/tests/roboextender/testTextareaSelections.html
@@ +1,5 @@
>  <html>
>    <head>
>      <title>Automated RTL/LTR Text Selection tests for Textareas</title>
>      <meta name="viewport" content="initial-scale=1.0"/>
> +    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>

Same comments from testInputSelections ... avoid irrelevant changes here and throughout.

@@ +7,3 @@
>      <script type="application/javascript">
>  
> +      const typeName = "Robocop:testTextareaSelections";

nit-picky: add a blank line after here.
Attachment #8557727 - Flags: feedback+
Attached patch Bug-1125527-v2.patch (obsolete) — Splinter Review
I fix the coding style and format of origin patch. Thanks for your feedback to help to improve the patch.
Attachment #8557727 - Attachment is obsolete: true
Attachment #8558963 - Flags: feedback?(markcapella)
Comment on attachment 8558963 [details] [diff] [review]
Bug-1125527-v2.patch

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

Hey Tommy! I'm sorry this stalled for a few days. There were some changes in this area that got in ahead of you, and rather than have you constantly re-basing code and trying to play catch-up, I waiting until the last one was done.

Today is Saturday. There are currently changes that are in fx-team that will move to mozilla-central Monday early am.

Can I ask you to:
   1) Wait until Monday around Noon EST
   2) Update your local repo from m-c
   3) Fix the two small nits I'll point out below
   4) Repost the patch with me for ?feedback

I'll push the patch to our TRY-server for integration/volume testing, then flag it Margaret for you for review.

Let me know if you have questions. I can quick-do this for you, and you could be done, but I thought you'd like to finish it yourself :-)

::: mobile/android/base/tests/roboextender/SelectionUtils.js
@@ +19,5 @@
> + */
> +function getSelectionHandler() {
> +  return (!this._selectionHandler) ?
> +    this._selectionHandler = Services.wm.getMostRecentWindow("navigator:browser").SelectionHandler :
> +      this._selectionHandler;

nit-picky: line up the line w/the one above it.

@@ +24,5 @@
> +}
> +
> +function todo(result, msg) {
> +  return Messaging.sendRequestForResult({
> +    type: typeName,

Since this is used / declared as a 'const' throughout, let's rename it closer to standard ...
Maybe TYPE_NAME ?
Attachment #8558963 - Flags: feedback?(markcapella) → feedback+
Actually, I just noticed that the sherrifs ran a weekend merge, and you can start with comment #6 part (2) above anytime you'd like now   :-)
Hi Mark, thank you for your feedback! I'll try to do it now. :)
Attached patch bug-1125527.patch (obsolete) — Splinter Review
I've done to rebase the source code after merged with m-c.

TryServer result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cff384da9a35
Attachment #8558963 - Attachment is obsolete: true
Attachment #8561201 - Flags: review?(margaret.leibovic)
Attachment #8561201 - Flags: feedback?(markcapella)
Comment on attachment 8561201 [details] [diff] [review]
bug-1125527.patch

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

I, confused by the TRY syntax you used ... I use:
try: -b do -p android-api-9,android-api-11 -u all -t none

And usually see results like:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f5d7741877d

You want to see successful 
Android 4.0 API11+ opt M(rc7) for these tests ...

In the example above, I actually triggered the test ~15 times to ensure there's no timing / race conditions present ... we want green all the time :)
(You probably don't need to go that far).

FWIW, if you correct and re-push, you don't have to request feedback from me anymore.

Just request r? from margaret from now on ... you won't need my advice :-D

::: mobile/android/base/tests/roboextender/SelectionUtils.js
@@ +19,5 @@
> + */
> +function getSelectionHandler() {
> +  return (!this._selectionHandler) ?
> +    this._selectionHandler = Services.wm.getMostRecentWindow("navigator:browser").SelectionHandler :
> +      this._selectionHandler;

nit: forgot to line this up
  return (...) ?
    this._ ... :
    this._ ...;
Attachment #8561201 - Flags: feedback?(markcapella) → feedback+
Comment on attachment 8561201 [details] [diff] [review]
bug-1125527.patch

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

Nice! I'm happy to see us created a shared place for these helper methods.

Capella, I'm giving you the responsibility of doing a full review here :)
Attachment #8561201 - Flags: review?(markcapella)
Attachment #8561201 - Flags: review?(margaret.leibovic)
Attachment #8561201 - Flags: feedback+
Hi, sorry for the mistake I forgot to fix that line! Now, I fix it. And the link of Try result is following:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=90bd7e4a5289

Thank you!
Attachment #8561201 - Attachment is obsolete: true
Attachment #8561201 - Flags: review?(markcapella)
Attachment #8561814 - Flags: review?(markcapella)
Comment on attachment 8561814 [details] [diff] [review]
Bug-1125527.patch

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

Nice job Tommy! Mark it checkin-needed, or push it yourself if you have access, and thanks so much for your help here :-D
Attachment #8561814 - Flags: review?(markcapella) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/276be70d7b06
Keywords: checkin-needed
Whiteboard: [lang=js] → [lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/276be70d7b06
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][fixed-in-fx-team] → [lang=js]
Target Milestone: --- → Firefox 38
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.