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

RESOLVED FIXED

Status

()

Core
HTML: Parser
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: Adam Lock, Assigned: Adam Lock)

Tracking

(Blocks: 1 bug)

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

17.53 KB, patch
Heikki Toivonen (remove -bugzilla when emailing directly)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
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
(Assignee)

Updated

15 years ago
Blocks: 115328
(Assignee)

Comment 1

15 years ago
Created attachment 112911 [details] [diff] [review]
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.
(Assignee)

Comment 2

15 years ago
Created attachment 118546 [details] [diff] [review]
Patch

Patch.

Patch removes nsIWebShell dependencies from HTML document files.
Attachment #112911 - Attachment is obsolete: true
(Assignee)

Comment 3

15 years ago
Comment on attachment 118546 [details] [diff] [review]
Patch

Can I have r/sr on this webshell cleanup?

Thanks
Attachment #118546 - Flags: superreview?(jst)
Attachment #118546 - Flags: review?(heikki)
Comment on attachment 118546 [details] [diff] [review]
Patch

sr=jst
Attachment #118546 - Flags: superreview?(jst) → superreview+
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.
Attachment #118546 - Flags: review?(heikki) → review+
(Assignee)

Comment 6

15 years ago
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).
(Assignee)

Comment 8

15 years ago
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
(Assignee)

Comment 9

15 years ago
Created attachment 118639 [details] [diff] [review]
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".
Attachment #118546 - Attachment is obsolete: true
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.
(Assignee)

Comment 11

15 years ago
Created attachment 118685 [details] [diff] [review]
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).
(Assignee)

Updated

15 years ago
Attachment #118639 - Attachment is obsolete: true
(Assignee)

Comment 12

15 years ago
Heikki, is the latest patch acceptable for check-in once 1.4a is out of the way?
Attachment #118685 - Flags: review+
(Assignee)

Comment 13

15 years ago
Fix is checked in
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.