Fix thumbnail store crashing on loading old files

Fix thumbnail store crashing on loading old files

- Was loading thumbnails in little endian, but since the files were from java's
output stream they were in big endian.  This was causing all sorts of issues
with old thumbnails.
- Added back ETC1 header and fixed the file format to match the old version.

BUG=398319
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290708

[David Trainor- moved to gerrit, 2014-08-19 03:33:34]

PTAL.  This isn't the ideal way to do this at all, but for M38 this looks good. 
I've tested it and it seems to work on both old and newly generated
thumbnails...  I'll do more testing tomorrow.

[Ted C, 2014-08-19 16:51:10]

https://codereview.chromium.org/486093002/diff/1/chrome/browser/android/thumb...
File chrome/browser/android/thumbnail/thumbnail_store.cc (right):

https://codereview.chromium.org/486093002/diff/1/chrome/browser/android/thumb...
chrome/browser/android/thumbnail/thumbnail_store.cc:73: if
(file.WriteAtCurrentPos(buffer, sizeof(T)) != sizeof(T))
Could you also just do:

return file.WriteAtCurrentPos(buffer, sizeof(T)) == sizeof(T);

https://codereview.chromium.org/486093002/diff/1/chrome/browser/android/thumb...
chrome/browser/android/thumbnail/thumbnail_store.cc:444: if (file.IsValid()) {
Would it be possible to pull out a helper function that simply takes the file
ref, compressed_data, scale, and content_size and returns a boolean instead of
tracking success like this?

Would be nice to early out instead of having to continually check for success on
each step.

You would probably need to lockPixels/unlockPixels before entering the function,
but I think it should clarify this code.

https://codereview.chromium.org/486093002/diff/1/chrome/browser/android/thumb...
chrome/browser/android/thumbnail/thumbnail_store.cc:458: unsigned char
etc1_buffer[ETC_PKM_HEADER_SIZE];
any reason these lines don't go into the conditional right below?  I would
probably redefine bytes_written in each one just for clarity as well if you do
move it.

https://codereview.chromium.org/486093002/diff/1/chrome/browser/android/thumb...
chrome/browser/android/thumbnail/thumbnail_store.cc:606: int key = 0;
same general comment about success early bail out

https://codereview.chromium.org/486093002/diff/1/chrome/browser/android/thumb...
chrome/browser/android/thumbnail/thumbnail_store.cc:629: if (bytes_read !=
ETC_PKM_HEADER_SIZE) {
no need for parens here since it's inconsistent

https://codereview.chromium.org/486093002/diff/1/chrome/browser/android/thumb...
chrome/browser/android/thumbnail/thumbnail_store.cc:644: // Do some simple
sanity check validation.  This is for Android only, for
this directory is under android, so I don't think you need this comment.

Also, I wonder if this could be smarter/future proof, by using 

std::max(
    DeviceDisplayInfo::GetDisplayHeight(),
    DeviceDisplayInfo::GetDisplayWidth());

https://codereview.chromium.org/486093002/diff/1/third_party/android_opengl/e...
File third_party/android_opengl/etc1/etc1.cpp (right):

https://codereview.chromium.org/486093002/diff/1/third_party/android_opengl/e...
third_party/android_opengl/etc1/etc1.cpp:614: etc1_uint32 encodedWidth = (width
+ 3) & ~3;
what sort of witch craft is all this magic?

[David Trainor- moved to gerrit, 2014-08-19 18:31:07]

Addressed comments.  ptal

https://codereview.chromium.org/486093002/diff/1/chrome/browser/android/thumb...
File chrome/browser/android/thumbnail/thumbnail_store.cc (right):

https://codereview.chromium.org/486093002/diff/1/chrome/browser/android/thumb...
chrome/browser/android/thumbnail/thumbnail_store.cc:73: if
(file.WriteAtCurrentPos(buffer, sizeof(T)) != sizeof(T))
On 2014/08/19 16:51:09, Ted C wrote:
> Could you also just do:
> 
> return file.WriteAtCurrentPos(buffer, sizeof(T)) == sizeof(T);

Good point.  Done

https://codereview.chromium.org/486093002/diff/1/chrome/browser/android/thumb...
chrome/browser/android/thumbnail/thumbnail_store.cc:444: if (file.IsValid()) {
On 2014/08/19 16:51:09, Ted C wrote:
> Would it be possible to pull out a helper function that simply takes the file
> ref, compressed_data, scale, and content_size and returns a boolean instead of
> tracking success like this?
> 
> Would be nice to early out instead of having to continually check for success
on
> each step.
> 
> You would probably need to lockPixels/unlockPixels before entering the
function,
> but I think it should clarify this code.

Done.

https://codereview.chromium.org/486093002/diff/1/chrome/browser/android/thumb...
chrome/browser/android/thumbnail/thumbnail_store.cc:458: unsigned char
etc1_buffer[ETC_PKM_HEADER_SIZE];
On 2014/08/19 16:51:09, Ted C wrote:
> any reason these lines don't go into the conditional right below?  I would
> probably redefine bytes_written in each one just for clarity as well if you do
> move it.

Done.

https://codereview.chromium.org/486093002/diff/1/chrome/browser/android/thumb...
chrome/browser/android/thumbnail/thumbnail_store.cc:606: int key = 0;
On 2014/08/19 16:51:09, Ted C wrote:
> same general comment about success early bail out

Done.

https://codereview.chromium.org/486093002/diff/1/chrome/browser/android/thumb...
chrome/browser/android/thumbnail/thumbnail_store.cc:629: if (bytes_read !=
ETC_PKM_HEADER_SIZE) {
On 2014/08/19 16:51:09, Ted C wrote:
> no need for parens here since it's inconsistent

Done.

https://codereview.chromium.org/486093002/diff/1/chrome/browser/android/thumb...
chrome/browser/android/thumbnail/thumbnail_store.cc:644: // Do some simple
sanity check validation.  This is for Android only, for
Ah good point I forgot we could access those.  Thanks!
On 2014/08/19 16:51:09, Ted C wrote:
> this directory is under android, so I don't think you need this comment.
> 
> Also, I wonder if this could be smarter/future proof, by using 
> 
> std::max(
>     DeviceDisplayInfo::GetDisplayHeight(),
>     DeviceDisplayInfo::GetDisplayWidth());

https://codereview.chromium.org/486093002/diff/1/third_party/android_opengl/e...
File third_party/android_opengl/etc1/etc1.cpp (right):

https://codereview.chromium.org/486093002/diff/1/third_party/android_opengl/e...
third_party/android_opengl/etc1/etc1.cpp:614: etc1_uint32 encodedWidth = (width
+ 3) & ~3;
On 2014/08/19 16:51:09, Ted C wrote:
> what sort of witch craft is all this magic?

The kind that is brought back from the original etc1.cc file that was removed
during the initial copy to third_party :(.  Rounds up the width/height to the
nearest 4?

[Ted C, 2014-08-19 21:02:03]

lgtm

https://codereview.chromium.org/486093002/diff/60001/chrome/browser/android/t...
File chrome/browser/android/thumbnail/thumbnail_store.cc (right):

https://codereview.chromium.org/486093002/diff/60001/chrome/browser/android/t...
chrome/browser/android/thumbnail/thumbnail_store.cc:510: }  // anonymous
namespace
Normally, people leave out "anonymous"

[David Trainor- moved to gerrit, 2014-08-19 22:21:51]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-08-19 22:25:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrainor@chromium.org/486093002/60001

[commit-bot: I haz the power, 2014-08-19 23:03:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-20 00:09:37]

Committed patchset #4 (60001) as 290708