Remove the ViewProperty frame property

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks 1 bug, {perf})

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments)

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.
(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 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 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+
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 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.
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?
(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.
See Also: → 1348993
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-
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 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.
>(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 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 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 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 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+
(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)
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
You need to log in before you can comment on or make changes to this bug.