Closed Bug 1417365 Opened 3 years ago Closed 3 years ago
Various unified build issues in DOM code
No description provided.
Attachment #8928462 - Flags: review?(amarchesini) → review+
Pushed by email@example.com: 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 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?
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.
You need to log in before you can comment on or make changes to this bug.