Comment on attachment 8814287[details]Bug 1304598 Part 3 - Rename nsPresShell.h/cpp to PresShell.h/cpp, and move exported header to mozilla/ subdir.
https://reviewboard.mozilla.org/r/95530/#review95804
Commit message nit:
> Bug 1304598 Part 2 - Rename nsPresShell.h/cpp to PresShell.h/cpp.
You should probably add...
", and move exported header to mozilla/ subdir"
...since that actually represents the bulk of the changes that are happening in this patch.
::: layout/base/TouchManager.cpp:10
(Diff revision 1)
> #include "mozilla/dom/EventTarget.h"
> #include "nsIFrame.h"
> -#include "nsPresShell.h"
> +#include "mozilla/PresShell.h"
Could you move the PresShell line up to be alongside the other mozilla/ includes here? (like you're doing in other files)
::: layout/base/TouchManager.cpp:234
(Diff revision 1)
> int32_t id = touch->Identifier();
> TouchInfo info;
> if (!sCaptureTouchList->Get(id, &info)) {
> continue;
> }
> - nsCOMPtr<EventTarget> targetPtr = info.mTouch->mTarget;
> + nsCOMPtr<dom::EventTarget> targetPtr = info.mTouch->mTarget;
This change (adding "dom") doesn't seem related to the rest of this patch. Could you revert it?
(Or: if for some reason this is actually needed for this patch to compile, it'd be better to fix this via a "using" declaration rather than by adding a "dom::" prefix here. And that change might want to happen in its own preliminary patch, to avoid crufting up this one, though maybe it doesn't matter. In .cpp files, we have very few nsCOMPtr<dom::EventTarget> instances, and very many nsCOMPtr<EventTarget> instances.)
::: layout/base/PresShell.cpp:45
(Diff revision 1)
> #include "gfxPrefs.h"
> #include "gfxUserFontSet.h"
> -#include "nsPresShell.h"
> +#include "mozilla/PresShell.h"
> #include "nsPresContext.h"
> #include "nsIContent.h"
> #include "nsIContentIterator.h"
> #include "mozilla/dom/BeforeAfterKeyboardEvent.h"
> #include "mozilla/dom/Element.h"
This #include actually belongs *at the top* of the #include list, since it's the header for this .cpp file. If you can't move it up there as part of this patch, you might consider adding a followup patch (or filing a followup bug) to address anything that's preventing that move from happening.

Comment on attachment 8814288[details]Bug 1304598 Part 6 - Rename nsViewportFrame.h/cpp to ViewportFrame.h/cpp, and move exported header to mozilla/ subdir.
https://reviewboard.mozilla.org/r/95532/#review95814
Hmm... So this is a noble attempt to bring things into a sensible state, but it's still a somewhat-bad state (with an unfortunate lack of namespacing for *both* the header & class name). Right now we lack namespacing for the class name -- with this patch, we'll lack it for the class as well as the filename.
I'd prefer we either:
(1) Take this patch and *also* move the ViewportFrame class into the mozilla namespace.
...OR:
(2) Rename the class to nsViewportFrame (to match its filename).
Either of those would leave us with *some* form of namespacing (either mozilla::Foo mozilla/Foo.h, OR "ns"-prefix), which would be preferable to the state that this patch leaves us in (with zero namespacing). Zero-namespacing issues leave us open to naming conflicts with external libraries.

Comment on attachment 8814289[details]Bug 1304598 Part 7 - Move BRFrame to mozilla namespace, and rename nsBRFrame.cpp to BRFrame.cpp.
https://reviewboard.mozilla.org/r/95534/#review95818
My notes on Part 3 apply here as well, though to a lesser extent since there's no header file here (and hence no possibility of clashing with 3rd-party libraries).
But basically, I tend to think we should only rename "nsFoo.*" to "Foo.*" when we migrate the file's contents to the mozilla namespace -- since generally our nsFoo.* files aren't mozilla-namespaced, vs. our Foo.* files are mozilla-namespaced. This file happens to have a weird naming inconsistency, but I feel like this fix is just replacing one form of inconsistency with a different form.

Comment on attachment 8814287[details]Bug 1304598 Part 3 - Rename nsPresShell.h/cpp to PresShell.h/cpp, and move exported header to mozilla/ subdir.
https://reviewboard.mozilla.org/r/95530/#review95804> Could you move the PresShell line up to be alongside the other mozilla/ includes here? (like you're doing in other files)
Interesting. This is the one that cannot be moved easily. Do it in a separate patch (See part 4).
> This change (adding "dom") doesn't seem related to the rest of this patch. Could you revert it?
>
> (Or: if for some reason this is actually needed for this patch to compile, it'd be better to fix this via a "using" declaration rather than by adding a "dom::" prefix here. And that change might want to happen in its own preliminary patch, to avoid crufting up this one, though maybe it doesn't matter. In .cpp files, we have very few nsCOMPtr<dom::EventTarget> instances, and very many nsCOMPtr<EventTarget> instances.)
The "dom::" prefix is needed to compile. I've declared "using EventTarget" in part 2 to avoid crufting.
> This #include actually belongs *at the top* of the #include list, since it's the header for this .cpp file. If you can't move it up there as part of this patch, you might consider adding a followup patch (or filing a followup bug) to address anything that's preventing that move from happening.
Yes. #include "mozilla/PresShell.h" should be the first include.

Comment on attachment 8814846[details]Bug 1304598 Part 4 - Sort #include statements in TouchManager.cpp and PresShell.h.
https://reviewboard.mozilla.org/r/95970/#review96056
From the extended commit message:
> Need to add #includes "nsPresContext.h" and "nsThreadUtils.h" in
> PresShell.h, and "nsViewportInfo.h" in MobileViewportManager.h to compile.
I suspect the "nsPresContext.h" and "nsViewportInfo.h" includes can simply be replaced with forward-declarations... Could you try that if you haven't already, and update the patch (and this extended commit message) if it works out? (Or if it doesn't work out: I'm curious why not)

Comment on attachment 8814288[details]Bug 1304598 Part 6 - Rename nsViewportFrame.h/cpp to ViewportFrame.h/cpp, and move exported header to mozilla/ subdir.
https://reviewboard.mozilla.org/r/95532/#review96068> Need to add #include "nsContainerFrame.h" in nsBRFrame.cpp to compile.
For future reference: at least for review purposes [not necessarily for commit-message purposes], please provide a bit more information (more than "to compile") for these sorts of side-effect #include changes. Even just the compilation error message that you're addressing would be helpful. In this case, it was entirely mysterious to me why we needed to add this #include here, since nsBRFrame.cpp doesn't mention nsContainerFrame directly at all...
In this case, to reduce review turnaround, I ended up satisfying my curiosity by applying the patch-stack locally and reverting this nsContainerFrame include. And I discovered it's because nsBRFrame uses the result of nsIFrame::GetParent(), which is of type nsContainerFrame:
https://dxr.mozilla.org/mozilla-central/rev/05328d3102efd4d5fc0696489734d7771d24459f/layout/generic/nsIFrame.h#720
So, this change makes sense.
Anyway -- this is the sort of thing you should anticipate a reviewer being curious about, and provide an answer up-front, if possible. Thanks!

(In reply to Daniel Holbert [:dholbert] from comment #24)
> For future reference: at least for review purposes [not necessarily for
> commit-message purposes], please provide a bit more information (more than
> "to compile") for these sorts of side-effect #include changes. Even just the
> compilation error message that you're addressing would be helpful. In this
> case, it was entirely mysterious to me why we needed to add this #include
> here, since nsBRFrame.cpp doesn't mention nsContainerFrame directly at all...
For review purposes, are inline comments in mozreview sufficient?
> In this case, to reduce review turnaround, I ended up satisfying my
> curiosity by applying the patch-stack locally and reverting this
> nsContainerFrame include. And I discovered it's because nsBRFrame uses the
> result of nsIFrame::GetParent(), which is of type nsContainerFrame:
> https://dxr.mozilla.org/mozilla-central/rev/
> 05328d3102efd4d5fc0696489734d7771d24459f/layout/generic/nsIFrame.h#720
> So, this change makes sense.
>
> Anyway -- this is the sort of thing you should anticipate a reviewer being
> curious about, and provide an answer up-front, if possible. Thanks!
Sorry for not being clear about the intention of those #includes, and thank you for heads-up.

Comment on attachment 8814289[details]Bug 1304598 Part 7 - Move BRFrame to mozilla namespace, and rename nsBRFrame.cpp to BRFrame.cpp.
https://reviewboard.mozilla.org/r/95534/#review96090> You're moving this "using" declaration to the bottom of the file, but I think I'd prefer that we left it where it was -- at the top.
>
> The Coding Style Guide says these belong "after all #includes", and I think it means to say *directly* after (or at least that's how we seem to interpret in in most/all files that I've seen):
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Namespaces
I've restored the "using" declartion to where it was in the lastest patchset. According to the style guide, It'll be better to wrap all the definition of class BRFrame and all the method implementations in "namespace mozilla" block. However, NS_NewBRFrame() need to be in the global namespace, but we cannot move it to before the definition of class BRFrame, so ...

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #36)
> If I cannot find other ways to resolve this, I'll just add "dom::" to the
> only one EventTarget in TouchManager without the prefix in [1] in part 2,
> and change the commit message accordingly.
Daniel,
Per your suggestion to avoid "dom::" prefix in comment 7, I think I'll replace "using EventTarget = dom::EventTarget;" in part 2 by adding "using namespace mozilla::dom;" directly after all the #includes. Before my patches moving the order of unified build files, TouchManager must gain this from other cpp files.

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #26)
> > For future reference: at least for review purposes [not necessarily for
> > commit-message purposes], please provide a bit more information (more than
> > "to compile") for these sorts of side-effect #include changes. [...]
>
> For review purposes, are inline comments in mozreview sufficient?
Sure -- any place where they'll be visible before/during review. Thanks!
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #35)
> I've restored the "using" declartion to where it was in the lastest
> patchset.
Thanks!
> According to the style guide, It'll be better to wrap all the
> definition of class BRFrame and all the method implementations in "namespace
> mozilla" block. However, NS_NewBRFrame() need to be in the global namespace,
> but we cannot move it to before the definition of class BRFrame, so ...
It sounds like you're worried that there's a contradiction/style-guide-violation here, but I'm not seeing it. NS_NewBRFrame is not a BRFrame method & not directly part of the BRFrame implementation, so it's fine for it to be outside of (after) "namespace mozilla{...}" IMO.
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #37)
> Per your suggestion to avoid "dom::" prefix in comment 7, I think I'll
> replace "using EventTarget = dom::EventTarget;" in part 2 by adding "using
> namespace mozilla::dom;" directly after all the #includes.
Great -- that's the best approach here, I think. Thanks! r=me still applies to this latest version of part 2.

One bigger picture comment here, though, is that it's confusing to have half the classes in files with an "ns" prefix and half of them without. We should probably be more aggressive about:
* removing the "ns" prefixes from both class names and file names
* putting things in the "mozilla" namespace
so that we don't have this odd mix for historical reasons.

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #46)
> One bigger picture comment here, though, is that it's confusing to have half
> the classes in files with an "ns" prefix and half of them without. We
> should probably be more aggressive about:
> * removing the "ns" prefixes from both class names and file names
> * putting things in the "mozilla" namespace
> so that we don't have this odd mix for historical reasons.
Agreed. In other words, we should file/fix more bugs of this sort. :) Thanks for pushing this forward, TYLin!

I'm very sorry that commits landed in comment 49 incorrectly treat file renames as file removed & added. (I was uploaded the patchset by git-cinnabar. This is a known issue [1], which is my bad.) Therefore the blame history of the files renamed in this bug won't be connected. Backouts in comment 50 doesn't help either :(
I'll summarize the file renames in this bug below so that people look at blame history could trace back to the old files.
a) layout/base/nsPresShell.h/cpp -> layout/base/PresShell.h/cpp
b) layout/generic/nsViewportFrame.h/cpp -> layout/generic/ViewportFrame.h/cpp
c) layout/generic/nsBRFrame.cpp - layout/generic/BRFrame.cpp
[1] https://github.com/glandium/git-cinnabar/issues/10

Whiteboard: [See comment 59 for the mapping of old and new file names]

Darn :-/ I hope I would've caught that in review (and it looks like the last version I reviewed was indeed tracked as a 'move' rather than a delete+add). It must've crept in during one of the review-comment-addressing tweaks.
(Not your fault; tools-footguns can attack anybody. Thanks for having identified the specific git-cinnabar bug, too.)
nsPresShell.cpp is a pretty important file to have lost useful hg blame on. I think we should strongly consider stripping all recent commits from autoland and re-landing stuff, if at all possible... Discussing in #developers now.