974901 - Cleanup static variable use in winrt widget

Status: VERIFIED FIXED

(Core Graveyard :: Widget: WinRT, defect, P1)

Description

[Jim Mathies [:jimm]]

Attachment: patch

Something I've been meaning to do for a while, this work gave me an excuse to do it.

I'm not 100% clear yet on the relationship between the base FrameworkView and the views we'll create for multiple windows. I think though they will be the same interface/class, in which case this static variable is going to get confusing. So I wanted to eliminate the use of it as much as possible.

Comment 1

[Jim Mathies [:jimm]]

Comment on attachment 8378992 [details] [diff] [review]
patch

I'm going to keep most of this, except I think I'll revert the global FrameworkView pointer. Having that as a member variable of MetroApp is going to make life harder in bug 974989 since MetroAppChild will want access to it.

Comment 2

[Jim Mathies [:jimm]]

Attachment: wip

I think this is pretty much complete.

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8379810 [details] [diff] [review]
wip

Review of attachment 8379810 [details] [diff] [review]:
-----------------------------------------------------------------

Cleanup/organization work is never done, but this looks like a step in the right direction :)

::: widget/windows/winrt/MetroApp.h
@@ +58,5 @@
>  
>  private:
>    EventRegistrationToken mSuspendEvent;
>    EventRegistrationToken mResumeEvent;
> +  Microsoft::WRL::ComPtr<FrameworkView> mView;

Is this used anywhere?

::: widget/windows/winrt/nsWinMetroUtils.cpp
@@ +247,5 @@
>  
>  NS_IMETHODIMP
>  nsWinMetroUtils::GetActivationURI(nsAString &aActivationURI)
>  {
> +  if (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Desktop) {

I don't really know the semantics of XRE_GetWindowsEnvironment, but should we be checking "not metro" instead of "windows desktop?" What does this do on --metrodesktop, or on Linux?

@@ +274,5 @@
>  
>  NS_IMETHODIMP
>  nsWinMetroUtils::GetKeyboardX(uint32_t *aX)
>  {
> +  *aX = (uint32_t)floor(FrameworkView::KeyboardVisibleRect().X);

While we're here, can we change this and the below casts to |static_cast|s?

Comment 4

[Jim Mathies [:jimm]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #3)
> Comment on attachment 8379810 [details] [diff] [review]
> wip
> 
> Review of attachment 8379810 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cleanup/organization work is never done, but this looks like a step in the
> right direction :)
> 
> ::: widget/windows/winrt/MetroApp.h
> @@ +58,5 @@
> >  
> >  private:
> >    EventRegistrationToken mSuspendEvent;
> >    EventRegistrationToken mResumeEvent;
> > +  Microsoft::WRL::ComPtr<FrameworkView> mView;
> 
> Is this used anywhere?

no it isn't. looks like it leaked into a different patch - 

https://bugzilla.mozilla.org/attachment.cgi?id=8379813&action=diff

I'll fix this up here and update the other.

> ::: widget/windows/winrt/nsWinMetroUtils.cpp
> @@ +247,5 @@
> >  
> >  NS_IMETHODIMP
> >  nsWinMetroUtils::GetActivationURI(nsAString &aActivationURI)
> >  {
> > +  if (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Desktop) {
> 
> I don't really know the semantics of XRE_GetWindowsEnvironment, but should
> we be checking "not metro" instead of "windows desktop?" What does this do
> on --metrodesktop, or on Linux?

There's only two values, desktop and metro. w/metrodesktop it would be desktop. We did it this way to provide a cleaner early exit, for example - 

http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/nsWinMetroUtils.cpp#118

vs. a big if block.

> 
> @@ +274,5 @@
> >  
> >  NS_IMETHODIMP
> >  nsWinMetroUtils::GetKeyboardX(uint32_t *aX)
> >  {
> > +  *aX = (uint32_t)floor(FrameworkView::KeyboardVisibleRect().X);
> 
> While we're here, can we change this and the below casts to |static_cast|s?

no problem!

Comment 5

[Jim Mathies [:jimm]]

Oh I see your point, != metro vs. == desktop in case we add a run type. I like that, I think I'll update all of them.

Comment 6

[Jim Mathies [:jimm]]

try push:
https://tbpl.mozilla.org/?tree=Try&showall=0&rev=44adaa10ec9b

Comment 7

[Jim Mathies [:jimm]]

try:
https://tbpl.mozilla.org/?tree=Try&showall=0&rev=ae65e2f08589

I had to make one change, HString is a winrt class that imports some win8 specific gunk when initialized. So I had to convert that navigation parameter to an HSTRING handle.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e403f6dd4947

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e403f6dd4947

Comment 9

[Marco Mucci [:MarcoM]]

For testing and verification.  Reopen if any defects found.

Comment 10

[Kamil Jozwiak [:kjozwiak]]

Talked Jim via IRC and made sure there wasn't anything that needed to be testing in this issue. Issue was basically cleaning up internal code. Marking as verified and [qa-].