Remove ReflowInput.h include from nsIFrame.h

See https://phabricator.services.mozilla.com/D81845#2501818.

Hello! I would like to work on this task if possible.

This would be my first ever attempt on contributing to the opensource scene for Mozilla. I thought this issue would be nice and simple to ease my way into the scene and get familiar with how the ecosystem works in here.

Comment on attachment 9407808 [details]
Bug 1650944 - Remove unused method PermissionManager::LegacyTestPermissionFromURI. r?pbz

Revision D213918 was moved to bug 1881694. Setting attachment 9407808 [details] to obsolete.

May I be assigned this issue, since it looks like the previous phabricator entry was for a different bug

Sure. (Submitting a patch would also auto-assign you)

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Hello, may I work on this bug?
I am a newer member of the open source club at UofT and would like a good first bug to work on.

Sure! So, since it hasn't been stated explicitly here... I think the point here is that:

• nsIFrame.h is included in a lot of places (directly or indirectly)
• It includes ReflowInput.h, which is quite huge.
• Because of those^ two points: whenever you touch ReflowInput.h (in local development or in a patch that lands), then everything that includes nsIFrame.h needs to be recompiled, which slows down build times.
• So if we can remove the include, it'd help with build times.

As a first-time contributor, the way forward here is:

(1) Get a Firefox build environment ( https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html ) and compile Firefox.

(2) Add ac_add_options --disable-unified-build to your .mozconfig file (a build file described in the quick-ref linked above), to disable unified builds, which will slow down your builds slightly but is important to be sure that the later patches here are valid (i.e. that you're including everything that you need to, after you adjust the include lists).

[now the actual patch-writing work begins]

(3) In order to do the main thing in this bug (removing ReflowInput.h from nsIFrame.h), we first need to address the fact that nsIFrame.h uses a type that's defined in ReflowInput, ReflowInput::BreakType. We need to move the (very small) BreakType definition out of the ReflowInput.h file entirely and into a separate file. [EDIT: It should probably move to layout/base/LayoutStructs.h per comment 10.]

This part^^ should probably be its own patch (either on this bug or a supporting bug, up to you), separate from the next part.

(4) Then (again, in separate patch from (3)), hopefully you can just move the ReflowInput.h include from nsIFrame.h into nsIFrame.cpp, and give nsIFrame a forward-declaration (class ReflowInput;) around here.

Most likely, our build process will be able to compile nsIFrame.cpp, but you'll hit build errors from other files that need ReflowInput.h and that have been including it indirectly from nsIFrame.h. So then you'll need to go through your build failures and decide whether those files actually need a ReflowInput.h include, or whether they can be satisfied by a forward-declaration. (totally fine for .cpp files, but worth avoiding for .h files if possible)

Actually in (3) we can probably move BreakType to this file instead of needing to create a new file:
https://searchfox.org/firefox-main/source/layout/base/LayoutStructs.h

And then nsIFrame.h should include that file directly (#include "mozilla/LayoutStructs.h")

And as Emilio observed in someone's previous attempt at writing a patch here, the OverflowAreas type might cause a bit of trouble here too -- it lives in ReflowOutput which is included by ReflowInput, and nsIFrame.h needs the full OverflowAreas class definition. So probably for now nsIFrame.h should just gain an #include "mozilla/ReflowOutput.h" to replace the version of that include that it's currently getting (indirectly) from the ReflowInput.h include that you'll be removing.

Thank you for all the helpful information, Daniel!

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Hi Daniel,
I have not seen any activity on this bug could I take it up?
If so I was not sure in what way I should move the definition of ReflowInput::BreakType, to LayoutStructs.h.
Should I define a ReflowInputBreakType enum then use a using statement? Or will it be necessary to rename
Regards Eugene.

(In reply to Eugeniusz Lewandowski from comment #14)

Hi Daniel,
I have not seen any activity on this bug could I take it up?

Sure, you're welcome to give it a try! I've assigned it to you, which I think unblocks

If so I was not sure in what way I should move the definition of ReflowInput::BreakType, to LayoutStructs.h.
Should I define a ReflowInputBreakType enum then use a using statement? Or will it be necessary to rename

I'd suggest just cutting and pasting the enum definition:
https://searchfox.org/firefox-main/rev/8b6198f56d80ba962e0570e5ae8a3c257e2e7bce/layout/generic/ReflowInput.h#388-392
... directly inside the namespace mozilla { ... } block in LayoutStructs.h. Probably no need to change the name (beyond that change-of-namespace).

And at its usage sites, it will now be called mozilla::BreakType rather than mozilla::ReflowInput::BreakType.

(In reply to Daniel Holbert [:dholbert] from comment #15)

(In reply to Eugeniusz Lewandowski from comment #14)

Hi Daniel,
I have not seen any activity on this bug could I take it up?

Sure, you're welcome to give it a try! I've assigned it to you, which I think unblocks

[er didn't finish this thought]

...which I think unblocks you from attaching patches via moz-phab.

See https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html for a quick-reference on getting started, if you haven't already found that.

Thanks!

Moved ReflowInput::BreakType enumuration outside ReflowInput class to mozilla::BreakType, and refactored the functions using it to reflect the change.

Comment on attachment 9576055 [details]
Bug 2036580 - Move BreakType definition out of ReflowInput class r?dholbert

Revision D298034 was moved to bug 2036580. Setting attachment 9576055 [details] to obsolete.

Hi Daniel,
Removing the #include "mozilla/ReflowInput.h" include with #include "mozilla/ReflowOutput.h" misses out the nsLineLayout forward declaration in ReflowInput.h. Should I:

1. Forward declare nsLineLayout in nsIFrame
2. Forward declare nsLineLayout each of nsBlockFrame.h, nsRubyBaseContainerFrame.h and nsLineLayout.h (all of the files that require it)
3. Include nsLineLayout.h (in either nsIFrame, or each of the files mentioned in 2)
   Regards Eugene.

Option 1. Definitely prefer forward decls where they are sufficient. Thanks!

Thank you both for your help.

Removed the Reflow Input in nsIFrame. Added includes for ReflowInput as
necessary in cpp files. Also Forward Declared nsLineLayout in 3 header
files.

https://hg.mozilla.org/mozilla-central/rev/c9f5c873bd4c