Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(38)

Issue 136453009: Fix for Issue 331895: Make gesturenav screenshot greyscale (Closed)

Created:
6 years, 7 months ago by mfomitchev
Modified:
6 years, 6 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Using grayscale screenshots for GestureNav so that the user can clearly tell when screenshot is shown vs. the live content. BUG=331895 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249431 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249782

Patch Set 1 #

Total comments: 3

Patch Set 2 : Changing the implementation to reduce the impact on PNGCodec API to a minimum. #

Patch Set 3 : Changing the implementation to reduce the impact on PNGCodec API to a minimum. #

Patch Set 4 : Getting rid of an unneeded conversion #

Patch Set 5 : Minor fix #

Patch Set 6 : Minor fix #

Patch Set 7 : Improving the logic for format detection in EncodeWithCompressionLevel() #

Total comments: 11

Patch Set 8 : Minor changes implementing review feedback. #

Patch Set 9 : Adding a unit test for EncodeA8SkBitmap #

Patch Set 10 : Fixing a failing assert #

Total comments: 7

Patch Set 11 : Implementing review feedback #

Patch Set 12 : Minor fix #

Total comments: 2

Patch Set 13 : Fixing memory leak #

Unified diffs Side-by-side diffs Delta from patch set Stats (+146 lines, -47 lines) Patch
M content/browser/frame_host/navigation_entry_screenshot_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +19 lines, -2 lines 0 comments Download
M ui/gfx/codec/png_codec.h View 1 2 3 4 5 6 7 3 chunks +16 lines, -5 lines 0 comments Download
M ui/gfx/codec/png_codec.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +66 lines, -34 lines 0 comments Download
M ui/gfx/codec/png_codec_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +45 lines, -6 lines 0 comments Download

Messages

Total messages: 50 (0 generated)
mfomitchev
Hey guys, This is not a full fix, but I would appreciate some quick feedback ...
6 years, 7 months ago (2014-01-15 16:03:02 UTC) #1
tdanderson
+danakj for her thoughts since this really isn't my area of expertise. > I've created ...
6 years, 7 months ago (2014-01-15 21:39:33 UTC) #2
danakj
+pkasting
6 years, 7 months ago (2014-01-15 21:40:39 UTC) #3
sadrul
I am not familiar with the actual encoding code, so looked for nits/readability only. https://codereview.chromium.org/136453009/diff/1/ui/gfx/codec/png_codec.cc ...
6 years, 7 months ago (2014-01-15 21:43:06 UTC) #4
Peter Kasting
On 2014/01/15 21:40:39, danakj wrote: > +pkasting You should probably say what you want my ...
6 years, 6 months ago (2014-01-16 05:51:35 UTC) #5
mfomitchev
> My instinct is that having a method that converts Skia images to greyscale separately ...
6 years, 6 months ago (2014-01-16 19:49:56 UTC) #6
Peter Kasting
On 2014/01/16 19:49:56, mfomitchev wrote: > > My instinct is that having a method that ...
6 years, 6 months ago (2014-01-16 20:53:55 UTC) #7
mfomitchev
On 2014/01/16 20:53:55, Peter Kasting wrote: > On 2014/01/16 19:49:56, mfomitchev wrote: > > > ...
6 years, 6 months ago (2014-01-16 21:07:09 UTC) #8
Peter Kasting
On 2014/01/16 21:07:09, mfomitchev wrote: > s_p_ending, not sending :) Oops! Sorry, I should learn ...
6 years, 6 months ago (2014-01-16 21:18:56 UTC) #9
mfomitchev
There is one of these screenshots (weakly referenced?) for every navigation entry (every page reachable ...
6 years, 6 months ago (2014-01-16 21:54:58 UTC) #10
sadrul
> > Unless I am missing something, if we convert to > > grayscale and ...
6 years, 6 months ago (2014-01-16 21:58:51 UTC) #11
Peter Kasting
On 2014/01/16 21:54:58, mfomitchev wrote: > can > you please comment on the approach which ...
6 years, 6 months ago (2014-01-16 22:01:10 UTC) #12
mfomitchev
I've tried using png_set_rgb_to_gray_fixed() to do the RGB -> gray transformation, it is excluded by ...
6 years, 6 months ago (2014-01-17 17:22:30 UTC) #13
mfomitchev
Ok, so I have spend most of Friday trying to figure out a way to ...
6 years, 6 months ago (2014-01-17 21:50:37 UTC) #14
Peter Kasting
On 2014/01/17 21:50:37, mfomitchev wrote: > Ok, so I have spend most of Friday trying ...
6 years, 6 months ago (2014-01-17 22:08:09 UTC) #15
mfomitchev
I've re-implemented the feature minimizing the impact on PNGCodec and moving the grayscale conversion to ...
6 years, 6 months ago (2014-01-24 17:27:05 UTC) #16
Peter Kasting
Awesome. LGTM. Don't know where the test should go, perhaps a ui/gfx/ OWNER can speak ...
6 years, 6 months ago (2014-01-25 03:29:57 UTC) #17
mfomitchev
+Nasko - Nasko, can you please review the changes to navigation_entry_screenshot_manager.cc? Peter, just one minor ...
6 years, 6 months ago (2014-01-27 17:15:28 UTC) #18
nasko
Rubberstamp LGTM
6 years, 6 months ago (2014-01-27 17:26:14 UTC) #19
danakj
On 2014/01/25 03:29:57, Peter Kasting wrote: > Awesome. LGTM. Don't know where the test should ...
6 years, 6 months ago (2014-01-27 18:40:58 UTC) #20
mfomitchev
Added the unit test
6 years, 6 months ago (2014-01-27 21:13:25 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/136453009/650001
6 years, 6 months ago (2014-01-30 18:25:57 UTC) #22
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=47252
6 years, 6 months ago (2014-01-30 19:02:14 UTC) #23
commit-bot: I haz the power
CQ bit was unchecked on CL. Ignoring.
6 years, 6 months ago (2014-01-30 19:02:23 UTC) #24
mfomitchev
Hi Elliot, Can you please review changes to PNGCodec? Thanks!
6 years, 6 months ago (2014-02-03 19:44:40 UTC) #25
Peter Kasting
LGTM https://codereview.chromium.org/136453009/diff/650001/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): https://codereview.chromium.org/136453009/diff/650001/ui/gfx/codec/png_codec.cc#newcode743 ui/gfx/codec/png_codec.cc:743: DCHECK(bpp == 1 || bpp == 4); // ...
6 years, 6 months ago (2014-02-03 19:53:03 UTC) #26
Elliot Glaysher
lgtm
6 years, 6 months ago (2014-02-03 23:22:02 UTC) #27
Peter Kasting
Just looked at the unittest. https://codereview.chromium.org/136453009/diff/650001/ui/gfx/codec/png_codec_unittest.cc File ui/gfx/codec/png_codec_unittest.cc (right): https://codereview.chromium.org/136453009/diff/650001/ui/gfx/codec/png_codec_unittest.cc#newcode252 ui/gfx/codec/png_codec_unittest.cc:252: // Returns true if ...
6 years, 6 months ago (2014-02-04 01:37:14 UTC) #28
mfomitchev
The CQ bit was checked by mfomitchev@chromium.org
6 years, 6 months ago (2014-02-04 20:08:11 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/136453009/560002
6 years, 6 months ago (2014-02-04 20:09:51 UTC) #30
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 6 months ago (2014-02-04 22:50:57 UTC) #31
commit-bot: I haz the power
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, sync_integration_tests, unit_tests ...
6 years, 6 months ago (2014-02-04 22:50:58 UTC) #32
mfomitchev
The CQ bit was checked by mfomitchev@chromium.org
6 years, 6 months ago (2014-02-04 22:53:21 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/136453009/560002
6 years, 6 months ago (2014-02-04 23:18:12 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/136453009/560002
6 years, 6 months ago (2014-02-05 01:30:55 UTC) #35
mfomitchev
The CQ bit was unchecked by mfomitchev@chromium.org
6 years, 6 months ago (2014-02-05 16:41:06 UTC) #36
mfomitchev
The CQ bit was checked by mfomitchev@chromium.org
6 years, 6 months ago (2014-02-05 16:41:07 UTC) #37
Paweł Hajdan Jr.
The CQ bit was unchecked by phajdan.jr@chromium.org
6 years, 6 months ago (2014-02-05 19:48:33 UTC) #38
Paweł Hajdan Jr.
The CQ bit was checked by phajdan.jr@chromium.org
6 years, 6 months ago (2014-02-05 19:52:47 UTC) #39
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 6 months ago (2014-02-06 09:24:54 UTC) #40
commit-bot: I haz the power
List of reviewers changed. phajdan.jr@chromium.org did a drive-by without LGTM'ing!
6 years, 6 months ago (2014-02-06 09:24:55 UTC) #41
mfomitchev
The CQ bit was checked by mfomitchev@chromium.org
6 years, 6 months ago (2014-02-06 15:17:03 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/136453009/560002
6 years, 6 months ago (2014-02-06 15:17:37 UTC) #43
commit-bot: I haz the power
Change committed as 249431
6 years, 6 months ago (2014-02-06 17:46:24 UTC) #44
bsalomon
https://codereview.chromium.org/136453009/diff/560002/content/browser/frame_host/navigation_entry_screenshot_manager.cc File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right): https://codereview.chromium.org/136453009/diff/560002/content/browser/frame_host/navigation_entry_screenshot_manager.cc#newcode64 content/browser/frame_host/navigation_entry_screenshot_manager.cc:64: paint.setColorFilter(SkLumaColorFilter::Create()); The color filter is leaking here. One fix ...
6 years, 6 months ago (2014-02-06 20:09:13 UTC) #45
tdanderson
The CQ bit was checked by tdanderson@chromium.org
6 years, 6 months ago (2014-02-07 19:02:55 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/136453009/120002
6 years, 6 months ago (2014-02-07 19:04:50 UTC) #47
mfomitchev
https://codereview.chromium.org/136453009/diff/560002/content/browser/frame_host/navigation_entry_screenshot_manager.cc File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right): https://codereview.chromium.org/136453009/diff/560002/content/browser/frame_host/navigation_entry_screenshot_manager.cc#newcode64 content/browser/frame_host/navigation_entry_screenshot_manager.cc:64: paint.setColorFilter(SkLumaColorFilter::Create()); On 2014/02/06 20:09:14, bsalomon wrote: > The color ...
6 years, 6 months ago (2014-02-07 19:05:43 UTC) #48
bsalomon
On 2014/02/07 19:05:43, mfomitchev wrote: > https://codereview.chromium.org/136453009/diff/560002/content/browser/frame_host/navigation_entry_screenshot_manager.cc > File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right): > > https://codereview.chromium.org/136453009/diff/560002/content/browser/frame_host/navigation_entry_screenshot_manager.cc#newcode64 > ...
6 years, 6 months ago (2014-02-07 20:50:01 UTC) #49
commit-bot: I haz the power
6 years, 6 months ago (2014-02-07 21:23:23 UTC) #50
Message was sent while issue was closed.
Change committed as 249782

Powered by Google App Engine
This is Rietveld 408576698