nsHTMLDocument has a built-in expectation that it will be owned by a docshell and rendered in a content view. The code should be made more tolerant so that it might be used by parsers that supply neither. Also get rid of nsIWebShell references

Patch encloses some bits of code that call nsIDocShell, nsIPresShell or nsIContentViewer into if blocks. I've left the assertions in, but the code is a bit more tolerant when it doesn't get what it expects. It also gets rid of nsIWebShell in favour of nsIDocShell or nsISupports. It also reformats MyPrefChangedCallback() into the style of the other source and deletes some obsolete debug code.

Patch. Patch removes nsIWebShell dependencies from HTML document files.

Comment on attachment 118546 [details] [diff] [review] Patch Can I have r/sr on this webshell cleanup? Thanks

Comment on attachment 118546 [details] [diff] [review] Patch sr=jst

Comment on attachment 118546 [details] [diff] [review] Patch >Index: mozilla/content/html/document/src/nsHTMLContentSink.cpp >=================================================================== >@@ -2460,18 +2457,17 @@ > loader->AddObserver(this); > > PRBool enabled = PR_TRUE; >- nsCOMPtr<nsIDocShell> docShell(do_QueryInterface(mWebShell)); >- NS_ASSERTION(docShell, "oops no docshell!"); >- if (docShell) { >- docShell->GetAllowSubframes(&enabled); >+ NS_ASSERTION(mDocShell, "oops no docshell!"); >+ if (mDocShell) { >+ mDocShell->GetAllowSubframes(&enabled); > if (enabled) { > mFlags |= NS_SINK_FLAG_FRAMES_ENABLED; > } >- } > >- // Find out if scripts are enabled, if not, show <noscript> content >- if (IsScriptEnabled(aDoc, aContainer)) { >- mFlags |= NS_SINK_FLAG_SCRIPT_ENABLED; >+ // Find out if scripts are enabled, if not, show <noscript> content >+ if (IsScriptEnabled(aDoc, mDocShell)) { >+ mFlags |= NS_SINK_FLAG_SCRIPT_ENABLED; >+ } > } You changed the behavior of this code slightly. Previously if !aContainer (!mDocShell), we would set the script enabled flag. After your change it would not be set. >Index: mozilla/content/html/document/src/nsHTMLDocument.cpp >=================================================================== >+ nsCOMPtr<nsIHTMLContentSink> sink; I don't think you need to define this variable here, there already is a sink variable defined earlier in this function, line 864 in the unchanged version. >Index: mozilla/layout/html/base/src/nsHTMLParts.h >=================================================================== Fix those, or explain why things need to be your way, and r=heikki.

The logic was changed slightly to move the call to mDocShell->GetAllowSubframes inside an if (mDocShell) statement. The old code just used to assume there was a mDocShell which there might not be if the HTMLContentSink is ever invoked without a docshell to go with it. PRBool enabled = PR_TRUE; NS_ASSERTION(mDocShell, "oops no docshell!"); if (mDocShell) { mDocShell->GetAllowSubframes(&enabled); if (enabled) { mFlags |= NS_SINK_FLAG_FRAMES_ENABLED; } // Find out if scripts are enabled, if not, show <noscript> content if (IsScriptEnabled(aDoc, mDocShell)) { mFlags |= NS_SINK_FLAG_SCRIPT_ENABLED; } } I could move the setting of NS_SINK_FLAG_FRAMES_ENABLED outside of this if statement if as it seems the default for "enabled" is PR_TRUE, i.e. PRBool enabled = PR_TRUE; NS_ASSERTION(mDocShell, "oops no docshell!"); if (mDocShell) { mDocShell->GetAllowSubframes(&enabled); // Find out if scripts are enabled, if not, show <noscript> content if (IsScriptEnabled(aDoc, mDocShell)) { mFlags |= NS_SINK_FLAG_SCRIPT_ENABLED; } } if (enabled) { mFlags |= NS_SINK_FLAG_FRAMES_ENABLED; } For the sink thing, I should probably move and rename the earlier version into the #idef rickgdebug section. Or should I just delete the #ifdef rickgdebug bits altogether?

I don't think you addressed my comment, but made further change. What I meant was that it IMO should be written as NS_ASSERTION(mDocShell, "oops no docshell!"); if (mDocShell) { mDocShell->GetAllowSubframes(&enabled); if (enabled) { mFlags |= NS_SINK_FLAG_FRAMES_ENABLED; } } // Find out if scripts are enabled, if not, show <noscript> content if (IsScriptEnabled(aDoc, mDocShell)) { mFlags |= NS_SINK_FLAG_SCRIPT_ENABLED; } Regarding the second comment, I am also a little confused... The old code (re)used the earlier sink variable in this code that you changed, so I see no need to add new variable or move the original definition. However, having said that I don't think anyone uses any |rickgdebug| stuff so it would probably be safe to remove that stuff althogether (in which case the best place for the sink variable would be where you put it).

The reason IsScriptEnabled is in that block too is because there is no point enabling scripting if you have no docshell... As regarding the sink, I was suggesting renaming the earlier sink decl as sinkDebug and sticking it inside #ifdef section. As this section seems to be cruft I'll just delete the #ifdef altogether and remove the earlier declaration with it. I'll submit a new patch

This patch is more like the original in form. I've moved the IsScriptEnabled out of the block after looking at what the function does, renamed "enabled" as "subFramesEnabled" for clarity and changed the assertion to a warning. I've also deleted the #ifdef rickgdebug and earlier decl of "sink".

Heh, now there is a difference in how NS_SINK_FLAG_FRAMES_ENABLED is set: it used to be set only if there was a docshell, now it will be set even if there is no docshell.

Same as before except I've moved the subframes code all inside the if block so the flag is only set when the docshell is there (which it will be normally).

Heikki, is the latest patch acceptable for check-in once 1.4a is out of the way?

Fix is checked in