1517241 - Rename nsIDocument to mozilla::dom::Document.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Patch

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.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35f90c8b74ad1b51d8b237dae5947ca8b03d5362

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

18000 lines to go. I'll finish it tomorrow.
The random unrelated changes make reviewing a bit slower than it could be.

Comment 4

[Andrew McCreight [:mccr8]]

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 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0a91d365872
Rename nsIDocument to mozilla::dom::Document. r=smaug

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/120fccb288c0
Fix some Android / non-unified bustage.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc3509de69e
Fix some gcc / msvc bustage.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df0c81365e99
Fix some more android bustage on a CLOSED TREE.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7944bf2a3077
Fix yet another MSVC-only bustage.

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/f0a91d365872
https://hg.mozilla.org/mozilla-central/rev/120fccb288c0
https://hg.mozilla.org/mozilla-central/rev/8dc3509de69e
https://hg.mozilla.org/mozilla-central/rev/df0c81365e99
https://hg.mozilla.org/mozilla-central/rev/7944bf2a3077

Comment 13

[Jan Henning [:JanH]]

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

Comment 14

[Emilio Cobos Álvarez [:emilio]]

(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.