Closed Bug 1296516 Opened 8 years ago Closed 7 years ago

Cleanup a bit of code in layout/base

Categories

(Core :: Layout, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(4 files, 3 obsolete files)

Bug 1296516: Tidy RestyleManager::ContentStateChanged.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1296516: Indentation nits in nsFrameManager.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1296516: Convert UndisplayedMap to a typed hashtable.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1296516: Cleanup infallible or unchecked nsCSSFrameConstructor methods.
58 bytes, text/x-review-board-request
heycam
: review+
Details 
Note that I don't know if all the patches are worth the churn, specially the indentation ones and the "remove unchecked nsresult" ones, but now I have them written, it seems like it would be nice to hear comments.
Comment on attachment 8782751 [details]
Bug 1296516: Tidy RestyleManager::ContentStateChanged.

https://reviewboard.mozilla.org/r/72798/#review70600
Attachment #8782751 - Flags: review?(cam) → review+
Comment on attachment 8782752 [details]
Bug 1296516: Indentation nits in nsFrameManager.

https://reviewboard.mozilla.org/r/72800/#review70602
Attachment #8782752 - Flags: review?(cam) → review+
Comment on attachment 8782753 [details]
Bug 1296516: Convert UndisplayedMap to a typed hashtable.

https://reviewboard.mozilla.org/r/72802/#review70604
Attachment #8782753 - Flags: review?(cam) → review+
Comment on attachment 8782756 [details]
Bug 1296516: Remove misindented and commented out preprocessor directives.

https://reviewboard.mozilla.org/r/72808/#review70612

::: layout/base/nsFrameManager.cpp
(Diff revision 1)
> -  #ifdef DEBUG
> -    //#define DEBUG_UNDISPLAYED_MAP
> -    //#define DEBUG_DISPLAY_CONTENTS_MAP
> -  #else
> -    #undef DEBUG_UNDISPLAYED_MAP
> -    #undef DEBUG_DISPLAY_CONTENTS_MAP
> -  #endif

Perhaps instead we should just leave the commented out #defines, so that if someone wants to enable this debugging they can find the right macros to use at the top of this file.  I agree we don't need to put them inside the #ifdef DEBUG though.  And we don't want them on all the time for DEBUG builds.
Attachment #8782756 - Flags: review?(cam) → review-
Comment on attachment 8782756 [details]
Bug 1296516: Remove misindented and commented out preprocessor directives.

https://reviewboard.mozilla.org/r/72808/#review70614

r=me with that change.
Attachment #8782756 - Flags: review- → review+
Comment on attachment 8782754 [details]
Bug 1296516: Cleanup infallible or unchecked nsCSSFrameConstructor methods.

https://reviewboard.mozilla.org/r/72804/#review120334

Per discussion a few weeks ago, I'm cancelling review on this for now, while there are other higher priority things to look at.
Attachment #8782754 - Flags: review?(cam)
Comment on attachment 8782755 [details]
Bug 1296516: Stop using the raw PLDHashTable API for the placeholder map.

https://reviewboard.mozilla.org/r/72806/#review120336
Attachment #8782755 - Flags: review?(cam)
Comment on attachment 8782757 [details]
Bug 1296516: Cleanup infallible or unchecked nsCSSFrameConstructor methods.

https://reviewboard.mozilla.org/r/72810/#review120340
Attachment #8782757 - Flags: review?(cam)
ni? myself to review these -- looks like we need them after all.
Flags: needinfo?(cam)
Comment on attachment 8782754 [details]
Bug 1296516: Cleanup infallible or unchecked nsCSSFrameConstructor methods.

https://reviewboard.mozilla.org/r/72804/#review123708

r=me with these addressed.

::: layout/base/nsFrameManager.h:36
(Diff revision 1)
>  namespace mozilla {
>  /**
>   * Node in a linked list, containing the style for an element that
>   * does not have a frame but whose parent does have a frame.
>   */
> -struct UndisplayedNode {
> +struct UndisplayedNode : public LinkedListElement<UndisplayedNode> {

Nit: '{' on new line.

::: layout/base/nsFrameManager.cpp:90
(Diff revision 1)
>  // XXXldb This seems too complicated for what I think it's doing, and it
>  // should also be using PLDHashTable rather than plhash to use less memory.

This comment can be removed.

::: layout/base/nsFrameManager.cpp:95
(Diff revision 1)
>  // XXXldb This seems too complicated for what I think it's doing, and it
>  // should also be using PLDHashTable rather than plhash to use less memory.
>  
> -class nsFrameManagerBase::UndisplayedMap {
> +/**
> + * The undisplayed map is a class that maps a parent content node to the
> + * undisplayed content children, and their style context.

s/context/contexts/.

::: layout/base/nsFrameManager.cpp:98
(Diff revision 1)
> -class nsFrameManagerBase::UndisplayedMap {
> +/**
> + * The undisplayed map is a class that maps a parent content node to the
> + * undisplayed content children, and their style context.
> + *
> + * The linked list of nodes holds strong references to the style contexts and
> + * the style context.

s/style context/content/.

::: layout/base/nsFrameManager.cpp:119
(Diff revision 1)
>  
>    void RemoveNodeFor(nsIContent* aParentContent,
>                       UndisplayedNode* aNode);
>  
>    void RemoveNodesFor(nsIContent* aParentContent);
> -  UndisplayedNode* UnlinkNodesFor(nsIContent* aParentContent);
> +  nsAutoPtr<LinkedList<UndisplayedNode>> UnlinkNodesFor(nsIContent* aParentContent);

I was going to suggest using UniquePtr rather than nsAutoPtr (per http://whereswalden.com/2014/07/31/mfbt-now-has-uniqueptr-and-makeunique-for-managing-singly-owned-resources/) but I see that nsClassHashtable's API works with nsAutoPtr.

::: layout/base/nsFrameManager.cpp:126
(Diff revision 1)
>    // Removes all entries from the hash table
>    void  Clear();
>  
>  protected:
>    /**
> -   * Gets the entry for the provided parent content. If the content
> +   * Gets the entry for the provided parent content.

s/entry/list/

::: layout/base/nsFrameManager.cpp
(Diff revision 1)
> -#if defined(DEBUG_UNDISPLAYED_MAP) || defined(DEBUG_DISPLAY_BOX_CONTENTS_MAP)
> -  static int i = 0;
> -  printf("SetStyleContextInMap(%d): p=%p \n", i++, (void *)aContent);
> -#endif

We should either leave this debugging info, or remove all of it from the file.  (If anyone is using it, it would be Mats, so if you think it should be removed, check with him first.)

::: layout/base/nsFrameManager.cpp:743
(Diff revision 1)
> -      if (node->mContent == aNode->mContent) {
> -        // We actually need to check this in optimized builds because
> +    // NOTE: In the original code there was a work around for this case, I want
> +    // to check it still happens before hacking around it the same way.

I think it's worth checking with dbaron to see if he thinks this might still be a problem.

::: layout/base/nsFrameManager.cpp:788
(Diff revision 1)
>  }
>  
>  void
>  nsFrameManagerBase::UndisplayedMap::RemoveNodesFor(nsIContent* aParentContent)
>  {
> -  delete UnlinkNodesFor(aParentContent);
> +  auto list = UnlinkNodesFor(aParentContent);

I would prefer not using auto with a type that has non-obvious copying semantics, like nsAutoPtr.  Please write the type out explicitly.
Attachment #8782754 - Flags: review+
Comment on attachment 8782755 [details]
Bug 1296516: Stop using the raw PLDHashTable API for the placeholder map.

https://reviewboard.mozilla.org/r/72806/#review123712

Is there a reason we can't get rid of the SetPlaceholderFrame method, and just always store mPlaceholderFrame in the PlaceholderMapEntry constructor?
Comment on attachment 8782757 [details]
Bug 1296516: Cleanup infallible or unchecked nsCSSFrameConstructor methods.

https://reviewboard.mozilla.org/r/72810/#review123714

This patch might have bitrotted a bit, sorry.

::: layout/base/nsPresShell.cpp:4480
(Diff revision 1)
>    mFrameConstructor->BeginUpdate();
> -  nsresult rv = mFrameConstructor->ReconstructDocElementHierarchy();
> +  mFrameConstructor->ReconstructDocElementHierarchy();
>    VERIFY_STYLE_TREE;
>    mFrameConstructor->EndUpdate();
>  
> -  return rv;
> +  return NS_OK;

PresShell::ReconstructFrames now always returns NS_OK, so feel free to remove the nsresult return value from it too.
Attachment #8782757 - Flags: review+
Flags: needinfo?(cam)
Comment on attachment 8782754 [details]
Bug 1296516: Cleanup infallible or unchecked nsCSSFrameConstructor methods.

https://reviewboard.mozilla.org/r/72804/#review123708

> I think it's worth checking with dbaron to see if he thinks this might still be a problem.

I did some bug archeology and seems that the proper fix for this was bug 145737. Will check with him though.

> I would prefer not using auto with a type that has non-obvious copying semantics, like nsAutoPtr.  Please write the type out explicitly.

done.
Attachment #8782755 - Attachment is obsolete: true
Attachment #8782756 - Attachment is obsolete: true
Attachment #8782757 - Attachment is obsolete: true
Blocks: 1348600
Comment on attachment 8782755 [details]
Bug 1296516: Stop using the raw PLDHashTable API for the placeholder map.

https://reviewboard.mozilla.org/r/72806/#review123712

I ended up dropping this change, for now at least.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b7c568cd42f
Tidy RestyleManager::ContentStateChanged. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/639016661f58
Indentation nits in nsFrameManager. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/60b20bf287b3
Convert UndisplayedMap to a typed hashtable. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec098c9386d3
Cleanup infallible or unchecked nsCSSFrameConstructor methods. r=heycam
I landed via inbound because mozreview ended up quite confused after dropping the placeholder map changeset.

IRC conversation with dbaron re. the insertBack change:

21:15 <~dbaron> emilio: I think if you assume it's fixed, you should make it a MOZ_ASSERT so that we find out if you're wrong
21:16 <~dbaron> emilio: but I'm not particularly knowledgable about whether it would be fixed.
21:16 <~dbaron> emilio: (I'd also maybe worry about imagemap areas)
21:16 <emilio> dbaron: yeah, I added a proper assertion walking up the list in debug builds. It seems like no test in try hits that assertion.
21:17 <~dbaron> emilio: ok.  (Keep an eye out for fuzzer bugs, though.)
21:17 <emilio> dbaron: ok, thanks for the reply! :)
Blocks: 560616
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: