Closed
Bug 829486
Opened 12 years ago
Closed 12 years ago
Fire mozbrowserfirstpaint after every mozbrowserloadstart
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: alive, Assigned: kk1fff)
References
Details
Attachments
(1 file, 3 obsolete files)
9.41 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
Follow up of bug 811243. We may need a firstpaint event after every time iframe reloads. Patrick said he could do this quickly.
Updated•12 years ago
|
Blocks: browser-api
Assignee | ||
Comment 1•12 years ago
|
||
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)
Reporter | ||
Comment 2•12 years ago
|
||
(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.
Reporter | ||
Comment 3•12 years ago
|
||
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+
Assignee | ||
Comment 4•12 years ago
|
||
Add test case.
Attachment #701415 -
Attachment is obsolete: true
Attachment #702753 -
Flags: review?(justin.lebar+bug)
Comment 5•12 years ago
|
||
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)
Assignee | ||
Comment 6•12 years ago
|
||
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 7•12 years ago
|
||
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)
Assignee | ||
Comment 8•12 years ago
|
||
Check docshell of event target in DOMWindowCreated's handler.
Attachment #704405 -
Attachment is obsolete: true
Attachment #705748 -
Flags: review?(justin.lebar+bug)
Comment 9•12 years ago
|
||
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+
Assignee | ||
Comment 10•12 years ago
|
||
I have tried without last QI. You are right, it is not necessary.
Assignee | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/41b5ec7f0293
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/41b5ec7f0293
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment 13•12 years ago
|
||
Patrick, Alive: Do we want this on b2g18, or are we OK with it being on trunk only?
Assignee | ||
Comment 14•12 years ago
|
||
(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.
Description
•