97687 - [serializer]cs/gecko: copying using keyboard shortcuts>paste>source pasted {Country=US}

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[anthonyd]

serializer bug, over to --> harishd
see bug# 7899

anthonyd

Comment 1

[harishd]

I'm not the current owner of plain text serializer....and I don't know the code. 
I'm not sure how I will be able to fix this. 

Comment 2

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

Hmm... nsPlainTextSerializer::DoAddLeaf() should prevent us from outputting
script contents...

Comment 3

[harishd]

Reassigning plaintext serializer bugs to Peter ;-)

Comment 4

[Peter Van der Beken [:peterv]]

Is this for the plaintext serializer? I can't reproduce it, script content is
never copied.

Comment 5

[harishd]

Yup, I wasn't able to reproduce it either. Marking WFM.

Comment 6

[Alex Weyers]

Harish - 

Could you take a look at:
http://www.cnn.com/2001/WORLD/asiapcf/central/09/24/ret.binladen.message/

Using NS6.1, when I make a selection starting from the end of the word Kabul in 
the second paragraph:
"The fax was sent to the Qatari-based news channel Al Jazeera at its offices in 
Kabul."

And drag my selection up and to the left so it includes the entire document up 
to that point... I can then paste the plaintext content into windows notepad 
and, in the midst of my text output I get the following:

"EDUCATION
CAREER
IN-DEPTH

<a href="http://ads.web.aol.com/link/93103220/aol"><img 
src="http://ads.web.aol.com/image/93103220/aol" alt="Click Here" width="120" 
height="60" border="0"></a>
QUICK NEWS
LOCAL
COMMUNITY"


As you can see, an anchor and image tag have made their way into the plaintext 
on the clipboard.  I'm not completely certain that this illustrates the 
original issue, but it definitely is an example of inappropriate source making 
its way into the plaintext copy.

Comment 7

[Peter Van der Beken [:peterv]]

This is caused by the <noscript></noscript> section, we seem to output
everything in the noscript section as html :/. Simplified testcase coming up.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Simplified testcase

Comment 9

[Peter Van der Beken [:peterv]]

Open the testcase, select the words "Begin" and "End", copy and paste as plaintext.

In a trunk build I get:
Begin
One<img src="http://www.mozilla.org/images/mozilla-banner.gif" alt="Test">Two
End

In 0.9.4 I get:
Begin One [Test] Two End

Anyone have any idea what might have caused the behaviour change between 0.9.4
and today? And I'm not sure we should include noscript content.

Comment 10

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

I discussed this with Harish a little... here is what we think should happen
(and why some things happen as they do right now).

When the script is enabled, the contents of noscript element will appear as text
leaf under the noscript element. This is done by the parser. Stylesheets (?)
then hide the noscript element. It is our opinion that when scripts are enabled
we should NOT serialize/copy the noscript element (after all, you can't even see
it so it comes as a surpise when you paste).

When the scripts are disabled, the contents of the noscript element will be made
into elements and made visible normally, as if there was no noscript element at
all (in fact, the start/end tags might be stripped off by the parser). In this
case the user sees the noscript content, therefore it should be serialized/copied.

I am not sure what should happen with the image element. It seems new code
serializes the contents of alt attribute. The functionality should obviously be
the same on both branch & trunk.
So how to fix this? In the serializer it would be simple to check the value of
the "enable JavaSCript in browser" pref and if enabled, do not output noscript.
I don't know if it needs something special to handle saving Editor documents or
 something (I have no idea how Editor saves documents).

I suspect/hope the issue with the img alt attribute would be the same on both
the branch/trunk by now.

Comment 11

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

topembed, this should be P1. If we get the fix soon, I'll think about nsbranch+

Comment 12

[Marek Z. Jeziorek]

raising severity per embedding customer request

Comment 13

[Peter Van der Beken [:peterv]]

I have a patch, but I need to clean it up.

Comment 14

[Jaime Rodriguez, Jr.]

what are the chances of getting this fixed for the 0.9.4 branch?

Comment 15

[Peter Van der Beken [:peterv]]

Proposing a fix. I copied some code from nsHTMLContentSink.cpp
(http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLContentSink.cpp#2426),
but only part of it as I don't have access to the webshell from the serializer.
So I'm not sure whether the check for JS wil always work. Looking for reviews.

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: Proposed fix v1

Comment 17

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

How about adding a new flag to nsIDocumentEncoder: OutputScript? If that flag is
on, output script tag contents, otherwise output noscript contents (these in
plaintext serializer of course, OutputScript flag would probably be ignored by
all other serializers). nsDocumentEncoder is the class that calls the
serializers, and its Init() method takes document as argument; you can compute
the flag in the document encoder Init() method which passes the flags to
serializers. This way you can get rid of both the new member variables.

But, I don't think it is necessary to ask the security manager if the script is
allowed to run, because we are not trying to run it. It should be enough to just
check the pref. (Looking... boolean pref "javascript.enabled" perhaps?). Still,
you could check that pref in the Init() method of the document encoder or
something, so that you only need to do it once.

Hmm... seems like HTMLCopyEncoder::Init() is similar, dunno when that is used,
tho. And you'd better check who calls these init methods with the flags, maybe
that is the place where the OutputScript flag should be set or not (I doubt it,
but...).

Comment 18

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

Looks like this is going to be a safe fix, and since it is topembed I'll give
this an nsbranch+.

Comment 19

[Peter Van der Beken [:peterv]]

Heikki, adding the flag sounds like a good idea. How do I detect that someone
actually wants the document encoder to honor the flag's value it passes in?
I think I need to go through the security manager. I think there's several prefs
that can influence JS enabling/disabling (browser/mail-news, per-site using
capabilities), so I think that going through the security manager is safer.

Comment 20

[Peter Van der Beken [:peterv]]

By saying "using capabilities I mean Configurable Security Policies
, see "Disabling All Javascript for a Site"
 in http://www.mozilla.org/projects/security/components/configPolicy.html.

Comment 21

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

Ok. There are also two prefs, one for browser and one for mail&news. I guess the
serializer has no idea who called it. So it seems like going
through the security manager is a good idea.

I don't understand what you mean by the flag honoring. Caller passes in flags
whose semantics have been explained in comments/flag name, and hopes the encoder
honors it, I guess. I don't think there is a way to know if it really did honor
them or not, or if it treated them wrongly. 

Inside the document encoder/serializer, I would think a respectable
implementation would try its best to honor all of the flags that apply to it. In
case of OutputScriptEnabled, the comment could say that it is set when scripts
are enabled and not set when scripts are disabled, and leave the serializers to
decide what they need to do about it, suggesting that at least plaintext
serializer should use it to decide whether or not to serialize noscript content.
How  does that sound?

Comment 22

[Peter Van der Beken [:peterv]]

Ok. I just worried about people passing in the flag (or not to turn it off) to
nsDocumentEncoder::Init through the aFlags parameter and expecting the
DocumentEncoder to respect that. If we then change the flag's state according to
script enabling in the document, they could be surprised. I suppose documenting
it will be sufficient.

Comment 23

[Peter Van der Beken [:peterv]]

Turns out the serializer is not called through the document encoder. I will have
to put everything inside the serializer or fix up all the other callers.

Comment 24

[Peter Van der Beken [:peterv]]

Attachment: Second try

Comment 25

[Peter Van der Beken [:peterv]]

You can't really tell from the patch, but these lines

+
+  if (!IsScriptEnabled(mDocument))
+    mFlags |= OutputNoScriptContent;

are in nsHTMLCopyEncoder::Init which is used for copying.

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 51548 [details] [diff] [review]
Second try

sr=jst

Comment 27

[Peter Van der Beken [:peterv]]

Checked in. Leaving open for PDT decision.

Comment 28

[Jaime Rodriguez, Jr.]

pls check this into the branch, if this only affects copy and paste operations -
PDT+

Comment 29

[John Morrison]

See bug 102809. This appears to have added an ABR when copy/cutting text.

Comment 30

[Peter Van der Beken [:peterv]]

Checked in corrected patch into the branch (includes attachment 51811 [details] [diff] [review] from bug
102809).

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I suspect this caused bug 103250.

Comment 32

[Moied]

Verified on build ID 2001-10-22, fixed on testcase and new bug:106586 spun of 
from the URL, Setting bug to Verified and adding keyword vtrunk

Comment 33

[Hansoo Kim]

Hi Peter,

I am not sure if this has been fixed or not.

I am using 9.4 branch with your fixes in it,
but I still see html tag inside of pasted text.

Yes, the code passes the simple testcase provided by you in commment #8.
However, as Alex's comment  ( commment #6 ), I still see a lot of html tags
in pasted text.

The interesting thing is that even though I copy & paste the same thing
sometimes it has html tags and sometimes it does not.

About 40% of time, it has html tags it should not have.

Since we might need more static page to test this,
I am going to attach Zip file containing local html file which we can test 
again.


Here is the ONE test result.
1) Open the local page.
2) Highlight from beginning "L" in "Letter to Senator Full of Anthrax"
   up to the half way of right column.
3) Copy by ctrl+C
4) paste it in any text viewer which does not understand html.

Result...

// **  BEGIN RESULT ***

Letter to Senator Full of Anthrax
By PETE YOST

WASHINGTON (AP) - A letter to Sen. Patrick Leahy was laced with billions of 
anthrax spores, authorities said, and a mysterious new case of the disease was 
confirmed in Connecticut.

//****************  OMITTED !!!

The outside of the Leahy letter appears virtually identical to the Daschle 
letter and bears the same fictitious ``Greendale School'' return address, all-
capital block letters and other characteristics.


11/21/01 06:59

© Copyright The Associated Press. All rights reserved. The information 
contained In this news report may not be published, broadcast or otherwise 
distributed without the prior written authority of the Associated Press.
 
<script></script> 
<http://ads.web.cs.com/content/B0/0/qW9O30WF_IT69xZimje5hWEul7I5XyZN7R_4RdOFz9RK
egbtxOVwu-HpICLMRK9wcg6ilN8MSUCPvYCjCwiPjunqukLt6Sq6E6Xx9twhp8w$/aol>
National
• Letter to Senator Full of Anthrax
• Marines May Be Sent Into Afghanistan

// **  END RESULT ***





As you can see, we still have *some* html code.

If you want to test this in 9.4 embedding environment,
please contact Roy Yokoyama at NS.

He should be able to help you with setting up the environment.

The following attachment will contain the zip file for local page for testing.

I am re-opening this bug.

Thanks.

P.S :  This is one of top stoppers for our client.

Comment 34

[Hansoo Kim]

Attachment: Zip file containing complete web page files

This is a Zip file containing complete web page files
as mentioned in comment #33.

Thanks.

Comment 35

[Peter Van der Beken [:peterv]]

Trying to reproduce this again. No luck so far.

Comment 36

[Marcia Knous [:marcia]]

EDT - Jennifer: Can you please test this with the latest Compuserve build?

Comment 37

[Jennifer Kobayashi]

I can't repro, tried a number of different test sites. However, this bug isn't 
100% reproducible. I'm pinging some other folks to see if they can repro, and 
will make additional comments when I hear back from them. Thanks. 

Comment 38

[Hansoo Kim]

Hi All,

I understand this bug is relly hard to re-produce.

Let's all use the STATIC page so that re-producing it
would be easier once we find a way to re-produce it.

The ZIP file I provided as #33 has a static page,
and as I mentioned it was easier to re-produce it from there.

Thanks.

Comment 39

[Hansoo Kim]

More testing with Ken reveals we occasionally see 

<script></script> 

in the pasted text.

It appears in place of the link for certain javascript-based CS web ads.
( Where the images for ads are. )

I could not re-pro the case where I see some tags other than
"<script></script>"

Thanks.

Comment 40

[Judson Valeski]

minusing this one; not 100% reproducible.

Comment 41

[Peter Van der Beken [:peterv]]

If we can reproduce reliably, I'll pull this back.

Comment 42

[Hansoo Kim]

Peter,

edt0.9.4- means we are not fixing this for 9.4 branch anymore ?

I just e-mailed one more time who filed this bug in the first place.
Hope he can provide us with more info on how to re-pro this reliably.

In the meanwhile, could you give me the status of this bug ?
Are we still trying to fix it ?

Thank you.

Comment 43

[Peter Van der Beken [:peterv]]

edt0.9.4- means we are not trying to fix this for 0.9.4 branch. Unless I can
reliably reproduce it, there's not much I can do about this bug. If this only
happens with the CS/AOL ads, maybe you're hitting bug 106586, which was fixed on
the trunk but not on the branch?

Comment 44

[Hansoo Kim]

Peter,

It looks like this bug has a lot of common things with 
http://bugzilla.mozilla.org/show_bug.cgi?id=106586.

I tested with the current CS build with the test cases from 
http://bugzilla.mozilla.org/show_bug.cgi?id=106586.
This is the result.
It looks like we need a fix in 9.4 branch...



IN-DEPTH
<script></script> QUICK NEWS

Before <script></script> After



I understand, for some reason, Marek removed edt9.4 kw,
however, can we put this into 9.4 branch ?

Thanks.

Comment 45

[Hansoo Kim]

Peter,

I have not seen any response from you for Comment #44 From Hansoo Kim 2002-01-
07 12:48 -------
Just wondering the status of this bug for 9.4 branch.

As far as I know we have two more pull from 9.4 branch b/f the GM.
I will update the bug woth the last day of the pull shortly.

In the meantime, could you please let me know the status of this bug for 9.4 
branch.

Thank you so much in advance.

HK ( Hansoo Kim )

Comment 46

[Hansoo Kim]

Peter,

Just found that we have ONLY ONE more pull from 9.4 branch
b/f GM and any other pulls after that are unclear ( might be none ).

The corresponidng Vantive bug has a status of Elvis.
However since we are not very unclear on whether there is more pull after the 
last pull for GM, can you put to 9.4 branch the patch made for
http://bugzilla.mozilla.org/show_bug.cgi?id=106586 ?

FYI, the last pull date is 1/15/02 Midnight.

Thank you so much in advance.

HK ( Hansoo Kim )

Comment 47

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

Ok, since topembed+ we need to take a hard look at this.

We really need a way to reproduce this. I suspect that if someone can reproduce
this like 40% of the time the difference is minute change in where you start &
end the text selection (before a character, after a character, on top of a
character). Anyone who has been able to reproduce this at least once, please try
to figure out how to exactly do this so we could have real changes of fixing
this. Oh, and please test on the trunk now ;)

Comment 48

[Winnie Lam]

-> moied, can you try to reproduce this using the latest build?

Comment 49

[Peter Van der Beken [:peterv]]

Is anyone still able to reproduce this?

Comment 50

[Michael Dunn]

Moied, Is this still an open issue?

Comment 51

[harishd]

peterv: All we need to do is check for noxxx ( noscript, noframes, nolayer,
noembed, etc. ) tag in the plain-text-serializer and ignore its content. Let me
give it a try and see if I can fix the problem.

Comment 52

[Peter Van der Beken [:peterv]]

If you think that's the problem, sure. BTW, I already made it ignore noscript
when JS is enabled, see attachment 51548 [details] [diff] [review].

Comment 53

[harishd]

This must be the problem area:

NS_IMETHODIMP
nsPlainTextSerializer::GetPref(PRInt32 aTag, PRBool& aPref)
{
  nsHTMLTag theHTMLTag = nsHTMLTag(aTag);

  if (theHTMLTag == eHTMLTag_script) {
    aPref = mFlags & nsIDocumentEncoder::OutputNoScriptContent;
  }

The above code should read

nsPlainTextSerializer::GetPref(PRInt32 aTag, PRBool& aPref)
{
  nsHTMLTag theHTMLTag = nsHTMLTag(aTag);

  if (theHTMLTag == eHTMLTag_script) {
    aPref = !(mFlags & nsIDocumentEncoder::OutputNoScriptContent);
  }

Comment 54

[harishd]

The change that I suggested in comment #53 does fix the problem. Well, almost!
That is, when I copy paste the text "Begin End" I see a line break between them.
I think I have a fix for that as well. Will post a patch soon.

Comment 55

[harishd]

Attachment: patch v1.1

Note: I'm thinking of changing the method name GetPref ( in nsIHTMLContentSink
) to IsEnabled or something that will make more sense. So, will be posting
another patch soon.

Comment 56

[harishd]

Attachment: patch v1.2

This patch includes, in addition to changes in patch v1.1, the method name
change ( GetPref -> IsEnabled ).

Comment 57

[Peter Van der Beken [:peterv]]

Comment on attachment 111101 [details] [diff] [review]
patch v1.2

>@@ -923,18 +923,22 @@
> nsresult
> nsPlainTextSerializer::DoCloseContainer(PRInt32 aTag)
> {
>-  eHTMLTags type = (eHTMLTags)aTag;
>-
>+  // If mTagStackIndex >= mIgnoreAboveIndex is true then we're 
>+  // dealing with the close tag whose matching open tag had
>+  // set the mIgnoreAboveIndex value. Since no processing was
>+  // done for the open tag ignore the close tag as well.
>+  PRBool ignore = mTagStackIndex >= mIgnoreAboveIndex;
>+  
>   if (mTagStackIndex > 0) {
>     --mTagStackIndex;
>   }
> 
>-  if (mTagStackIndex >= mIgnoreAboveIndex) {
>+  if (ignore) {
>+    mIgnoreAboveIndex = (PRUint32)kNotFound;
>     return NS_OK;
>   }
> 
>-  mIgnoreAboveIndex = (PRUint32)kNotFound;
>-
>+  eHTMLTags type = (eHTMLTags)aTag;
>   // End current line if we're ending a block level tag
>   if((type == eHTMLTag_body) || (type == eHTMLTag_html)) {
>     // We want the output to end with a new line,

I don't understand how this works after your patch. DoOpenContainer does:

mTagStack[mTagStackIndex++] = type;
...
if (type == eHTMLTag_noscript ...) {
  mIgnoreAboveIndex = mTagStackIndex;
  return NS_OK;
}

so mIgnoreAboveIndex is the index just above the noscript.

After your patch DoCloseContainer now checks if mTagStackIndex >=
mIgnoreAboveIndex which will be true for anything that came in after the
noscript, but not for the noscript itself. Moreover you reset mIgnoreAboveIndex
for the first DoCloseContainer after noscript???

Comment 58

[harishd]

>After your patch DoCloseContainer now checks if mTagStackIndex >=
>mIgnoreAboveIndex which will be true for anything that came in after the
>noscript, but not for the noscript itself.

Since the open tag that sets mIgnoreAboveIndex is ignored we should ignore its
matching close tag. Ex.
Index 1 <html> 
Index 2 <body>
         Text
Index 3 <noscript>
          some content
        </noscript>
        <p>
Based on the above example <noscript>, before getting ignored, would set
mIgnoreAboveIndex to 3. Note: at this point the tag index is 3 as well. Since
<noscript> got ignored we should ignore </noscript> as well. Before my patch,
on encountering </noscript> ( Note: mTagStackIndex is still 3 ) we would
decrement mTagStackIndex and then checked whether mTagStackIndex >=
mIgnoreAboveIndex was true or not. Since mTagStackIndex was decremented the
expression mTagStackIndex >= mIgnoreAboveIndex will not evaluate to true and
hence will end up handling </noscript>

May be I should replace mTagStackIndex >= mIgnoreAboveIndex, in
DoCloseContainer, to mIgnoreAboveIndex != kNotFound; similar to the line in
DoOpenContainer. What do you think?

Comment 59

[harishd]

>Moreover you reset mIgnoreAboveIndex for the first DoCloseContainer after
>noscript???

You're right. This does not sound right. However, if I don't reset it I'm not
able to copy and paste completely. That is, it only pastes the word "Begin".
Wonder what's going on here. Let me investigate further.

Comment 60

[harishd]

>That is, it only pastes the word "Begin". Wonder what's going on here. Let me
>investigate further.

If I don't reset mIgnoreAboveIndex then the content that follows </noscript> is
ignored as well. Therefore we have reset mIgnoreAboveIndex when mTagStackIndex
is equal tomIgnoreAboveIndex.

Comment 61

[harishd]

Attachment: patch v1.2

Comment 62

[harishd]

Attachment: patch v1.3

Comment 63

[Peter Van der Beken [:peterv]]

It seems to me that on
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsPlainTextSerializer.cpp#637
we should be doing

  mIgnoreAboveIndex = mTagStackIndex - 1;

so that the index corresponds to the position of the tag to ignore (eg noscript)
and in nsPlainTextSerializer::DoCloseContainer(PRInt32 aTag):

  if (mTagStackIndex > 0) {
    --mTagStackIndex;
  }

  if (mTagStackIndex == mIgnoreAboveIndex) {
    mIgnoreAboveIndex = (PRUint32)kNotFound;
    return NS_OK;
  }

  if (mTagStackIndex > mIgnoreAboveIndex) {
    return NS_OK;
  }

mIgnoreAboveIndex would still be badly named, since it's really
IgnoreAboveOrEqualTo.

Comment 64

[harishd]

peterv: I like your suggestions. The only thing that I would do a little
different is to replace to following lines

  if (mTagStackIndex == mIgnoreAboveIndex) {
    mIgnoreAboveIndex = (PRUint32)kNotFound;
    return NS_OK;
  }

  if (mTagStackIndex > mIgnoreAboveIndex) {
    return NS_OK;
  }

to 

  if ((mTagStackIndex  >= mIgnoreAboveIndex) {
    if ((mTagStackIndex  == mIgnoreAboveIndex) {
      // We're dealing with the close tag whose matching
      // open tag had set the mIgnoreAboveIndex value.
      // Reset mIgnoreAboveIndex before discarding this tag.
      mIgnoreAboveIndex = (PRUint32)kNotFound;
    }
    return NS_OK;
  }

Comment 65

[Peter Van der Beken [:peterv]]

Fine by me. New patch coming?

Comment 66

[harishd]

Attachment: patch v1.4

Includes peterv's suggestion.

Comment 67

[harishd]

Comment on attachment 111654 [details] [diff] [review]
patch v1.4

Heikki verbally okayed to carry forward his sr=

Comment 68

[Peter Van der Beken [:peterv]]

Comment on attachment 111654 [details] [diff] [review]
patch v1.4

>Index: content/base/src/nsPlainTextSerializer.cpp
>===================================================================

>@@ -634,7 +634,9 @@
>        !(mFlags & nsIDocumentEncoder::OutputNoScriptContent)) ||
>       ((type == eHTMLTag_iframe || type == eHTMLTag_noframes) &&
>        !(mFlags & nsIDocumentEncoder::OutputNoFramesContent))) {
>-    mIgnoreAboveIndex = mTagStackIndex;
>+    // Ignore everything that follows the current tag in 
>+    // question unitl a matching end tag is encountered.

    // Ignore everything that follows the current tag until
    // a matching end tag is encountered.

>Index: content/html/document/src/nsHTMLContentSink.cpp
>===================================================================

> NS_IMETHODIMP
>-HTMLContentSink::GetPref(PRInt32 aTag, PRBool& aPref)
>+HTMLContentSink::IsEnabled(PRInt32 aTag, PRBool* aReturn)
> {
>   nsHTMLTag theHTMLTag = nsHTMLTag(aTag);
> 
>   if (theHTMLTag == eHTMLTag_script) {
>-    aPref = mFlags & NS_SINK_FLAG_SCRIPT_ENABLED;
>+    *aReturn = mFlags & NS_SINK_FLAG_SCRIPT_ENABLED;
>   } else if (theHTMLTag == eHTMLTag_frameset) {
>-    aPref = mFlags & NS_SINK_FLAG_FRAMES_ENABLED;
>+    *aReturn = mFlags & NS_SINK_FLAG_FRAMES_ENABLED;

These should be either

*aReturn = mFlags & NS_SINK_FLAG_SCRIPT_ENABLED != 0;

or

*aReturn = !!(mFlags & NS_SINK_FLAG_SCRIPT_ENABLED);

or

*aReturn = (mFlags & NS_SINK_FLAG_SCRIPT_ENABLED) ==
NS_SINK_FLAG_SCRIPT_ENABLED;

I wonder if we should just rename mIgnoreAboveIndex to mIgnoreIndex. Either
way, r=peterv.

BTW, that nsXMLContentSink.cpp change is unrelated, right?

Comment 69

[harishd]

>   if (theHTMLTag == eHTMLTag_script) {
>-    aPref = mFlags & NS_SINK_FLAG_SCRIPT_ENABLED;
>+    *aReturn = mFlags & NS_SINK_FLAG_SCRIPT_ENABLED;
>   } else if (theHTMLTag == eHTMLTag_frameset) {
>-    aPref = mFlags & NS_SINK_FLAG_FRAMES_ENABLED;
>+    *aReturn = mFlags & NS_SINK_FLAG_FRAMES_ENABLED;
>These should be either
>*aReturn = mFlags & NS_SINK_FLAG_SCRIPT_ENABLED != 0;
>or
>*aReturn = !!(mFlags & NS_SINK_FLAG_SCRIPT_ENABLED);

Why? I understand that mFlags & NS_SINK_FLAG_SCRIPT_ENABLED does not evalutate
to TRUE or FALSE rather it evaluates to value greater than zero or zero. And
it's no different from mFlags & NS_SINK_FLAG_SCRIPT_ENABLED != 0 or !!(mFlags &
NS_SINK_FLAG_SCRIPT_ENABLED)....I think. Also, the caller does not compare to
aReturn to PR_TRUE or PR_FALSE.

Comment 70

[Peter Van der Beken [:peterv]]

As Sicking points out, a caller might compare to another PRBool and this is a
public API. Don't put the burden of checking this on the caller.
Afaik all our supported compilers promote true and false to 1 and 0, as they
should according to my C++ books. (! and == return boolean values)

(Another easy to make mistake would be:
PRPackedBool mFramesEnabled;
PRBool framesEnabled;
IsEnabled(eHTMLTag_frameset, &framesEnabled);
mFramesEnabled = framesEnabled;)

Comment 71

[harishd]

Fixed.