Closed Bug 1125527 Opened 9 years ago Closed 9 years ago

Create common Utilities script for testSelectionHandler, testTextareaSelections and testInputSelections

Tracking

(firefox38 fixed)

Status:

RESOLVED FIXED

Milestone:

Firefox 38

Tracking Flags:

Tracking

Status

firefox38

---

fixed

People

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

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 3 obsolete files)

Bug-1125527.patch 9 years ago Tommy Kuo (away forever...) 109.04 KB, patch	capella : feedback+	Details \| Diff \| Splinter Review
Bug-1125527-v2.patch 9 years ago Tommy Kuo (away forever...) 17.87 KB, patch	capella : feedback+	Details \| Diff \| Splinter Review
bug-1125527.patch 9 years ago Tommy Kuo (away forever...) 17.92 KB, patch	capella : feedback+ Margaret : feedback+	Details \| Diff \| Splinter Review
Bug-1125527.patch 9 years ago Tommy Kuo (away forever...) 17.92 KB, patch	capella : review+	Details \| Diff \| Splinter Review

Mark Capella [:capella]

Reporter

Description

•

9 years ago

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

Mark Capella [:capella]

Reporter

Updated

•

9 years ago

Mentor: markcapella

Whiteboard: [lang=js]

Tommy Kuo (away forever...)

Assignee

Comment 1

•

9 years ago

I would like to try this!

Assignee: nobody → kuoe0

Mark Capella [:capella]

Reporter

Updated

•

9 years ago

Status: NEW → ASSIGNED

Tommy Kuo (away forever...)

Assignee

Comment 2

•

9 years ago

Attached patch Bug-1125527.patch (obsolete) — Details — Splinter Review

I merge the common code of tests into common.js at the same folder of testing programs.

Tommy Kuo (away forever...)

Assignee

Comment 3

•

9 years ago

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?

Mark Capella [:capella]

Reporter

Comment 4

•

9 years ago

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.

Mark Capella [:capella]

Reporter

Updated

•

9 years ago

Attachment #8557727 - Flags: feedback+

Tommy Kuo (away forever...)

Assignee

Comment 5

•

9 years ago

Attached patch Bug-1125527-v2.patch (obsolete) — Details — 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)

Mark Capella [:capella]

Reporter

Comment 6

•

9 years ago

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+

Mark Capella [:capella]

Reporter

Comment 7

•

9 years ago

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   :-)

Tommy Kuo (away forever...)

Assignee

Comment 8

•

9 years ago

Hi Mark, thank you for your feedback! I'll try to do it now. :)

Tommy Kuo (away forever...)

Assignee

Comment 9

•

9 years ago

Attached patch bug-1125527.patch (obsolete) — Details — 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)

Mark Capella [:capella]

Reporter

Comment 10

•

9 years ago

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+

:Margaret Leibovic

Comment 11

•

9 years ago

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+

Tommy Kuo (away forever...)

Assignee

Comment 12

•

9 years ago

Attached patch Bug-1125527.patch — Details — Splinter Review

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)

Mark Capella [:capella]

Reporter

Comment 13

•

9 years ago

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+

Tommy Kuo (away forever...)

Assignee

Updated

•

9 years ago

Keywords: checkin-needed

Carsten Book [:Tomcat]

Comment 14

•

9 years ago

https://hg.mozilla.org/integration/fx-team/rev/276be70d7b06

Keywords: checkin-needed

Whiteboard: [lang=js] → [lang=js][fixed-in-fx-team]

Ryan VanderMeulen [:RyanVM]

Comment 15

•

9 years ago

https://hg.mozilla.org/mozilla-central/rev/276be70d7b06

Status: ASSIGNED → RESOLVED

Closed: 9 years ago

status-firefox38: --- → fixed

Resolution: --- → FIXED

Whiteboard: [lang=js][fixed-in-fx-team] → [lang=js]

Target Milestone: --- → Firefox 38

BMO Automation

Updated

•

3 years ago

Product: Firefox for Android → Firefox for Android Graveyard

You need to log in before you can comment on or make changes to this bug.