191023 - Make nsHTMLDocument less docshell/preshell/content view demanding, cleanup webshell

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Adam Lock]

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

Comment 1

[Adam Lock]

Attachment: Work in progress

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.

Comment 2

[Adam Lock]

Attachment: Patch

Patch.

Patch removes nsIWebShell dependencies from HTML document files.

Comment 3

[Adam Lock]

Comment on attachment 118546 [details] [diff] [review]
Patch

Can I have r/sr on this webshell cleanup?

Thanks

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 118546 [details] [diff] [review]
Patch

sr=jst

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 6

[Adam Lock]

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?

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

Comment 8

[Adam Lock]

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

Comment 9

[Adam Lock]

Attachment: Patch 2

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

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 11

[Adam Lock]

Attachment: Patch 3

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

Comment 12

[Adam Lock]

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

Comment 13

[Adam Lock]

Fix is checked in