This is an _exact copy_ of nsGlobalWindow.*. It's being done this way to give a solid starting point for future modifications. MozReview-Commit-ID: 4YfXytd4TEz

This will make future diffs, which rewrite this file completely, much easier to read. MozReview-Commit-ID: HqomlBjrEW7

MozReview-Commit-ID: JRvPtQTJqSX

MozReview-Commit-ID: CmKx5jtvtrT

MozReview-Commit-ID: 4Yz8hRMZEJC

There are many helper methods and structs in nsGlobalWindow.cpp. Many of these are used by only the inner or only the outer window, while some are used by both. In the case of the items used by both, I extracted them into nsGlobalWindow.cpp, which includes nsGlobalWindowInner.cpp and nsGlobalWindowOuter.cpp as the compilation unit entry point. In the case of items used by just one or the other, I removed them from the other file, and deleted the bodies of functions which used them, replacing them, with a MOZ_CRASH. This gets gecko building again, so that we can make further incremental improvements. MozReview-Commit-ID: 8QnJ1PX6TAO

This was needed before as the base to nsGlobalWindow, but now that nsGlobalWindow doesn't exist, and we only have specific versions, we no longer need this type. MozReview-Commit-ID: 6IJmJtnSkMr

MozReview-Commit-ID: CV6rrA0M2ZV

MozReview-Commit-ID: FzaGKmdDtmy

MozReview-Commit-ID: GIiSlDzjgWb

MozReview-Commit-ID: AZMWwKFnvG9

r?smaug (you have your review queue closed) This is the approach I took: A) Copy nsGlobalWindow.cpp into nsGlobalWindow{Inner,Outer}.cpp (part 1) B) Get each file building individually, and then deduplicate symbols until the entire program builds again (part 2) C) Change nsGlobalWindow{Inner,Outer} to inherit from nsPIDOMWindow{Inner,Outer} instead of nsPIDOMWindow<nsISupports> (part 3) D) Remove methods which should only exist on the other window type (parts 4, 5) E) Remove all calls to Assert{Inner,Outer} and As{Inner,Outer} from nsGlobalWindow (part 6) - NOTE: This doesn't remove calls to As{Inner,Outer} from outside of nsGlobalWindow.cpp, despite them no longer being necessary. In total the changes were: 15 files changed, 19726 insertions(+), 16923 deletions(-) So there were definitely a large number of duplicated functions (~3k new lines which are likely from duplicated functions). We should look into how we can improve this in the future. I think this should be done in a follow-up. This isn't a complete final version (obviously), but this is the first time that I've felt that the state of the tree was not making the situation significantly worse, so I think we should try to land it.

Current state of the tree on github: https://github.com/mystor/mozilla-central/tree/split

Not trying to block you here, but I'm really really trying to get my 1 year effort in bug 1293277 reviewed and landed. This is going to lengthen that with rebasing. If there is any way it could wait for a couple weeks while I finish there, I would appreciate it. If not, no problem. I'll do the rebasing.

Please use `hg cp` and `hg rm` for part 1 and part 2a to prevent commit history from being lost. (I'm not sure how git-cinnabar deals with copy/move operations.)

Sounds reasonable. Are there patterns in code duplication?

(In reply to Ben Kelly [:bkelly] from comment #14) > Not trying to block you here, but I'm really really trying to get my 1 year > effort in bug 1293277 reviewed and landed. This is going to lengthen that > with rebasing. If there is any way it could wait for a couple weeks while I > finish there, I would appreciate it. If not, no problem. I'll do the > rebasing. Ideally I'd like to land this ASAP as it was a bunch of tedious work to write the patch & rebasing it after a significant number of changes to nsGlobalWindow will be very tedious/tricky. I really don't want to block your work on bug 1293277 though, so if you'd like I can try to rebase the patches for you? (In reply to Masatoshi Kimura [:emk] from comment #15) > Please use `hg cp` and `hg rm` for part 1 and part 2a to prevent commit > history from being lost. > (I'm not sure how git-cinnabar deals with copy/move operations.) I'm using git-cinnabar locally - I'm not sure if it'll translate the commit information like hg cp and hg rm does. If it's possible to preserve history I might try to hand the patches over to a hg user before pushing to make sure that we don't mangle history any more than we have to? (In reply to Olli Pettay [:smaug] from comment #16) > Sounds reasonable. > Are there patterns in code duplication? Things which I've noticed are duplicated mostly in full: 1. Constructor 2. Destructor 3. Init/Shutdown 4. Cleanup 5. Cycle Collection logic 6. EnsureScriptEnvironment (This should probably just be on the outer, and forwarded to it by the inner?) 7. GetScriptCOntext (should be on outer & forwarded) 8. UpdateParentTarget 9. DisableDialogs/EnableDialogs (Again could probably be on outer & forward from inner) 10. GetPrincipal 11. GetChildWindow (Should be on outer & forward from inner) 12. GetNearestWidget ("") 13. CanSetProperty (a static method - looks like its only used in the outer window, so can probably remove from inner) 14. CallerInnerWindow (Only used from the outer window - can remove from inner) 15. IsInModalState (calls methods on ScriptableTop()) 16. GetCachedXBLPrototypeHandler/CacheXBLPrototypeHandler (Should only be on the inner) 17. UpdateCommands (might be outer window only? Need to audit callsites) 18. RemoveEventListener/AddEventListener (forwards to ELM which is on inner) 19. GetContextForEventHandlers 20. IsTopLevelWindowActive (Fetches GetDocShell() and works with it - outer?) 21. GetInterface 22. AddSizeOfIncludingThis (not fully duplicated, but there's some duplicated logic - might go away as we deduplicate members) 23. RedefineProperty (might be inner window only?) 24. CheckForDPIChange (should be outer window) 25. Dispatch,EventTargetFor,AbstractMainThreadFor 26. TemporariallyDisableDialogs (This is currently defined on both - we should probably only define it on Outer) I think that's close-ish to everything which is literally duplicated (I just ran meld between the two files & looked at the common blocks). -- A lot of these methods call methods to fetch a different window (like GetScriptableTop() or GetDocShell()) and then never touch `this` again. They could probably be moved to the outer window & forwarded to, because most of these getters will return null without an outer window. There are a few methods which never are used within one or the other window type, and I just missed them in my first pass. And then there are the methods which have to be duplicated, as they're things like cycle collection & the constructor.

(In reply to Nika Layzell [:mystor] from comment #17) > I'm using git-cinnabar locally - I'm not sure if it'll translate the commit > information like hg cp and hg rm does. If it's possible to preserve history > I might try to hand the patches over to a hg user before pushing to make > sure that we don't mangle history any more than we have to? I think with git if you do a file move/copy in a separate commit from any changes to those files then it will preserve the history. But it would be good to double check the resulting HG commit to make sure this is true.

> I might try to hand the patches over to a hg user before pushing to make sure > that we don't mangle history any more than we have to I'm happy to be that hg user, fwiw.

MozReview-Commit-ID: GIiSlDzjgWb

(In reply to Andrew McCreight [:mccr8] from comment #18) > (In reply to Nika Layzell [:mystor] from comment #17) > > I'm using git-cinnabar locally - I'm not sure if it'll translate the commit > > information like hg cp and hg rm does. If it's possible to preserve history > > I might try to hand the patches over to a hg user before pushing to make > > sure that we don't mangle history any more than we have to? > > I think with git if you do a file move/copy in a separate commit from any > changes to those files then it will preserve the history. But it would be > good to double check the resulting HG commit to make sure this is true. Rather, git will NOT detect a copy by default if the copy source is not modified at all. At least git fails to detect the copy about attached patches. I think part 1 and part 2a should be squashed into one commit to help copy/move detection.

Comment on attachment 8927501 [details] [diff] [review] Part 1: Copy nsGlobalWindow.{h,cpp} to nsGlobalWindow{Inner,Outer}.{h,cpp}, r=smaug I guess I need to give r- to this since we shouldn't land this, but a patch which does hg cp. But r+ for a patch which does the hg cp.

Comment on attachment 8927503 [details] [diff] [review] Part 2b: Get split headers building but not linking, r=smaug I don't understand how stuff like DEFAULT_SUCCESSIVE_DIALOG_TIME_LIMIT can be removed.

Comment on attachment 8927503 [details] [diff] [review] Part 2b: Get split headers building but not linking, r=smaug But I'm sure you fix it somewhere, since otherwise the stuff doesn't compile

Comment on attachment 8927504 [details] [diff] [review] Part 2c: Get nsGlobalWindowInner.cpp building, r=smaug + /* virtual */ void -nsGlobalWindow::FullscreenWillChange(bool aIsFullscreen) +nsGlobalWindowInner::FullscreenWillChange(bool aIsFullscreen) Mysterious extra newline

There will need to be followup patches to fix alignment of arguments

Comment on attachment 8927507 [details] [diff] [review] Part 3: Remove nsPIDOMWindow<nsISupports>, r=smaug +explicit nsPIDOMWindowInner(nsPIDOMWindowOuter *aOuterWindow) * goes with the type. Can be fixed when fixing other stuff

MozReview-Commit-ID: KDllmZzdn6m

MozReview-Commit-ID: 1mzNDOFUNep

Comment on attachment 8928317 [details] [diff] [review] Part 8: Stylistic fixes in nsGlobalWindow{Inner,Outer}.cpp ok, not going to complain about * usage not being fixed. That is old code.

This is an _exact copy_ of nsGlobalWindow.*. It's being done this way to give a solid starting point for future modifications. MozReview-Commit-ID: 4YfXytd4TEz

This will make future diffs, which rewrite this file completely, much easier to read. MozReview-Commit-ID: HqomlBjrEW7

MozReview-Commit-ID: JRvPtQTJqSX

MozReview-Commit-ID: CmKx5jtvtrT

MozReview-Commit-ID: 4Yz8hRMZEJC

There are many helper methods and structs in nsGlobalWindow.cpp. Many of these are used by only the inner or only the outer window, while some are used by both. In the case of the items used by both, I extracted them into nsGlobalWindow.cpp, which includes nsGlobalWindowInner.cpp and nsGlobalWindowOuter.cpp as the compilation unit entry point. In the case of items used by just one or the other, I removed them from the other file, and deleted the bodies of functions which used them, replacing them, with a MOZ_CRASH. This gets gecko building again, so that we can make further incremental improvements. MozReview-Commit-ID: 8QnJ1PX6TAO

This was needed before as the base to nsGlobalWindow, but now that nsGlobalWindow doesn't exist, and we only have specific versions, we no longer need this type. MozReview-Commit-ID: 6IJmJtnSkMr

MozReview-Commit-ID: CV6rrA0M2ZV

MozReview-Commit-ID: FzaGKmdDtmy

MozReview-Commit-ID: GIiSlDzjgWb

MozReview-Commit-ID: AZMWwKFnvG9

MozReview-Commit-ID: KDllmZzdn6m

MozReview-Commit-ID: 1mzNDOFUNep

Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/74086caf9463 Part 1: Copy nsGlobalWindow.{h,cpp} to nsGlobalWindow{Inner,Outer}.{h,cpp}, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b15fe9224e Part 2a: Delete nsGlobalWindow.{h,cpp}, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/028e876d02bf Part 2b: Get split headers building but not linking, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/71a0e504ebd0 Part 2c: Get nsGlobalWindowInner.cpp building, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/d1fc9865c557 Part 2d: Get nsGlobalWindowOuter.cpp building, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/862355bc5c58 Part 2e: Eliminate duplicate declarations, and get gecko building again, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/ba47537d1844 Part 3: Remove nsPIDOMWindow<nsISupports>, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/2d5ba51ff7e3 Part 4: Eliminate outer window only methods from nsGlobalWindowInner, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/aaed1999291a Part 5: Eliminate inner window only methods from nsGlobalWindowOuter, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/10eb6ec39ef1 Part 6: Eliminate calls to Assert{Inner,Outer} and As{Inner,Outer} in nsGlobalWindow, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/149ba9431791 Part 7: Move nsPIDOMWindow{Inner,Outer}::TabGroup into their respective cpps, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/476642ee328b Part 8: Stylistic fixes in nsGlobalWindow{Inner,Outer}.cpp, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/7b2f3fa349b0 Part 9: Deduplicate more code when possible, r=smaug

https://hg.mozilla.org/mozilla-central/rev/74086caf9463 https://hg.mozilla.org/mozilla-central/rev/a1b15fe9224e https://hg.mozilla.org/mozilla-central/rev/028e876d02bf https://hg.mozilla.org/mozilla-central/rev/71a0e504ebd0 https://hg.mozilla.org/mozilla-central/rev/d1fc9865c557 https://hg.mozilla.org/mozilla-central/rev/862355bc5c58 https://hg.mozilla.org/mozilla-central/rev/ba47537d1844 https://hg.mozilla.org/mozilla-central/rev/2d5ba51ff7e3 https://hg.mozilla.org/mozilla-central/rev/aaed1999291a https://hg.mozilla.org/mozilla-central/rev/10eb6ec39ef1 https://hg.mozilla.org/mozilla-central/rev/149ba9431791 https://hg.mozilla.org/mozilla-central/rev/476642ee328b https://hg.mozilla.org/mozilla-central/rev/7b2f3fa349b0