Closed
Bug 846906
Opened 11 years ago
Closed 11 years ago
Expose a method to create windowless docshells
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: ejpbruel, Assigned: ejpbruel)
References
Details
Attachments
(2 files, 2 obsolete files)
8.10 KB,
patch
|
bzbarsky
:
review+
benjamin
:
superreview+
|
Details | Diff | Splinter Review |
3.72 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
For the Add-on SDK, we'd like to be able to create doc shells that are not associated with a particular OS window. Since doc shells require a parent widget, this requires us to implement a widget that has no associated OS window. It looks like TabChild::Init already does what we want to do, by creating an instance of PuppetWidget, and associating that with the doc shell. I'd like to propose exposing a method to JS that duplicates this functionality. Presumably, the best place for this would be in nsIAppShellService.
Assignee | ||
Comment 1•11 years ago
|
||
The way TabChild::Init works is that it creates an instance of nsIWebBrowser and then calls webBrowser->SetContainerWindow(this) to set itself as the container window. To make this work, TabChild needs to implement nsIWebBrowserChrome2. It actually does this, but only provides a stub implementation of the functions in that interface. We need to do something similar to make this work.
Comment 2•11 years ago
|
||
Exactly. That's the bulk of the work: hooking up all the nsIWebBrowser* bits.
Assignee | ||
Comment 3•11 years ago
|
||
Here's my first attempt. It basically does the exact same thing TabChild does, except that I've created my own stub implementation of nsWebBrowserChrome2 (which is also based on TabChild).
Attachment #720156 -
Flags: review?(bzbarsky)
Comment 4•11 years ago
|
||
Comment on attachment 720156 [details] [diff] [review] Implement nsAppShellService::CreateWindowlessDocshell >+ nsCOMPtr<nsIWebNavigation> navigation = do_QueryInterface(browser); This is unused as far as I can tell. Why is it needed? >+ nsCOMPtr<nsIWidget> widget = nsIWidget::CreatePuppetWidget(NULL); nullptr, not NULL. >+ *aResult = shell; shell.forget(aResult); or else you'll get crashes. r=me with those fixed, but how was this tested? Seems like in practice you may need various other interfaces on here too (e.g. nsIWebBrowserChromeFocus or nsIWindowProvider). I guess we can see if it's needed in practice...
Attachment #720156 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 5•11 years ago
|
||
Essentially the same patch as the previous one (with comments by bz addressed). The only difference is that we're returning nsWebBrowser instead of nsDocShell. This change was necessary because nsWebBrowser calls nsDocShell::Destroy when it is destroyed.
Attachment #720156 -
Attachment is obsolete: true
Attachment #730257 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 6•11 years ago
|
||
Test for the latest patch
Attachment #730259 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•11 years ago
|
||
Overlooked a line in Makefile.in which would make this test not compile on any machine but my own. Oops :-)
Attachment #730259 -
Attachment is obsolete: true
Attachment #730259 -
Flags: review?(bzbarsky)
Attachment #730260 -
Flags: review?(bzbarsky)
Comment 8•11 years ago
|
||
Comment on attachment 730257 [details] [diff] [review] Implement nsAppShellService::CreateWindowlessDocshell (v2) >+ nsCOMPtr<nsIWebNavigation> navigation = do_QueryInterface(browser); This is unused as far as I can tell. Why is it needed? (Take two.) I'm not sure what I think of returning an nsIWebBrowser. That seems like an implementation detail... Can you return an nsIWebNavigation instead? r=me with that.
Attachment #730257 -
Flags: review?(bzbarsky) → review+
Comment 9•11 years ago
|
||
Comment on attachment 730257 [details] [diff] [review] Implement nsAppShellService::CreateWindowlessDocshell (v2) But also, maybe the right thing to return is some new interface that will have a .docShell or whatever on it and will keep all the needed bits alive....
Comment 10•11 years ago
|
||
Comment on attachment 730260 [details] [diff] [review] Test r=me
Attachment #730260 -
Flags: review?(bzbarsky) → review+
Comment 11•11 years ago
|
||
Comment on attachment 730257 [details] [diff] [review] Implement nsAppShellService::CreateWindowlessDocshell (v2) This is a case where I think we could actually use an sr.... Benjamin, any thoughts on what the right return type here is?
Attachment #730257 -
Flags: superreview?(benjamin)
Comment 12•11 years ago
|
||
Given that we're shipping the addon SDK in-product now, I'm not extremely worried about getting a perfect API. That said, I do think nsIWebNavigation is a better existing "holder" class than nsIWebBrowser, and so the API should probably return that.
Comment 13•11 years ago
|
||
Comment on attachment 730257 [details] [diff] [review] Implement nsAppShellService::CreateWindowlessDocshell (v2) So I don't know whether to mark this sr- or sr+, given that comment, but I'm going to say "sr+ with the interface changed to nsIWebNavigation and the method name changed to createWindowlessBrowser".
Attachment #730257 -
Flags: superreview?(benjamin) → superreview+
Assignee | ||
Comment 14•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/061b9318815b
Comment 15•11 years ago
|
||
(In reply to Eddy Bruel [:ejpbruel] from comment #14) > https://hg.mozilla.org/integration/mozilla-inbound/rev/061b9318815b This managed to remove mach along the way. Backed out and re-landed. https://hg.mozilla.org/integration/mozilla-inbound/rev/d5d98779e401
Comment 16•11 years ago
|
||
And backed out again for test failures. http://hg.mozilla.org/integration/mozilla-inbound/rev/a199d6b86acb https://tbpl.mozilla.org/php/getParsedLog.php?id=21431566&tree=Mozilla-Inbound 07:37:38 INFO - 3573 INFO TEST-START | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul 07:37:38 INFO - 3574 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | Should be able to get app shell service 07:37:38 INFO - 3575 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | Should be able to create windowless browser 07:37:38 INFO - 3576 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | Should be able to query interface requestor interface 07:37:38 INFO - 3577 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | Should be able to get doc shell interface 07:37:38 INFO - 3578 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | Should be able to query web navigation interface 07:37:38 INFO - 3579 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | Should be able to get document 07:37:38 INFO - 3580 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | Should be able to create iframe 07:37:38 INFO - 3581 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | Should receive initial onload event 07:37:38 INFO - 3582 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | Should receive onload event 07:37:38 INFO - 3583 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | Should be able to get content document 07:37:38 INFO - 3584 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | Should be able to get element by id 07:37:38 INFO - 3585 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | Should be able to get bounding client rect 07:37:38 INFO - 3586 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | undefined 07:37:38 INFO - 3587 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | undefined 07:37:38 INFO - 3588 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | undefined 07:37:38 INFO - 3589 INFO TEST-PASS | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | undefined 07:37:38 INFO - 3590 INFO TEST-END | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug846906.xul | finished in 61ms
Assignee | ||
Comment 17•11 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/429e15d02de3
Comment 18•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/429e15d02de3
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•