superreview requested: [Bug 751541] Review and land metro specific toolkit/library changes : [Attachment 624723] vcorlib shim for non-win8 platforms v.2

[bugzill...@mozilla.org]

Jim Mathies [:jimm] <jmat...@mozilla.com> has asked Kyle Huey [:khuey]
(kh...@mozilla.com) <kh...@kylehuey.com> for superreview:
Bug 751541: Review and land metro specific toolkit/library changes
https://bugzilla.mozilla.org/show_bug.cgi?id=751541

Attachment 624723: vcorlib shim for non-win8 platforms v.2
https://bugzilla.mozilla.org/attachment.cgi?id=624723&action=edit


------- Additional Comments from Jim Mathies [:jimm] <jmat...@mozilla.com>
(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> Comment on attachment 624028
> vcorlib shim for non-win8 platforms v.1
>
> Review of attachment 624028:
> -----------------------------------------------------------------
>
> This looks generally good. I have made a fair number of nits, and some
> actual comments. r=me with those addressed. Also, please ask review from a
> build system peer. The build system changes look benign, but one never
> knows. ;-)

Thanks!

> ::: toolkit/library/nsDllMain.cpp
> @@ +100,5 @@
> > + osInfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX);
> > + ::GetVersionEx((OSVERSIONINFO*)&osInfo);
> > + version =
> > + (osInfo.dwMajorVersion & 0xff) << 8 | (osInfo.dwMinorVersion & 0xff);
> > + return (version >= 0x602);
>
> This matches both Windows 8 and Windows 8 server (whatever Microsoft ends up
> calling it.) I'm assuming that this OK, but please add a comment mentioning
> this explicitly.

I would assume the new runtime will be present on all os greater to or equal to
win8, including server. I've added a comment.

>
> Can we also include the .dll suffix here? This will potentially also match
> vccorlibcustom.dll...

No, there's a version number on the end of the filename. The dummy lib is
dummyvccorlib so it's safe. It would never result in a call here anyway because
we load it directly with LoadLibrary.

> This should be a fatal assertion IMO. Also, why is this check only enabled
> in debug builds?

Didn't want to add the overhead to release builds. Not a big deal though. I've
taken the ifdefing out in the updated patch.

> @@ +131,5 @@
> > + }
> > + return NULL;
> > +}
> > +
> > +PfnDliHook __pfnDliNotifyHook2 = delayDllLoadHook;
>
> Hmm, this should be EXTERN_C, right?

ExternC I guess based on the header, although this works just fine without it.
Updated.

> @@ +21,5 @@
> > + case DLL_PROCESS_DETACH:
> > + break;
> > + }
> > + return TRUE;
> > +}
>
> Also, this DllMain seems to be completely redundant.

I nixed all the default project code.

[bugzill...@mozilla.org]

Jim Mathies [:jimm] <jmat...@mozilla.com> has canceled Jim Mathies [:jimm]
<jmat...@mozilla.com>'s request for superreview:

Bug 751541: Review and land metro specific toolkit/library changes
https://bugzilla.mozilla.org/show_bug.cgi?id=751541

Attachment 624723: vcorlib shim for non-win8 platforms v.2
https://bugzilla.mozilla.org/attachment.cgi?id=624723&action=edit


------- Additional Comments from Jim Mathies [:jimm] <jmat...@mozilla.com>

Updated to remove the uiautomation core delay load code, and fixed the
assertion which had a reversed test.

[bugzill...@mozilla.org]

Jim Mathies [:jimm] <jmat...@mozilla.com> has asked Kyle Huey [:khuey]

(kh...@mozilla.com) <kh...@kylehuey.com> for superreview:

Bug 751541: Review and land metro specific toolkit/library changes
https://bugzilla.mozilla.org/show_bug.cgi?id=751541

Attachment 625084: vcorlib shim for non-win8 platforms v.2
https://bugzilla.mozilla.org/attachment.cgi?id=625084&action=edit