Closed Bug 1304598 Opened 9 years ago Closed 9 years ago

Drop "ns" prefix in file name to make it consistent with the class name

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

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

Attachments

(7 files)

Bug 1304598 Part 1 - Move PresShell to mozilla namespace.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1304598 Part 3 - Rename nsPresShell.h/cpp to PresShell.h/cpp, and move exported header to mozilla/ subdir.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1304598 Part 6 - Rename nsViewportFrame.h/cpp to ViewportFrame.h/cpp, and move exported header to mozilla/ subdir.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1304598 Part 7 - Move BRFrame to mozilla namespace, and rename nsBRFrame.cpp to BRFrame.cpp.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1304598 Part 2 - Strip "dom::" prefix by using namespace in TouchManager.cpp
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1304598 Part 4 - Sort #include statements in TouchManager.cpp and PresShell.h.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1304598 Part 5 - Move ViewportFrame to mozilla namespace.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
For example, we have ViewportFrame class in file nsViewportFrame.h/cpp, and PresShell in nsPresShell.h/cpp. There might be more.
Priority: -- → P3
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Attachment #8814286 - Flags: review?(dholbert)
Attachment #8814287 - Flags: review?(dholbert)
Attachment #8814288 - Flags: review?(dholbert)
Attachment #8814289 - Flags: review?(dholbert)
Comment on attachment 8814286 [details] Bug 1304598 Part 1 - Move PresShell to mozilla namespace. https://reviewboard.mozilla.org/r/95528/#review95802
Attachment #8814286 - Flags: review?(dholbert) → review+
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.
Attachment #8814288 - Flags: review?(dholbert) → review-
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.
Attachment #8814289 - Flags: review?(dholbert) → review-
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.
In the latest patch set, I've moved both ViewportFrame and BRFrame to mozilla namespace. Thanks.
Comment on attachment 8814845 [details] Bug 1304598 Part 2 - Strip "dom::" prefix by using namespace in TouchManager.cpp https://reviewboard.mozilla.org/r/95968/#review96036
Attachment #8814845 - Flags: review?(dholbert) → review+
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/#review96052
Attachment #8814287 - Flags: review?(dholbert) → review+
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 8814846 [details] Bug 1304598 Part 4 - Sort #include statements in TouchManager.cpp and PresShell.h. https://reviewboard.mozilla.org/r/95970/#review96072 ::: layout/base/MobileViewportManager.h:12 (Diff revision 1) > #define MobileViewportManager_h_ > > #include "mozilla/Maybe.h" > #include "nsIDOMEventListener.h" > #include "nsIObserver.h" > +#include "nsViewportInfo.h" (Since MozReview doesn't let me create "issues" for commit-message review comments, I'll create an issue here so that comment 21 is tracked somewhere.)
Comment on attachment 8814847 [details] Bug 1304598 Part 5 - Move ViewportFrame to mozilla namespace. https://reviewboard.mozilla.org/r/95972/#review96074
Attachment #8814847 - Flags: review?(dholbert) → review+
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!
Attachment #8814288 - Flags: review?(dholbert) → review+
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 ::: layout/generic/BRFrame.cpp:72 (Diff revision 2) > +using namespace mozilla; > + 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
Attachment #8814289 - Flags: review?(dholbert) → review+
(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 8814846 [details] Bug 1304598 Part 4 - Sort #include statements in TouchManager.cpp and PresShell.h. https://reviewboard.mozilla.org/r/95970/#review96290 ::: layout/base/MobileViewportManager.h:12 (Diff revision 1) > #define MobileViewportManager_h_ > > #include "mozilla/Maybe.h" > #include "nsIDOMEventListener.h" > #include "nsIObserver.h" > +#include "nsViewportInfo.h" In PresShell.h: a) #include "nsPresContext.h" is needed due to IsLayoutFlushObserver() calls GetPresContext()->RefreshDriver() [1]. b) #include "nsThreadUtils.h" is needed due to PresShell declares nsRevocableEventPtr member variables in [2]. And the the #include in MobileViewportManager.h could be changed to forward-declarations since nsViewportInfo is used by reference only. [1] http://searchfox.org/mozilla-central/rev/957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/layout/base/nsPresShell.h#383 [2] http://searchfox.org/mozilla-central/rev/957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/layout/base/nsPresShell.h#794 [3] http://searchfox.org/mozilla-central/rev/957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/layout/base/MobileViewportManager.h#63
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 ...
Part 2 introduces EventTarget to mozilla namespace which makes EventTarget ambiguous on msvc. Perhaps that's why most of the EventTarget have "dom::" prefix in TouchManager. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a26007929b7&selectedJob=32011366 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. [1] http://searchfox.org/mozilla-central/rev/957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/layout/base/TouchManager.cpp#234
(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.
Comment on attachment 8814846 [details] Bug 1304598 Part 4 - Sort #include statements in TouchManager.cpp and PresShell.h. https://reviewboard.mozilla.org/r/95970/#review96506 r=me on part 4.
Attachment #8814846 - Flags: review?(dholbert) → review+
(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.
bug 1321184 is filed on cleaning up the blame stuff here.
Depends on: 1321184
Pushed by gszorc@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/185137e0830f Part 1 - Move PresShell to mozilla namespace. r=dholbert https://hg.mozilla.org/integration/autoland/rev/92d9d60f0a82 Part 2 - Strip "dom::" prefix by using namespace in TouchManager.cpp r=dholbert https://hg.mozilla.org/integration/autoland/rev/2753f88a9d7b Part 3 - Rename nsPresShell.h/cpp to PresShell.h/cpp, and move exported header to mozilla/ subdir. r=dholbert https://hg.mozilla.org/integration/autoland/rev/c1d2cf17018b Part 4 - Sort #include statements in TouchManager.cpp and PresShell.h. r=dholbert https://hg.mozilla.org/integration/autoland/rev/e0f9c3f63df0 Part 5 - Move ViewportFrame to mozilla namespace. r=dholbert https://hg.mozilla.org/integration/autoland/rev/345db402d87c Part 6 - Rename nsViewportFrame.h/cpp to ViewportFrame.h/cpp, and move exported header to mozilla/ subdir. r=dholbert https://hg.mozilla.org/integration/autoland/rev/549da59b304f Part 7 - Move BRFrame to mozilla namespace, and rename nsBRFrame.cpp to BRFrame.cpp. r=dholbert
Update: the landings in comment 49, comment 50, and comment 58 have been removed from autoland history, as part of the cleanup in bug 1321184 -- and gps has now relanded this (the good version) in Comment 62.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: