540462 - Move GetDocument from nsIDocumentViewer to nsIContentViewer

Status: RESOLVED FIXED

(Core :: General, defect)

Description

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

Attachment: WIP

This would be the first step of bug 540433.

Removing the whole nsIDocumentViewer needs some more work after all
and this part is more important for performance reasons.

I'll push the patch to tryserver.

Comment 1

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

Comment on attachment 422231 [details] [diff] [review]
WIP

First this and then figuring out how to fix nsDocShell::GetEldestPresContext before removing nsIDocumentViewer.

(GetEldestPresContext is probably pretty simple to fix, just use prescontext via nsIDocument)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 422231 [details] [diff] [review]
WIP

>+++ b/accessible/src/base/nsCoreUtils.cpp
>@@ -555,22 +554,17 @@ nsCoreUtils::GetDOMNodeForContainer(nsID
>+  nsCOMPtr<nsIDocument> doc = cv->GetDocument();

This can just be nsIDocument*, right?

>+++ b/content/base/src/nsDocument.cpp
>@@ -893,17 +893,17 @@ nsExternalResourceMap::AddExternalResour
>   nsCOMPtr<nsIDocument> doc;

What about this one?

In fact, this probably applies to all the places you're making this change.  Check whether the refcounting is needed?

>+++ b/docshell/base/nsDocShell.cpp
>@@ -10092,21 +10077,19 @@ nsDocShell::SetBaseUrlForWyciwyg(nsICont
>+        aContentViewer->GetDocument();
>+        if (document) {
>+            rv = document->SetBaseURI(baseURI);

You need to actually assign to |document|, right?  And should it be declared in the block where it's used?

Do we not have unit tests covering this codepath?

r=me with the above fixed.

Comment 3

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

(In reply to comment #2)
> Check whether the refcounting is needed?
ok

> Do we not have unit tests covering this codepath?
Uh, right. I guess I need to add some test, if possible.

Comment 4

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

I couldn't figure out a testcase which would fail with that change.

Comment 5

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

Ah, maybe I found the testcase.

Comment 6

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

Attachment: +test for wyciwyg handling

Comment 7

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

http://hg.mozilla.org/mozilla-central/rev/582e1c0c2b21