1417365 - Various unified build issues in DOM code

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95e08bf1b1a3
Unified build issues in dom/base. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/53e90f217ced
Unified build issues in dom/bindings. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/5009b892ddb5
Unified build issues in dom/animation. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c40ecf4fefee
Unified build issues in dom/cache. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/62bed90e6a11
Unified build issues in dom/clients. r=baku

Comment 3

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/95e08bf1b1a3
https://hg.mozilla.org/mozilla-central/rev/53e90f217ced
https://hg.mozilla.org/mozilla-central/rev/5009b892ddb5
https://hg.mozilla.org/mozilla-central/rev/c40ecf4fefee
https://hg.mozilla.org/mozilla-central/rev/62bed90e6a11

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8928462 [details] [diff] [review]
patch

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

Please ask me for review on changes to dom/clients.  I'm actively working on this and changes like this create more conflicts and don't align with the final patches well.

Also, I don't understand bugs like this.  Unified build is going to regress almost immediately without automation to catch problems.  Pretty sure we've explicitly decided not to do that for developer productivity.

::: dom/clients/manager/ClientManager.h
@@ +8,5 @@
>  
>  #include "mozilla/dom/ClientOpPromise.h"
>  #include "mozilla/dom/ClientThing.h"
> +#include "mozilla/ipc/PBackgroundSharedTypes.h"
> +#include "nsIPrincipal.h"

This is not right.  We only need forward declares here.  I wish I had been asked for review on this code I am actively working on and landing.

::: dom/clients/manager/ClientManagerOpChild.cpp
@@ +19,5 @@
>      mPromise = nullptr;
>    }
>  }
>  
> +mozilla::ipc::IPCResult

I would have preferred a using statement here.

::: dom/clients/manager/ClientManagerService.h
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  #ifndef _mozilla_dom_ClientManagerService_h
>  #define _mozilla_dom_ClientManagerService_h
>  
> +#include "mozilla/ipc/PBackgroundSharedTypes.h"

This should be a forward declare.

Comment 5

[Jonathan Watt [:jwatt]]

Sorry this patch has conflicted with your work and caused you issues, Ben. :/

I occasionally submit patches like this to make Eclipse CDT handle bits of our code better (it doesn't know about unified compilation). Local Eclipse is much more powerful and faster than DXR/searchfox. Eclipse's code intelligence features don't need the includes to be perfect, so while you're right that non-unified processing will be technically broken soon after landing fixes, in my experience it takes about six months to a year or so before I start to notice issues in any given section of the code.

Regarding your specific issues with the patch, I'm happy to provide a follow-up patch. Would you prefer I do that, or leave well alone for the moment while you're working on this code?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Its fine.  I've already worked around it.  It was just an extra surprise on the same day I was dealing with nsGlobalWindow being refactored.