1416384 - Split nsGlobalWindow's implementation into nsGlobalWindow{Inner,Outer}

Status: RESOLVED FIXED

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

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Copy nsGlobalWindow.{h,cpp} to nsGlobalWindow{Inner,Outer}.{h,cpp}, r=smaug

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

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2a: Delete nsGlobalWindow.{h,cpp}, r=smaug

This will make future diffs, which rewrite this file completely, much easier to
read.

MozReview-Commit-ID: HqomlBjrEW7

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2b: Get split headers building but not linking, r=smaug

MozReview-Commit-ID: JRvPtQTJqSX

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2c: Get nsGlobalWindowInner.cpp building, r=smaug

MozReview-Commit-ID: CmKx5jtvtrT

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2d: Get nsGlobalWindowOuter.cpp building, r=smaug

MozReview-Commit-ID: 4Yz8hRMZEJC

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2e: Eliminate duplicate declarations, and get gecko building again, r=smaug

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

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Remove nsPIDOMWindow<nsISupports>, r=smaug

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

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Eliminate outer window only methods from nsGlobalWindowInner, r=smaug

MozReview-Commit-ID: CV6rrA0M2ZV

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 5: Eliminate inner window only methods from nsGlobalWindowOuter, r=smaug

MozReview-Commit-ID: FzaGKmdDtmy

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 6: Eliminate calls to Assert{Inner,Outer} and As{Inner,Outer} in nsGlobalWindow, r=smaug

MozReview-Commit-ID: GIiSlDzjgWb

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 7: Move nsPIDOMWindow{Inner,Outer}::TabGroup into their respective cpps, r=smaug

MozReview-Commit-ID: AZMWwKFnvG9

Comment 12

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 13

[Nika Layzell [:nika] (ni? for response)]

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

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 15

[Masatoshi Kimura [:emk]]

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.)

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Sounds reasonable.
Are there patterns in code duplication?

Comment 17

[Nika Layzell [:nika] (ni? for response)]

(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.

Comment 18

[Andrew McCreight [:mccr8]]

(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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 20

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 6: Eliminate calls to Assert{Inner,Outer} and As{Inner,Outer} in nsGlobalWindow

MozReview-Commit-ID: GIiSlDzjgWb

Comment 21

[Masatoshi Kimura [:emk]]

(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 22

[Olli Pettay [:smaug][[e-mail]]]

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 23

[Olli Pettay [:smaug][[e-mail]]]

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 24

[Olli Pettay [:smaug][[e-mail]]]

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 25

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 28

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 8: Stylistic fixes in nsGlobalWindow{Inner,Outer}.cpp

MozReview-Commit-ID: KDllmZzdn6m

Comment 29

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 9: Deduplicate more code when possible

MozReview-Commit-ID: 1mzNDOFUNep

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 31

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Copy nsGlobalWindow.{h,cpp} to nsGlobalWindow{Inner,Outer}.{h,cpp}

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

Comment 32

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2a: Delete nsGlobalWindow.{h,cpp}

This will make future diffs, which rewrite this file completely, much easier to
read.

MozReview-Commit-ID: HqomlBjrEW7

Comment 33

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2b: Get split headers building but not linking

MozReview-Commit-ID: JRvPtQTJqSX

Comment 34

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2c: Get nsGlobalWindowInner.cpp building

MozReview-Commit-ID: CmKx5jtvtrT

Comment 35

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2d: Get nsGlobalWindowOuter.cpp building

MozReview-Commit-ID: 4Yz8hRMZEJC

Comment 36

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2e: Eliminate duplicate declarations, and get gecko building again

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

Comment 37

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Remove nsPIDOMWindow<nsISupports>

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

Comment 38

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Eliminate outer window only methods from nsGlobalWindowInner

MozReview-Commit-ID: CV6rrA0M2ZV

Comment 39

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 5: Eliminate inner window only methods from nsGlobalWindowOuter

MozReview-Commit-ID: FzaGKmdDtmy

Comment 40

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 6: Eliminate calls to Assert{Inner,Outer} and As{Inner,Outer} in nsGlobalWindow

MozReview-Commit-ID: GIiSlDzjgWb

Comment 41

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 7: Move nsPIDOMWindow{Inner,Outer}::TabGroup into their respective cpps

MozReview-Commit-ID: AZMWwKFnvG9

Comment 42

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 8: Stylistic fixes in nsGlobalWindow{Inner,Outer}.cpp

MozReview-Commit-ID: KDllmZzdn6m

Comment 43

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 9: Deduplicate more code when possible

MozReview-Commit-ID: 1mzNDOFUNep

Comment 44

[Pulsebot]

Pushed by [e-mail]:
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

Comment 45

[Cristian Brindusan [:cbrindusan]]

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