Implement metro widgets for input thread separation

RESOLVED WONTFIX

Status

defect
P1
normal
RESOLVED WONTFIX
5 years ago
26 days ago

People

(Reporter: jimm, Unassigned)

Tracking

Trunk
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=0)

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
Posted patch wip (obsolete) — Splinter Review
TODO:
  - implement required nsIWidget method stubs
  - add override stubs we use in MetroWidget
  - figure out if we can use MetroWidget for MetroWidgetInput without
    breaking non thread sep functionality. We might just go ahead and
    copy code over to a new MetroWidgetInput class. Probably simpler.
  - deal with tricky sFrameworkView calls on the gecko side in MetroApp
  - implement communication between MetroWidgetGecko and MetroWidgetInput
  - get MetroWidgetInput talking to the apzc
  - get MetroInput talking to MetroWidgetInput
  - test MetroApp browser shutdown sequence
(Reporter)

Updated

5 years ago
Depends on: 974989
(Reporter)

Comment 1

5 years ago
Tim is this something you could pick up and run with, or would you prefer I take this a bit further?
Blocks: 911133
Flags: needinfo?(tabraldes)
Whiteboard: p=12
Yeah I'd be super happy to sink my teeth into this!
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
Flags: needinfo?(tabraldes)
(Reporter)

Comment 3

5 years ago
One helpful tip - when you build this up you can find the ipdl generated headers here -

obj\ipc\ipdl\_ipdlheaders\mozilla\widget\winrt

you'll need the patches in bug 974901 & bug 974989. if those don't apply cleanly ping me on irc.
(Reporter)

Comment 4

5 years ago
One other tip - we had talked about how input events between the two widgets would transfer from one side to the other as gecko events. Thankfully we already have gecko event serializers for these which saves us a ton of work - 

http://mxr.mozilla.org/mozilla-central/source/widget/nsGUIEventIPC.h
(Reporter)

Comment 5

5 years ago
Posted patch wip (obsolete) — Splinter Review
Updated to get things building.

I think we should limit the work in this bug to getting the plumbing fleshed out and the browser painting. The more complex interfaces between the widgets (ime, input) and things like apzc support can get fleshed out in other bugs.
Attachment #8379814 - Attachment is obsolete: true
(Reporter)

Comment 6

5 years ago
I've worked around the original issue with the constructor, but I've come up on something else that's more serious. We attempt to connect when the first widget gets created, but this happens before nsAppShell::Run is called and as such the gecko thread isn't in a chromium message loop yet. So when the input thread attempts to connect, there's no loop set up on the other side. Not sure yet how to fix this.
(Reporter)

Comment 7

5 years ago
(Reporter)

Comment 8

5 years ago
Posted patch combined wip (obsolete) — Splinter Review
This combines the winrt code from bug 974989 plus all the changes here.
Attachment #8380204 - Attachment is obsolete: true
Attachment #8382428 - Attachment is obsolete: true
(Reporter)

Comment 9

5 years ago
I was thinking about the idle service problem, I think the best solution in cases like this would be to check to see if we're running on the main thread before accessing the main thread resource. If we are, make the call, if we aren't bounce the call to the main thread using an nsRunnable class.

We might find a use for a common set of nsRunnables included via a common header, or maybe this problem isn't that common.
(Reporter)

Updated

5 years ago
Depends on: 977546
(Reporter)

Comment 10

5 years ago
Breaking these up so they are more manageable.
Attachment #8382462 - Attachment is obsolete: true
(Reporter)

Comment 11

5 years ago
Posted patch widget wip (obsolete) — Splinter Review
(Reporter)

Comment 12

5 years ago
(In reply to Jim Mathies [:jimm] from comment #9)
> I was thinking about the idle service problem, I think the best solution in
> cases like this would be to check to see if we're running on the main thread
> before accessing the main thread resource. If we are, make the call, if we
> aren't bounce the call to the main thread using an nsRunnable class.
> 
> We might find a use for a common set of nsRunnables included via a common
> header, or maybe this problem isn't that common.

Note this won't solve the case where we need a return result, and as it turns out I found another of these in nsBaseWidget::BaseCreate, which initializes a new nsDeviceContext which creates a nsIScreenManager.

We'll have to take each of these on individually to get a feel for how wide spread the problem is.
Blocks: metrobacklog
Whiteboard: p=12 → p=0
Updated to apply cleanly over 170933:4cfb6c61b137
Attachment #8383035 - Attachment is obsolete: true
Updated to apply cleanly over the part 1 patch
Attachment #8383037 - Attachment is obsolete: true
Priority: -- → P1
This patch contains miscellaneous fixes that I've been using to get metroFx running.

1) It enables the metroFx debug delay
2) It comments out most of MetroUtils::ScaleFactor and just returns 1.0 - WinRT was throwing a RoOriginateError when we tried to do "dispInfoStatics->GetForCurrentView(&dispInfo)"
3) It makes MetroWidget::AnswerLateCreate create and initialize an nsDeviceContext, then pass that in to the call to |Create|. I think we'll actually want to keep this change.
4) It expands the macros that were used to implement AddRef, QI, and Release for nsScreenManagerWin. I'm not sure why, but when the macros were being used I was seeing an exception thrown in AddRef and the debugger was reporting that I was in nsSimpleArrayEnumerator::AddRef.

Next to tackle: A RoOriginateError is being thrown from FrameworkView::ActivateView when called from MetroApp::ActivateBaseView. This looks like it might be happening on the wrong thread (FrameworkView probably shouldn't be accessed from the gecko thread)
(Reporter)

Comment 16

5 years ago
> 4) It expands the macros that were used to implement AddRef, QI, and Release
> for nsScreenManagerWin. I'm not sure why, but when the macros were being
> used I was seeing an exception thrown in AddRef and the debugger was
> reporting that I was in nsSimpleArrayEnumerator::AddRef.

xpcom interfaces that aren't thread safe (most of them) assert if the thread accessing them isn't the thread that creates the object - 
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#54
I'd bet this is the source of the assertions.
(Reporter)

Comment 17

5 years ago
Yeah I bet the assert is in the component manager somewhere when the input thread tries to create the screen manager via do_GetService.

This is the heart of the problem we'll have to solve. MetroWidget(input) can't access non-thread safe xpcom interfaces directly. We'll have to find a way to bypass these types of calls. I think you can get away with just stubbing stuff like screen manager out in base widget. MetroWidget(input) doesn't need it, so there's no point in initializing or accessing it.

MetroWidgetGecko on the other hand will have to call BaseCreate and create a valid screen manager. We have to look at these interfaces and figure out what they do - if they query for system information that can happen on the gecko thread, great. If they query for information only the input thread can access, we'll need our own version of these objects that intr calls over to a corresponding input thread class.

Both the MetroApp and MetroWidget protocols can create additional protocols (objects) when we need them.
(Reporter)

Comment 18

5 years ago
If you go the root you were thinking of taking, landing this all at once, MetroWidget doesn't need to inherit from base widget or implement nsIWidget, which solves the screen manager problem and other potential xpcom issues in that class.
I don't expect to work on metroFx in the near future, though it would be fun to try to use the lessons/approach here for desktopFx.
Assignee: tabraldes → nobody
Status: ASSIGNED → NEW
We never shipped the metro support, closing!
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → WONTFIX

Updated

26 days ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.