100649 - Length() being used when IsEmpty() is meant

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

I'm seeing this in a lot of the content/html/style/src stuff (hence the
component).  Also in content/html/content/src.

I'll go through and fix those usages and attach a patch.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for content/

Comment 2

[Boris Zbarsky [:bzbarsky]]

alec, jag, would you review?  This was fairly straightforward, no build changes
needed.

Comment 3

[jag (Peter Annema)]

Comment on attachment 64712 [details] [diff] [review]
Patch for content/

>Index: content/html/document/src/nsHTMLDocument.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v
>retrieving revision 3.399
>diff -u -r3.399 nsHTMLDocument.cpp
>--- content/html/document/src/nsHTMLDocument.cpp	2002/01/10 14:07:41	3.399
>+++ content/html/document/src/nsHTMLDocument.cpp	2002/01/13 05:07:35
>@@ -1150,7 +1150,7 @@
> NS_IMETHODIMP 
> nsHTMLDocument::SetReferrer(const nsAReadableString& aReferrer)
> {
>-  if (0 < aReferrer.Length()) {
>+  if (aReferrer.IsEmpty()) {

Should be !aReferrer.IsEmpty()

>Index: content/xul/document/src/nsXULDocument.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v
>retrieving revision 1.471
>diff -u -r1.471 nsXULDocument.cpp
>--- content/xul/document/src/nsXULDocument.cpp	2002/01/12 01:17:49	1.471
>+++ content/xul/document/src/nsXULDocument.cpp	2002/01/13 05:08:10
>@@ -6819,12 +6819,12 @@

...

> 
>-            if (rv == NS_CONTENT_ATTR_HAS_VALUE && broadcasterID.Length() > 0) {
>+            if (rv == NS_CONTENT_ATTR_HAS_VALUE && broadcasterID.IsEmpty()) {

Should be !broadcasterID.IsEmpty()

Fix that and r=jag

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for content v 1.1 (includes Jag's changes)

Comment 5

[Boris Zbarsky [:bzbarsky]]

not happening tonight.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 64750 [details] [diff] [review]
Patch for content v 1.1 (includes Jag's changes)

sr=jst

Comment 7

[Boris Zbarsky [:bzbarsky]]

Patch checked in.  Setting component to layout as that's the next module I plan
to do this to.  Not happening for a bit, though....

Comment 8

[Boris Zbarsky [:bzbarsky]]

No time to work on this.

Comment 9

[Ariel Fatecha]

I don't understand... why this bug is still open?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Because I only made changes to content/.  We have other modules too.

Comment 11

[Ariel Fatecha]

ok, I want to help in this bug.
I matched for '\.Length() > 0' , '0 <.*\.Length()' , '\.Length() == 0' , '0
==.*\.Length()'
into all mozilla code... now I'll check for this matchs correspond to ns??String
or derived from it...
I have make it in all directories? or only for layout's directories?
if only for layout's directories: which directories are that? (I need know only
the firt deep, ej: "content/" or "layout")...
Anything else that I need to know... please comment.

Comment 12

[Ariel Fatecha]

of course, before doing anything, I have downloaded the source for 1.3b... (too
many files to change)

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

This should ideally be done for all files in the source code.

But you need not do it all at once :)
(Layout code consists of layout/ and maybe content/ and maybe view/, to my
knowledge)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ariel, the main point of this change is to make the code clearer.  It's not an
urgent change, so it can be done at whatever pace you prefer; I suggest doing it
in pieces, since reviewing a full-tree diff would be next to impossible.

Would you like the bug to be assigned to you?

Comment 15

[Ariel Fatecha]

is your comment an answer to mine in bug 178212?
anyway I'd like it to be assigned to me. But... can you do it? even if I can
only open a bug as _UNCONFIRMED_?

Comment 16

[Boris Zbarsky [:bzbarsky]]

NO, my comment was for this bug

This is all yours.  ;)

Comment 17

[Ariel Fatecha]

Thanks. I'm working in layout/. :-)

Comment 18

[Ariel Fatecha]

Attachment: patch for layout directory

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 114454 [details] [diff] [review]
patch for layout directory

sr=jst, but I'd loose the extra parens in cases like:

-    if (myList->mListStyleImage.Length() > 0)
+    if (!(myList->mListStyleImage.IsEmpty()))

... and...

-	 if (mimeType || (src.Length() > 0)) {
+	 if (mimeType || (!src.IsEmpty())) {

Comment 20

[Ariel Fatecha]

Attachment: patch for layout with jst changes

Comment 21

[Ariel Fatecha]

Attachment: patch for caps/ directory

Comment 22

[Ariel Fatecha]

Attachment: patch for content/ directory

Comment 23

[Ariel Fatecha]

Attachment: patch for directory/ directory

Comment 24

[Ariel Fatecha]

Attachment: patch for docshell/ directory

Comment 25

[Ariel Fatecha]

Attachment: patch for editor/ directory

Comment 26

[Ariel Fatecha]

Attachment: patch for embedding/ directory

Comment 27

[Ariel Fatecha]

Attachment: patch for extensions/ directory

Comment 28

[Ariel Fatecha]

Attachment: patch for gfx/ directory

Comment 29

[Ariel Fatecha]

Attachment: patch for htmlparser/ directory

Comment 30

[Ariel Fatecha]

Attachment: patch for mailnews/ directory

Comment 31

[Ariel Fatecha]

Attachment: patch for modules/ directory

Comment 32

[Ariel Fatecha]

Attachment: patch for netwerk/ directory

Comment 33

[Ariel Fatecha]

Attachment: patch for profile/ directory

Comment 34

[Ariel Fatecha]

Attachment: patch for rdf/ directory

Comment 35

[Ariel Fatecha]

Attachment: patch for security/ directory

Comment 36

[Ariel Fatecha]

Attachment: patch for webshell/ directory

Comment 37

[Ariel Fatecha]

Attachment: patch for widget/ directory

Comment 38

[Ariel Fatecha]

Attachment: patchfor xpcom/ directory

Comment 39

[Ariel Fatecha]

Attachment: patch for xpfe/ directory

Comment 40

[Ariel Fatecha]

Attachment: patch for xpinstall/ directory

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114553 [details] [diff] [review]
patch for docshell/ directory

There's no way this patch can apply to a tree.	Please use "cvs diff" so that
the filename will be correct, ok?

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114552 [details] [diff] [review]
patch for directory/ directory

Same here.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114551 [details] [diff] [review]
patch for content/ directory

and here.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114550 [details] [diff] [review]
patch for caps/ directory 

and here.

Comment 45

[Ariel Fatecha]

what is "cvs diff"?

Comment 46

[Ariel Fatecha]

I want to kill my self!!!!! :-(
ok... can I do just a change in the .diff file?

Comment 47

[Boris Zbarsky [:bzbarsky]]

Ariel, if you have a CVS tree (not just a tarball; instructions at
http://mozilla.org/cvs.html) you can do this (from toplevel):

cvs diff -u8 content/html/content/src/nsHTMLImageElement.cpp

or even

cvs diff -u8 content

(the second one may take a while).

That will create a diff that can be applied to a tree using "patch".

One other thing.  No need for separate patches for each dir if the patches are
small.  The goal here is not to create huge unreviewable patches.  But lumping
together a bunch of the small ones in a single diff is fine.  ;)

Comment 48

[Ariel Fatecha]

A question:
I did my cvs checkout.
I'm making "cvs diff", ok... but it founds other diffs with my version code...
What assumes that I must to do? have I leave that in .diff? or have I clean it?

Comment 49

[Ariel Fatecha]

Attachment: patch for all directories 1/5

Comment 50

[Ariel Fatecha]

Attachment: patch for all directories 2/5

Comment 51

[Ariel Fatecha]

Attachment: patch for all directories 3/5

Comment 52

[Ariel Fatecha]

Attachment: patch for all directories 3/5

Comment 53

[Ariel Fatecha]

Attachment: patch for all directories 4/5

Comment 54

[Ariel Fatecha]

Attachment: patch for all directories 5/5

Comment 55

[Ariel Fatecha]

my last question was really stupid question!!! :-)
well, I hope all it's ok...

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114686 [details] [diff] [review]
patch for all directories 1/5

>Index: mailnews/imap/src/nsImapMailFolder.cpp

>-      if (currentFolderNameStr.Length() > 0)
>+      if (!currentFolderNameStr.IsEmpty)

You mean IsEmpty() with the parens, right?  ;)

>-        if (onlineName.Length() > 0)
>+        if (!onlineName.IsEmpty)

Same.  You did build mailnews with these changes, right?  ;)

>-  *userName = (m_ownerUserName.Length()) ? ToNewCString(m_ownerUserName) : nsnull;
>+  *userName = (!m_ownerUserName.IsEmpty()) ? ToNewCString(m_ownerUserName) : nsnull;

*userName = m_ownerUserName.IsEmpty() ? nsnull : ToNewCString(m_ownerUserName);

seems a lot less confusing....

The rest of patch 1/5 (mailnews, looks like) looks good.

I'll try to do the other patches soon, but it may take me a few days.....

Comment 57

[Ariel Fatecha]

Attachment: new patch for all directories 1/5

fixed both .Empty) and (!blabla.IsEmpty()) ?...

Comment 58

[Ariel Fatecha]

Attachment: new patch for all directories 2/5

fixed two: 
(!someone.IsEmpty()) ? ...:  ...

Comment 59

[Ariel Fatecha]

Comment on attachment 114715 [details] [diff] [review]
new patch for all directories 1/5

yeah, I was wrong ;)

Comment 60

[Ariel Fatecha]

Comment on attachment 114717 [details] [diff] [review]
new patch for all directories 2/5

I was wrong twice... :-)

Comment 61

[Ariel Fatecha]

Attachment: new patch for all directories 3/5

I found one: !someone.IsEmtpy() ... :-(

Comment 62

[Ariel Fatecha]

Target Milestone: 1.5 is apropiated according to new roadmap.

Comment 63

[[e-mail]]

Comment on attachment 114690 [details] [diff] [review]
patch for all directories 4/5

>Index: netwerk/protocol/data/src/nsDataChannel.cpp
...
>-    NS_ASSERTION(mContentType.Length() > 0, "data protocol should have content type by now");
>+    NS_ASSERTION(!mContentType.IsEmtpy(), "data protocol should have content type by now");

You really need to compile your patches before you post them and request r/sr.
You can't expect reviewers to do the job of a compiler for you. It's great you
got the hang of cvs diff, now you just need to set up your build environment
too ;)

It's great you're doing this - even better now that IsEmpty isn't a virtual
fncall (thanks to darin's keen eye). I'll be happy to review the patches for
you, to take some load off sicking. However, I don't think jst is listed as a
string guy in the sr list - probably a better choice would be alecf, jag, or
scc.

Comment 64

[Ariel Fatecha]

tonigth I'll try to compile code and to remove sr asked (I will not ask sr until
to have r+).
thanks :)

Comment 65

[[e-mail]]

sure ;)

by the way, I should probably check with the sr's whether they're okay with me
reviewing tree-wide code like this (trivial though it is)... bz/alecf/jag, what
say you?

Comment 66

[Ariel Fatecha]

it's fine for me ;)

Comment 67

[Johnny Stenback (:jst)]

I'm happy to sr this once it has an r=, but if some other sr chooses to step up
before I get to it, feel free. And Dan, I think it's fine for you to r= this
change, even if it's tree-wide (given the nature of the change).

Comment 68

[Ariel Fatecha]

on wesnesday's night I will try to compile the code. I'm a little busy these
days. ;)

Comment 69

[Ariel Fatecha]

I had a lot of compiling problems.... patches are too old and are bad...
I'll re-make its, but this will take me a few days. I think next week will be
done (monday or tuesday).
any comment about compilin's problems on Linux or how to make my life easy, will
be wellcome ;)
Dan: is right for you?

Comment 70

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. not to discourage cleanups like this, they are really nice to see (though
tedious to review), but now is not really a good time to do it. Things like this
should probably wait until the tree opens for 1.5 development. 

Ariel: will you keep these patches up-to-date until then?

Comment 71

[[e-mail]]

I'll be happy to take care of merge conflicts, assuming I end up landing the
patches.

Ariel: the timeframe is fine, but just be aware that it'll be easier to get this
in if it's finished by the time the tree opens in 3 weeks - there are less
checkins during freeze and thus less chance of the patches bitrotting. don't
forget that getting r/sr will take time. also, compiling on linux should be a
no-brainer provided you read all the build instructions on mozilla.org. good
luck. ;)

btw - thanks jst!

Comment 72

[Ariel Fatecha]

patches will be done on next week, after compile and resolve conflicts. 
I think I will have no time for to extends patches to cover new code (posted
after 20-02-2003) if there is it, but I'll try.
thanks for comments ;)

Comment 73

[Ariel Fatecha]

In my next life I 'll remember never make a "cvs update" ;)
I'll post patches. these are the same files, but conflicts removed and compiled.

Comment 74

[Ariel Fatecha]

Attachment: compiled 0/4

one

Comment 75

[Ariel Fatecha]

Attachment: compiled 1/4

two

Comment 76

[Ariel Fatecha]

Attachment: compiled 2/4

three

Comment 77

[Ariel Fatecha]

Attachment: compiled 3/4

four

Comment 78

[Ariel Fatecha]

Attachment: compiled 4/4

five

Comment 79

[Ariel Fatecha]

Comment on attachment 123169 [details] [diff] [review]
compiled 0/4

asking for r && sr.

Comment 80

[Ariel Fatecha]

Comment on attachment 123171 [details] [diff] [review]
compiled 2/4

asking for r && sr.

Comment 81

[Ariel Fatecha]

Comment on attachment 123173 [details] [diff] [review]
compiled 4/4

asking for r && sr.

Comment 82

[Ariel Fatecha]

Comment on attachment 123172 [details] [diff] [review]
compiled 3/4

asking for r && sr.

Comment 83

[Ariel Fatecha]

Comment on attachment 123170 [details] [diff] [review]
compiled 1/4

asking for r && sr.

Comment 84

[[e-mail]]

Comment on attachment 123169 [details] [diff] [review]
compiled 0/4

great, all your patches apply and compile now. ;)

btw, you may want to update your tree more regularly - all the patches applied
okay, but about 70% of the files you touched needed an offset or fuzz to patch
properly.

>Index: mailnews/imap/src/nsImapServerResponseParser.cpp
>===================================================================
>-					if (!fZeroLengthMessageUidString.Length())
>+					if (fZeroLengthMessageUidString.IsEmpty())
> 						fZeroLengthMessageUidString = uidString;
> 					else
> 					{
> 						fZeroLengthMessageUidString += ",";
> 						fZeroLengthMessageUidString += uidString;
> 					}

while you're in here, can you change this to:

if (!fZeroLengthMessageUidString.IsEmpty())
	fZeroLengthMessageUidString += ",";

fZeroLengthMessageUidString += uidString;

for a small codesize win?

>Index: mailnews/import/eudora/src/nsEudoraAddress.cpp
>===================================================================
>-	return( (m_email.Length() != 0));
>+	return (!m_email.IsEmpty());

nit: the rest of the file uses that (weird) convention, so best to stick to it.

	return( !m_email.IsEmpty());

>Index: mailnews/import/text/src/nsTextAddress.cpp
>===================================================================
>-    if (m_ldifLine.Length() > 0 && m_ldifLine.Find("groupOfNames") == -1)
>+    if (!m_ldifLine.IsEmpty() && m_ldifLine.Find("groupOfNames") == -1)

nit: s/-1/kNotFound/ here please?

rest looks great. r=dwitte with those nits addressed.

Comment 85

[[e-mail]]

Comment on attachment 123170 [details] [diff] [review]
compiled 1/4

>Index: editor/libeditor/html/nsHTMLEditUtils.cpp
>===================================================================
>   if (anchor)
>   {
>     nsAutoString tmpText;
>-    if (NS_SUCCEEDED(anchor->GetHref(tmpText)) && tmpText.get() && tmpText.Length() != 0)
>+    if (NS_SUCCEEDED(anchor->GetHref(tmpText)) && tmpText.get() && !tmpText.IsEmpty())
>       return PR_TRUE;
>   }

remove the |tmpText.get()|, it's unnecessary.

>Index: embedding/browser/activex/src/plugin/LegacyPlugin.cpp
>===================================================================
>-            if (!paramName.m_str || paramName.Length() == 0)
>+            if (!paramName.m_str || paramName.IsEmpty())

this isn't a moz string! better remove this change. ;)

>Index: embedding/browser/activex/src/pluginhostctrl/nsPluginHostCtrl.cpp
>===================================================================
>-    if (m_bstrContentType.Length() == 0 &&
>-        m_bstrSource.Length() != 0)
>+    if (m_bstrContentType.IsEmpty() &&
>+        !m_bstrSource.IsEmpty())

same here.

>-            if (m_bstrSource.Length())
>+            if (!m_bstrSource.IsEmpty())

and here.

the rest looks good, but I'll review- because of the bustage.

Comment 86

[[e-mail]]

Comment on attachment 123171 [details] [diff] [review]
compiled 2/4

>Index: extensions/wallet/src/singsign.cpp
>===================================================================
>   /* return if a password was found */
>-  if (password.Length() != 0) {
>+  if (!password.IsEmpty()) {
>     *pwd = ToNewUnicode(password);
>     *pressedOK = PR_TRUE;
>     return NS_OK;
>   }
> 
>   /* no password found, get new password from user */
>   *pwd = ToNewUnicode(password);

hmm, we do the ToNewUnicode() unconditionally. wanna shift it above the if()
and bag one of them?

>Index: extensions/wallet/src/wallet.cpp
>===================================================================
>-    if (text.Length()) {
>+    if (!text.IsEmpty()) {
>       someTextFound = PR_TRUE;
> 
>       TextToSchema(text, schema);
>       if (!schema.IsEmpty()) {
> 
>         /* schema found, process positional schema if any */
>         if (!schema.IsEmpty() && schema.First() == '%') {
>           wallet_ResolvePositionalSchema(elementNode, schema);

sorry, I know it's nothing to do with your changes, but can you kill that
second |!schema.IsEmpty()|? ;)

r=dwitte with nits addressed.

Comment 87

[[e-mail]]

Comment on attachment 123172 [details] [diff] [review]
compiled 3/4

>Index: xpcom/io/nsLocalFileOSX.cpp
>===================================================================
> NS_IMETHODIMP nsLocalFile::InitWithNativePath(const nsACString& filePath)
> {
>-  if (filePath.First() != '/' || filePath.Length() == 0)
>+  if (filePath.First() != '/' || filePath.IsEmpty())

ugh, that'll assert if the string is empty. please change this to:

  if (filePath.IsEmpty() || filePath.First() != '/')

>Index: xpfe/components/search/src/nsInternetSearchService.cpp
>===================================================================
>-	if (fullURL.Length() > 0)
>+	if (!fullURL.IsEmpty())
> 	{
> 	}
> 	else if (methodStr.EqualsIgnoreCase("get"))

uhh, that looks weird. how about:
	if (fullURL.IsEmpty() && methodStr.EqualsIgnoreCase("get"))

r=dwitte with nits addressed.

Comment 88

[[e-mail]]

Comment on attachment 123173 [details] [diff] [review]
compiled 4/4

>Index: layout/html/base/src/nsObjectFrame.cpp
>===================================================================
>-    if (NS_SUCCEEDED(rv) && value && *value && (value.Length() > 0))
>+    if (NS_SUCCEEDED(rv) && value && *value && !value.IsEmpty())

how about just:
    if (NS_SUCCEEDED(rv) && !value.IsEmpty())

sorry for finding a nit in this one. :)
r=dwitte

Comment 89

[Ariel Fatecha]

ok, tonigth I'll change it. ;)
Dan, is possible change files without re-post file and deprecate older file,
just like modify (o simple change) posted file?

Comment 90

[[e-mail]]

unfortunately, it's not. normally, if you make a small change to a big patch,
you can make a diffdiff - just like it sounds, it's a diff between two diffs. so
it shows the changes between the two patches.

since jst hasn't looked at them yet, and i've given conditional r+ on most, it's
probably easier just to post the 5 full patches again. :)

Comment 91

[Ariel Fatecha]

tomorrow night I'll make it... no time today :(

Comment 92

[Ariel Fatecha]

I remake diff file, so all is in one file. I'll post it. ;)

Comment 93

[Ariel Fatecha]

Attachment: Dan's corrections

it's all-in-one

Comment 94

[[e-mail]]

Comment on attachment 123588 [details] [diff] [review]
Dan's corrections

r=dwitte

note that your patch has now rotted - 8 files didn't apply correctly. no need
to post a new patch now - just wait until jst has sr'ed and we're closer to the
tree opening.

over to you jst.

Comment 95

[Johnny Stenback (:jst)]

Comment on attachment 123588 [details] [diff] [review]
Dan's corrections

- In mozilla/mailnews/base/util/nsMsgUtils.cpp:

-      if (pathPiece.Length() > 0) {
+      if (!pathPiece.IsEmpty()) {

	 // add .sbd onto the previous path
	 if (haveFirst) {
	   pathString+=".sbd";
	   pathString += "/";

Want to combine those two lines while you're at it? I.e.

	   pathString+=".sbd/";

sr=jst

Comment 96

[Ariel Fatecha]

I'll change it for next patch.
when do I have to post it (what date)?
btw, thanks jst and dwitte ;)

Comment 97

[Johnny Stenback (:jst)]

This could be checked in once the tree opens for 1.5a (fairly soon), when that
happens, just coordinate with someone who has cvs commit access (myself, if you
don't know someone else) and update the patches when that time comes.

Comment 98

[Ariel Fatecha]

I had worked for other bugs with Boris Zbarsky, who isn't reading bugmail, and
Jan Varga but he works with XUL's staff.
dan, will you can do it? ;)
btw, thanks jst

Comment 99

[[e-mail]]

according to npm.seamonkey we'll be branching tomorrow, so if you can update the
patch by tomorrow ~10am that'd be best. if not, then at your earliest
convenience is fine.

(remember that the patch doesn't apply properly, as noted in comment 94, so if
you could fix it that'd be great too.)

either myself or jst can check this in for you. i'll try to do it tomorrow
morning so there's as small a chance of rottage as possible. ;)

Comment 100

[[e-mail]]

oh btw, i have no great desire to torture you, so if updating the patch to
remove rot is too much of a pain, i can take care of that for you.

Comment 101

[Ariel Fatecha]

thanks Dan ;)
I can't do this before tomorrow morning :(
If you can do it before this time, much better (I think...) else tomorrow I'll
make it.

Comment 102

[[e-mail]]

i've applied the patch, fixed rot, and rolled a build. i'll check it in when the
tree opens, in a few chunks.

Comment 103

[Ariel Fatecha]

I'm back (who cares ;) )
thanks Dan ;) (again)
can I change resolution for this?
or are you going to do it?

Comment 104

[[e-mail]]

all yours.

now, i'm sure there's plenty of other string fu that could be cleaned up around
the codebase after this... ;)

Comment 105

[Ariel Fatecha]

ok.
fixed.
thanks to all ;)