Rename nsIDocument to mozilla::dom::Document.

RESOLVED FIXED in Firefox 66

Status

()

enhancement
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment)

No description provided.
Assignee

Updated

5 months ago
See Also: → 1517247
Assignee

Comment 1

5 months ago
Posted patch PatchSplinter Review
Really sorry for the size of the patch. It's mostly automatic
s/nsIDocument/Document/ but I had to fix up in a bunch of places manually to
add the right namespacing and such.

Overall it's not a very interesting patch I think.

nsDocument.cpp turns into Document.cpp, nsIDocument.h into Document.h and
nsIDocumentInlines.h into DocumentInlines.h.

I also changed a bunch of nsCOMPtr usage to RefPtr, but not all of it.

While fixing up some of the bits I also removed some unneeded OwnerDoc() null
checks and such, but I didn't do anything riskier than that.
Attachment #9033954 - Flags: review?(bugs)
18000 lines to go. I'll finish it tomorrow.
The random unrelated changes make reviewing a bit slower than it could be.
Yeah, I think it is better to have a giant autogenerated patch, and then split out random other fixes into a separate patch. The autogenerated patch can be skimmed more easily. The two patches can be landed together, but splitting things like that makes it much easier to review.
Comment on attachment 9033954 [details] [diff] [review]
Patch


>@@ -125,9 +125,7 @@ void ApplicationAccessible::Init() {
>   for (auto iter = windowsById->Iter(); !iter.Done(); iter.Next()) {
>     nsGlobalWindowOuter* window = iter.Data();
>     if (window->GetDocShell() && window->IsRootOuterWindow()) {
>-      nsCOMPtr<nsIDocument> docNode = window->GetExtantDoc();
>-
>-      if (docNode) {
>+      if (RefPtr<dom::Document> docNode = window->GetExtantDoc()) {
(unrelated changes in this kind of massive patch should be avoided.)



>-  // nsIDocument has a pretty complex destructor, so we're going to
>+  // mozilla/dom/Document.has a pretty complex destructor, so we're going to
replace . with space before has



>@@ -57,8 +57,7 @@ PostMessageEvent::Run() {
>   // The document is just used for the principal mismatch error message below.
>   // Use a stack variable so mSourceDocument is not held onto after this method
>   // finishes, regardless of the method outcome.
>-  nsCOMPtr<nsIDocument> sourceDocument;
>-  sourceDocument.swap(mSourceDocument);
>+  RefPtr<Document> sourceDocument = mSourceDocument.forget();
(and this kind unrelated changes really shouldn't be in a massive patch like this, but fine)


> void nsIPresShell::SetNeedLayoutFlush() {
>   mNeedLayoutFlush = true;
>-  if (nsIDocument* doc = mDocument->GetDisplayDocument()) {
>+  if (auto* doc = mDocument->GetDisplayDocument()) {
Please don't change the type to auto. It isn't 100% clear to the reader what GetDisplayDocument() returns.
(If I was to read that first time, I would expect there to be some 'DisplayDocument' type which the getter returns.)


>@@ -26,7 +26,7 @@ void nsIPresShell::SetNeedLayoutFlush() {
> 
> void nsIPresShell::SetNeedStyleFlush() {
>   mNeedStyleFlush = true;
>-  if (nsIDocument* doc = mDocument->GetDisplayDocument()) {
>+  if (auto* doc = mDocument->GetDisplayDocument()) {
No auto here.


>     if (nsIPresShell* shell = doc->GetShell()) {
>       shell->mNeedStyleFlush = true;
>     }
>@@ -46,7 +46,7 @@ void nsIPresShell::EnsureStyleFlush() {
> 
> void nsIPresShell::SetNeedThrottledAnimationFlush() {
>   mNeedThrottledAnimationFlush = true;
>-  if (nsIDocument* doc = mDocument->GetDisplayDocument()) {
>+  if (auto* doc = mDocument->GetDisplayDocument()) {
No auto here


>@@ -129,20 +130,14 @@ class ServoCSSParser {
>       float& aStretch, float& aWeight);
> 
>   /**
>-   * Get a URLExtraData from |nsIDocument|.
>-   *
>-   * @param aDocument The current document.
>-   * @return The URLExtraData object.
>+   * Get a URLExtraData from a document.
>    */
>-  static already_AddRefed<URLExtraData> GetURLExtraData(nsIDocument* aDocument);
>+  static already_AddRefed<URLExtraData> GetURLExtraData(dom::Document*);
> 
>   /**
>-   * Get a ParsingEnvironment from |nsIDocument|.
>-   *
>-   * @param aDocument The current document.
>-   * @return The ParsingEnvironment object.
>+   * Get a ParsingEnvironment from a document.
>    */
>-  static ParsingEnvironment GetParsingEnvironment(nsIDocument* aDocument);
>+  static ParsingEnvironment GetParsingEnvironment(dom::Document*);
Why are you removing the argument name. Doesn't of course matter too much here, but still, weird change.
Attachment #9033954 - Flags: review?(bugs) → review+
Assignee

Comment 6

5 months ago
(In reply to Andrew McCreight [:mccr8] from comment #4)
> Yeah, I think it is better to have a giant autogenerated patch, and then
> split out random other fixes into a separate patch. The autogenerated patch
> can be skimmed more easily. The two patches can be landed together, but
> splitting things like that makes it much easier to review.

Yeah, that's fair, unfortunately this patch kinda grew as I wrote it. First I moved the files and change the includes, then I found a massive wall of errors that I had to break down directory by directory... :(

> > void nsIPresShell::SetNeedLayoutFlush() {
> >   mNeedLayoutFlush = true;
> >-  if (nsIDocument* doc = mDocument->GetDisplayDocument()) {
> >+  if (auto* doc = mDocument->GetDisplayDocument()) {
> Please don't change the type to auto. It isn't 100% clear to the reader what
> GetDisplayDocument() returns.
> (If I was to read that first time, I would expect there to be some
> 'DisplayDocument' type which the getter returns.)

Fair enough, done here and the others :)

> GetParsingEnvironment(dom::Document*);
> Why are you removing the argument name. Doesn't of course matter too much
> here, but still, weird change.

Because keeping it went over the line limit, and made clang-format format this in a very ugly way, and the argument name was redundant anyway.

Comment 7

5 months ago
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0a91d365872
Rename nsIDocument to mozilla::dom::Document. r=smaug

Comment 8

5 months ago
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/120fccb288c0
Fix some Android / non-unified bustage.

Comment 10

5 months ago
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df0c81365e99
Fix some more android bustage on a CLOSED TREE.

Hmm, this didn't use hg rename and accordingly broke blame :-(

Assignee

Comment 14

3 months ago

(In reply to Jan Henning [:JanH] from comment #13)

Hmm, this didn't use hg rename and accordingly broke blame :-(

Ouch :(. I use git, which doesn't have renaming stuff, and so does searchfox. https://searchfox.org/mozilla-central/source/dom/base/Document.cpp shows proper blame info.

You need to log in before you can comment on or make changes to this bug.