Closed
Bug 1348665
Opened 7 years ago
Closed 7 years ago
Remove the ViewProperty frame property
Categories
(Core :: Layout, enhancement)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
(Blocks 1 open bug)
Details
(Keywords: perf)
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
dholbert
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
tnikkel
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
tnikkel
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
tnikkel
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
tnikkel
:
review+
|
Details |
The ViewProperty stores the nsView* for a frame. Very few frames use nsViews these days, only: SubDocumentFrame ListControlFrame (only when used for (non-e10s?) comboboxes) PluginFrame ViewportFrame MenuPopupFrame We can get rid of the frame property and instead store the view pointer in a field on those frame classes.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d86c01ae9a0d3ad9ae50a5896ed45b9973739524
Assignee | ||
Comment 7•7 years ago
|
||
(In part 4, we could get rid of the aView param too if we moved the SetView call before the SyncFrameViewProperties call here: http://searchfox.org/mozilla-central/rev/c9f5cc6b4593d461e87a6221dc0286b3859fd515/layout/generic/nsContainerFrame.cpp#440,459 I haven't checked if that would be OK to do though.)
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8848900 [details] Bug 1348665 part 1 - Move a few nsView related methods from nsContainerFrame to nsIFrame/nsFrame (idempotent patch). https://reviewboard.mozilla.org/r/121770/#review124098 r- on this part, since I don't think it compiles yet. (Might be trivial to make it compile, or might not be; r- for now since I'm not sure) ::: layout/generic/nsContainerFrame.cpp (Diff revision 1) > -void > -nsContainerFrame::SyncFrameViewProperties(nsPresContext* aPresContext, > - nsIFrame* aFrame, > - nsStyleContext* aStyleContext, Two things regarding this old function-signature: (1) We have several calls to this function, using the old function-signature, which this patch doesn't yet modify. Those need to be updated, or else this patch won't build on its own: https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsContainerFrame%3A%3ACreateViewForFrame%28nsIFrame+%2A%2C+bool%29%22 Otherwise, this isn't really an idempotent or atomic commit. (It looks like some [maybe all?] of these callers get modified or removed by later patches here, so presumably they're all fine by the end of the patch stack. But still good to have each intermediate point be buildable.) (2) There's a comment that mentions the old function signature (including "nsContainerFrame::") here: https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsMenuPopupFrame.cpp#2439 Please update that comment as part of this patch, now that the method signature is changing.
Attachment #8848900 -
Flags: review?(dholbert) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8848900 [details] Bug 1348665 part 1 - Move a few nsView related methods from nsContainerFrame to nsIFrame/nsFrame (idempotent patch). https://reviewboard.mozilla.org/r/121770/#review124124 Thanks -- r=me
Attachment #8848900 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 15•7 years ago
|
||
OK, I've addressed those points and verified that each part compiles. (I think they should also pass tests). I made the nsMenuPopupFrame.cpp code comment in part 1 use the new name nsFrame::CreateView after the renaming in part 2, to avoid having to edit it twice.
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8848900 [details] Bug 1348665 part 1 - Move a few nsView related methods from nsContainerFrame to nsIFrame/nsFrame (idempotent patch). https://reviewboard.mozilla.org/r/121770/#review124128 ::: commit-message-e5ca7:6 (Diff revisions 1 - 2) > Bug 1348665 part 1 - Move a few nsView related methods from nsContainerFrame to nsIFrame/nsFrame (idempotent patch). r=dholbert > > Views are used for PluginFrame which inherits nsFrame, not nsContainerFrame, > so it's more appropriate that these methods should live in nsIFrame/nsFrame. > > -MozReview-Commit-ID: EmDtVdUdQFD > +MozReview-Commit-ID: 8zxTDneOE4k BTW, it looks like your some piece of your workflow must've regenerated the MozReview-Commit-ID on some of these patches (parts 1 and 2 at least, haven't checked the others), in the update that you just pushed. That's something you want to avoid, because it makes it harder for MozReview to figure out oldpatch<-->newpatch correspondences & generate useful interdiffs. So, might be useful to see if you can figure out which of your steps caused that change & adjust that step to prevent the MozReview-Commit-ID bump in the future.
Assignee | ||
Comment 17•7 years ago
|
||
I don't know where those "MozReview-Commit-ID" things come from. The command I use is: hg push ssh://reviewboard-hg.mozilla.org/gecko I guess I've updated the tree since the last push though, is that a problem?
Comment 18•7 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #17) > I don't know where those "MozReview-Commit-ID" things come from. > The command I use is: hg push ssh://reviewboard-hg.mozilla.org/gecko They're part of the commit message, and they're supposed to be generated when you first create a commit -- long before you push. (They're auto-generated by one of the local extensions that are installed as part of ./mach mercurial-setup I think.) You might want to run ./mach mercurial-setup if you haven't done so recently -- it's possible one of your extensions is out of date. > I guess I've updated the tree since the last push though, is that a problem? No, that shouldn't matter. In any case, it's fine for this bug at the moment -- MozReview is smart enough to match up Old-Part-N against New-Part-N, so all is well. It only causes problems when you insert a new patch into the stack (e.g. suppose you insert a new patch between parts 2 and 3 and renumber parts 3-5 to now be parts 4-6. MozReview isn't smart enough to figure that out -- it'll generate a bizarre-looking interdiff between Old Part 3 and the completely-different New Part 3, rather than New Part 4.) Anyway, if you're curious to poke around more, you should be able to experiment without needing to push to MozReview by simply inspecting your changesets locally (e.g. "hg export [CSETID] | head -n20") after making more changes like the ones you just made.
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8848901 [details] Bug 1348665 part 2 - Remove the ViewProperty and store the nsView* in a field on the relevant frame classes instead. https://reviewboard.mozilla.org/r/121772/#review124104 ::: layout/forms/nsListControlFrame.h:405 (Diff revision 2) > int32_t mStartSelectionIndex; > int32_t mEndSelectionIndex; > > - nsIComboboxControlFrame *mComboboxFrame; > - uint32_t mNumDisplayRows; > + nsIComboboxControlFrame* mComboboxFrame; > + > + // The view is only created if IsInDropDownMode() is true. s/created/created (& non-null)/ Without that clarification, it kinda sounds like this pointer might just be uninitialized/bogus in cases where IsInDropDownMode returns false. ::: layout/forms/nsListControlFrame.cpp:981 (Diff revision 2) > + CreateView(); > + } > + Fix trailing whitespace on blank line at the end here. ::: layout/generic/nsFrame.h:599 (Diff revision 2) > - * Helper method to wrap views around frames. Used by containers > - * under special circumstances (can be used by leaf frames as well) > + * Helper method to create a view for a frame. Only used by a few sub-classes > + * that needs a view. > */ > - static void CreateViewForFrame(nsIFrame* aFrame, > + void CreateView(); s/needs/need/ ::: layout/generic/nsFrame.cpp:5989 (Diff revision 2) > - // Set a property on the frame > - Properties().Set(ViewProperty(), aView); > + // Store the view on the subclass frame. > + SetViewInternal(aView); Nit: s/on the subclass frame/via the frame's subclass/ (or something like that). The term "subclass frame" here sounds like a specific obscure type of frame, i.e. "nsSubclassFrame". ::: layout/generic/nsFrame.cpp:5999 (Diff revision 2) > f->AddStateBits(NS_FRAME_HAS_CHILD_WITH_VIEW); > + } else { > + MOZ_ASSERT_UNREACHABLE("Destroying a view while the frame is alive?"); > + RemoveStateBits(NS_FRAME_HAS_VIEW); > + SetViewInternal(nullptr); > } The MOZ_ASSERT_UNREACHABLE here implies that we're expecting the arg must always be non-null. Instead of sort-of-gracefully handling the unexpected scenario here with some unreachable cleanup code, perhaps we should make this method (and SetViewInternal) take type mozilla::NotNull<nsView*>? That would give us better enforcement & documentation of this arg's non-nullness, and we wouldn't need to bother with this unreachable cleanup code I think. (It'd kinda be nice to do this up-front as part of this patch, so that you never have to introduce this unreachable code in the first place. Though if you'd prefer a followup, that's fine too.) Relatedly, I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1348993 on making CreateView more explicitly infallible (with NotNull) -- maybe that's where you'd want to follow up on this. ::: layout/generic/nsIFrame.h:616 (Diff revision 2) > * the frame. > * > * If the frame is a continuing frame, then aPrevInFlow indicates the previous > * frame (the frame that was split). > * > - * If you want a view associated with your frame, you should create the view > + * Subclasses that needs a view should override this method and call s/needs/need/ ::: layout/generic/nsIFrame.h:617 (Diff revision 2) > * > * If the frame is a continuing frame, then aPrevInFlow indicates the previous > * frame (the frame that was split). > * > - * If you want a view associated with your frame, you should create the view > - * after Init() has returned. > + * Subclasses that needs a view should override this method and call > + * CreateView() after calling its baseclass Init(). s/its baseclass/the base class's/ (The word "its" doesn't make grammatical sense here, since we're referring to something plural, "Subclasses".) Alternately, you could rephrase this sentence as "Each subclass that needs a view should [...] its baseclass Init()" ::: layout/generic/nsIFrame.h:2459 (Diff revision 2) > + { > + return MOZ_LIKELY(!HasView()) ? nullptr : GetViewInternal(); This is elegant as a one-liner, but you probably should make it a bit more complex so that you can verify that all of our subclass's GetViewInternal methods are actually correct. I think this impl should be something like: ======== if (MOZ_LIKELY(!HasView)) { return nullptr; } nsView* view = GetViewInternal(); MOZ_ASSERT(view, "GetViewInternal() should agree with HasView()"); return view; ======== (That might mean that the impl wants to move to nsFrame.cpp, too, since that's a bit long for an inline function in a header file.) ::: layout/generic/nsSubDocumentFrame.cpp:125 (Diff revision 2) > - // We are going to create an inner view. If we need a view for the > + CreateView(); > - // OuterFrame but we wait for the normal view creation path in > - // nsCSSFrameConstructor, then we will lose because the inner view's > - // parent will already have been set to some outer view (e.g., the > - // canvas) when it really needs to have this frame's view as its > - // parent. So, create this frame's view right away, whether we > - // really need it or not, and the inner view will get it as the > - // parent. > - if (!HasView()) { > - nsFrame::CreateViewForFrame(this, true); > - } > EnsureInnerView(); It would be nice to preserve some version of this comment, since we've got two views in play here and their relationship is kind of confusing. (The comment that you're removing explained their relationship a bit.) If nothing else, it'd be good to clearly-document here that the ordering of "CreateView" & "EnsureInnerView" is very important (because the latter depends on the former). ::: layout/xul/nsMenuPopupFrame.h:560 (Diff revision 2) > nsMenuFrame* mCurrentMenu; // The current menu that is active. > + nsView* mView; Don't forget to add this to the init list -- after mCurrentMenu here: https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/layout/xul/nsMenuPopupFrame.cpp#96
Attachment #8848901 -
Flags: review?(dholbert) → review-
Comment 20•7 years ago
|
||
You should probably tag tnikkel for the remaining patches here (& for final-round review on part 2 after nits are addressed), since he understands views better than I do and would more likely to know about invariants that you might be incidentally violating here. (stuff that I'm blissfully unaware of) I should've realized that before I dove into reviewing here in the first place. :) Hopefully my review comments so far have been helpful anyway though. (I could probably r+ the remaining parts, too, but tnikkel would be more likely than me to know whether e.g. we're really intending to use some of the currently-unused-and-being-refactored-away stuff.)
Flags: needinfo?(mats)
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8848901 [details] Bug 1348665 part 2 - Remove the ViewProperty and store the nsView* in a field on the relevant frame classes instead. https://reviewboard.mozilla.org/r/121772/#review124170 ::: layout/generic/nsPluginFrame.h:338 (Diff revision 2) > // updates. > RefPtr<nsRootPresContext> mRootPresContextRegisteredWith; > > mozilla::UniquePtr<PluginFrameDidCompositeObserver> mDidCompositeObserver; > + > + nsView* mView; We also have an nsView* mInnerView. We should declare them in the same place. And maybe call this the outer view if you think that's clearer.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•7 years ago
|
||
>(That might mean that the impl wants to move to nsFrame.cpp, too, since that's >a bit long for an inline function in a header file.) I tried to move GetView() to nsIFrameInlines.h but that file includes among other things nsPlaceholderFrame.h which isn't exported, so include-hell ensued... So I decided that the method is short enough for nsIFrame.h after all. :-) > mozilla::NotNull<nsView*> Hmm, that's a mouthful for sure. Has there been any policy decision about this for layout code? I'd really hate to spell out mozilla::NotNull<nsIFrame*> everywhere it's expected to be non-null, which is most places. I punted on this to bug 1348993 for now... > We also have an nsView* mInnerView. Oh, nsPluginFrame also has an inner view? I missed that. I'll rename it and move them next to each other.
Flags: needinfo?(mats)
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8848901 [details] Bug 1348665 part 2 - Remove the ViewProperty and store the nsView* in a field on the relevant frame classes instead. https://reviewboard.mozilla.org/r/121772/#review124204
Attachment #8848901 -
Flags: review?(tnikkel) → review+
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8848902 [details] Bug 1348665 part 3 - Remove the aFlags param to SyncFrameViewProperties because all callers pass zero. https://reviewboard.mozilla.org/r/121774/#review124206
Attachment #8848902 -
Flags: review?(tnikkel) → review+
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8848903 [details] Bug 1348665 part 4 - Remove some params to SyncFrameViewProperties and make it a member function instead. Make the aView param optional, use the frame's view if none provided. https://reviewboard.mozilla.org/r/121776/#review124208
Attachment #8848903 -
Flags: review?(tnikkel) → review+
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8848904 [details] Bug 1348665 part 5 - Remove the aFrame param to ReparentFrameViewTo and make it a member function instead. https://reviewboard.mozilla.org/r/121778/#review124212
Attachment #8848904 -
Flags: review?(tnikkel) → review+
Comment 31•7 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #26) > Hmm, that's a mouthful for sure. (You can skip the "mozilla::" in .cpp files at least, of course, which makes it less of a mouthful.) > Has there been any policy decision about this for layout code? I'm not sure a policy decision is needed; this is just something (like UniquePtr) that we can & should just use for APIs as-needed, where it's helpful and where its validity (guaranteed-not-null-ness) is obvious. It was announced here: https://groups.google.com/forum/#!searchin/mozilla.dev.platform/notnull%7Csort:relevance/mozilla.dev.platform/6M_afibWhBA/MCmrYktaBgAJ > I'd really hate to spell out mozilla::NotNull<nsIFrame*> > everywhere it's expected to be non-null, which is most places. As noted above, the "mozilla::" prefix would only be in .h files, but setting that aside -- I agree that for nsIFrame*, a "NotNull"-correctness effort would be a large undertaking, and I think it'd perhaps benefit from a convenience typedef. Anyway, we're not considering NotNull<nsIFrame*> in this bug here -- I was suggesting it for nsView*, in the nsView-creation-to-SetView pipeline.) > I punted on this to bug 1348993 for now... Fine by me. Thanks! (and thanks tnikkel for taking over the review)
Comment 32•7 years ago
|
||
Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c90ad4b528aa part 1 - Move a few nsView related methods from nsContainerFrame to nsIFrame/nsFrame (idempotent patch). r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/4261e71cf24c part 2 - Remove the ViewProperty and store the nsView* in a field on the relevant frame classes instead. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/50de31a80717 part 3 - Remove the aFlags param to SyncFrameViewProperties because all callers pass zero. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/d767466dcdf7 part 4 - Remove some params to SyncFrameViewProperties and make it a member function instead. Make the aView param optional, use the frame's view if none provided. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/d6766b291e3b part 5 - Remove the aFrame param to ReparentFrameViewTo and make it a member function instead. r=tnikkel
Comment 33•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c90ad4b528aa https://hg.mozilla.org/mozilla-central/rev/4261e71cf24c https://hg.mozilla.org/mozilla-central/rev/50de31a80717 https://hg.mozilla.org/mozilla-central/rev/d767466dcdf7 https://hg.mozilla.org/mozilla-central/rev/d6766b291e3b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•