Closed Bug 1029943 Opened 7 years ago Closed 6 years ago

[Text selection] Enable selection carets on B2G and fix the existing test case failures

Categories

(Core :: DOM: Selection, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: slee, Assigned: TYLin)

References

Details

Attachments

(5 files, 11 obsolete files)

8.80 KB, text/plain
Details
1.54 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
11.85 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
898 bytes, patch
TYLin
: review+
Details | Diff | Splinter Review
1.72 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
No longer blocks: 1016184
Depends on: 1016184
Since bug 1016184 which turn on touch caret on B2G will be landed soon, let me to steal this bug, and change it to "Enable selection carets on B2G."
Summary: [Touch Caret] Enable touch caret on B2G → [Text selection] Enable selection carets on B2G and fix the existing test case failures
Try result after selection carets is enabled.
https://tbpl.mozilla.org/?tree=Try&rev=695d172644e3
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Duplicate of this bug: 1020725
Blocks: 921965
These test cases compare between non-edit and editable elements. So we should disable these tests.
Attachment #8472129 - Attachment is obsolete: true
Attachment #8476431 - Flags: review?(ehsan)
Some test cases would capture screenshot after element is blurred. If element is blurred, we should hide the selection carets.
Attachment #8476432 - Flags: review?(roc)
Rebased. We should wait until all tests are green.
Attachment #8472130 - Attachment is obsolete: true
Attachment #8476472 - Flags: review?(ehsan)
Comment on attachment 8476431 [details] [diff] [review]
Fix reftest fails on 824080-3/5/7.html, bug558663.html.

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

(Splinter doesn't seem to like this patch...)

+  <script>
+  // Selection caret's pref is checked only when PresShell is initialized. To turn
+  // off the pref, we test bug 558663 in an iframe.
+  SpecialPowers.pushPrefEnv({"set": [['selectioncaret.enabled', false]]}, function() {
+    var iframe = document.createElement("iframe");
+    iframe.src = "bug558663.html";
+    document.getElementById('container').appendChild(iframe);
+  });
+  </script>

You need to use SimpleTest.waitForExplicitFinish and SimpleTest.finish to make sure the test framework waits for these tests to be finished before loading the next page.  Also, in the test iframe you should not use your own copy of the SimpleTest.js API.  Usually people define shim functions like this at the beginning of their test frame code to forward all of the calls to these testing functions to the SimpleTest framework loaded in the main test page:

var ok = parent.ok;
var is = parent.is;
// same for todo, isnot, etc if you need them.
Attachment #8476431 - Flags: review?(ehsan) → review-
Attachment #8476472 - Flags: review?(ehsan) → review+
About B2G ICS Emulator Debug mochitest 9 timeout, I did some experiments. I found disable selection carets, test_collapse.html would running about 78 seconds. But when I enabled selection carets, it running about 476 seconds so that it would timeout mochitest in try server. 

Next, I commented out sendAsyncMsg at BrowserElementChildPreload.js [1], then it reduced to 170 seconds.
Then, I commented out addEventListener for mozselectionchange at BrowserElementChildPreload.js [2], then it reduced to 79 seconds.

So I think selection carets increased too much overhead here, Ehsan, do you have any suggestions?
I have some opinions,
1) Only send mozselectionchange when we have specific reason, so that we can reduce overhead. Add this change test case running about 80 seconds which is good!
2) Increase timeout from 330 seconds to maybe 600 seconds so the testcase won't timeout.
3) Split test case into many smaller test cases.  

[1]: http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#658
[2]: http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#210
Flags: needinfo?(ehsan)
Depends on: 1057256
I would be fine with increasing the timeout on that test, I think.  But note that this is an upstream test (from https://dvcs.w3.org/hg/editing), so you need to convince either Aryeh or Ms2ger about increasing the timeout.
Flags: needinfo?(ehsan)
Flags: needinfo?(ayg)
Flags: needinfo?(Ms2ger)
Feel free to fork the editing ones as needed.
Flags: needinfo?(Ms2ger)
If you're asking about test_runtest.html, it's not really maintained upstream, so it's safe enough to fork it.  I do have a project to split it up so it's not so ridiculously unwieldy (bug 1008146), but I don't know if I'll get to it in the foreseeable future.  The current location in the tree does suggest it's imported, though, so if you make any significant changes there's always the risk they'll get overwritten sometime in the future.  Maybe disable the import script for editing somehow with a comment explaining why, if you're doing anything that you don't want overwritten.
Flags: needinfo?(ayg)
Update to address comment.
Attachment #8476431 - Attachment is obsolete: true
Attachment #8478817 - Flags: review?(ehsan)
This patch is for mozilla-central. But I need modify upstream as well. Ms2ger, how do I modify upstream? Provide a patch for upstream or there is other way to do this? Thanks.
Attachment #8478819 - Flags: review?(ehsan)
Flags: needinfo?(Ms2ger)
Just land on m-c.
Flags: needinfo?(Ms2ger)
Comment on attachment 8478817 [details] [diff] [review]
Fix reftest fails on 824080-3/5/7.html, bug558663.html v2.

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

::: layout/base/tests/bug558663.html
@@ +8,2 @@
>    <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
>    <script type="application/javascript" src="/tests/SimpleTest/WindowSnapshot.js"></script>

You should forward the stuff coming from these two scripts to the parent similar to ok and SimpleTest below...
Attachment #8478817 - Flags: review?(ehsan) → review-
Attachment #8478819 - Flags: review?(ehsan) → review+
Update address comment.
Attachment #8478817 - Attachment is obsolete: true
Attachment #8479685 - Flags: review?(ehsan)
Comment on attachment 8479685 [details] [diff] [review]
Fix reftest fails on 824080-3/5/7.html, bug558663.html v3.

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

Looks great!
Attachment #8479685 - Flags: review?(ehsan) → review+
Rebased.
Attachment #8476472 - Attachment is obsolete: true
Attachment #8481168 - Flags: review+
Update since v3 is corrupt.
Attachment #8481168 - Attachment is obsolete: true
Attachment #8481174 - Flags: review+
Latest try result. We still have an orange M-10 on B2G ICS Emulator Debug.
https://tbpl.mozilla.org/?tree=Try&rev=3b865f4dcb21
https://tbpl.mozilla.org/?tree=Try&rev=7ff721c347c8

Here is revision from August 1st and apply 4 patches which are provided in this bug. All tries are green. So there is something wrong since August 1st.
Apply all patches in this bug on master, and disable touch caret. All green ...
https://tbpl.mozilla.org/?tree=Try&rev=bbb1a64c8b1c
Another try results which have M-10 failure on different test case, but the call stack are the same with Comment 23. Both failures are at the end of the test, which seem like a shutdown crash.

https://tbpl.mozilla.org/?tree=Try&rev=0ca253c83134
Priority: -- → P1
Steps to efficiently reproduce M-10 crash locally:
1) Modify dom/tests/mochitest/bugs/mochitest.ini so that it only contains [test_bug265203.html] and [test_bug346659.html].
2) $ ./mach mochitest-remote dom/tests/mochitest/bugs/

The callstack is attached as CheckAcquire_callstack.txt
latest try run: https://tbpl.mozilla.org/?tree=Try&rev=9e56b6d715e4

Looks like only Cpp test failed. But other tries also failed on this test recently. So it should be ok.
Keywords: checkin-needed
The Cpp test fail is a known issue.
https://bugzilla.mozilla.org/show_bug.cgi?id=1062937
Awesome!  Thanks for all of your hard work on this guys.
This was the backout asked in bug 1066515
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla35 → ---
Depends on: 1082306
Depends on: 1065955
After resolving bug 1066515 and bug 1065955, the performance of word suggestion and correction in message app shouldn't be a problem now. We can enable copy/paste on B2G again.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=08d4f7dfd26f
Keep in mind I have observed there is a timeout failure in test_extend.html test. Like this try push ( https://tbpl.mozilla.org/?tree=Try&rev=248161574608 ). Please see emulator mochitest-5. So, let wait and see if latest try run still have this failure!
The test_extend.html is actually timeout on B2G ICS Emulator opt Mochitest-4 in comment 35 after enabling selection carets. (Treeherder is green though ...) And I found that the way we double the timeout for test_collapse.html in attachment 8478819 [details] [diff] [review] might not be correct. The running time of test_collapse.html is too short, and it makes feel that the test does not run.

I will try rebase those patches, and double the timeout for test_extend.html and test_collapse.html by using SimpleTest.requestLongerTimeout();
Status: REOPENED → ASSIGNED
Per comment 37, if I enable selection caret on b2g, test_extend.html and test_collapse.html will timeout on B2G ICS Emulator. Since we set "explicit_timeout": true in [1], no timeout should be set by  testharness.js. So the only timeout should be set by TestRunner.js. However, my attempt to use SimpleTest.requestLongerTimeout(2); in part3 does not work as comment 42 shown. Simply including SimpleTest.js in the two tests introduces errors.

Aryeh and Ms2ger, I need your advice on how to correctly increase the timeout limit on these two tests.

Disabling tests on b2g should be treated as a last resort, right? Thought some tests had being disabled on b2g in [2].

[1] http://hg.mozilla.org/mozilla-central/annotate/0e631971b841/dom/imptests/testharnessreport.js#l307
[2] http://dxr.mozilla.org/mozilla-central/source/dom/imptests/editing/mochitest.ini
Flags: needinfo?(ayg)
Flags: needinfo?(Ms2ger)
Comment on attachment 8515057 [details] [diff] [review]
Part 3 - Double timeout for test_collapse.html and test_extend.html. (v2)

No, this isn't going to work. Perhaps something like

if (W3CTest.runner) {
  W3CTest.runner.requestLongerTimeout(2);
}

will.
Flags: needinfo?(ayg)
Flags: needinfo?(Ms2ger)
Attachment #8515057 - Flags: review-
test_extend.html and test_collapse.html run in B2G ICS Emulator opt Mochitest-4
and debug Mochitest-10.

Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5991c01071b3
Attachment #8515057 - Attachment is obsolete: true
Attachment #8515605 - Flags: review?(Ms2ger)
Comment on attachment 8515605 [details] [diff] [review]
Part 3 - Double timeout for test_collapse.html and test_extend.html. r=Ms2ger (v3)

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

r=me if it works.
Attachment #8515605 - Flags: review?(Ms2ger) → review+
The try fails in comment 45 seem to be known issues on m-c.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.