Closed Bug 750901 (metro-widget) Opened 8 years ago Closed 7 years ago

Review and land metro related widget code on mc

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [metro-mvp][LOE:-][completed-elm][metro-it2])

Attachments

(3 files, 2 obsolete files)

No description provided.
Attached patch link xullib winrt module v.1 (obsolete) — Splinter Review
Attached patch xre changes v.1 (obsolete) — Splinter Review
Depends on: 786979
Whiteboard: metro-beta
Whiteboard: metro-beta → [metro-mvp]
Whiteboard: [metro-mvp] → [metro-mvp][LOE:-]
Whiteboard: [metro-mvp][LOE:-] → metro-mvp, LOE:-, leave-open
Whiteboard: metro-mvp, LOE:-, leave-open → metro-mvp, LOE:-, leave-open, completed-elm
Assignee: nobody → jmathies
Attachment #622712 - Attachment is obsolete: true
Attachment #626890 - Attachment is obsolete: true
Comment on attachment 677355 [details] [diff] [review]
[landed] top level widget changes

Top level widget changes for metro. Might as well get these changes in to limit differences.

pushed to try:
https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=56e9f5b64735
Attachment #677355 - Flags: review?(netzen)
Comment on attachment 677355 [details] [diff] [review]
[landed] top level widget changes

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

::: widget/windows/AudioSession.h
@@ +7,5 @@
> +#include "nscore.h"
> +#include "nsdefs.h"
> +#include "prlog.h"
> +#include "nsAutoPtr.h"
> +#include "nsString.h"

I don't think these are needed here, if so I'd prefer to remove them from elm instead

::: widget/windows/TaskbarPreview.h
@@ +15,5 @@
>  #include <nsAutoPtr.h>
>  #include <nsString.h>
>  #include <nsWeakPtr.h>
>  #include <nsIDocShell.h>
> +#include <gfxWindowsSurface.h>

Ditto is this needed?

::: widget/windows/WinMouseScrollHandler.cpp
@@ +112,5 @@
>  void
>  MouseScrollHandler::Shutdown()
>  {
> +  if (sInstance)
> +    delete sInstance;

Deleting a null pointer has no effect so this change isn't needed.

::: widget/windows/nsAppShell.h
@@ +30,5 @@
>  
>    static UINT GetTaskbarButtonCreatedMessage();
>  
> +#ifdef MOZ_METRO
> +  void NativeCallback();

Where is this implementation?

::: widget/windows/nsWidgetFactory.cpp
@@ +99,5 @@
> +}
> +
> +static nsresult
> +FilePickerConstructor(nsISupports *aOuter, REFNSIID aIID,
> +                      void **aResult)

All of this was only needed because of out of process stuff :( ah well.
Best to leave it as is for now.

::: widget/xpwidgets/nsAppShellSingleton.h
@@ +38,5 @@
>  nsAppShellInit()
>  {
>    NS_ASSERTION(!sAppShell, "already initialized");
>  
> +#if !defined(MOZ_METRO) || !defined(XP_WIN)

When would this ever be !defined(XP_WIN) I think it can be simplified to:
#if !defined(MOZ_METRO)

@@ +52,5 @@
>      return NS_ERROR_OUT_OF_MEMORY;
>    NS_ADDREF(sAppShell);
>  
> +  nsresult rv;
> +#if !defined(MOZ_METRO) || !defined(XP_WIN)

ditto re XP_WIN
Attachment #677355 - Flags: review?(netzen) → review+
(In reply to Brian R. Bondy [:bbondy] from comment #5)
> Comment on attachment 677355 [details] [diff] [review]
> top level widget changes
> 
> Review of attachment 677355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/AudioSession.h
> @@ +7,5 @@
> > +#include "nscore.h"
> > +#include "nsdefs.h"
> > +#include "prlog.h"
> > +#include "nsAutoPtr.h"
> > +#include "nsString.h"
> 
> I don't think these are needed here, if so I'd prefer to remove them from
> elm instead
> 
> ::: widget/windows/TaskbarPreview.h
> @@ +15,5 @@
> >  #include <nsAutoPtr.h>
> >  #include <nsString.h>
> >  #include <nsWeakPtr.h>
> >  #include <nsIDocShell.h>
> > +#include <gfxWindowsSurface.h>
> 
> Ditto is this needed?

These were originally in there due to compile problems, but I'll pull them and see.

> ::: widget/windows/nsAppShell.h
> @@ +30,5 @@
> >  
> >    static UINT GetTaskbarButtonCreatedMessage();
> >  
> > +#ifdef MOZ_METRO
> > +  void NativeCallback();
> 
> Where is this implementation?

I think this can go away, it's down in MetroAppShell now.

> 
> ::: widget/windows/nsWidgetFactory.cpp
> @@ +99,5 @@
> > +}
> > +
> > +static nsresult
> > +FilePickerConstructor(nsISupports *aOuter, REFNSIID aIID,
> > +                      void **aResult)
> 
> All of this was only needed because of out of process stuff :( ah well.
> Best to leave it as is for now.

We need this, since we compile both classes into xul.dll and have to chose between the two dynamically based on the run type.
re: the widget factory stuff: oops ya, just read the code, and yup :)
Attachment #677355 - Attachment description: top level widget changes → [landed] top level widget changes
Poking through this patch I noticed one update that shouldn't have occurred. On elm we had a line commented out in nsClipboard that I assumed was our own change due to the lack of an HWND for MetroWidget - 

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac4bb259db3#l11.41

Turns out this commented out line has been around since the beginning of time -

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/windows/nsClipboard.cpp&rev=1.109 
line 334

This patch cleans this line up, and restores it to the original null value.
Attachment #677735 - Flags: review?(netzen)
Comment on attachment 677735 [details] [diff] [review]
[landed] nsClipboard fix

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

Ya I was wondering about that, but thought it must have been changed in TimA's clipboard bug.
Attachment #677735 - Flags: review?(netzen) → review+
Comment on attachment 677735 [details] [diff] [review]
[landed] nsClipboard fix

https://hg.mozilla.org/integration/mozilla-inbound/rev/b095fe02009e
Attachment #677735 - Attachment description: nsClipboard fix → [landed] nsClipboard fix
Depends on: 789050
Whiteboard: metro-mvp, LOE:-, leave-open, completed-elm → [metro-mvp][LOE:-][leave-open][completed-elm]
Whiteboard: [metro-mvp][LOE:-][leave-open][completed-elm] → [metro-mvp][LOE:-][leave-open][completed-elm][metro-it2]
Alias: metro-widget
This will get refreshed before the final checkin.
Attachment #712442 - Flags: review?(netzen)
Attachment #712442 - Flags: review?(netzen) → review+
https://hg.mozilla.org/mozilla-central/rev/5f0882ee58c0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [metro-mvp][LOE:-][leave-open][completed-elm][metro-it2] → [metro-mvp][LOE:-][completed-elm][metro-it2]
Note for future historians:  This code was originally developed on the "elm" disposable project branch.  We did not merge the full history into mozilla-central because much of it was not very clean and because of merge problems caused by hg copy relationships on elm, but you can find the complete history preserved here:

http://hg.mozilla.org/users/mbrubeck_mozilla.com/elm-metro-firefox/
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.