[webkit-reviews] review denied: [Bug 23566] base64Decode() patch : [Attachment 27068] base64Decode patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Paul Pedriana <ppedriana at gmail.com>'s
request for review:
Bug 23566: base64Decode() patch
https://bugs.webkit.org/show_bug.cgi?id=23566

Attachment 27068: base64Decode patch
https://bugs.webkit.org/attachment.cgi?id=27068&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good!

> +2009-01-26  Paul  <set EMAIL_ADDRESS environment variable>

This should have your full name and your email address. Remember that
prepare-ChangeLog helps you write a change log, but doesn't write it for you.
You may have to fix things.

> +	     Fixed bug whereby base64Decode() was failing when unrecognized
encoding chars

You call this a bug in base64Decode, but it's actually the design of
base64Decode. For example, this comment is in the header:

    // this decoder is not general purpose - it returns an error if it
encounters a linefeed, as needed for window.atob

Changing this behavior might be helpful for some callers and harmful for
others.

> +	     were present, whereas the base64 specification requires such chars
be ignored.
> +	     This allows some base64 tests, such as that in Acid3, to succeed
where they previous failed.

I don't understand this. I believe the Acid3 test already passes. Perhaps
you're talking specifically about something in the curl or soup handling of
data URLs? If so, please be specific.

I believe your change may have improved data URLs for those protocols, but
incompatibly changed the behavior of window.atob. Please supply some test cases
to demonstrate the change in behavior of atob and check the behavior of other
browsers, or submit a patch that does not change the behavior of atob.

Also, please be more specific in your comments. I don't think "some base64
tests, such as that in Acid3" points clearly enough to the curl and soup
implementations, if indeed that's what you're fixing.

review- because either:

    1) This fixes a bug in atob, but doesn't include a test case demonstrating
that bug.

or

    2) This introduces a bug in atob, so shouldn't be checked in as-is.