Closed Bug 1388939 Opened 7 years ago Closed 7 years ago

Clean up lots of misleadingly named and confusing nsFrameManager code

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(8 files)

Bug 1388939, part 1 - Rename nsFrameManagerBase::mUndisplayedMap to mDisplayNoneMap.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1388939, part 2 - Pair up and document the various nsFrameManager methods for keeping track of "undisplayed" style contexts.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1388939, part 3 - Give clear names to the nsFrameManager methods for registering undisplayed style contexts.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1388939, part 4 - Give clear names to the nsFrameManager methods for updating the registered undisplayed style contexts.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1388939, part 5 - Rename nsFrameManager::GetUndisplayedContent to GetDisplayNoneStyleFor.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1388939, part 6 - Give clear names to the nsFrameManager methods for obtaining the linked list of undisplayed style contexts for a node.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1388939, part 7 - Give clear names to the nsFrameManager methods for unregistering undisplayed style contexts.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1388939, part 8 - Remove nsFrameManager::GetDisplayContentsNodeFor.
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
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 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 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 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 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 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 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 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 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+
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
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: