Closed Bug 807593 (metro-misc) Opened 7 years ago Closed 7 years ago

[tracking] mc / elm diff issues

Categories

(Firefox for Metro Graveyard :: General, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [leave-open][metro-it1][metro-mvp][LOE:1])

Attachments

(5 files, 7 obsolete files)

Filing this for tracking misc. issues found while diffing mc and elm.
Attached patch gfx differences (obsolete) — Splinter Review
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.
Attachment #677326 - Flags: feedback?(netzen)
Attached patch gfx differences (obsolete) — Splinter Review
additional gfx files.
Attachment #677326 - Attachment is obsolete: true
Attachment #677326 - Flags: feedback?(netzen)
Attachment #677328 - Flags: review?(netzen)
Attached patch accessible (obsolete) — Splinter Review
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.
Attached patch browserdom (obsolete) — Splinter Review
Looks like this needs to land on mc.
Attachment #677331 - Flags: feedback?(netzen)
Depends on: 771271
Depends on: 803671
Depends on: metro-build
Attached patch huffdec (obsolete) — Splinter Review
This can probably come out, needs to be tested first.
Depends on: 795887
Assignee: nobody → jmathies
Attachment #677383 - Flags: review?(netzen)
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.
Attachment #677328 - Flags: review?(netzen)
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?
Attachment #677383 - Flags: review?(netzen)
> ::: 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.
*doesn't effect
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.
Attachment #677383 - Flags: review+
(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 on attachment 677383 [details] [diff] [review]
[landed] misc. toolkit lib makefile differences

https://hg.mozilla.org/integration/mozilla-inbound/rev/a72426c4eda1
Attachment #677383 - Attachment description: misc. toolkit lib makefile differences → [landed] misc. toolkit lib makefile differences
Whiteboard: leave-open
Comment on attachment 678263 [details] [diff] [review]
[landed] app runner changes

https://hg.mozilla.org/integration/mozilla-inbound/rev/31784b0d6334
Attachment #678263 - Attachment description: app runner changes → [landed] app runner changes
sync 'app runner changes' review comments to elm - 

https://hg.mozilla.org/projects/elm/rev/2afec7dbfd58
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/*
I can handle hte nsIBrowserDOMWindow and gfx ones, but probably not today because I'm working on a B2G updater task.
No longer depends on: 803671
I'll work on the remaining items this week.
Whiteboard: leave-open → [leave-open][metro-it1][metro-mvp][LOE:1]
No longer depends on: metro-build
Blocks: elm-merge
No longer depends on: 771271
Depends on: 815172
Comment on attachment 677331 [details] [diff] [review]
browserdom

Canceling feedback flag.  This is being cleaned up in bug 815172
Attachment #677331 - Attachment is obsolete: true
Attachment #677331 - Flags: feedback?(netzen)
Depends on: 815177
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.
Attachment #677328 - Attachment is obsolete: true
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.
We'll reopen if needed.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Alias: metro-misc
No longer depends on: 795887
Attached patch nsbrowserapp diff (obsolete) — Splinter Review
This was already reviewed in another bug, but some change were introduces due to metro build. One change was better handling of utf8 strings.
Attachment #712126 - Flags: review?(netzen)
Attached patch nsbrowserapp diff (obsolete) — Splinter Review
missed some..
Attachment #712126 - Attachment is obsolete: true
Attachment #712126 - Flags: review?(netzen)
Attachment #712127 - Flags: review?(netzen)
one more update.
Attachment #712127 - Attachment is obsolete: true
Attachment #712127 - Flags: review?(netzen)
Attachment #712128 - Flags: review?(netzen)
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.
Attachment #712128 - Flags: review?(netzen) → review+
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.
Attached patch package-manifestSplinter Review
straight copy over from elm.
Attachment #712420 - Flags: review?(mh+mozilla)
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 :)
Attachment #712420 - Flags: review?(mh+mozilla) → review+
Attached patch browser sub dirSplinter Review
Attachment #712426 - Flags: review+
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.