Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(519)

Issue 2816733002: Photo Picker Dialog: Use sandboxed utility process for decoding images. (Closed)

Created:
3 years, 8 months ago by Finnur
Modified:
3 years, 7 months ago
Reviewers:
Ted C, Theresa, Yusuf
CC:
chromium-reviews, srahim+watch_chromium.org, agrieve+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Photo Picker Dialog: Use sandboxed utility process for decoding images. BUG=656015 Review-Url: https://codereview.chromium.org/2816733002 Cr-Original-Commit-Position: refs/heads/master@{#468209} Committed: https://chromium.googlesource.com/chromium/src/+/83c9f3f4faeb35a3a2607446def65f1a8da7aa49 Review-Url: https://codereview.chromium.org/2816733002 Cr-Commit-Position: refs/heads/master@{#468923} Committed: https://chromium.googlesource.com/chromium/src/+/ff4ca2efee2c251b4fded429984f4be90e1d43d5

Patch Set 1 #

Total comments: 58

Patch Set 2 : Sync #

Patch Set 3 : Address comments from Theresa #

Patch Set 4 : Change inSampleSize calculations #

Patch Set 5 : Address comments from Yusuf #

Patch Set 6 : Simplify decoder #

Total comments: 30

Patch Set 7 : Remove TODO (nullchecks) and fix findbugs #

Patch Set 8 : Address feedback from Theresa #

Total comments: 10

Patch Set 9 : Address feedback from both #

Total comments: 10

Patch Set 10 : No code change, just sync'ing #

Patch Set 11 : Address comments from Theresa #

Unified diffs Side-by-side diffs Delta from patch set Stats (+638 lines, -51 lines) Patch
M chrome/android/java/AndroidManifest.xml View 1 2 3 4 5 6 7 8 9 1 chunk +8 lines, -0 lines 0 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java View 1 2 3 4 5 6 7 8 9 10 1 chunk +110 lines, -0 lines 0 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java View 1 2 3 4 5 6 7 8 1 chunk +135 lines, -0 lines 0 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java View 1 2 3 4 5 6 7 8 9 10 1 chunk +294 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/photo_picker/MimeTypeFileFilter.java View 1 2 2 chunks +6 lines, -6 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java View 1 2 3 4 5 6 4 chunks +11 lines, -8 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java View 1 2 3 4 5 6 4 chunks +9 lines, -28 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java View 1 2 3 4 5 6 7 10 chunks +57 lines, -9 lines 0 comments Download
M chrome/android/java/strings/android_chrome_strings.grd View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/android/java_sources.gni View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 74 (43 generated)
Finnur
https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java#newcode54 chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java:54: } This is also in the other CL in ...
3 years, 8 months ago (2017-04-12 14:57:42 UTC) #4
Theresa
+yusufo@ for service creation/binding/communication https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/AndroidManifest.xml File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/AndroidManifest.xml#newcode675 chrome/android/java/AndroidManifest.xml:675: android:process=":decoder_service" /> This should have ...
3 years, 8 months ago (2017-04-13 02:25:27 UTC) #8
Yusuf
https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java#newcode124 chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:124: case MSG_REGISTER_CLIENT: why do we need the two separate ...
3 years, 8 months ago (2017-04-13 19:05:25 UTC) #9
Finnur
Back from Easter vacation... Thanks to both for reviewing. Note: This update addresses _only_ the ...
3 years, 8 months ago (2017-04-18 17:21:15 UTC) #14
Theresa
https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java#newcode61 chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:61: while ((halfHeight / inSampleSize) >= height || (halfWidth / ...
3 years, 8 months ago (2017-04-18 19:06:20 UTC) #15
Finnur
Yusuf is still off-the-hook; between reviews, meetings and trying (unsuccessfully) to get the View system ...
3 years, 8 months ago (2017-04-19 16:01:36 UTC) #18
Theresa
https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java#newcode47 chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:47: // A method for getFileDescriptor, obtained via Reflection. Can ...
3 years, 8 months ago (2017-04-19 17:31:15 UTC) #19
Finnur
More or less, yes. When the decode request comes in, we open an inputFile, obtain ...
3 years, 8 months ago (2017-04-19 20:42:16 UTC) #20
Finnur
This update addresses comments made by Yusuf only. Theresa is off the hook this time. ...
3 years, 8 months ago (2017-04-21 14:47:55 UTC) #21
palmer
Hi all, Sorry for the drive-by. :) I think you can achieve the goal by ...
3 years, 8 months ago (2017-04-21 17:56:12 UTC) #22
Finnur
I added you to a doc, Chris. Basically, the latency of the TumbnailProvider kills the ...
3 years, 8 months ago (2017-04-21 18:55:47 UTC) #23
Finnur
On 2017/04/21 18:55:47, Finnur wrote: > I added you to a doc, Chris. Basically, the ...
3 years, 8 months ago (2017-04-21 18:56:12 UTC) #24
Finnur
Updated. Both: PTAL.
3 years, 8 months ago (2017-04-24 13:06:47 UTC) #27
Theresa
Thank you for getting rid of the reflection! https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java#newcode61 chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:61: while ...
3 years, 8 months ago (2017-04-24 16:57:39 UTC) #32
Finnur
Theresa and Yusuf, PTAL. > Thank you for getting rid of the reflection! Thank you ...
3 years, 8 months ago (2017-04-25 15:38:01 UTC) #37
Theresa
This is really shaping up well! I'm going to do another full pass later today ...
3 years, 8 months ago (2017-04-25 17:43:00 UTC) #40
Yusuf
https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java#newcode163 chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:163: if (mRequests.size() == 1) { one line https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java#newcode211 chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:211: ...
3 years, 8 months ago (2017-04-25 19:01:08 UTC) #41
Finnur
Both: PTAL. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java#newcode62 chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:62: } while ((height / inSampleSize) >= minSize ...
3 years, 8 months ago (2017-04-26 12:00:57 UTC) #44
Theresa
https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java#newcode62 chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:62: } while ((height / inSampleSize) >= minSize && (width ...
3 years, 8 months ago (2017-04-26 16:53:17 UTC) #47
Yusuf
lgtm after twellington@'s comments are addressed.
3 years, 8 months ago (2017-04-26 18:34:40 UTC) #48
Finnur
Thanks, Yusuf! Updated. Theresa: PTAL. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java#newcode62 chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:62: } while ((height / ...
3 years, 7 months ago (2017-04-27 11:40:52 UTC) #51
Theresa
lgtm !
3 years, 7 months ago (2017-04-27 16:07:16 UTC) #54
Finnur
Thanks! Miguel, mind doing an OWNERS check for android_chrome_strings.grd and AndroidManifest.xml?
3 years, 7 months ago (2017-04-27 16:48:22 UTC) #57
Finnur
Ted, I'm missing an OWNERS check for android_chrome_strings.grd and AndroidManifest.xml?
3 years, 7 months ago (2017-04-28 10:43:07 UTC) #59
Finnur
On 2017/04/28 10:43:07, Finnur OOO wrote: > Ted, I'm missing an OWNERS check for android_chrome_strings.grd ...
3 years, 7 months ago (2017-04-28 10:43:31 UTC) #60
Ted C
On 2017/04/28 10:43:31, Finnur OOO wrote: > On 2017/04/28 10:43:07, Finnur OOO wrote: > > ...
3 years, 7 months ago (2017-04-28 20:18:52 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2816733002/240001
3 years, 7 months ago (2017-04-28 23:47:00 UTC) #64
commit-bot: I haz the power
Committed patchset #11 (id:240001) as https://chromium.googlesource.com/chromium/src/+/83c9f3f4faeb35a3a2607446def65f1a8da7aa49
3 years, 7 months ago (2017-04-29 02:03:23 UTC) #67
aelias_OOO_until_Jul13
A revert of this CL (patchset #11 id:240001) has been created in https://codereview.chromium.org/2853793003/ by aelias@chromium.org. ...
3 years, 7 months ago (2017-05-01 16:54:53 UTC) #68
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2816733002/240001
3 years, 7 months ago (2017-05-03 08:00:36 UTC) #71
commit-bot: I haz the power
3 years, 7 months ago (2017-05-03 09:07:01 UTC) #74
Message was sent while issue was closed.
Committed patchset #11 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/ff4ca2efee2c251b4fded429984f...

Powered by Google App Engine
This is Rietveld 408576698