[webkit-reviews] review denied: [Bug 34458] Refactor texImage2D and texSubImage2D taking Image to use common code : [Attachment 48688] Revised patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Kenneth Russell <kbr at google.com>'s
request for review:
Bug 34458: Refactor texImage2D and texSubImage2D taking Image to use common
code
https://bugs.webkit.org/show_bug.cgi?id=34458

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

------- Additional Comments from Oliver Hunt <oliver at apple.com>
Whoops, i forgot to comment on the comments originally -- we really don't
comment in this manner, in general if your code needs that degree of commentary
the code should probably be reworked.

That said all of this code is simple, and so this commentary is simply
unnecessary, and merely makes it difficult to read the code proper.  Basically
there shouldn't be more than about two sentences, tops, for the getImageData or
processImageData definitions.

Anyhoo, while reading through the patch 

I also have some concerns about the CG implementation of getImageData, I don't
believe you should be stripping colour space information away (which is
essentially what you're currently doing).  The target context should simply be
set to either sRGB or LinearRGB (if this is not specified in the WebGL spec, it
should be) so the there's a consistent colour space.  I think as a byproduct of
such logic it becomes sensible to simply use as ImageBuffer to do the image to
byte conversion in which case that can also be cross paltform, and it also
saves us from having a separate implementation of [un]premultiplying.

Sorry for both the delay of this response, and for the substantial change in
review :-/