Closed Bug 846906 Opened 11 years ago Closed 11 years ago

Expose a method to create windowless docshells

Categories

(Core :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
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.
Exactly.  That's the bulk of the work: hooking up all the nsIWebBrowser* bits.
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 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+
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)
Attached patch Test (obsolete) — Splinter Review
Test for the latest patch
Attachment #730259 - Flags: review?(bzbarsky)
Attached patch TestSplinter Review
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 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 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 on attachment 730260 [details] [diff] [review]
Test

r=me
Attachment #730260 - Flags: review?(bzbarsky) → review+
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)
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 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+
(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
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
https://hg.mozilla.org/mozilla-central/rev/429e15d02de3
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 1167081
You need to log in before you can comment on or make changes to this bug.