Fire mozbrowserfirstpaint after every mozbrowserloadstart

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: alive, Assigned: kk1fff)

Tracking

(Blocks: 1 bug)

unspecified
ARM
Gonk (Firefox OS)
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

Patrick said he could do this quickly.
Blocks: 693515
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?
Attachment #701415 - Flags: feedback?(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 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!
Attachment #701415 - Flags: feedback?(alive) → feedback+
Created attachment 702753 [details] [diff] [review]
Patch: fire firstpaint event every time a window is created v2

Add test case.
Attachment #701415 - Attachment is obsolete: true
Attachment #702753 - Flags: review?(justin.lebar+bug)
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.  :)
Attachment #702753 - Flags: review?(justin.lebar+bug)
Created attachment 704405 [details] [diff] [review]
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.
Attachment #702753 - Attachment is obsolete: true
Attachment #704405 - Flags: review?(justin.lebar+bug)
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.
Attachment #704405 - Flags: review?(justin.lebar+bug)
Created attachment 705748 [details] [diff] [review]
Patch: fire documentfirstpaint event every time a window is created v4

Check docshell of event target in DOMWindowCreated's handler.
Attachment #704405 - Attachment is obsolete: true
Attachment #705748 - Flags: review?(justin.lebar+bug)
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).
Attachment #705748 - Flags: review?(justin.lebar+bug) → review+
I have tried without last QI. You are right, it is not necessary.
https://hg.mozilla.org/integration/mozilla-inbound/rev/41b5ec7f0293
https://hg.mozilla.org/mozilla-central/rev/41b5ec7f0293
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Patrick, Alive: Do we want this on b2g18, or are we OK with it being on trunk only?
(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.
You need to log in before you can comment on or make changes to this bug.