Closed Bug 1416384 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

(Blocks 1 open bug)

Details

Attachments

(13 files, 14 obsolete files)

1.07 MB, patch
Details | Diff | Splinter Review
548.64 KB, patch
Details | Diff | Splinter Review
45.39 KB, patch
Details | Diff | Splinter Review
252.59 KB, patch
Details | Diff | Splinter Review
242.55 KB, patch
Details | Diff | Splinter Review
253.27 KB, patch
Details | Diff | Splinter Review
13.47 KB, patch
Details | Diff | Splinter Review
246.44 KB, patch
Details | Diff | Splinter Review
260.90 KB, patch
Details | Diff | Splinter Review
79.85 KB, patch
Details | Diff | Splinter Review
2.68 KB, patch
Details | Diff | Splinter Review
103.91 KB, patch
Details | Diff | Splinter Review
32.62 KB, patch
Details | Diff | Splinter Review
No description provided.
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
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.
Flags: needinfo?(bugs)
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?
Flags: needinfo?(bugs)
(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
Attachment #8927931 - Flags: review?(bugs)
Attachment #8927510 - Attachment is obsolete: true
Attachment #8927501 - Flags: review?(bugs)
Attachment #8927502 - Flags: review?(bugs)
Attachment #8927503 - Flags: review?(bugs)
Attachment #8927504 - Flags: review?(bugs)
Attachment #8927505 - Flags: review?(bugs)
Attachment #8927506 - Flags: review?(bugs)
Attachment #8927507 - Flags: review?(bugs)
Attachment #8927508 - Flags: review?(bugs)
Attachment #8927509 - Flags: review?(bugs)
Attachment #8927511 - Flags: review?(bugs)
Priority: -- → P3
(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.
Attachment #8927501 - Flags: review?(bugs) → review-
Attachment #8927502 - Flags: review?(bugs) → review+
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
Attachment #8927503 - Flags: review?(bugs) → review+
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
Attachment #8927504 - Flags: review?(bugs) → review+
Attachment #8927505 - Flags: review?(bugs) → review+
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
Attachment #8927507 - Flags: review?(bugs) → review+
Attachment #8927511 - Flags: review?(bugs) → review+
Attachment #8927506 - Flags: review?(bugs) → review+
Attachment #8927508 - Flags: review?(bugs) → review+
Attachment #8927509 - Flags: review?(bugs) → review+
Attachment #8927931 - Flags: review?(bugs) → review+
MozReview-Commit-ID: KDllmZzdn6m
Attachment #8928317 - Flags: review?(bugs)
MozReview-Commit-ID: 1mzNDOFUNep
Attachment #8928318 - Flags: review?(bugs)
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.
Attachment #8928317 - Flags: review?(bugs) → review+
Attachment #8928318 - Flags: review?(bugs) → review+
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
Attachment #8927501 - Attachment is obsolete: true
Attachment #8927502 - Attachment is obsolete: true
Attachment #8927503 - Attachment is obsolete: true
Attachment #8927504 - Attachment is obsolete: true
Attachment #8927505 - Attachment is obsolete: true
Attachment #8927506 - Attachment is obsolete: true
Attachment #8927507 - Attachment is obsolete: true
Attachment #8927508 - Attachment is obsolete: true
Attachment #8927509 - Attachment is obsolete: true
Attachment #8927511 - Attachment is obsolete: true
Attachment #8927931 - Attachment is obsolete: true
Attachment #8928317 - Attachment is obsolete: true
Attachment #8928318 - Attachment is obsolete: true
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: 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
Blocks: 1454536
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.