[webkit-reviews] review denied: [Bug 93171] Remove decoding of frames in BitmapImage::frameHasAlphaAtIndex and BitmapImage::frameOrientationAtIndex : [Attachment 157324] Patch

Wed Aug 8 16:23:42 PDT 2012

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Hin-Chung Lam
<hclam at google.com>'s request for review:
Bug 93171: Remove decoding of frames in BitmapImage::frameHasAlphaAtIndex and
BitmapImage::frameOrientationAtIndex
https://bugs.webkit.org/show_bug.cgi?id=93171

Attachment 157324: Patch
https://bugs.webkit.org/attachment.cgi?id=157324&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=157324&action=review

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:284
> +    return m_frameBufferCache[index].status() != ImageFrame::FrameComplete
|| m_frameBufferCache[index].hasAlpha();

I find this confusing to read. It would be clearer as 

if (m_frameBufferCache[index].status() == ImageFrame::FrameComplete)
  return  m_frameBufferCache[index].hasAlpha();
return true;

> Source/WebCore/platform/image-decoders/ImageDecoder.h:280
> +	   virtual bool frameHasAlphaAtIndex(size_t);

Can this be const?

> LayoutTests/fast/images/jpg-image-with-background.html:11
> +<html>
> +<head>
> +<script>
> +if (window.testRunner)
> +    testRunner.dumpAsText(true);
> +</script>
> +</head>
> +<body>
> +<img src="resources/test-load.jpg" style="background-color: blue;"></img>
> +</body>
> +</html>

I'm not sure if this is worth testing.

> LayoutTests/http/tests/resources/load-and-stall.php:37
> +<?php
> +$name = $_GET['name'];
> +$stallAt = $_GET['stallAt'];
> +$stallFor = $_GET['stallFor'];
> +$mimeType = $_GET['mimeType'];
> +
> +$file = fopen($name, "rb");
> +if (!$file)
> +  die("Cannot open file.");
> +
> +header("Content-Type: " . $mimeType);
> +header("Content-Length: " . filesize($name));
> +
> +if (isset($stallAt) && isset($stallFor)) {
> +  $stallAt = (int)$stallAt;
> +  $stallFor = (int)$stallFor;
> +  if ($stallAt > filesize($name))
> +    die("Incorrect value for stallAt.");
> +
> +  while ($written < $stallAt) {
> +    $write = 1024;
> +    if ($write > $stallAt - $written)
> +	 $write = $stallAt - $written;
> +
> +    echo(fread($file, $write));
> +    $written += $write;
> +    flush();
> +    ob_flush();
> +  }
> +  sleep($stallFor);
> +  echo(fread($file, filesize($name) - $stallAt));
> +} else {
> +  echo(fread($file, filesize($name)));
> +}
> +
> +fclose($file);
> +?>

Does this match the style we use for other PHP files?