Closed Bug 970777 Opened 10 years ago Closed 10 years ago

Remove redundant |numLoaded| checks in dom/browser-element/mochitests/*.js

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: timdream, Assigned: CuveeHsu)

Details

(Whiteboard: [good first bug][mentor=timdream][mentor-lang=zh][lang=js])

Attachments

(1 file, 1 obsolete file)

remove_redundant_numloaded.patch
5.42 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
The mozbrowser mochitests that create iframes currently wait for two mozbrowserloadend events. The first mozbrowserloadend event loads about:blank, and we can actually avoid loading that page by set iframe.src *before* appendChild() the iframe to the DOM, not after.

The assignee of this bug is expected to identify the test JavaScript files with this code pattern, move the appendChild() line, remove |numLoaded|. It would also be better if the assignee can run mochitests locally [1] to verify her/his fix, and also push to try [2] to validate her/his work on all platforms (I can push it too if the bug taker doesn't have level 1).

[1] https://developer.mozilla.org/en/docs/Mochitest
[2] https://wiki.mozilla.org/ReleaseEngineering/TryServer

See also related discussion on bug 959066 comment 10 and onward.
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
hai Tim , 
     Can u guide me on this bug :)
Sure, no problem!
Assignee: nobody → lviknesh
Flags: needinfo?(lviknesh)
Hai Tim , 
  Sorry for not replying , i am sorry to say i cant work on this i am not good in mochitest :(
Flags: needinfo?(lviknesh)
It's OK. Feel free to find a better bug.
Assignee: lviknesh → nobody
Hi Tim,
I'm looking for simple bugs in order to be familiar with DOM,
and I'd like to take this.

And I am confused with the following fact:

http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/mochitest/browserElement_GetScreenshot.js#142
As you mentioned, the |iframeLoadedHandler| is called twice.

However, |iframe1.src| are not |about:blank| when I try to figure out what the first event is.
Could you please give me some hint to find out the fact?
Assignee: nobody → juhsu
Flags: needinfo?(timdream)
One more idea is:
I'd like to make #38 invoked right after the |iframe1| is onload again.
Therefore, I can remove the |filter| argument from |waitForScreenshot|.
(In reply to Junior Hsu from comment #5)
> However, |iframe1.src| are not |about:blank| when I try to figure out what
> the first event is.
> Could you please give me some hint to find out the fact?

Could you tell me what's two |src|s when the events happens?
Flags: needinfo?(timdream)
Both data:text/html,<html><body%20style="background:green">hello</body></html>
Flags: needinfo?(timdream)
(In reply to Junior Hsu from comment #8)
> Both
> data:text/html,<html><body%20style="background:green">hello</body></html>

OK, it seems that mozbrowser API is doing something strange.

For the purpose of this bug, let's still try to move |src| after |appendChild|, and see how many events you have received. If there is only one, this bug is still valid (and you should patch all tests).
Flags: needinfo?(timdream)
Attached patch remove_redundant_numloaded.patch (obsolete) — — Splinter Review 
At first, I modify one test case.

Moreover, I postpone the second |takeScreenshot| right after the |iframe1| is onload again.
Therefore, I could make the test more solid.

try result: https://tbpl.mozilla.org/?tree=Try&rev=c30d83bef4d2
Attachment #8419312 - Flags: feedback?(timdream)
Comment on attachment 8419312 [details] [diff] [review]
remove_redundant_numloaded.patch

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

If you do |ok(false)| test will fail immediately. There is no need to tryAgain() later on.

tryAgain() and waitForScreenshot() was copied from another test. If we don't need to do that, it's ok to clean that up too.
:smaug should be the reviewer of your patch so I am setting feedback? to him.

I think my mentor is done here actually :) You are doing very well.
Attachment #8419312 - Flags: feedback?(timdream)
Attachment #8419312 - Flags: feedback?(bugs)
Attachment #8419312 - Flags: feedback+
Comment on attachment 8419312 [details] [diff] [review]
remove_redundant_numloaded.patch

Looks ok to me.

In general when fixing tests it is best to keep changes at minimum.
Attachment #8419312 - Flags: feedback?(bugs) → feedback+
For comment 8  "(and you should patch all tests)."

I try to find other test cases with the pattern but in vain.
|browserElement_GetScreenshot.js| is the only .js with |numLoaded| symbol.

I try to use the following commands to find candidates and there's only one |browserElement_DocumentFirstPaint.js|

egrep -z "mozbrowserloadend.*appendChild.*src" *  | tr '[\000-\011\013-\037\177-\377]' '.' | grep ".js"
egrep -z "appendChild.*src.*mozbrowserloadend" *  | tr '[\000-\011\013-\037\177-\377]' '.' | grep ".js"

Maybe my last task is to look into |browserElement_DocumentFirstPaint.js|?
Flags: needinfo?(timdream)
It seems other tests have since been re-written already.
Flags: needinfo?(timdream)
Update according to feedback comments.

try result: https://tbpl.mozilla.org/?tree=Try&rev=f100bf5237fd
Attachment #8419312 - Attachment is obsolete: true
Attachment #8419993 - Flags: review?(bugs)
Attachment #8419993 - Flags: review?(bugs) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/680d2e36db7b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: