227057 - nsIResumableChannel and nsIResumableEntityID cleanup

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P5)

Description

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

51      * OnStartRequest wil have a status of NS_ERROR_NOT_RESUMABLE if the file
 52      *  cannot be resumed, eg because the server doesn't support this, or the
 53      *  nsIFileInfo doesn't match. This error may be occur even if startPos
 54      *  is 0, so that the front end can warn the user.

1. "this error may be occur" isn't english, it should be "This error may occur"
2. nsIFileInfo doesn't exist, this should be nsIResumableEntityID I presume
3. OnStartRequest has no status argument!?

(this interface also falls flat on its face for files >4 gb, but that's a
separate issue...)

Comment 1

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

also:
 49      * @param info [...]
there is no param called info

Comment 2

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

and one more thing:
 56      * The request given to the nsIStreamListener will be QIable to
 57      * nsIResumableInfo

There is no nsIResumableInfo interface

Comment 3

[Darin Fisher]

morphing into general cleanup bug... there's much more work to be done in this area.

Comment 4

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

please don't bitrot the bug 219556 patch too much though, if possible ;)

Comment 5

[Darin Fisher]

no worries... this patch can come later.

Comment 6

[Bradley Baetz (:bbaetz)]

Hmm. I think I changed the names several times while developing this...

Has anyone ever written a user for this stuff, BTW?

Comment 7

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

bbaetz: I'm planning to write one, hopefully for 1.7alpha (in the download manager)

also darin tells me beng wants to write one for firebird

Comment 8

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

Attachment: use strings instead of nsIResumableEntityID

most of the issues w/ this interface were resolved with bug 243974, I'd say...
especially nsIResumableChannel now has a resumeAt function that does not open
the channel itself.

this patch now removes nsIResumableEntityID, and uses strings instead.

this is not necessarily ready for review yet; just attaching it so I don't
loose it. I'm unsure whether the entityID attribute should return
NS_ERROR_NOT_RESUMABLE or just return an empty string, when resuming is not
possible.

Comment 9

[Darin Fisher]

thanks for the patch christian

Comment 10

[Darin Fisher]

Comment on attachment 152733 [details] [diff] [review]
use strings instead of nsIResumableEntityID

christian: are you interested in owning this bug?

Comment 11

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

sure, why not

Comment 12

[Darin Fisher]

Comment on attachment 152733 [details] [diff] [review]
use strings instead of nsIResumableEntityID

>Index: base/public/nsIResumableChannel.idl

>     void resumeAt(in unsigned long long startPos,
>-                  in nsIResumableEntityID entityID);
>+                  in ACString entityID);

don't forget to rev the interface's uuid! ;-)


>Index: protocol/http/src/nsHttpChannel.cpp

>+        if (!mEntityID.IsEmpty()) {
>             // Also, we want an error if this resource changed in the meantime
>+            // Format of the entity id is: escaped_etag/size/lastmod
>+            nsCString::const_iterator start = mEntityID.BeginReading(start),
>+                                      end = mEntityID.EndReading(end),
>+                                      slash = mEntityID.BeginReading(slash);

nit: hmm... double assignment to iterators doesn't make much sense.


>+            FindCharInReadable('/', slash, end);
>+
>+            nsCAutoString entityTag(Substring(start, slash));
>+            if (!entityTag.IsEmpty()) {
>+                NS_UnescapeURL(entityTag);
>                 mRequestHead.SetHeader(nsHttp::If_Match, entityTag);
>             }

how about this instead:

  if (FindCharInReadable('/', slash, end)) {
      nsCAutoString buf;
      mRequestHead.SetHeader(nsHttp::If_Match,
	      NS_UnescapeURL(Substring(start, slash), 0, buf));
  }


>+            if (slash != end)
>+                ++slash;
>+            FindCharInReadable('/', slash, end);
>+
>+            const nsDependentCSubstring& lastMod = Substring(slash, end);
>+            if (!lastMod.IsEmpty()) {
>                 mRequestHead.SetHeader(nsHttp::If_Unmodified_Since, lastMod);

don't you need to increment |slash| before generating the
substring?  you might want to utilize the boolean return
value of FindCharInReadable here as well.


>+nsHttpChannel::GetEntityID(nsACString& aEntityID)
...
>+    nsCString entityID;
>+    NS_EscapeURL(etag.BeginReading(), etag.Length(), esc_AlwaysCopy | esc_Directory, entityID);

wait... esc_Directory does not escape forward slashes.	what if the
ETag has a forward slash in it?  also, what if there is already a %xy
sequence in the ETag?  You probably want to use esc_Forced to make sure
that any existing '%' is escaped ;-)

Comment 13

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

Attachment: patch v2

Comment 14

[Darin Fisher]

Comment on attachment 155512 [details] [diff] [review]
patch v2

r=darin

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 155512 [details] [diff] [review]
patch v2

>Index: netwerk/base/public/nsIResumableChannel.idl
>      * The nsIResumableEntityID for this URI. Available after 

s/nsIResumableEntityID/entity ID/

>Index: netwerk/protocol/ftp/src/nsFTPChannel.cpp

>+    if (resumable) {
>+        resumable->GetEntityID(mEntityID);
>+    }

Don't you need an mEntityID.Truncate() if that fails?  The old code nulled out
in the case when GetEntityID returned null...

Or is mEntityID guaranteed empty here?

Alternately, should GetEntityID truncate in the failure case?

>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

>+            nsCString::const_iterator start, end, slash;
>+            mEntityID.BeginReading(start);
>+            mEntityID.EndReading(end);
>+            mEntityID.BeginReading(slash);

How about

  slash = mEntityID.BeginReading(start);

to init them both at once?

>@@ -815,23 +823,21 @@ nsHttpChannel::ProcessNormal()

>+        nsCString id;

Why not nsCAutoString?

sr=bzbarsky with these issues addressed.

Comment 16

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

(In reply to comment #15)
> Or is mEntityID guaranteed empty here?

AsyncOpen truncated mEntityID. So, it should be empty by the time onstartr is
called.

>   slash = mEntityID.BeginReading(start);

hm... I don't think that's particularly readable...

Comment 17

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

Attachment: final patch

Comment 18

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

checked in.