Closed Bug 1377902 Opened 7 years ago Closed 7 years ago

Trivially cleanup a bit the frame constructor.

Categories

(Core :: Layout, enhancement)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: emilio, Assigned: emilio)

Details

Attachments

(1 file)

Bug 1377902: Trivially cleanup a bit of the FC code.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details 
I was poking a bit at the frame constructor for bug 1377848, and meanwhile I went along and cleaned up a bit.

I wanted to run clang-format on it, but there were a few changes that were fairly debatable, so I haven't added them.
Comment on attachment 8883045 [details]
Bug 1377902: Trivially cleanup a bit of the FC code.

https://reviewboard.mozilla.org/r/154008/#review159154

::: layout/base/nsCSSFrameConstructor.cpp:7358
(Diff revision 1)
>    NS_ASSERTION(!rootElement || !rootElement->HasFlag(NODE_NEEDS_FRAME),
>      "root element should not have frame created lazily");
>    if (rootElement && rootElement->HasFlag(NODE_DESCENDANTS_NEED_FRAMES)) {
>      BeginUpdate();
> -    TreeMatchContext treeMatchContext(mDocument, TreeMatchContext::ForFrameConstruction);
> +    TreeMatchContext treeMatchContext(
> +        mDocument, TreeMatchContext::ForFrameConstruction);

Fwiw, personally I think it's ridiculous that we in 2017 are still adhering to this 80-column rule that is an artifact from IBM punch-cards from the early 19th century.  We should change this rule rather than formatting our code to be less readable.

::: layout/base/nsCSSFrameConstructor.cpp
(Diff revision 1)
>                    "Should be in an update while creating frames");
>  
>    NS_PRECONDITION(aStartChild, "must always pass a child");
>  
> -  // XXXldb Do we need to re-resolve style to handle the CSS2 + combinator and
> -  // the :empty pseudo-class?

You should probably check with dbaron if he wants to keep these comments (occurs twice).

::: layout/base/nsCSSFrameConstructor.cpp:8529
(Diff revision 1)
> -nsCSSFrameConstructor::ContentRemoved(nsIContent*  aContainer,
> -                                      nsIContent*  aChild,
> -                                      nsIContent*  aOldNextSibling,
> -                                      RemoveFlags  aFlags,
> -                                      bool*        aDidReconstruct,
> +nsCSSFrameConstructor::ContentRemoved(nsIContent* aContainer,
> +                                      nsIContent* aChild,
> +                                      nsIContent* aOldNextSibling,
> +                                      RemoveFlags aFlags,
> +                                      bool* aDidReconstruct,
>                                        nsIContent** aDestroyedFramesFor)

Personally, I find the old code more readable.

::: layout/base/nsCSSFrameConstructor.cpp:9100
(Diff revision 1)
> -                                                nsIAtom*       aTag,  // content object's tag
> +nsCSSFrameConstructor::GetAlternateTextFor(nsIContent* aContent,
> +                                           nsIAtom* aTag,  // content object's tag
> -                                                nsXPIDLString& aAltText)
> +                                           nsXPIDLString& aAltText)

ditto
Attachment #8883045 - Flags: review?(mats) → review+
Comment on attachment 8883045 [details]
Bug 1377902: Trivially cleanup a bit of the FC code.

https://reviewboard.mozilla.org/r/154008/#review159154

> Fwiw, personally I think it's ridiculous that we in 2017 are still adhering to this 80-column rule that is an artifact from IBM punch-cards from the early 19th century.  We should change this rule rather than formatting our code to be less readable.

Yeah, I think it's nice because it allows me to split the same file across multiple tabs, but I get the point that enforcing it is somewhat silly...

> You should probably check with dbaron if he wants to keep these comments (occurs twice).

Will do, though I seem to recall heycam suggested me to remove these a while ago.

> ditto

You mean the spacing on the arguments, or the general change in this specific function?

If the first, I agree, but then I see stuff like [1] (there are multiple occurences everywhere, that's just the first one I saw) and think that it gets broken so easily and makes diffs extra-noisy that it's really not worth it.

[1]: http://searchfox.org/mozilla-central/rev/5bb6dfcb3355a912c20383578bc435c1d4ecf575/layout/forms/nsProgressFrame.cpp#102
Comment on attachment 8883045 [details]
Bug 1377902: Trivially cleanup a bit of the FC code.

https://reviewboard.mozilla.org/r/154008/#review159154

> You mean the spacing on the arguments, or the general change in this specific function?
> 
> If the first, I agree, but then I see stuff like [1] (there are multiple occurences everywhere, that's just the first one I saw) and think that it gets broken so easily and makes diffs extra-noisy that it's really not worth it.
> 
> [1]: http://searchfox.org/mozilla-central/rev/5bb6dfcb3355a912c20383578bc435c1d4ecf575/layout/forms/nsProgressFrame.cpp#102

(Oh, and for the second, I did that just trying to make the comments more easy to follow, but happy to revert).
>  You mean the spacing on the arguments, or the general change in this specific function?

I meant just the spacing of the arguments.

Yeah, I'm aware of the fact that people have done automated name changes
that made the names unaligned.  I would have r- those patches, because
it's fairly easy to correct that manually by editing the patch file.
I don't feel that strongly about it, but I think long argument lists
are usually more readable with the names aligned so I think it's an
acceptable burden for the author for the benefit of the readers.
Comment on attachment 8883045 [details]
Bug 1377902: Trivially cleanup a bit of the FC code.

https://reviewboard.mozilla.org/r/154008/#review159154

> Will do, though I seem to recall heycam suggested me to remove these a while ago.

(David confirmed today that it's fine to remove them)

> Personally, I find the old code more readable.

I ended up not reverting this just because it's how clang-format formats it... Perhaps it's worth to raise it in https://bugzilla.mozilla.org/show_bug.cgi?id=1188202 or in the mailing list. If people agree, I think it'd be fine to enforce it with automation.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9b2f0f827468
Trivially cleanup a bit of the FC code. r=mats
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=111957617&repo=autoland
Flags: needinfo?(emilio+bugs)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6fd73447f6cd
Backed out changeset 9b2f0f827468 for bustage
Wait, what?

> cc1plus: error: to generate dependencies you must specify either -M or -MM

And it happens only on release builds, since I had push to try in debug and it was green...

That error message is one of the most unhelpful ones I've seen lately... I'll see if there's something I've missed, maybe when removing some ifdef?
Sounds like it...

Gah, I guess it's Wunused + Werror on the doc element frame construction... I'll revert that bit for now.
Flags: needinfo?(emilio+bugs)
Well, or I can also just use mRootElementFrame in the debug code... just did that. I'll push to try again just in case.

Thanks for pointing me to bug 1376593 Mats!
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/123e91f2e03d
Trivially cleanup a bit of the FC code. r=mats
https://hg.mozilla.org/mozilla-central/rev/123e91f2e03d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: