Closed Bug 1417365 Opened 2 years ago Closed 2 years ago

Various unified build issues in DOM code

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

Details

Attachments

(1 file)

No description provided.
Attached patch patchSplinter Review
Attachment #8928462 - Flags: review?(amarchesini)
Attachment #8928462 - Flags: review?(amarchesini) → review+
Pushed by jwatt@jwatt.org:
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
Priority: -- → P3
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.
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?
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.