807593 - (metro-misc) [tracking] mc / elm diff issues

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Jim Mathies [:jimm]]

Filing this for tracking misc. issues found while diffing mc and elm.

Comment 1

[Jim Mathies [:jimm]]

Attachment: gfx differences

I think there's one open bug on the error handling. For the rest it looks like we need to land some updates on mc.

Comment 2

[Jim Mathies [:jimm]]

Attachment: gfx differences

additional gfx files.

Comment 3

[Jim Mathies [:jimm]]

Attachment: accessible

I believe these changes can come out, they are obsolete now that we have an hwnd for MetroWidget. Although some testing is needed to be sure allowing accessible access to the hwnd doesn't break something.

Comment 4

[Jim Mathies [:jimm]]

Attachment: browserdom

Looks like this needs to land on mc.

Comment 5

[Jim Mathies [:jimm]]

Attachment: huffdec

This can probably come out, needs to be tested first.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 677329 [details] [diff] [review]
accessible

https://hg.mozilla.org/projects/elm/rev/f2ffb7efbbbc

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 677342 [details] [diff] [review]
huffdec

https://hg.mozilla.org/projects/elm/rev/d858b6e4e78e

Comment 8

[Jim Mathies [:jimm]]

Attachment: [landed] misc. toolkit lib makefile differences

Comment 9

[Brian R. Bondy [:bbondy]]

Comment on attachment 677328 [details] [diff] [review]
gfx differences

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

I can't review this, but I can sync up the changes either to m-c or elm soon.

Comment 10

[Brian R. Bondy [:bbondy]]

Comment on attachment 677383 [details] [diff] [review]
[landed] misc. toolkit lib makefile differences

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

::: toolkit/library/Makefile.in
@@ +17,5 @@
>  FORCE_SHARED_LIB = 1
>  MOZILLA_INTERNAL_API = 1
>  
>  ifdef MOZ_METRO
> +ifeq ($(OS_ARCH),WINNT)

Maybe we should just give an error in configure.in if we don't already have one in this case.

@@ +603,5 @@
>    -DELAYLOAD:comdlg32.dll \
>    -DELAYLOAD:winspool.drv \
>    -DELAYLOAD:secur32.dll \
>    -DELAYLOAD:wininet.dll \
> +  -DELAYLOAD:uiautomationcore.dll \

Should this be conditionally delay loaded only with MOZ_METRO?

Comment 11

[Jim Mathies [:jimm]]

> ::: toolkit/library/Makefile.in
> @@ +17,5 @@
> >  FORCE_SHARED_LIB = 1
> >  MOZILLA_INTERNAL_API = 1
> >  
> >  ifdef MOZ_METRO
> > +ifeq ($(OS_ARCH),WINNT)
> 
> Maybe we should just give an error in configure.in if we don't already have
> one in this case.

mbrubeck added this so he could compile MOZ_METRO bits on other platforms.

> 
> @@ +603,5 @@
> >    -DELAYLOAD:comdlg32.dll \
> >    -DELAYLOAD:winspool.drv \
> >    -DELAYLOAD:secur32.dll \
> >    -DELAYLOAD:wininet.dll \
> > +  -DELAYLOAD:uiautomationcore.dll \
> 
> Should this be conditionally delay loaded only with MOZ_METRO?

Does effect the build since the linker will just ignore it.

Comment 12

[Jim Mathies [:jimm]]

*doesn't effect

Comment 13

[Brian R. Bondy [:bbondy]]

Comment on attachment 677383 [details] [diff] [review]
[landed] misc. toolkit lib makefile differences

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

Sounds good thanks for the answers.

Comment 14

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #10)
> Should this be conditionally delay loaded only with MOZ_METRO?

I went ahead and moved this down into the moz_metro block below. I was just being sloppy.

Comment 15

[Jim Mathies [:jimm]]

Comment on attachment 677383 [details] [diff] [review]
[landed] misc. toolkit lib makefile differences

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

Comment 16

[Jim Mathies [:jimm]]

updates to elm - 
https://hg.mozilla.org/projects/elm/rev/a70cec0475ee

Comment 17

[Phil Ringnalda (:philor)]

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

Comment 18

[Jim Mathies [:jimm]]

Attachment: [landed] app runner changes

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 678263 [details] [diff] [review]
[landed] app runner changes

https://hg.mozilla.org/integration/mozilla-inbound/rev/31784b0d6334

Comment 20

[Jim Mathies [:jimm]]

sync 'app runner changes' review comments to elm - 

https://hg.mozilla.org/projects/elm/rev/2afec7dbfd58

Comment 21

[Jim Mathies [:jimm]]

Things are looking better now, we have a few areas left, most have specific bugs related to the changes.

- gfx differences: posted here, differences look pretty tame and should be easy to mesh up.
- nsIBrowserDOMWindow: posted here, not sure where the implementation is for this. - tests framework: (bug 771271) I'm going to get the latest patches posted today and kick off reviews.
- packager: bug metro-build and children.
- directory svcs: bug 791694 is the primary bug, there are some related bugs as well.
- observer events: bug 795887
- browser/metro/*
- widget/windows/winrt/*

Comment 22

[Brian R. Bondy [:bbondy]]

I can handle hte nsIBrowserDOMWindow and gfx ones, but probably not today because I'm working on a B2G updater task.

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/31784b0d6334

Comment 24

[Brian R. Bondy [:bbondy]]

I'll work on the remaining items this week.

Comment 25

[Brian R. Bondy [:bbondy]]

Comment on attachment 677331 [details] [diff] [review]
browserdom

Canceling feedback flag.  This is being cleaned up in bug 815172

Comment 26

[Brian R. Bondy [:bbondy]]

Comment on attachment 677328 [details] [diff] [review]
gfx differences

Obsoleting this differences patch since most of these changes were reverted in bug 815177 and the rest are covered in bug 777703.

Comment 27

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/projects/elm/rev/10db67c36550

Comment 28

[Brian R. Bondy [:bbondy]]

Had to back this out, not doing a clean build bit me. 
I think widget is built before browser so it doesn't find MetroUIUtils.h when doing a clean build.

I think maybe I'll have to move that js component inside widget?  Any other suggestion?  I'm not sure how else to access PanelUI for showing panels from C++ code.

Comment 29

[Jim Mathies [:jimm]]

We'll reopen if needed.

Comment 30

[Jim Mathies [:jimm]]

Attachment: nsbrowserapp diff

This was already reviewed in another bug, but some change were introduces due to metro build. One change was better handling of utf8 strings.

Comment 31

[Jim Mathies [:jimm]]

Attachment: nsbrowserapp diff

missed some..

Comment 32

[Jim Mathies [:jimm]]

Attachment: nsbrowserapp diff

one more update.

Comment 33

[Brian R. Bondy [:bbondy]]

Comment on attachment 712128 [details] [diff] [review]
nsbrowserapp diff

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

::: browser/app/nsBrowserApp.cpp
@@ +292,5 @@
> +      }
> +      CloseHandle(hTestFile);
> +
> +      // Build new args array
> +      char* newArgv[20];

It would be good to log an error if we end up trimming the args below due to array size, or allocate this dynamically but this is just for the test harness so I'm fine with it.

Comment 34

[Mike Hommey [:glandium]]

Comment on attachment 712128 [details] [diff] [review]
nsbrowserapp diff

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

::: browser/app/nsBrowserApp.cpp
@@ +193,5 @@
> +      for (int idx = 1; idx < argc; idx++) {
> +        if (IsArg(argv[idx], "metrodesktop")) {
> +          metroOnDesktop = true;
> +          break;
> +        } 

Please remove the trailing whitespace.

@@ +213,5 @@
> +    nsCOMPtr<nsIFile> greDir;
> +    exeFile->GetParent(getter_AddRefs(greDir));
> +
> +    nsCOMPtr<nsIFile> appSubdir;
> +    greDir->Clone(getter_AddRefs(appSubdir));

Since you're reusing greDir, you don't need to Clone here.

Comment 35

[Jim Mathies [:jimm]]

Attachment: package-manifest

straight copy over from elm.

Comment 36

[Mike Hommey [:glandium]]

Comment on attachment 712420 [details] [diff] [review]
package-manifest

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

::: browser/installer/package-manifest.in
@@ +748,5 @@
> +@BINPATH@/components/MetroUIUtils.js
> +[metro]
> +; gre resources
> +@BINPATH@/CommandExecuteHandler@BIN_SUFFIX@
> +@BINPATH@/dummyvccorlib.dll

If you use @BIN_SUFFIX@ above, use @DLL_SUFFIX@ here :)

Comment 37

[Jim Mathies [:jimm]]

Attachment: browser sub dir

Comment 38

[Jim Mathies [:jimm]]

https://hg.mozilla.org/mozilla-central/rev/523c662eeb79
https://hg.mozilla.org/mozilla-central/rev/db5d55b48b3f
https://hg.mozilla.org/mozilla-central/rev/93453a5836b5