If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Clean up lots of misleadingly named and confusing nsFrameManager code

RESOLVED FIXED in Firefox 57

Status

()

Core
Layout
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments)

59 bytes, text/x-review-board-request
dholbert
: review+
Details | Review
59 bytes, text/x-review-board-request
dholbert
: review+
Details | Review
59 bytes, text/x-review-board-request
dholbert
: review+
Details | Review
59 bytes, text/x-review-board-request
dholbert
: review+
Details | Review
59 bytes, text/x-review-board-request
dholbert
: review+
Details | Review
59 bytes, text/x-review-board-request
dholbert
: review+
Details | Review
59 bytes, text/x-review-board-request
dholbert
: review+
Details | Review
59 bytes, text/x-review-board-request
dholbert
: review+
Details | Review
The code in nsFrameManager that deals with keeping track of style contexts for display:none and display:contents content is significantly harder to grok and work on than it should be. Particularly the use of "UndisplayedContent" in function and variable names sometimes refers to display:none content, and sometimes refers to both display:none and display:contents content. Inconsistent naming between corresponding display:none and display:contents methods further compounds the problems.

This mess has made it harder to work on fixing bug 1367214 and has no doubt lengthened the review turnaround times. We really should clean this up.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

2 months ago
mozreview-review
Comment on attachment 8895613 [details]
Bug 1388939, part 1 - Rename nsFrameManagerBase::mUndisplayedMap to mDisplayNoneMap.

https://reviewboard.mozilla.org/r/166852/#review171962

r=me
Attachment #8895613 - Flags: review?(dholbert) → review+
Comment hidden (mozreview-request)

Comment 10

2 months ago
mozreview-review
Comment on attachment 8895614 [details]
Bug 1388939, part 2 - Pair up and document the various nsFrameManager methods for keeping track of "undisplayed" style contexts.

https://reviewboard.mozilla.org/r/166854/#review171964

::: layout/base/nsFrameManager.h:140
(Diff revision 1)
> -   * Return the linked list of UndisplayedNodes containing the registered
> -   * display:contents children of aParentContent, if any.
> +   * Return a linked list of style contexts registered for display:none
> +   * children of aParentContent.
> +   */
> +  UndisplayedNode* GetAllUndisplayedContentIn(nsIContent* aParentContent);

"linked list of style contexts" isn't quite right here.

This should probably return to saying "linked list of UndisplayedNodes" -- perhaps adding "(which are tuples of nsStyleContext + nsIContent)"

(Note that the next method's documentation says "Return the relevant undisplayed node" -- so we're not really hand-waving away the UndisplayedNode type in the documentation here.)

Feel free to address this & other documentation nits in a final followup, if that avoids bitrot hell.

(This applies to both methods in this pair)

::: layout/base/nsFrameManager.h:163
(Diff revision 1)
>    /**
> -   * Register aContent having a display:contents style context.
> -   */
> -  void SetDisplayContents(nsIContent* aContent, nsStyleContext* aStyleContext);
> +   * Unregister the style context for the display:none content, aContent,
> +   * if any.  If found, then also unregisters the style contexts for any
> +   * display:contents and display:none descendants of aContent.

s/also unregisters/also unregister/ (drop the trailing "s"), for consistency with the first sentence's phrasing.

("Unregister $stuff.  If $CONDITION, also unregister $other_stuff.")

Alternately, if you prefer: "If found, then this method also unregisters"

(This applies to both methods in this pair.)
Attachment #8895614 - Flags: review?(dholbert) → review+

Comment 11

2 months ago
mozreview-review
Comment on attachment 8895615 [details]
Bug 1388939, part 3 - Give clear names to the nsFrameManager methods for registering undisplayed style contexts.

https://reviewboard.mozilla.org/r/166856/#review171976

r=me
Attachment #8895615 - Flags: review?(dholbert) → review+

Comment 12

2 months ago
mozreview-review
Comment on attachment 8895616 [details]
Bug 1388939, part 4 - Give clear names to the nsFrameManager methods for updating the registered undisplayed style contexts.

https://reviewboard.mozilla.org/r/166858/#review171978

r=me
Attachment #8895616 - Flags: review?(dholbert) → review+

Comment 13

2 months ago
mozreview-review
Comment on attachment 8895617 [details]
Bug 1388939, part 5 - Rename nsFrameManager::GetUndisplayedContent to GetDisplayNoneStyleFor.

https://reviewboard.mozilla.org/r/166860/#review171980

::: dom/xbl/nsXBLResourceLoader.cpp:261
(Diff revision 1)
>              // Check to see if it's in the undisplayed content map, or the
>              // display: contents map.

nit: you should update this comment ("undisplayed content map" is now an obsolete-ish term).

Perhaps:
"...in the display:none or display:contents maps."

Probably good to do a grep for "undisplayed content" and see if there are any other mentions that might want an update, too.

::: dom/xbl/nsXBLService.cpp:148
(Diff revision 1)
>          // Check to see if it's in the undisplayed content map...
>          nsFrameManager* fm = shell->FrameManager();
> -        nsStyleContext* sc = fm->GetUndisplayedContent(mBoundElement);
> +        nsStyleContext* sc = fm->GetDisplayNoneStyleFor(mBoundElement);
>          if (!sc) {
>            // or in the display:contents map.
>            sc = fm->GetDisplayContentsStyleFor(mBoundElement);

Same here.
Attachment #8895617 - Flags: review?(dholbert) → review+

Comment 14

2 months ago
mozreview-review
Comment on attachment 8895618 [details]
Bug 1388939, part 6 - Give clear names to the nsFrameManager methods for obtaining the linked list of undisplayed style contexts for a node.

https://reviewboard.mozilla.org/r/166862/#review171986

::: layout/base/nsFrameManager.h:141
(Diff revision 1)
>      }
>      return GetStyleContextInMap(mDisplayContentsMap, aContent);
>    }
>  
>    /**
> -   * Return a linked list of style contexts registered for display:none
> +   * Return the linked list of style contexts registered for display:none

Extreme nit: looks like you're making a small tweak here (s/a/the/), in part 6, to some documentation that you rewrote in part 2 (which made the opposite change from "the" to "a" in the original documentation). :)

Perhaps best to fold this documentation-tweak back into part 2 -- or alternately, if you're adding an extra patch to address other documentation nits, fold this into that patch as well?

(This applies to both methods in this pair)
Attachment #8895618 - Flags: review?(dholbert) → review+

Comment 15

2 months ago
mozreview-review
Comment on attachment 8895619 [details]
Bug 1388939, part 7 - Give clear names to the nsFrameManager methods for unregistering undisplayed style contexts.

https://reviewboard.mozilla.org/r/166864/#review171988

r=me
Attachment #8895619 - Flags: review?(dholbert) → review+

Comment 16

2 months ago
mozreview-review
Comment on attachment 8895639 [details]
Bug 1388939, part 8 - Remove nsFrameManager::GetDisplayContentsNodeFor.

https://reviewboard.mozilla.org/r/166886/#review171992

This mostly seems like an OK simplification, but it does mean we'll do two lookups instead of one in some cases. Maybe that's OK because 'display:contents' is rare enough, though? (And I'm haven't looked closely to see how frequently we hit the code that does the second lookup -- do you know if that second lookup is the common case vs. uncommon case?)

If it's common that the first lookup implies we'll make the second lookup, then I tend to think we should pass on this.  But otherwise, *shrug*, seems like an OK simplification.

::: commit-message-e8137:1
(Diff revision 1)
> +Bug 1388939, part 8 - Remove nsFrameManager::GetDisplayContentsNodeFor. r=dholbert

Perhaps add:
"...and rewrite its caller to use other APIs"

Since: most of the patch is about that rewrite, rather than the function removal.
Attachment #8895639 - Flags: review?(dholbert) → review+

Comment 17

2 months ago
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5615257eb337
part 1 - Rename nsFrameManagerBase::mUndisplayedMap to mDisplayNoneMap. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/031a3b30aadf
part 2 - Pair up and document the various nsFrameManager methods for keeping track of "undisplayed" style contexts. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/0090af474246
part 3 - Give clear names to the nsFrameManager methods for registering undisplayed style contexts. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a86aaef19f7
part 4 - Give clear names to the nsFrameManager methods for updating the registered undisplayed style contexts. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d13c2c74262
part 5 - Rename nsFrameManager::GetUndisplayedContent to GetDisplayNoneStyleFor. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f02e4c2fc42
part 6 - Give clear names to the nsFrameManager methods for obtaining the linked list of undisplayed style contexts for a node. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f529070cc66
part 7 - Give clear names to the nsFrameManager methods for unregistering undisplayed style contexts. r=dholbert

Comment 18

2 months ago
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28226c771f11
part 8 - Remove nsFrameManager::GetDisplayContentsNodeFor and rewrite its caller to use other APIs. r=dholbert

Comment 19

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5615257eb337
https://hg.mozilla.org/mozilla-central/rev/031a3b30aadf
https://hg.mozilla.org/mozilla-central/rev/0090af474246
https://hg.mozilla.org/mozilla-central/rev/8a86aaef19f7
https://hg.mozilla.org/mozilla-central/rev/4d13c2c74262
https://hg.mozilla.org/mozilla-central/rev/6f02e4c2fc42
https://hg.mozilla.org/mozilla-central/rev/9f529070cc66
https://hg.mozilla.org/mozilla-central/rev/28226c771f11
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.