Bas Schouten (:bas.schouten) Assignee
	Description • 12 years ago

We need an implementation of the new Compositor API in D3D11.

	Jim Mathies [:jimm]
	Comment 4 • 11 years ago

part 2 is failing to apply on mc tip due to a problem in CompositorParent. It looks like AllocPLayer has changed to AllocPLayerTransaction, and returns a LayerTransactionParent.

	Nicolas Silva [:nical]
	Comment 5 • 11 years ago

Comment on attachment 741821 [details] [diff] [review]
Part 2: Add D3D11 Compositor to Layers.

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

::: gfx/layers/Makefile.in
@@ +93,5 @@
>          ReadbackManagerD3D10.cpp \
>          ShadowLayerUtilsD3D10.cpp \
>          ThebesLayerD3D10.cpp \
> +        CompositorD3D11.cpp \
> +        TextureD3D11.cpp \

looks like the files should be in alphabetical order here.

::: gfx/layers/client/CompositableClient.cpp
@@ +112,2 @@
>    case TEXTURE_SHMEM:
> +    MOZ_ASSERT(parentBackend == LAYERS_OPENGL || parentBackend == LAYERS_D3D11);

I wouldn't go for a fatal assertion here. Not even sure that we actually want to ensure this. I expect software backend would use TextureClientShmem in this situation as well. Let's drop the assertion.

::: gfx/layers/client/ImageClient.cpp
@@ +28,5 @@
>    RefPtr<ImageClient> result = nullptr;
>    switch (aCompositableHostType) {
>    case BUFFER_IMAGE_SINGLE:
> +    if (aForwarder->GetCompositorBackendType() == LAYERS_OPENGL ||
> +        aForwarder->GetCompositorBackendType() == LAYERS_D3D11) {

We should remove these 3 if statements instead.

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +350,5 @@
> +
> +  return rt;
> +}
> +
> +// TODO[Bas] this method doesn't actually use aSource

Maybe this should be fixed before landing?

@@ +593,5 @@
> +  memcpy(&mVSConstants.projection, &projection, 64);
> +}
> +
> +const nsIntSize&
> +CompositorD3D11::GetWidgetSize()

This is not exactly the same behavior as Compositor OGL which returns the size of the last beginning of a frame (see BeginFrame). I haven't gone into the details of looking whether this could lead to race conditions but it doesn't seem unreasonable that the size stays the same during a composition. Can you justify than one way is better than the other?

::: gfx/layers/d3d11/CompositorD3D11.fx
@@ +98,5 @@
> +float4 TransformedPostion(float2 aInPosition)
> +{
> +  // the current vertex's position on the quad
> +  float4 position = float4(0, 0, 0, 1);
> +  

nit: trailing space

@@ +121,5 @@
> +  result.w = aTransformedPosition.w;
> +  result.xyz = aTransformedPosition.xyz / aTransformedPosition.w;
> +  result -= vRenderTargetOffset;
> +  result.xyz *= result.w;
> +  

nit: trailing space

::: gfx/layers/d3d11/CompositorD3D11.h
@@ +34,5 @@
> +};
> +
> +struct DeviceAttachmentsD3D11;
> +
> +class CompositorD3D11 : public Compositor

Please use MOZ_OVERRIDE whenever applicable in this class

@@ +144,5 @@
> +  nsRefPtr<gfxContext> mTarget;
> +
> +  nsIWidget *mWidget;
> +  // XXX - Bas - wth?
> +  nsIntSize mSize;

See my other comment about resize behavior in GL vs D3D compositors, This looks like a left-over from a copy-pasting of the CompositorOGL stuff.

::: gfx/layers/d3d11/CompositorD3D11Shaders.h
@@ +3,5 @@
> +// Generated by Microsoft (R) HLSL Shader Compiler 9.30.9200.16384
> +//
> +//
> +///
> +// Buffer Definitions: 

nit: trailing space

@@ +32,5 @@
> +// Input signature:
> +//
> +// Name                 Index   Mask Register SysValue  Format   Used
> +// -------------------- ----- ------ -------- -------- ------- ------
> +// POSITION                 0   xy          0     NONE   float   xy  

nit: trailing space

@@ +40,5 @@
> +//
> +// Name                 Index   Mask Register SysValue  Format   Used
> +// -------------------- ----- ------ -------- -------- ------- ------
> +// SV_Position              0   xyzw        0      POS   float   xyzw
> +// TEXCOORD                 0   xy          1     NONE   float   xy  

nit: trailing space

@@ +97,5 @@
> +mad r1.xyzw, cb0[4].xyzw, r0.xxxx, r1.xyzw
> +mad r1.xyzw, cb0[6].xyzw, r0.zzzz, r1.xyzw
> +mad o0.xyzw, cb0[7].xyzw, r0.wwww, r1.xyzw
> +mad o1.xy, v0.xyxx, cb0[9].zwzz, cb0[9].xyxx
> +ret 

trailing space

@@ +103,5 @@
> +#endif
> +
> +const BYTE LayerQuadVS[] =
> +{
> +     68,  88,  66,  67,  94, 179, 

trailing spaces on every line in this array

@@ +426,5 @@
> +//
> +// Name                 Index   Mask Register SysValue  Format   Used
> +// -------------------- ----- ------ -------- -------- ------- ------
> +// SV_Position              0   xyzw        0      POS   float       
> +// TEXCOORD                 0   xy          1     NONE   float       

trailing spaces

@@ +453,5 @@
> +ps_4_0
> +dcl_constantbuffer cb0[1], immediateIndexed
> +dcl_output o0.xyzw
> +mov o0.xyzw, cb0[0].xyzw
> +ret 

trailing spaces a little bit of everywhere in this file (I'll not point them all out)

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +279,5 @@
> +  IntRect rect = GetTileRect(mCurrentTile);
> +  return nsIntRect(rect.x, rect.y, rect.width, rect.height);
> +}
> +
> +uint32_t GetRequiredTiles(uint32_t aSize, uint32_t aMaxSize)

should be static

::: gfx/layers/d3d11/TextureD3D11.h
@@ +48,5 @@
> +public:
> +  // Use aTexture == nullptr for rendering to the window
> +  CompositingRenderTargetD3D11(ID3D11Texture2D *aTexture);
> +
> +  virtual TextureSourceD3D11* AsSourceD3D11() { return this; }

Please use MOZ_OVERRIDE whenever applicable

	Jeff Muizelaar [:jrmuizel]
	Comment 6 • 11 years ago

Comment on attachment 741819 [details] [diff] [review]
Part 1: Add D3D11 device to gfxWindowsPlatform.

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

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +572,5 @@
>      if (!IsRunningInWindowsMetro()) {
>        supportedFeatureLevelsCount--;
>      }
>  
> +    nsRefPtr<IDXGIAdapter1> adapter1 = GetDXGIAdapter();

It looks like you're playing it a bit loose with nsRefPtr vs RefPtr. Try to be consistent:)

@@ +1451,5 @@
>      }
>  #endif
>  }
>  
> +ID3D11Device*

This should probably return an already_addrefed.

@@ +1500,5 @@
>  {
>    return GetModuleHandleA("nvumdshim.dll");
>  }
> +
> +IDXGIAdapter1*

This should probably return an already_addrefed.

	Jim Mathies [:jimm]
	Comment 7 • 11 years ago

Comment on attachment 741826 [details] [diff] [review]
Part 3: Integrate CompositorD3D11 into windows Widget.

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

::: widget/xpwidgets/nsBaseWidget.cpp
@@ +886,5 @@
>  }
>  
>  void nsBaseWidget::CreateCompositor(int aWidth, int aHeight)
>  {
> +  // Recreating this is tricky as when we 

nit - unfinished comment?

	Jeff Muizelaar [:jrmuizel]
	Comment 8 • 11 years ago

Comment on attachment 741821 [details] [diff] [review]
Part 2: Add D3D11 Compositor to Layers.

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

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +536,5 @@
> +
> +  ID3D11Buffer *buffer = mAttachments->mVertexBuffer;
> +  UINT size = sizeof(Vertex);
> +  UINT offset = 0;
> +  mContext->IASetVertexBuffers(0, 1, &buffer, &size, &offset);

Do we need to do this every frame?

@@ +589,5 @@
> +
> +  gfx3DMatrix projection = gfx3DMatrix::From2D(viewMatrix);
> +  projection._33 = 0.0f;
> +
> +  memcpy(&mVSConstants.projection, &projection, 64);

sizeof(mVSConstants.projection) instead of 64

@@ +629,5 @@
> +                              0);
> +  } else {
> +    mSwapChain->ResizeBuffers(1, rect.width, rect.height,
> +                              DXGI_FORMAT_B8G8R8A8_UNORM,
> +                              DXGI_SWAP_CHAIN_FLAG_GDI_COMPATIBLE);

Do we need GDI_COMPATBILE?

@@ +707,5 @@
> +  *(VertexShaderConstants*)resource.pData = mVSConstants;
> +  mContext->Unmap(mAttachments->mVSConstantBuffer, 0);
> +  mContext->Map(mAttachments->mPSConstantBuffer, 0, D3D11_MAP_WRITE_DISCARD, 0, &resource);
> +  *(PixelShaderConstants*)resource.pData = mPSConstants;
> +  mContext->Unmap(mAttachments->mPSConstantBuffer, 0);

Does it make sense to put pixel and vertex shaders into a single buffer so that we don't have to do multiple Maps per frame?

@@ +729,5 @@
> +  case FILTER_POINT:
> +    sampler = mAttachments->mPointSamplerState;
> +    break;
> +  default:
> +    sampler = mAttachments->mLinearSamplerState;

Drop the 'default' case so that we get compiler warnings when we don't handle the enum.

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +92,5 @@
> +    }
> +
> +    if (mTexture) {
> +      return;
> +    }

Joe and I both had a bit of a problem reading this code:
how about:

if (desc.Width == aSize.width && desc.Height == aSize.height) {
   // we can reuse our texture
   return;
}

mTexture = nullptr
mSurface = nullptr
CreateDT();

@@ +332,5 @@
> +
> +  CD3D11_TEXTURE2D_DESC desc(dxgiFormat, size.width, size.height,
> +                            1, 1, D3D11_BIND_SHADER_RESOURCE, D3D11_USAGE_IMMUTABLE);
> +
> +  uint32_t maxSize = GetMaxTextureSizeForFeatureLevel(mDevice->GetFeatureLevel());

Shouldn't this be using the actual max texture size?

	Bas Schouten (:bas.schouten) Assignee
	Comment 9 • 11 years ago

(In reply to Nicolas Silva [:nical] from comment #5)
> Comment on attachment 741821 [details] [diff] [review]
> Part 2: Add D3D11 Compositor to Layers.
> 
> Review of attachment 741821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/Makefile.in
> @@ +93,5 @@
> >          ReadbackManagerD3D10.cpp \
> >          ShadowLayerUtilsD3D10.cpp \
> >          ThebesLayerD3D10.cpp \
> > +        CompositorD3D11.cpp \
> > +        TextureD3D11.cpp \
> 
> looks like the files should be in alphabetical order here.

If that makes you happy I'm all for it! :)

> ::: gfx/layers/client/CompositableClient.cpp
> @@ +112,2 @@
> >    case TEXTURE_SHMEM:
> > +    MOZ_ASSERT(parentBackend == LAYERS_OPENGL || parentBackend == LAYERS_D3D11);
> 
> I wouldn't go for a fatal assertion here. Not even sure that we actually
> want to ensure this. I expect software backend would use TextureClientShmem
> in this situation as well. Let's drop the assertion.

Agreed!

> 
> ::: gfx/layers/client/ImageClient.cpp
> @@ +28,5 @@
> >    RefPtr<ImageClient> result = nullptr;
> >    switch (aCompositableHostType) {
> >    case BUFFER_IMAGE_SINGLE:
> > +    if (aForwarder->GetCompositorBackendType() == LAYERS_OPENGL ||
> > +        aForwarder->GetCompositorBackendType() == LAYERS_D3D11) {
> 
> We should remove these 3 if statements instead.

Even better!

> 
> ::: gfx/layers/d3d11/CompositorD3D11.cpp
> @@ +350,5 @@
> > +
> > +  return rt;
> > +}
> > +
> > +// TODO[Bas] this method doesn't actually use aSource
> 
> Maybe this should be fixed before landing?

No, this is only important for subpixel AA and we don't really need to worry about that for initial landing. I was told we might not even care about subpixel AA for metro release.

> @@ +593,5 @@
> > +  memcpy(&mVSConstants.projection, &projection, 64);
> > +}
> > +
> > +const nsIntSize&
> > +CompositorD3D11::GetWidgetSize()
> 
> This is not exactly the same behavior as Compositor OGL which returns the
> size of the last beginning of a frame (see BeginFrame). I haven't gone into
> the details of looking whether this could lead to race conditions but it
> doesn't seem unreasonable that the size stays the same during a composition.
> Can you justify than one way is better than the other?

No, except that -this- returns what the function is asking for, if we want to change what it returns, let's rename it :)

> ::: gfx/layers/d3d11/CompositorD3D11Shaders.h
> @@ +3,5 @@
> > +// Generated by Microsoft (R) HLSL Shader Compiler 9.30.9200.16384
> > +//
> > +//
> > +///
> > +// Buffer Definitions: 
> 
> nit: trailing space

This file is autogenerated so you'll have to live with them ;).

	Bas Schouten (:bas.schouten) Assignee
	Comment 10 • 11 years ago

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> Comment on attachment 741821 [details] [diff] [review]
> Part 2: Add D3D11 Compositor to Layers.
> 
> Review of attachment 741821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d11/CompositorD3D11.cpp
> @@ +536,5 @@
> > +
> > +  ID3D11Buffer *buffer = mAttachments->mVertexBuffer;
> > +  UINT size = sizeof(Vertex);
> > +  UINT offset = 0;
> > +  mContext->IASetVertexBuffers(0, 1, &buffer, &size, &offset);
> 
> Do we need to do this every frame?

It seems like the safest thing, once per frame is not that high overhead and otherwise we'd have to guarantee no external party changes the state of the D3D11 device. Right now that's easy but it might be different in the future.

> @@ +629,5 @@
> > +                              0);
> > +  } else {
> > +    mSwapChain->ResizeBuffers(1, rect.width, rect.height,
> > +                              DXGI_FORMAT_B8G8R8A8_UNORM,
> > +                              DXGI_SWAP_CHAIN_FLAG_GDI_COMPATIBLE);
> 
> Do we need GDI_COMPATBILE?

I believe we do to avoid bugs on some hardware on desktop :s.

> @@ +707,5 @@
> > +  *(VertexShaderConstants*)resource.pData = mVSConstants;
> > +  mContext->Unmap(mAttachments->mVSConstantBuffer, 0);
> > +  mContext->Map(mAttachments->mPSConstantBuffer, 0, D3D11_MAP_WRITE_DISCARD, 0, &resource);
> > +  *(PixelShaderConstants*)resource.pData = mPSConstants;
> > +  mContext->Unmap(mAttachments->mPSConstantBuffer, 0);
> 
> Does it make sense to put pixel and vertex shaders into a single buffer so
> that we don't have to do multiple Maps per frame?

I don't think that's possible? But I'll have another look.

> @@ +332,5 @@
> > +
> > +  CD3D11_TEXTURE2D_DESC desc(dxgiFormat, size.width, size.height,
> > +                            1, 1, D3D11_BIND_SHADER_RESOURCE, D3D11_USAGE_IMMUTABLE);
> > +
> > +  uint32_t maxSize = GetMaxTextureSizeForFeatureLevel(mDevice->GetFeatureLevel());
> 
> Shouldn't this be using the actual max texture size?

I was unable to find such a thing in D3D11.

	Nicolas Silva [:nical]
	Comment 11 • 11 years ago

(In reply to Bas Schouten (:bas.schouten) from comment #9)
> (In reply to Nicolas Silva [:nical] from comment #5)
> > Comment on attachment 741821 [details] [diff] [review]
> > @@ +593,5 @@
> > > +  memcpy(&mVSConstants.projection, &projection, 64);
> > > +}
> > > +
> > > +const nsIntSize&
> > > +CompositorD3D11::GetWidgetSize()
> > 
> > This is not exactly the same behavior as Compositor OGL which returns the
> > size of the last beginning of a frame (see BeginFrame). I haven't gone into
> > the details of looking whether this could lead to race conditions but it
> > doesn't seem unreasonable that the size stays the same during a composition.
> > Can you justify than one way is better than the other?
> 
> No, except that -this- returns what the function is asking for, if we want
> to change what it returns, let's rename it :)

I'll file a followup bug to make all backends do the same thing.
This method is called by backend independent logic so all backends should have the same semantics.
Since querying the widget size to a Widget that belongs to another thread without any form of synchronization I don't know that the way it is done in the D3D compositor is correct. By looking at our code it doesn't appear that realy bad things would happen (at most some things could be a bit glitchy while the window is resized I suppose).
If you keep it like this, you should remove the mWidgetSize member that is unused.

	Jeff Muizelaar [:jrmuizel]
	Comment 12 • 11 years ago

Comment on attachment 741821 [details] [diff] [review]
Part 2: Add D3D11 Compositor to Layers.

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

::: gfx/layers/d3d11/TextureD3D11.h
@@ +228,5 @@
> +  int32_t maxTextureSize;
> +  switch (aFeatureLevel) {
> +  case D3D_FEATURE_LEVEL_11_1:
> +  case D3D_FEATURE_LEVEL_11_0:
> +    maxTextureSize = 16384;

D3D11_REQ_TEXTURE2D_U_OR_V_DIMENSION

@@ +232,5 @@
> +    maxTextureSize = 16384;
> +    break;
> +  case D3D_FEATURE_LEVEL_10_1:
> +  case D3D_FEATURE_LEVEL_10_0:
> +    maxTextureSize = 8192;

D3D10_REQ_TEXTURE2D_U_OR_V_DIMENSION

@@ +235,5 @@
> +  case D3D_FEATURE_LEVEL_10_0:
> +    maxTextureSize = 8192;
> +    break;
> +  case D3D_FEATURE_LEVEL_9_3:
> +    maxTextureSize = 4096;

D3D_FL9_3_REQ_TEXTURE2D_U_OR_V_DIMENSION

@@ +238,5 @@
> +  case D3D_FEATURE_LEVEL_9_3:
> +    maxTextureSize = 4096;
> +    break;
> +  default:
> +    maxTextureSize = 2048;

D3D_FL9_1_REQ_TEXTURE2D_U_OR_V_DIMENSION

	Jeff Muizelaar [:jrmuizel]
	Comment 13 • 11 years ago

(In reply to Bas Schouten (:bas.schouten) from comment #10)
> > @@ +629,5 @@
> > > +                              0);
> > > +  } else {
> > > +    mSwapChain->ResizeBuffers(1, rect.width, rect.height,
> > > +                              DXGI_FORMAT_B8G8R8A8_UNORM,
> > > +                              DXGI_SWAP_CHAIN_FLAG_GDI_COMPATIBLE);
> > 
> > Do we need GDI_COMPATBILE?
> 
> I believe we do to avoid bugs on some hardware on desktop :s.

Can we put a bug number in a comment.


> > > +  uint32_t maxSize = GetMaxTextureSizeForFeatureLevel(mDevice->GetFeatureLevel());
> > 
> > Shouldn't this be using the actual max texture size?
> 
> I was unable to find such a thing in D3D11.

Neither was I.

	Jim Mathies [:jimm]
	Comment 14 • 11 years ago

Is this blocked on something or is it ready to land?

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 15 • 11 years ago

Last we talked, today looked like a day for it.

	Bas Schouten (:bas.schouten) Assignee
	Comment 16 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/05762bbaf268
https://hg.mozilla.org/integration/mozilla-inbound/rev/478c8b6a9736
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f52629c9d08

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/05762bbaf268
https://hg.mozilla.org/mozilla-central/rev/478c8b6a9736
https://hg.mozilla.org/mozilla-central/rev/6f52629c9d08

	Philip Chee
	Comment 18 • 11 years ago

Is there a missing include somewhere?

c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(226) : error C2065: 'D3D_FEATURE_LEVEL_11_1' : undeclared identifier
c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(226) : error C2051: case expression not constant
c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(235) : error C2065: 'D3D_FL9_3_REQ_TEXTURE2D_U_OR_V_DIMENSION' : undeclared identifier
c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(238) : error C2065: 'D3D_FL9_1_REQ_TEXTURE2D_U_OR_V_DIMENSION' : undeclared identifier

	Jacek Caban
	Comment 19 • 11 years ago

Pushed fix for mingw compilation:

https://hg.mozilla.org/integration/mozilla-inbound/rev/182f3878a17b

The error was:

TextureD3D11.h:115:89: error: operands to ?: have different types ‘__gnu_cxx::__alloc_traits<std::allocator<mozilla::RefPtr<ID3D11Texture2D> > >::value_type {aka mozilla::RefPtr<ID3D11Texture2D>}’ and ‘ID3D11Texture2D*’

	Jim Mathies [:jimm]
	Comment 20 • 11 years ago

(In reply to Philip Chee from comment #18)
> Is there a missing include somewhere?
> 
> c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(226) : error
> C2065: 'D3D_FEATURE_LEVEL_11_1' : undeclared identifier
> c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(226) : error
> C2051: case expression not constant
> c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(235) : error
> C2065: 'D3D_FL9_3_REQ_TEXTURE2D_U_OR_V_DIMENSION' : undeclared identifier
> c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(238) : error
> C2065: 'D3D_FL9_1_REQ_TEXTURE2D_U_OR_V_DIMENSION' : undeclared identifier

I wonder if this landing is going to force the requirement of the Win 8 sdk, looks like it might.

	Bill Gianopoulos [:WG9s]
	Comment 21 • 11 years ago

Does requiring windows 8 SDK mean we also need to drop support for older Visual C compilers?  See bug 866425.

	Bill Gianopoulos [:WG9s]
	Comment 22 • 11 years ago

Can we do this with ifdefs to make it not a requirement?

	Jim Mathies [:jimm]
	Comment 23 • 11 years ago

This is being discussed in bug 869868.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 24 • 11 years ago

Backed out

https://hg.mozilla.org/integration/mozilla-inbound/rev/24d361942d24

	Philip Chee
	Comment 25 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/24d361942d24

	Jim Mathies [:jimm]
	Comment 27 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e223c0cdc8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a8d467615f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a2a23786f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1d7931af7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5daf23a9a20

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 28 • 11 years ago

(In reply to Jim Mathies [:jimm] from comment #26)
> Created attachment 747398 [details] [diff] [review]
> 601 sdk fix

This "fix" actually breaks the build if you're using the Windows 8 SDK with regular Firefox (without --enable-metro).  Then the target version is still 601 and so these shims are included and conflict with the real values in the SDK headers.

I worked around this with --with-windows-version=602, but we should fix this.

	Ed Morley [:emorley]
	Comment 29 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/5e223c0cdc8d
https://hg.mozilla.org/mozilla-central/rev/b1a8d467615f
https://hg.mozilla.org/mozilla-central/rev/d5a2a23786f5
https://hg.mozilla.org/mozilla-central/rev/5a1d7931af7b
https://hg.mozilla.org/mozilla-central/rev/c5daf23a9a20

	Jim Mathies [:jimm]
	Comment 30 • 11 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #28)
> (In reply to Jim Mathies [:jimm] from comment #26)
> > Created attachment 747398 [details] [diff] [review]
> > 601 sdk fix
> 
> This "fix" actually breaks the build if you're using the Windows 8 SDK with
> regular Firefox (without --enable-metro).  Then the target version is still
> 601 and so these shims are included and conflict with the real values in the
> SDK headers.
> 
> I worked around this with --with-windows-version=602, but we should fix this.

Sorry about that, I'll touch this up today.