836176 - Remove nsIHTMLDocument

Status: RESOLVED FIXED

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

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Part 1: Add nsIDocument::AsHTMLDocument

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Remove nsIHTMLDocument

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 707983 [details] [diff] [review]
Part 1: Add nsIDocument::AsHTMLDocument

r=me

If we cared to inline it, we could put the impl in nsHTMLDocument.h, but this is probably fine.

Comment 4

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1412c8db2f22

Comment 5

[Masatoshi Kimura [:emk]]

Are no binary add-ons using nsIHTMLDocument?

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 707984 [details] [diff] [review]
Part 2: Remove nsIHTMLDocument

Review of attachment 707984 [details] [diff] [review]:
-----------------------------------------------------------------

Without looking further than the context included in the patch, we may need a number of null checks.

::: content/base/src/nsContentUtils.cpp
@@ +2183,5 @@
>    if (IsAutocompleteOff(aContent)) {
>      return NS_OK;
>    }
>  
> +  nsHTMLDocument* htmlDocument = aContent->GetCurrentDoc()->AsHTMLDocument();

Is GetCurrentDoc() never null?

@@ +4065,5 @@
>    // for compiling event handlers... so just bail out.
>    nsCOMPtr<nsIDocument> document = aContextNode->OwnerDoc();
>    bool isHTML = document->IsHTML();
>  #ifdef DEBUG
> +  NS_ASSERTION(!isHTML || document->AsHTMLDocument(), "Should have HTMLDocument here!");

If you think this assertion is still useful, get rid of the ifdef, at least.

::: content/base/src/nsCopySupport.cpp
@@ +413,5 @@
>    // also consider ourselves in a text widget if we can't find an html
>    // document. Note that XHTML is not counted as HTML here, because we can't
>    // copy it properly (all the copy code for non-plaintext assumes using HTML
>    // serializers and parsers is OK, and those mess up XHTML).
> +  if (!aDoc->IsHTML())

aDoc is never null?

::: content/base/src/nsDocumentEncoder.cpp
@@ +1393,5 @@
>      }
>    }
>    
>    // also consider ourselves in a text widget if we can't find an html document
> +  if (!mDocument->IsHTML())

And here?

::: content/base/src/nsXMLHttpRequest.cpp
@@ +2206,5 @@
>      }
>  
>      if (mState & XML_HTTP_REQUEST_USE_XSITE_AC) {
> +      nsCOMPtr<nsIDocument> doc = do_QueryInterface(mResponseXML);
> +      if (doc->IsHTML()) {

And doc here?

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +527,5 @@
>      return false;                       // Not spellchecked by default
>    }
>  
>    if (IsCurrentBodyElement()) {
> +    nsHTMLDocument* doc = GetCurrentDoc()->AsHTMLDocument();

I guess IsCurrentBodyElement() == true ==> IsInDoc() == true ==> GetCurrentDoc() != null, so this is safe... Assert, please.

@@ +645,5 @@
>  
>    RemoveFromNameTable();
>  
>    if (GetContentEditableValue() == eTrue) {
> +    nsIDocument* doc = GetCurrentDoc();

How about here?

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +445,5 @@
>                                                   aCompileEventHandlers);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  if (aDocument->IsHTML()) {
> +    aDocument->AsHTMLDocument()->AddedForm();

This is probably fine

@@ +510,5 @@
>  
>  void
>  nsHTMLFormElement::UnbindFromTree(bool aDeep, bool aNullParent)
>  {
> +  nsCOMPtr<nsIDocument> oldDocument = do_QueryInterface(GetCurrentDoc());

GetCurrentDoc() already returns nsIDocument

@@ +1473,5 @@
>    // a document inside XUL.
>  
>    nsCOMPtr<nsIURI> actionURL;
>    if (action.IsEmpty()) {
> +    if (!document->IsHTML()) {

Can document be null?

::: content/html/document/src/nsHTMLDocument.cpp
@@ +1196,4 @@
>                     "Huh, how did this happen? This should only be used with "
>                     "HTML documents!");
>      }
>  #endif

Get rid of the extra scoping and the ifdef

@@ +1239,4 @@
>                   "Huh, how did this happen? This should only be used with "
>                   "HTML documents!");
>    }
>  #endif

Same here

::: content/xml/document/src/nsXMLContentSink.cpp
@@ +377,5 @@
>    if (NS_SUCCEEDED(aResult) || aResultDocument) {
>      // Transform succeeded or it failed and we have an error
>      // document to display.
>      mDocument = aResultDocument;
> +    if (mDocument->IsHTML()) {

Looks like mDocument could be null here

::: docshell/base/nsDocShellEditorData.cpp
@@ +210,5 @@
>  
>    nsCOMPtr<nsIDOMDocument> domDoc;
>    domWindow->GetDocument(getter_AddRefs(domDoc));
> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> +  if (doc->IsHTML())

domDoc could most likely be null

@@ +233,5 @@
>  
>    nsCOMPtr<nsIDOMDocument> domDoc;
>    domWindow->GetDocument(getter_AddRefs(domDoc));
> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> +  if (doc->IsHTML())

Same here

::: docshell/base/nsDocShellEditorData.h
@@ +64,5 @@
>  
>    // Backup for mMakeEditable while the editor is detached.
>    bool mDetachedMakeEditable;
>  
> +  // Backup for the corresponding nsHTMLDocument's  editing state while

While you're here, want to fix the weird double space?

::: dom/base/nsDOMClassInfo.cpp
@@ +8376,2 @@
>        xpc::Throw(cx, rv);
>        return JS_FALSE;

You don't want to xpc::Throw a success result.

::: dom/base/nsFocusManager.cpp
@@ +2031,5 @@
>    if (editorDocShell) {
>      editorDocShell->GetEditable(&isEditable);
>  
>      if (isEditable) {
> +      nsHTMLDocument* doc = presShell->GetDocument()->AsHTMLDocument();

Can GetDocument() be null?

@@ +2983,5 @@
>    }
>  
>    // Finally, check if this is a frameset
> +  if (aDocument->IsHTML() &&
> +      aDocument->AsHTMLDocument()->GetHtmlChildElement(nsGkAtoms::frameset)) {

How about aDocument?

::: dom/base/nsGlobalWindow.cpp
@@ +2341,2 @@
>      nsWindowSH::InstallGlobalScopePolluter(cx, newInnerWindow->mJSObject,
> +                                           doc->AsHTMLDocument());

mDocument can be null, I think

::: editor/composer/src/nsEditingSession.cpp
@@ +541,5 @@
>    // Check if we're turning off editing (from contentEditable or designMode).
>    nsCOMPtr<nsIDOMDocument> domDoc;
>    aWindow->GetDocument(getter_AddRefs(domDoc));
> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> +  bool stopEditing = doc->IsHTML() && doc->AsHTMLDocument()->IsEditingOn();

doc &&

@@ +687,2 @@
>  
> +        nsHTMLDocument* htmlDoc = doc->AsHTMLDocument();

And here

::: parser/html/Makefile.in
@@ +92,5 @@
>  
>  include $(topsrcdir)/config/rules.mk
>  
>  INCLUDES	+= \
> +		-I$(srcdir)/../../layout/style \

Which headers is this for? Can we export them, or not have nsHTMLDocument.h include them?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1412c8db2f22

Comment 9

[David Zbarsky (:dzbarsky)]

(In reply to :Ms2ger from comment #6)
> Comment on attachment 707984 [details] [diff] [review]
> Part 2: Remove nsIHTMLDocument
> 
> Review of attachment 707984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

> 
> ::: content/base/src/nsCopySupport.cpp
> @@ +413,5 @@
> >    // also consider ourselves in a text widget if we can't find an html
> >    // document. Note that XHTML is not counted as HTML here, because we can't
> >    // copy it properly (all the copy code for non-plaintext assumes using HTML
> >    // serializers and parsers is OK, and those mess up XHTML).
> > +  if (!aDoc->IsHTML())
> 
> aDoc is never null?

Nope, I added an assert.

> 
> ::: content/base/src/nsXMLHttpRequest.cpp
> @@ +2206,5 @@
> >      }
> >  
> >      if (mState & XML_HTTP_REQUEST_USE_XSITE_AC) {
> > +      nsCOMPtr<nsIDocument> doc = do_QueryInterface(mResponseXML);
> > +      if (doc->IsHTML()) {
> 
> And doc here?

The QI is not needed and we deref mResponseXML right above.
 

> 
> ::: content/html/content/src/nsHTMLFormElement.cpp
> @@ +445,5 @@
> >                                                   aCompileEventHandlers);
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +  if (aDocument->IsHTML()) {
> > +    aDocument->AsHTMLDocument()->AddedForm();
> 
> This is probably fine
> 

This needs a null check actually.


> GetCurrentDoc() already returns nsIDocument
> 
> @@ +1473,5 @@
> >    // a document inside XUL.
> >  
> >    nsCOMPtr<nsIURI> actionURL;
> >    if (action.IsEmpty()) {
> > +    if (!document->IsHTML()) {
> 
> Can document be null?

If it is we'll crash when calling GetDocumentURI right above these lines.

> 
> @@ +2983,5 @@
> >    }
> >  
> >    // Finally, check if this is a frameset
> > +  if (aDocument->IsHTML() &&
> > +      aDocument->AsHTMLDocument()->GetHtmlChildElement(nsGkAtoms::frameset)) {
> 
> How about aDocument?
> 

No, its dereferenced right above this.

Comment 10

[Masatoshi Kimura [:emk]]

(In reply to David Zbarsky (:dzbarsky) from comment #9)
> > aDoc is never null?
> 
> Nope, I added an assert.

If aDoc can be null for a valid reason, an error check should be added rather than an assertion.

Comment 11

[David Zbarsky (:dzbarsky)]

Attachment: Part 1.5: Fix AsHTMLDocument

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 738853 [details] [diff] [review]
Part 1.5: Fix AsHTMLDocument

r=me

Comment 13

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ad7cce5c01

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e3ad7cce5c01

Comment 15

[(no longer active)]

Attachment: Bug 836176 - Part 2: Remove nsIHTMLDocument::SetIsXHTML();

Comment 16

[(no longer active)]

Attachment: Bug 836176 - Part 3: Remove nsIHTMLDocument;

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c20ef4aadca
Part 2: Remove nsIHTMLDocument::SetIsXHTML(); r=farre
https://hg.mozilla.org/integration/autoland/rev/f848dfc22ff4
Part 3: Remove nsIHTMLDocument; r=farre

Comment 18

[(no longer active)]

Attachment: Bug 836176 - Part 4: drop a useless null check

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57b2544a30e4
Part 4: drop a useless null check

Comment 20

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/7c20ef4aadca
https://hg.mozilla.org/mozilla-central/rev/f848dfc22ff4
https://hg.mozilla.org/mozilla-central/rev/57b2544a30e4