Summary: | Remove decoding of frames in BitmapImage::frameHasAlphaAtIndex and BitmapImage::frameOrientationAtIndex | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hin-Chung Lam <hclam> | ||||||||||
Component: | Images | Assignee: | Hin-Chung Lam <hclam> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dino, dongseong.hwang, eric.carlson, eric, hclam, hyatt, nduca, nick, noel.gordon, pkasting, simon.fraser, skyul, vangelis, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 90375 | ||||||||||||
Bug Blocks: | 93636 | ||||||||||||
Attachments: |
|
Description
Hin-Chung Lam
2012-08-03 18:45:41 PDT
Created attachment 156504 [details]
Patch
Looks good to me. :) Created attachment 157324 [details]
Patch
There's no new code in the second patch. It added a test for partially loaded JPEG images. This shows that this change doesn't affect rendering results when an image is fully loaded and partially loaded. Comment on attachment 157324 [details] Patch 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? Created attachment 157333 [details]
Patch
> > 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; Done. > > Source/WebCore/platform/image-decoders/ImageDecoder.h:280 > > + virtual bool frameHasAlphaAtIndex(size_t); > > Can this be const? Done. > > LayoutTests/fast/images/jpg-image-with-background.html:11 > > I'm not sure if this is worth testing. Removed. > > LayoutTests/http/tests/resources/load-and-stall.php:37 > > Does this match the style we use for other PHP files? Matched the style of this file with reset-temp-file.php in the same directory. Indentation using 4 spaces. Comment on attachment 157333 [details]
Patch
Thanks for review. I have addressed your comments in the last review.
Comment on attachment 157333 [details] Patch Rejecting attachment 157333 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: Kit/chromium/third_party/yasm/source/patched-yasm --revision 134927 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 47>At revision 134927. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/13460408 Created attachment 157378 [details]
Patch for landing
Comment on attachment 157378 [details] Patch for landing Clearing flags on attachment: 157378 Committed r125154: <http://trac.webkit.org/changeset/125154> All reviewed patches have been landed. Closing bug. This appears to have caused http/tests/images/jpg-img-partial-load.html to regress on Mac Lion. http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r125185%20(2163)/results.html I'm looking into this. This test is written on chromium's DRT which uses different image decoders than CG port. I think this maybe the reason. I'll try this on lion to verify. In the mean time please mark this as timeout in TestExpectations. Thanks. Followup in https://bugs.webkit.org/show_bug.cgi?id=93636 |