Distinguish between old and new FTP LIST response parsers

Distinguish between old and new FTP LIST response parsers
in FTP histograms. This should allow us to see how compatible the
new parser is.

TEST=none
BUG=25520
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31591

[Paweł Hajdan Jr., 2009-11-10 17:00:13]

I'm really curious what the results will be...

I consider another approach to this: tracking the percentage of requests that
the new parser handled successfully. But I think that approach used in this
patch will give us more information (if the new parser failed, what type of
server was it, as recognized by Mozilla code).

My plan for the future (when the Mozilla code is finally removed) is to also
remove the old "server types" from the enum (and reserving their numbers), and
renaming the constants describing the new "server types" (removing the "NEW"
part. What do you think?

[Paweł Hajdan Jr., 2009-11-10 17:00:36]

Reviewers: wtc, eroman,

Message:
I'm really curious what the results will be...

I consider another approach to this: tracking the percentage of requests  
that
the new parser handled successfully. But I think that approach used in this
patch will give us more information (if the new parser failed, what type of
server was it, as recognized by Mozilla code).

My plan for the future (when the Mozilla code is finally removed) is to also
remove the old "server types" from the enum (and reserving their numbers),  
and
renaming the constants describing the new "server types" (removing the "NEW"
part. What do you think?

Description:
Distinguish between old and new FTP LIST response parsers
in FTP histograms. This should allow us to see how compatible the
new parser is.

TEST=none
BUG=25520

Please review this at http://codereview.chromium.org/384007

Affected files:
   M net/ftp/ftp_directory_listing_parsers.h
   M net/ftp/ftp_server_type_histograms.h


Index: net/ftp/ftp_directory_listing_parsers.h
diff --git a/net/ftp/ftp_directory_listing_parsers.h  
b/net/ftp/ftp_directory_listing_parsers.h
index  
6906861721fd4ae7e0d83778fe15dd006fc77b57..4d1d45bf6a462aebb16559eaa08671cb9334c74b
 
100644
--- a/net/ftp/ftp_directory_listing_parsers.h
+++ b/net/ftp/ftp_directory_listing_parsers.h
@@ -52,7 +52,7 @@ class FtpLsDirectoryListingParser : public  
FtpDirectoryListingParser {
    FtpLsDirectoryListingParser();

    // FtpDirectoryListingParser methods:
-  virtual FtpServerType GetServerType() const { return SERVER_LSL; }
+  virtual FtpServerType GetServerType() const { return SERVER_NEW_LS; }
    virtual bool ConsumeLine(const string16& line);
    virtual bool EntryAvailable() const;
    virtual FtpDirectoryListingEntry PopEntry();
@@ -70,7 +70,7 @@ class FtpWindowsDirectoryListingParser : public  
FtpDirectoryListingParser {
    FtpWindowsDirectoryListingParser();

    // FtpDirectoryListingParser methods:
-  virtual FtpServerType GetServerType() const { return SERVER_DOS; }
+  virtual FtpServerType GetServerType() const { return SERVER_NEW_WINDOWS;  
}
    virtual bool ConsumeLine(const string16& line);
    virtual bool EntryAvailable() const;
    virtual FtpDirectoryListingEntry PopEntry();
@@ -87,7 +87,7 @@ class FtpVmsDirectoryListingParser : public  
FtpDirectoryListingParser {
    FtpVmsDirectoryListingParser();

    // FtpDirectoryListingParser methods:
-  virtual FtpServerType GetServerType() const { return SERVER_VMS; }
+  virtual FtpServerType GetServerType() const { return SERVER_NEW_VMS; }
    virtual bool ConsumeLine(const string16& line);
    virtual bool EntryAvailable() const;
    virtual FtpDirectoryListingEntry PopEntry();
Index: net/ftp/ftp_server_type_histograms.h
diff --git a/net/ftp/ftp_server_type_histograms.h  
b/net/ftp/ftp_server_type_histograms.h
index  
08ca4240c562aec940be4b89621bd80aad4f1c5b..ca179207498d19f7e4c89a3a2d148837a7959744
 
100644
--- a/net/ftp/ftp_server_type_histograms.h
+++ b/net/ftp/ftp_server_type_histograms.h
@@ -19,6 +19,8 @@ enum FtpServerType {
    // connecting to isn't an FTP server), or a broken server.
    SERVER_UNKNOWN = 0,

+  // The types below are recognized by ParseFTPList code from Mozilla. If  
we hit
+  // one of these, it means that our new LIST parser failed for that  
server.
    SERVER_LSL = 1,   // Server using /bin/ls -l and variants.
    SERVER_DLS = 2,   // Server using /bin/dls.
    SERVER_EPLF = 3,  // Server using EPLF format.
@@ -28,6 +30,12 @@ enum FtpServerType {
    SERVER_OS2 = 7,   // OS/2 FTP Server.
    SERVER_W16 = 8,   // win16 hosts: SuperTCP or NetManage Chameleon.

+  // The types below are recognized by our new LIST parser. If we hit one  
of
+  // these, it means that it's working quite well.
+  SERVER_NEW_LS = 9,        // Server using /bin/ls -l listing style.
+  SERVER_NEW_WINDOWS = 10,  // Server using Windows listing style.
+  SERVER_NEW_VMS = 11,      // Server using VMS listing style.
+
    NUM_OF_SERVER_TYPES
  };

[Paweł Hajdan Jr., 2009-11-10 17:16:43]

Reviewers: wtc, eroman,

Message:
I'm really curious what the results will be...

I consider another approach to this: tracking the percentage of requests  
that
the new parser handled successfully. But I think that approach used in this
patch will give us more information (if the new parser failed, what type of
server was it, as recognized by Mozilla code).

My plan for the future (when the Mozilla code is finally removed) is to also
remove the old "server types" from the enum (and reserving their numbers),  
and
renaming the constants describing the new "server types" (removing the "NEW"
part. What do you think?

Description:
Distinguish between old and new FTP LIST response parsers
in FTP histograms. This should allow us to see how compatible the
new parser is.

TEST=none
BUG=25520

Please review this at http://codereview.chromium.org/384007

Affected files:
   M net/ftp/ftp_directory_listing_parsers.h
   M net/ftp/ftp_server_type_histograms.h


Index: net/ftp/ftp_directory_listing_parsers.h
diff --git a/net/ftp/ftp_directory_listing_parsers.h  
b/net/ftp/ftp_directory_listing_parsers.h
index  
6906861721fd4ae7e0d83778fe15dd006fc77b57..4d1d45bf6a462aebb16559eaa08671cb9334c74b
 
100644
--- a/net/ftp/ftp_directory_listing_parsers.h
+++ b/net/ftp/ftp_directory_listing_parsers.h
@@ -52,7 +52,7 @@ class FtpLsDirectoryListingParser : public  
FtpDirectoryListingParser {
    FtpLsDirectoryListingParser();

    // FtpDirectoryListingParser methods:
-  virtual FtpServerType GetServerType() const { return SERVER_LSL; }
+  virtual FtpServerType GetServerType() const { return SERVER_NEW_LS; }
    virtual bool ConsumeLine(const string16& line);
    virtual bool EntryAvailable() const;
    virtual FtpDirectoryListingEntry PopEntry();
@@ -70,7 +70,7 @@ class FtpWindowsDirectoryListingParser : public  
FtpDirectoryListingParser {
    FtpWindowsDirectoryListingParser();

    // FtpDirectoryListingParser methods:
-  virtual FtpServerType GetServerType() const { return SERVER_DOS; }
+  virtual FtpServerType GetServerType() const { return SERVER_NEW_WINDOWS;  
}
    virtual bool ConsumeLine(const string16& line);
    virtual bool EntryAvailable() const;
    virtual FtpDirectoryListingEntry PopEntry();
@@ -87,7 +87,7 @@ class FtpVmsDirectoryListingParser : public  
FtpDirectoryListingParser {
    FtpVmsDirectoryListingParser();

    // FtpDirectoryListingParser methods:
-  virtual FtpServerType GetServerType() const { return SERVER_VMS; }
+  virtual FtpServerType GetServerType() const { return SERVER_NEW_VMS; }
    virtual bool ConsumeLine(const string16& line);
    virtual bool EntryAvailable() const;
    virtual FtpDirectoryListingEntry PopEntry();
Index: net/ftp/ftp_server_type_histograms.h
diff --git a/net/ftp/ftp_server_type_histograms.h  
b/net/ftp/ftp_server_type_histograms.h
index  
08ca4240c562aec940be4b89621bd80aad4f1c5b..ca179207498d19f7e4c89a3a2d148837a7959744
 
100644
--- a/net/ftp/ftp_server_type_histograms.h
+++ b/net/ftp/ftp_server_type_histograms.h
@@ -19,6 +19,8 @@ enum FtpServerType {
    // connecting to isn't an FTP server), or a broken server.
    SERVER_UNKNOWN = 0,

+  // The types below are recognized by ParseFTPList code from Mozilla. If  
we hit
+  // one of these, it means that our new LIST parser failed for that  
server.
    SERVER_LSL = 1,   // Server using /bin/ls -l and variants.
    SERVER_DLS = 2,   // Server using /bin/dls.
    SERVER_EPLF = 3,  // Server using EPLF format.
@@ -28,6 +30,12 @@ enum FtpServerType {
    SERVER_OS2 = 7,   // OS/2 FTP Server.
    SERVER_W16 = 8,   // win16 hosts: SuperTCP or NetManage Chameleon.

+  // The types below are recognized by our new LIST parser. If we hit one  
of
+  // these, it means that it's working quite well.
+  SERVER_NEW_LS = 9,        // Server using /bin/ls -l listing style.
+  SERVER_NEW_WINDOWS = 10,  // Server using Windows listing style.
+  SERVER_NEW_VMS = 11,      // Server using VMS listing style.
+
    NUM_OF_SERVER_TYPES
  };

[eroman, 2009-11-10 19:18:02]

lgtm

http://codereview.chromium.org/384007/diff/1/3
File net/ftp/ftp_server_type_histograms.h (right):

http://codereview.chromium.org/384007/diff/1/3#newcode35
Line 35: SERVER_NEW_LS = 9,        // Server using /bin/ls -l listing style.
I would reverse the naming.

Call the new types the regular name, and the old types

   SERVER_MOZ_*

[eroman, 2009-11-10 19:18:20]

lgtm


http://codereview.chromium.org/384007/diff/1/3
File net/ftp/ftp_server_type_histograms.h (right):

http://codereview.chromium.org/384007/diff/1/3#newcode35
Line 35: SERVER_NEW_LS = 9,        // Server using /bin/ls -l listing
style.
I would reverse the naming.

Call the new types the regular name, and the old types

    SERVER_MOZ_*

http://codereview.chromium.org/384007

[eroman, 2009-11-10 19:24:22]

lgtm


http://codereview.chromium.org/384007/diff/1/3
File net/ftp/ftp_server_type_histograms.h (right):

http://codereview.chromium.org/384007/diff/1/3#newcode35
Line 35: SERVER_NEW_LS = 9,        // Server using /bin/ls -l listing
style.
I would reverse the naming.

Call the new types the regular name, and the old types

    SERVER_MOZ_*

http://codereview.chromium.org/384007

[Paweł Hajdan Jr., 2009-11-10 19:47:53]

Thanks for the suggestion. Patch updated.

[Paweł Hajdan Jr., 2009-11-10 19:48:14]

Thanks for the suggestion. Patch updated.

http://codereview.chromium.org/384007

[eroman, 2009-11-10 20:11:49]

lgtm

[eroman, 2009-11-10 20:12:08]

lgtm

http://codereview.chromium.org/384007