829486 - Fire mozbrowserfirstpaint after every mozbrowserloadstart

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[(inactive after 6/18) Alive Kuo [:alive]]

Follow up of bug 811243.
We may need a firstpaint event after every time iframe reloads.

Patrick said he could do this quickly.

Comment 1

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: firing firstpaint event every time a window is created

Sorry I don't submit this patch as quick as I thought. I spent lots of time to install develop environment on my nb :(.

Alive, would you try this patch out?

Comment 2

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Patrick Wang [:kk1fff] from comment #1)
> Created attachment 701415 [details] [diff] [review]
> Patch: firing firstpaint event every time a window is created
> 
> Sorry I don't submit this patch as quick as I thought. I spent lots of time
> to install develop environment on my nb :(.
> 
> Alive, would you try this patch out?

It's O.K. This is not that urgent . And I couldn't try it coz I am at Frankfurt Airport now, anyway.

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 701415 [details] [diff] [review]
Patch: firing firstpaint event every time a window is created

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

Seems WFM!

Comment 4

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: fire firstpaint event every time a window is created v2

Add test case.

Comment 5

[Justin Lebar (not reading bugmail)]

Comment on attachment 702753 [details] [diff] [review]
Patch: fire firstpaint event every time a window is created v2

If this works, I'm cool with it.

But Gaia currently uses mozbrowserfirstpaint in a few places, and we're changing the semantics of that event.

My feeling is that we should leave mozbrowserfirstpaint alone and create a new event, e.g. "mozbrowserdocumentfirstpaint".  If we later convert all of Gaia to using mozbrowserdocumentfirstpaint, we can remove the old mozbrowserfirstpaint.

OTOH, if all of the Gaia code is happy with the changed semantics, I'm happy if we just change the semantics here.  I still think we should change the event name, though, and it's easier to introduce a new event than it is to change the name and change gaia and gecko at the same time.

>+ this._createdHandler.bind(this),

Nit: I know you're just copying this._closeHandler above, but could we call this _windowCreatedHandler instead?  Bonus points if you change _closeHandler too.  :)

Comment 6

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: fire documentfirstpaint event every time a window is created v3

Changed event name to 'documentfirstpaint'. Fix event handler function names of 'DOMWindowCreated' and 'DOMWindowClose' according to previous review comment.

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 704405 [details] [diff] [review]
Patch: fire documentfirstpaint event every time a window is created v3

Sorry, there's a bug in here that I didn't catch earlier.

DOMWindowCreated is a global notification; it tells you whenever /any/ DOM window is created in the process.

But we're only interested here when there's a new DOM window created and its docshell == docShell.

I'm sorry again for not catching this one earlier.

Comment 8

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: fire documentfirstpaint event every time a window is created v4

Check docshell of event target in DOMWindowCreated's handler.

Comment 9

[Justin Lebar (not reading bugmail)]

Comment on attachment 705748 [details] [diff] [review]
Patch: fire documentfirstpaint event every time a window is created v4

> +    let targetDocShell = e.target.defaultView
> +          .QueryInterface(Ci.nsIInterfaceRequestor)
> +          .getInterface(Ci.nsIWebNavigation)
> +          .QueryInterface(Ci.nsIDocShell);

I don't /think/ you need this final QI; would you mind trying without it?

r=me with that change (or without it, if I'm wrong).

Comment 10

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

I have tried without last QI. You are right, it is not necessary.

Comment 11

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41b5ec7f0293

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/41b5ec7f0293

Comment 13

[Justin Lebar (not reading bugmail)]

Patrick, Alive: Do we want this on b2g18, or are we OK with it being on trunk only?

Comment 14

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Justin Lebar [:jlebar] from comment #13)
> Patrick, Alive: Do we want this on b2g18, or are we OK with it being on
> trunk only?

Per talk with Alive, Gaia doesn't need to use it on 18, so I think it is fine to just make it on trunk.