If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Length() being used when IsEmpty() is meant

RESOLVED FIXED in mozilla1.5alpha

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
16 years ago
15 years ago

People

(Reporter: bz, Assigned: Ariel Fatecha)

Tracking

Trunk
mozilla1.5alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 38 obsolete attachments)

253.40 KB, patch
dwitte@gmail.com
: review+
Details | Diff | Splinter Review
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.
Priority: -- → P3
Target Milestone: --- → mozilla0.9.8
Created attachment 64712 [details] [diff] [review]
Patch for content/
alec, jag, would you review?  This was fairly straightforward, no build changes
needed.

Comment 3

16 years ago
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
Attachment #64712 - Flags: review+
Created attachment 64750 [details] [diff] [review]
Patch for content v 1.1 (includes Jag's changes)
Attachment #64712 - Attachment is obsolete: true
Attachment #64750 - Flags: review+
not happening tonight.
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Comment on attachment 64750 [details] [diff] [review]
Patch for content v 1.1 (includes Jag's changes)

sr=jst
Attachment #64750 - Flags: superreview+
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....
Component: DOM Style → Layout
Priority: P3 → P4
Target Milestone: mozilla0.9.9 → mozilla1.0
Attachment #64750 - Attachment is obsolete: true
No time to work on this.
Target Milestone: mozilla1.0 → mozilla1.2
Target Milestone: mozilla1.2alpha → Future
(Assignee)

Comment 9

15 years ago
I don't understand... why this bug is still open?
Because I only made changes to content/.  We have other modules too.
Keywords: helpwanted
Whiteboard: [good first bug]
(Assignee)

Comment 11

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

Comment 12

15 years ago
of course, before doing anything, I have downloaded the source for 1.3b... (too
many files to change)
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)
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?
(Assignee)

Comment 15

15 years ago
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_?
NO, my comment was for this bug

This is all yours.  ;)
Assignee: bzbarsky → afatecha
Priority: P4 → --
Target Milestone: Future → ---
(Assignee)

Comment 17

15 years ago
Thanks. I'm working in layout/. :-)
(Assignee)

Comment 18

15 years ago
Created attachment 114454 [details] [diff] [review]
patch for layout directory
(Assignee)

Updated

15 years ago
Attachment #114454 - Flags: superreview?(jst)
Attachment #114454 - Flags: review?(bzbarsky)
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())) {
Attachment #114454 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 20

15 years ago
Created attachment 114456 [details] [diff] [review]
patch for layout with jst changes
Attachment #114454 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #114454 - Flags: review?(bzbarsky)
(Assignee)

Updated

15 years ago
Attachment #114456 - Flags: superreview?(jst)
Attachment #114456 - Flags: review?(bzbarsky)
Attachment #114456 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 21

15 years ago
Created attachment 114550 [details] [diff] [review]
patch for caps/ directory
(Assignee)

Comment 22

15 years ago
Created attachment 114551 [details] [diff] [review]
patch for content/ directory
(Assignee)

Comment 23

15 years ago
Created attachment 114552 [details] [diff] [review]
patch for directory/ directory
(Assignee)

Comment 24

15 years ago
Created attachment 114553 [details] [diff] [review]
patch for docshell/ directory
(Assignee)

Comment 25

15 years ago
Created attachment 114554 [details] [diff] [review]
patch for editor/ directory
(Assignee)

Comment 26

15 years ago
Created attachment 114555 [details] [diff] [review]
patch for embedding/ directory
(Assignee)

Comment 27

15 years ago
Created attachment 114556 [details] [diff] [review]
patch for extensions/ directory
(Assignee)

Comment 28

15 years ago
Created attachment 114557 [details] [diff] [review]
patch for gfx/ directory
(Assignee)

Comment 29

15 years ago
Created attachment 114558 [details] [diff] [review]
patch for htmlparser/ directory
(Assignee)

Comment 30

15 years ago
Created attachment 114559 [details] [diff] [review]
patch for mailnews/ directory
(Assignee)

Comment 31

15 years ago
Created attachment 114560 [details] [diff] [review]
patch for modules/ directory
(Assignee)

Comment 32

15 years ago
Created attachment 114561 [details] [diff] [review]
patch for netwerk/ directory
(Assignee)

Comment 33

15 years ago
Created attachment 114562 [details] [diff] [review]
patch for profile/ directory
(Assignee)

Comment 34

15 years ago
Created attachment 114563 [details] [diff] [review]
patch for rdf/ directory
(Assignee)

Comment 35

15 years ago
Created attachment 114564 [details] [diff] [review]
patch for security/ directory
(Assignee)

Comment 36

15 years ago
Created attachment 114565 [details] [diff] [review]
patch for webshell/ directory
(Assignee)

Comment 37

15 years ago
Created attachment 114566 [details] [diff] [review]
patch for widget/ directory
(Assignee)

Comment 38

15 years ago
Created attachment 114567 [details] [diff] [review]
patchfor xpcom/ directory
(Assignee)

Comment 39

15 years ago
Created attachment 114568 [details] [diff] [review]
patch for xpfe/ directory
(Assignee)

Comment 40

15 years ago
Created attachment 114569 [details] [diff] [review]
patch for xpinstall/ directory
(Assignee)

Updated

15 years ago
Attachment #114550 - Flags: superreview?(jst)
Attachment #114550 - Flags: review?(bzbarsky)
(Assignee)

Updated

15 years ago
Attachment #114551 - Flags: superreview?(jst)
Attachment #114551 - Flags: review?(bzbarsky)
(Assignee)

Updated

15 years ago
Attachment #114552 - Flags: superreview?(jst)
Attachment #114552 - Flags: review?(bzbarsky)
(Assignee)

Updated

15 years ago
Attachment #114553 - Flags: superreview?(jst)
Attachment #114553 - Flags: review?(bzbarsky)
(Assignee)

Updated

15 years ago
Attachment #114554 - Flags: superreview?(jst)
Attachment #114554 - Flags: review?(bzbarsky)
(Assignee)

Updated

15 years ago
Attachment #114555 - Flags: superreview?(jst)
Attachment #114555 - Flags: review?(bzbarsky)
(Assignee)

Updated

15 years ago
Attachment #114556 - Flags: superreview?(jst)
Attachment #114556 - Flags: review?(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?
Attachment #114553 - Flags: review?(bzbarsky) → review-
Comment on attachment 114552 [details] [diff] [review]
patch for directory/ directory

Same here.
Attachment #114552 - Flags: review?(bzbarsky) → review-
Comment on attachment 114551 [details] [diff] [review]
patch for content/ directory

and here.
Attachment #114551 - Flags: review?(bzbarsky) → review-
Comment on attachment 114550 [details] [diff] [review]
patch for caps/ directory 

and here.
Attachment #114550 - Flags: review?(bzbarsky) → review-
(Assignee)

Updated

15 years ago
Attachment #114557 - Flags: superreview?(jst)
Attachment #114557 - Flags: review?(bzbarsky)
Attachment #114557 - Flags: superreview?(jst)
Attachment #114557 - Flags: review?(bzbarsky)
Attachment #114556 - Flags: superreview?(jst)
Attachment #114556 - Flags: review?(bzbarsky)
Attachment #114555 - Flags: superreview?(jst)
Attachment #114555 - Flags: review?(bzbarsky)
Attachment #114554 - Flags: superreview?(jst)
Attachment #114554 - Flags: review?(bzbarsky)
(Assignee)

Comment 45

15 years ago
what is "cvs diff"?
(Assignee)

Comment 46

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

Updated

15 years ago
Attachment #114550 - Flags: superreview?(jst)
(Assignee)

Updated

15 years ago
Attachment #114551 - Attachment is obsolete: true
Attachment #114551 - Flags: superreview?(jst)
(Assignee)

Updated

15 years ago
Attachment #114552 - Attachment is obsolete: true
Attachment #114552 - Flags: superreview?(jst)
(Assignee)

Updated

15 years ago
Attachment #114553 - Attachment is obsolete: true
Attachment #114553 - Flags: superreview?(jst)
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

15 years ago
Attachment #114569 - Attachment is obsolete: true
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.  ;)
(Assignee)

Comment 48

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

Updated

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

Updated

15 years ago
Attachment #114456 - Attachment is obsolete: true
Attachment #114456 - Flags: superreview?(jst)
(Assignee)

Comment 49

15 years ago
Created attachment 114686 [details] [diff] [review]
patch for all directories 1/5
(Assignee)

Comment 50

15 years ago
Created attachment 114687 [details] [diff] [review]
patch for all directories 2/5
(Assignee)

Comment 51

15 years ago
Created attachment 114688 [details] [diff] [review]
patch for all directories 3/5
(Assignee)

Comment 52

15 years ago
Created attachment 114689 [details] [diff] [review]
patch for all directories 3/5
(Assignee)

Comment 53

15 years ago
Created attachment 114690 [details] [diff] [review]
patch for all directories 4/5
(Assignee)

Comment 54

15 years ago
Created attachment 114691 [details] [diff] [review]
patch for all directories 5/5
(Assignee)

Updated

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

Updated

15 years ago
Attachment #114686 - Flags: superreview?(jst)
Attachment #114686 - Flags: review?(bzbarsky)
(Assignee)

Updated

15 years ago
Attachment #114687 - Flags: superreview?(jst)
Attachment #114687 - Flags: review?(bzbarsky)
(Assignee)

Updated

15 years ago
Attachment #114689 - Flags: superreview?(jst)
Attachment #114689 - Flags: review?(bzbarsky)
(Assignee)

Updated

15 years ago
Attachment #114690 - Flags: superreview?(jst)
Attachment #114690 - Flags: review?(bzbarsky)
(Assignee)

Updated

15 years ago
Attachment #114691 - Flags: superreview?(jst)
Attachment #114691 - Flags: review?(bzbarsky)
(Assignee)

Comment 55

15 years ago
my last question was really stupid question!!! :-)
well, I hope all it's ok...
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.....
Attachment #114686 - Flags: superreview?(jst)
Attachment #114686 - Flags: review?(bzbarsky)
Attachment #114686 - Flags: review-
(Assignee)

Updated

15 years ago
Attachment #114687 - Attachment is obsolete: true
Attachment #114687 - Flags: superreview?(jst)
Attachment #114687 - Flags: review?(bzbarsky)
(Assignee)

Updated

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

Comment 57

15 years ago
Created attachment 114715 [details] [diff] [review]
new patch for all directories 1/5

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

Comment 58

15 years ago
Created attachment 114717 [details] [diff] [review]
new patch for all directories 2/5

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

Comment 59

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

yeah, I was wrong ;)
Attachment #114715 - Flags: superreview?(jst)
Attachment #114715 - Flags: review?(bzbarsky)
(Assignee)

Comment 60

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

I was wrong twice... :-)
Attachment #114717 - Flags: superreview?(jst)
Attachment #114717 - Flags: review?(bzbarsky)
(Assignee)

Comment 61

15 years ago
Created attachment 114904 [details] [diff] [review]
new patch for all directories 3/5

I found one: !someone.IsEmtpy() ... :-(
Attachment #114689 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #114689 - Flags: superreview?(jst)
Attachment #114689 - Flags: review?(bzbarsky)
(Assignee)

Updated

15 years ago
Attachment #114904 - Flags: superreview?(jst)
Attachment #114904 - Flags: review?(bzbarsky)
(Assignee)

Updated

15 years ago
Attachment #114690 - Flags: review?(bzbarsky) → review?(bugmail)
(Assignee)

Updated

15 years ago
Attachment #114717 - Flags: review?(bzbarsky) → review?(bugmail)
(Assignee)

Updated

15 years ago
Attachment #114904 - Flags: review?(bzbarsky) → review?(bugmail)
(Assignee)

Updated

15 years ago
Attachment #114715 - Flags: review?(bzbarsky) → review?(bugmail)
(Assignee)

Updated

15 years ago
Attachment #114691 - Flags: review?(bzbarsky) → review?(bugmail)
(Assignee)

Comment 62

15 years ago
Target Milestone: 1.5 is apropiated according to new roadmap.
Priority: -- → P3
Whiteboard: [good first bug] → [good first bug][patch, review]
Target Milestone: --- → mozilla1.5alpha

Comment 63

15 years ago
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.
Attachment #114690 - Flags: superreview?(jst)
Attachment #114690 - Flags: review?(bugmail)
Attachment #114690 - Flags: review-
(Assignee)

Comment 64

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

Comment 65

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

Comment 66

15 years ago
it's fine for me ;)
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).
(Assignee)

Comment 68

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

Comment 69

15 years ago
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?
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

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

Comment 72

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

Comment 73

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

Comment 74

15 years ago
Created attachment 123169 [details] [diff] [review]
compiled 0/4

one
Attachment #114715 - Attachment is obsolete: true
(Assignee)

Comment 75

15 years ago
Created attachment 123170 [details] [diff] [review]
compiled 1/4

two
Attachment #114717 - Attachment is obsolete: true
(Assignee)

Comment 76

15 years ago
Created attachment 123171 [details] [diff] [review]
compiled 2/4

three
Attachment #114904 - Attachment is obsolete: true
(Assignee)

Comment 77

15 years ago
Created attachment 123172 [details] [diff] [review]
compiled 3/4

four
Attachment #114690 - Attachment is obsolete: true
(Assignee)

Comment 78

15 years ago
Created attachment 123173 [details] [diff] [review]
compiled 4/4

five
Attachment #114691 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #114691 - Flags: superreview?(jst)
Attachment #114691 - Flags: review?(bugmail)
(Assignee)

Updated

15 years ago
Attachment #114717 - Flags: superreview?(jst)
Attachment #114717 - Flags: review?(bugmail)
(Assignee)

Updated

15 years ago
Attachment #114715 - Flags: superreview?(jst)
Attachment #114715 - Flags: review?(bugmail)
(Assignee)

Updated

15 years ago
Attachment #114904 - Flags: superreview?(jst)
Attachment #114904 - Flags: review?(bugmail)
(Assignee)

Comment 79

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

asking for r && sr.
Attachment #123169 - Flags: superreview?(jst)
Attachment #123169 - Flags: review?(dwitte)
(Assignee)

Comment 80

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

asking for r && sr.
Attachment #123171 - Flags: superreview?(jst)
Attachment #123171 - Flags: review?(dwitte)
(Assignee)

Comment 81

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

asking for r && sr.
Attachment #123173 - Flags: superreview?(jst)
Attachment #123173 - Flags: review?(dwitte)
(Assignee)

Comment 82

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

asking for r && sr.
Attachment #123172 - Flags: superreview?(jst)
Attachment #123172 - Flags: review?(dwitte)
(Assignee)

Comment 83

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

asking for r && sr.
Attachment #123170 - Flags: superreview?(jst)
Attachment #123170 - Flags: review?(dwitte)

Comment 84

15 years ago
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.
Attachment #123169 - Flags: review?(dwitte) → review+

Comment 85

15 years ago
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.
Attachment #123170 - Flags: superreview?(jst)
Attachment #123170 - Flags: review?(dwitte)
Attachment #123170 - Flags: review-

Comment 86

15 years ago
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.
Attachment #123171 - Flags: review?(dwitte) → review+

Comment 87

15 years ago
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.
Attachment #123172 - Flags: review?(dwitte) → review+

Comment 88

15 years ago
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
Attachment #123173 - Flags: review?(dwitte) → review+
(Assignee)

Comment 89

15 years ago
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

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

Comment 91

15 years ago
tomorrow night I'll make it... no time today :(
(Assignee)

Comment 92

15 years ago
I remake diff file, so all is in one file. I'll post it. ;)
(Assignee)

Comment 93

15 years ago
Created attachment 123588 [details] [diff] [review]
Dan's corrections

it's all-in-one
(Assignee)

Updated

15 years ago
Attachment #123169 - Attachment is obsolete: true
Attachment #123170 - Attachment is obsolete: true
Attachment #123171 - Attachment is obsolete: true
Attachment #123172 - Attachment is obsolete: true
Attachment #123173 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #123172 - Flags: superreview?(jst)
(Assignee)

Updated

15 years ago
Attachment #123171 - Flags: superreview?(jst)
(Assignee)

Updated

15 years ago
Attachment #123169 - Flags: superreview?(jst)
(Assignee)

Updated

15 years ago
Attachment #123173 - Flags: superreview?(jst)
(Assignee)

Updated

15 years ago
Attachment #123588 - Flags: superreview?(jst)
Attachment #123588 - Flags: review?(dwitte)

Comment 94

15 years ago
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.
Attachment #123588 - Flags: review?(dwitte) → review+
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
Attachment #123588 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 96

15 years ago
I'll change it for next patch.
when do I have to post it (what date)?
btw, thanks jst and dwitte ;)
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.
(Assignee)

Comment 98

15 years ago
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

15 years ago
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

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

Comment 101

15 years ago
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

15 years ago
i've applied the patch, fixed rot, and rolled a build. i'll check it in when the
tree opens, in a few chunks.
(Assignee)

Comment 103

15 years ago
I'm back (who cares ;) )
thanks Dan ;) (again)
can I change resolution for this?
or are you going to do it?
Whiteboard: [good first bug][patch, review]

Comment 104

15 years ago
all yours.

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

Comment 105

15 years ago
ok.
fixed.
thanks to all ;)
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Keywords: helpwanted
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.