Closed Bug 1419597 Opened 2 years ago Closed 2 years ago

Remove unused members from nsGlobalWindow{Inner,Outer} and nsPIDOMWindow{Inner,Outer}

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

Details

Attachments

(18 files, 2 obsolete files)

85.52 KB, patch
smaug
: review+
Details | Diff | Splinter Review
25.66 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.40 KB, patch
smaug
: review+
Details | Diff | Splinter Review
14.41 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.03 KB, patch
enndeakin
: review+
smaug
: review+
Details | Diff | Splinter Review
6.79 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.96 KB, patch
smaug
: review+
Details | Diff | Splinter Review
26.56 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.56 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.68 KB, patch
asuth
: review+
Details | Diff | Splinter Review
4.84 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.96 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.96 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.38 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.66 KB, patch
smaug
: review+
Details | Diff | Splinter Review
197.84 KB, patch
smaug
: review+
Details | Diff | Splinter Review
22.19 KB, patch
smaug
: review+
Details | Diff | Splinter Review
28.02 KB, patch
smaug
: review+
Details | Diff | Splinter Review
No description provided.
MozReview-Commit-ID: KXsnYLYtICV
Attachment #8930728 - Flags: review?(bugs)
MozReview-Commit-ID: 9TUURbj9s7N
Attachment #8930729 - Flags: review?(bugs)
MozReview-Commit-ID: FJQ6GmxE5gq
Attachment #8930730 - Flags: review?(bugs)
MozReview-Commit-ID: 4VXKn6dnYKH
Attachment #8930731 - Flags: review?(bugs)
MozReview-Commit-ID: 62pgCjfIsvu
Attachment #8930732 - Flags: review?(bugs)
MozReview-Commit-ID: KeZR0knQcbl
Attachment #8930733 - Flags: review?(bugs)
MozReview-Commit-ID: 5fj3pBMmR8E
Attachment #8930734 - Flags: review?(bugs)
MozReview-Commit-ID: 8VaBoUj9nOj
Attachment #8930735 - Flags: review?(bzbarsky)
MozReview-Commit-ID: CQRRyTImnUG
Attachment #8930736 - Flags: review?(bugs)
MozReview-Commit-ID: A5xJcTnicyb
Attachment #8930737 - Flags: review?(bugs)
MozReview-Commit-ID: KkG6hQentSH
Attachment #8930738 - Flags: review?(bugs)
MozReview-Commit-ID: 7qES6MrIxK9
Attachment #8930739 - Flags: review?(bugmail)
MozReview-Commit-ID: Jk6BDE3PzF
Attachment #8930741 - Flags: review?(bugs)
MozReview-Commit-ID: 7a1jiE7FKcs
Attachment #8930742 - Flags: review?(bugs)
MozReview-Commit-ID: 8yLkJE4cGra
Attachment #8930743 - Flags: review?(bugs)
MozReview-Commit-ID: HBRWzlBqnCT
Attachment #8930744 - Flags: review?(bugs)
MozReview-Commit-ID: DAAm6tLubhJ
Attachment #8930745 - Flags: review?(bugs)
In total these patches kill about 1.5K lines - probably mostly due to removing the IsInnerWindow and IsOuterWindow methods.
Comment on attachment 8930739 [details] [diff] [review]
Part 12: Move storage pref enabled check into a static method on dom::Storage

Review of attachment 8930739 [details] [diff] [review]:
-----------------------------------------------------------------

Yay, cleanup!
Attachment #8930739 - Flags: review?(bugmail) → review+
Comment on attachment 8930728 [details] [diff] [review]
Part 1: Split the methods on nsPIDOMWindow<T>

Code moves and random styling fixes really shouldn't go to the same patch. Make comparing code moves really tricky.
Attachment #8930728 - Flags: review?(bugs) → review+
Attachment #8930729 - Flags: review?(bugs) → review+
Comment on attachment 8930730 [details] [diff] [review]
Part 3: Remove unused fields from nsPIDOMWindowInner

// XXX: mDocShell is never set on the outer window? 
sounds like a buggy comment. Shouldn't you say inner.

I commented in https://bugzilla.mozilla.org/show_bug.cgi?id=1281949#c6
Attachment #8930730 - Flags: review?(bugs) → review+
Attachment #8930731 - Flags: review?(bugs) → review+
Comment on attachment 8930732 [details] [diff] [review]
Part 5: Remove mFocusedNode from the outer window

better to push this kinds of patches to try, all platforms.
Attachment #8930732 - Flags: review?(enndeakin)
Attachment #8930732 - Flags: review?(bugs)
Attachment #8930732 - Flags: review+
Attachment #8930735 - Flags: review?(bzbarsky) → review+
Attachment #8930736 - Flags: review?(bugs) → review+
Attachment #8930743 - Flags: review?(bugs) → review+
Attachment #8930742 - Flags: review?(bugs) → review+
Attachment #8930740 - Flags: review?(bugs) → review+
Attachment #8930741 - Flags: review?(bugs) → review+
Comment on attachment 8930738 [details] [diff] [review]
Part 11: Move gRefCnt/gSerialCounter into nsContentUtils

> PopupControlState nsContentUtils::sPopupControlState = openAbused;
> 
>+int32_t nsContentUtils::sInnerOrOuterWindowCount = 0;
>+uint32_t nsContentUtils::sInnerOrOuterSerialCounter = 0;
sInnerOrOuterWindowSerialCounter
Attachment #8930738 - Flags: review?(bugs) → review+
Attachment #8930737 - Flags: review?(bugs) → review+
Comment on attachment 8930733 [details] [diff] [review]
Part 6: Remove unused data members from nsGlobalWindowInner

@@ -2586,16 +2521,17 @@ nsGlobalWindowInner::GetU2f(ErrorResult& aError)
   if (!mU2F) {
     RefPtr<U2F> u2f = new U2F(this);
     u2f->Init(aError);
     if (NS_WARN_IF(aError.Failed())) {
       return nullptr;
     }
 
     mU2F = u2f;
+
   }
   return mU2F;
 }
odd new line


Hmm, have we regressed mWindowUtils handling.
Could you explain why GetInterface on inner doesn't support that anymore.
Attachment #8930733 - Flags: review?(bugs) → review-
Comment on attachment 8930734 [details] [diff] [review]
Part 7: Remove unused data members from nsGlobalWindowOuter

Why mScreen isn't removed?
Attachment #8930734 - Flags: review?(bugs) → review-
Attachment #8930744 - Flags: review?(bugs) → review+
Attachment #8930745 - Flags: review?(bugs) → review+
Attachment #8930732 - Flags: review?(enndeakin) → review+
(In reply to Olli Pettay [:smaug] from comment #25)
> Hmm, have we regressed mWindowUtils handling.
> Could you explain why GetInterface on inner doesn't support that anymore.

GetInterface() used to be implemented like this:
https://dxr.mozilla.org/mozilla-release/rev/3702966a64c80e17d01f613b0a464f92695524fc/dom/base/nsGlobalWindow.cpp#11431-11497

In each of the branches other than the last one (which calls QueryInterface()), we would first get the outer window, and then attempt to query interface some field from that object. When I performed the split, I moved this logic into nsGlobalWindowOuter::GetInterfaceInternal https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/dom/base/nsGlobalWindowOuter.cpp#7032-7095.

In both nsGlobalWindowInner::GetInterface and nsGlobalWindowOuter::GetInterface, I would first call nsGlobalWindowOuter::GetInterfaceInternal, and if that didn't find anything I would try to QueryInterface `this` into the requested type before failing. This kept all of the behaviour of the old system. https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/dom/base/nsGlobalWindowInner.cpp#4894-4905

The only difference here was mWindowUtils. In the nsIDOMWindowUtils branch, previously we would cache the nsDOMWindowUtils for our outer window directly on the inner window. Under the new system, we only cache this object on the outer window, and always go from the inner window to the outer window in order to fetch it. This makes 2 differences:

1) We only ever allocate one nsDOMWindowUtils per nsGlobalWindowOuter, when previously we could have multiple identical nsDOMWindowUtils objects per nsGlobalWindowOuter. As far as I know this should be a strict improvement, as nsDOMWindowUtils holds no state other than the window which it wraps.
2) We now have to reach into the outer window to fetch the nsDOMWindowUtils, as we don't have a local reference cached. I doubt this has any performance impact, and IMO it is worth the reduced complexity.

Other than those two things, the new code should act exactly the same :-). 

(In reply to Olli Pettay [:smaug] from comment #26)
> Why mScreen isn't removed?

I grepped & saw the usage in FullscreenTransitionTask, and my brain just marked it as used - It's gone now (in the new patch I'll attach soon).
MozReview-Commit-ID: KeZR0knQcbl
Attachment #8931093 - Flags: review?(bugs)
Attachment #8930733 - Attachment is obsolete: true
Attachment #8930734 - Attachment is obsolete: true
MozReview-Commit-ID: 5fj3pBMmR8E
Attachment #8931094 - Flags: review?(bugs)
Attachment #8931093 - Flags: review?(bugs) → review+
Attachment #8931094 - Flags: review?(bugs) → review+
Comment on attachment 8930735 [details] [diff] [review]
Part 8: Completely remove unused DialogValueHolder

r=me
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45bc58676473
Part 1: Split the methods on nsPIDOMWindow<T>, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f118ef2d9bc5
Part 2: Remove nsPIDOMWindow, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3541d1a5aa
Part 3: Remove unused fields from nsPIDOMWindowInner, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d481d31aa173
Part 4: Remove unused fields from nsPIDOMWindowOuter, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbe03a8a73b
Part 5: Remove mFocusedNode from the outer window, r=enndeakin
https://hg.mozilla.org/integration/mozilla-inbound/rev/e44017bfb64c
Part 6: Remove unused data members from nsGlobalWindowInner, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9d439ff95a
Part 7: Remove unused data members from nsGlobalWindowOuter, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f675487935bb
Part 8: Completely remove unused DialogValueHolder, r=smaug,bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd202150d3a
Part 9: Move the remaining defines out of nsGlobalWindow.h, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/587d50fa8a00
Part 10: Move includes into nsGlobalWindow{Inner,Outer}.cpp, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea002e5cd379
Part 11: Move gRefCnt/gSerialCounter into nsContentUtils, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/900b67493489
Part 12: Move storage pref enabled check into a static method on dom::Storage, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f5b1304142
Part 13: Move gDragServiceDisabled and gMouseDown to be statics on nsGlobalWindowInner, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/21361dc9f44b
Part 14: Clean up some out of date documentation, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a96b4dda871e
Part 15: Build nsGlobalWindow{Inner,Outer}.cpp separately, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9ec03aaf650
Part 16: Clarify crash message in some outer window unsupported methods, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f512066ce3f
Part 17: Avoid implementing *OpenerForInitialContentBrowser on the inner, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4605bc50d4e
Part 18: Remove IsInnerWindow and IsOuterWindow methods, r=smaug
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a84e38913297
Part 19: Remove accidental version control marker on a CLOSED TREE, a=bustage
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.