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

Issue 13105002: Screenshot effect non-obvious (Closed)

Created:
7 years, 9 months ago by sschmitz
Modified:
7 years, 8 months ago
Reviewers:
James Cook, Jun Mukai, sky
CC:
chromium-reviews, sadrul, oshima+watch_chromium.org, ben+watch_chromium.org, Jun Mukai
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Screenshot effect non-obvious Added new (rich) notifications when a screenshot is taken on Chrome OS. There are two different notifications, one for success and one for failure. In case of success, the user can click on the notification to bring up the file manager with the screenshot file hi-lighted. I've attached screenshots to the BUG. Added unit test for the screenshot taker. BUG=150017 TEST=manual; for now run with --enable-rich-notifications. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192472

Patch Set 1 #

Total comments: 16

Patch Set 2 : review #

Total comments: 4

Patch Set 3 : simplify #

Total comments: 4

Patch Set 4 : unittest #

Total comments: 6

Patch Set 5 : browsertest #

Patch Set 6 : observer for test #

Total comments: 16

Patch Set 7 : review #

Total comments: 14

Patch Set 8 : update #

Total comments: 18

Patch Set 9 : unit test #

Total comments: 22

Patch Set 10 : update #

Total comments: 4

Patch Set 11 : handle windows #

Total comments: 2

Patch Set 12 : window fix #

Total comments: 4

Patch Set 13 : another fix for windows #

Patch Set 14 : removed chrome_tests.gypi #

Patch Set 15 : Callback #

Total comments: 10

Patch Set 16 : cleanup #

Unified diffs Side-by-side diffs Delta from patch set Stats (+390 lines, -73 lines) Patch
M ash/ash_strings.grd View 1 chunk +12 lines, -0 lines 0 comments Download
M chrome/app/theme/theme_resources.grd View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/notifications/message_center_notification_manager.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/ash/ash_init.cc View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/ui/ash/screenshot_taker.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +54 lines, -11 lines 0 comments Download
M chrome/browser/ui/ash/screenshot_taker.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 8 chunks +198 lines, -59 lines 0 comments Download
A chrome/browser/ui/ash/screenshot_taker_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +118 lines, -0 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 41 (0 generated)
sschmitz
7 years, 9 months ago (2013-03-26 23:36:19 UTC) #1
James Cook
https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screenshot_taker.cc#newcode47 chrome/browser/ui/ash/screenshot_taker.cc:47: const float kVisualFeedbackLayerOpacity = 0.25f; Is this still needed? ...
7 years, 9 months ago (2013-03-26 23:54:25 UTC) #2
sschmitz
https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screenshot_taker.cc#newcode47 chrome/browser/ui/ash/screenshot_taker.cc:47: const float kVisualFeedbackLayerOpacity = 0.25f; On 2013/03/26 23:54:25, James ...
7 years, 9 months ago (2013-03-27 14:58:48 UTC) #3
James Cook
And we need some kind of test, maybe a browser_test https://codereview.chromium.org/13105002/diff/6001/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/6001/chrome/browser/ui/ash/screenshot_taker.cc#newcode60 ...
7 years, 9 months ago (2013-03-27 16:49:17 UTC) #4
sschmitz
Will look into the unit test next. https://codereview.chromium.org/13105002/diff/6001/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/6001/chrome/browser/ui/ash/screenshot_taker.cc#newcode60 chrome/browser/ui/ash/screenshot_taker.cc:60: notification_icon = ...
7 years, 9 months ago (2013-03-27 17:44:29 UTC) #5
James Cook
The static id is fine, though I might comment that it's only being used to ...
7 years, 9 months ago (2013-03-27 22:40:29 UTC) #6
sschmitz
Added unittest in browser_tests https://codereview.chromium.org/13105002/diff/7006/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/7006/chrome/browser/ui/ash/screenshot_taker.cc#newcode116 chrome/browser/ui/ash/screenshot_taker.cc:116: DCHECK(g_browser_process); On 2013/03/27 22:40:29, James ...
7 years, 9 months ago (2013-03-28 18:46:22 UTC) #7
James Cook
Also, technically our browser tests aren't really "unit" tests, since they don't test individual code ...
7 years, 9 months ago (2013-03-28 19:49:36 UTC) #8
sschmitz
Great feedback. This is simpler. https://codereview.chromium.org/13105002/diff/15001/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/15001/chrome/browser/ui/ash/screenshot_taker.cc#newcode274 chrome/browser/ui/ash/screenshot_taker.cc:274: ShowNotification(false, screenshot_path); On 2013/03/28 ...
7 years, 9 months ago (2013-03-28 23:52:40 UTC) #9
sschmitz
Rebased Added ScreenshotTakerObserver for test Added result codes so the test indicates the type of ...
7 years, 8 months ago (2013-03-29 19:03:41 UTC) #10
James Cook
https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/screenshot_taker.cc#newcode250 chrome/browser/ui/ash/screenshot_taker.cc:250: ScreenshotTakerObserver* ScreenshotTaker::s_observer_for_test = 0; I think we prefer NULL ...
7 years, 8 months ago (2013-03-29 20:00:32 UTC) #11
sschmitz
https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/screenshot_taker.cc#newcode250 chrome/browser/ui/ash/screenshot_taker.cc:250: ScreenshotTakerObserver* ScreenshotTaker::s_observer_for_test = 0; On 2013/03/29 20:00:32, James Cook ...
7 years, 8 months ago (2013-03-29 21:02:35 UTC) #12
James Cook
LGTM with one final suggestion https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/screenshot_taker.h File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/screenshot_taker.h#newcode64 chrome/browser/ui/ash/screenshot_taker.h:64: scoped_ptr<base::FilePath> screenshot_directory_for_test_; I don't ...
7 years, 8 months ago (2013-03-29 22:49:33 UTC) #13
Jun Mukai
sorry for drive-by comments at the last minutes. I didn't check with the previous reviews/feedback ...
7 years, 8 months ago (2013-03-29 23:01:37 UTC) #14
sschmitz
minor updates https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/screenshot_taker.cc#newcode92 chrome/browser/ui/ash/screenshot_taker.cc:92: // Notifications only on Chrome OS. On ...
7 years, 8 months ago (2013-03-29 23:40:44 UTC) #15
James Cook
lgtm
7 years, 8 months ago (2013-03-29 23:47:17 UTC) #16
Jun Mukai
https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/screenshot_taker.cc#newcode92 chrome/browser/ui/ash/screenshot_taker.cc:92: // Notifications only on Chrome OS. On 2013/03/29 23:40:44, ...
7 years, 8 months ago (2013-03-30 00:37:17 UTC) #17
sky
https://codereview.chromium.org/13105002/diff/48001/chrome/app/theme/theme_resources.grd File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/13105002/diff/48001/chrome/app/theme/theme_resources.grd#newcode683 chrome/app/theme/theme_resources.grd:683: <if expr="pp_ifdef('chromeos')"> Should this be ash and not chromeos? ...
7 years, 8 months ago (2013-03-31 01:16:03 UTC) #18
sschmitz
Addressed all (I hope) comments. Changed from browser to unit test. https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): ...
7 years, 8 months ago (2013-04-02 21:23:25 UTC) #19
James Cook
Almost there. I like this better as a unit test. Sky can comment more on ...
7 years, 8 months ago (2013-04-02 22:47:18 UTC) #20
sky
https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/screenshot_taker.cc#newcode48 chrome/browser/ui/ash/screenshot_taker.cc:48: class ScreenshotTakerNotificationDelegate : public NotificationDelegate { nit: add description ...
7 years, 8 months ago (2013-04-03 04:24:46 UTC) #21
sschmitz
https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/screenshot_taker.cc#newcode48 chrome/browser/ui/ash/screenshot_taker.cc:48: class ScreenshotTakerNotificationDelegate : public NotificationDelegate { On 2013/04/03 04:24:46, ...
7 years, 8 months ago (2013-04-03 17:57:40 UTC) #22
sschmitz
An issue arose. I believe the action on clicking the notification, namely to open the ...
7 years, 8 months ago (2013-04-03 19:43:11 UTC) #23
James Cook
For win7_aura I would be OK with either "Done" or just the empty string. I ...
7 years, 8 months ago (2013-04-03 19:50:19 UTC) #24
sschmitz
Fixed Windows https://codereview.chromium.org/13105002/diff/68001/chrome/browser/ui/ash/screenshot_taker_unittest.cc File chrome/browser/ui/ash/screenshot_taker_unittest.cc (right): https://codereview.chromium.org/13105002/diff/68001/chrome/browser/ui/ash/screenshot_taker_unittest.cc#newcode60 chrome/browser/ui/ash/screenshot_taker_unittest.cc:60: ScreenshotTaker& screenshot_taker, On 2013/04/03 19:50:19, James Cook ...
7 years, 8 months ago (2013-04-03 21:13:09 UTC) #25
James Cook
LGTM with nit https://codereview.chromium.org/13105002/diff/77001/chrome/browser/ui/ash/screenshot_taker_unittest.cc File chrome/browser/ui/ash/screenshot_taker_unittest.cc (right): https://codereview.chromium.org/13105002/diff/77001/chrome/browser/ui/ash/screenshot_taker_unittest.cc#newcode62 chrome/browser/ui/ash/screenshot_taker_unittest.cc:62: ASSERT_TRUE(screenshot_taker); nit: You don't need to ...
7 years, 8 months ago (2013-04-03 21:25:04 UTC) #26
sschmitz
Oops... fixed #ifdef chrome os for windows https://codereview.chromium.org/13105002/diff/77001/chrome/browser/ui/ash/screenshot_taker_unittest.cc File chrome/browser/ui/ash/screenshot_taker_unittest.cc (right): https://codereview.chromium.org/13105002/diff/77001/chrome/browser/ui/ash/screenshot_taker_unittest.cc#newcode62 chrome/browser/ui/ash/screenshot_taker_unittest.cc:62: ASSERT_TRUE(screenshot_taker); On ...
7 years, 8 months ago (2013-04-03 21:51:54 UTC) #27
sky
https://codereview.chromium.org/13105002/diff/83001/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/83001/chrome/browser/ui/ash/screenshot_taker.cc#newcode89 chrome/browser/ui/ash/screenshot_taker.cc:89: void ShowNotificationCaller(base::WeakPtr<ScreenshotTaker> screenshot_taker, Is there a reason you went ...
7 years, 8 months ago (2013-04-03 23:16:09 UTC) #28
sschmitz
Thanks again. I ifdef-ed out sending the Notification on Windows. Otherwise it fails with NOTIMPLEMENTED ...
7 years, 8 months ago (2013-04-04 01:06:08 UTC) #29
James Cook
Despite my earlier L G T M please don't commit this until you talk to ...
7 years, 8 months ago (2013-04-04 02:13:32 UTC) #30
James Cook
Never mind. This is why I shouldn't review code after hours. base::Bind() handles weak pointers, ...
7 years, 8 months ago (2013-04-04 04:44:57 UTC) #31
sky
LGTM https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/screenshot_taker.cc#newcode93 chrome/browser/ui/ash/screenshot_taker.cc:93: void PostShowNotification(const ShowNotificationCallback& callback, Calling this function takes ...
7 years, 8 months ago (2013-04-04 15:38:31 UTC) #32
sschmitz
https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/screenshot_taker.cc File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/screenshot_taker.cc#newcode93 chrome/browser/ui/ash/screenshot_taker.cc:93: void PostShowNotification(const ShowNotificationCallback& callback, On 2013/04/04 15:38:31, sky wrote: ...
7 years, 8 months ago (2013-04-04 16:49:47 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13105002/100001
7 years, 8 months ago (2013-04-04 17:04:34 UTC) #34
commit-bot: I haz the power
Step "update" is always a major failure. Look at the try server FAQ for more ...
7 years, 8 months ago (2013-04-04 17:13:30 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13105002/100001
7 years, 8 months ago (2013-04-04 18:59:19 UTC) #36
commit-bot: I haz the power
Step "update" is always a major failure. Look at the try server FAQ for more ...
7 years, 8 months ago (2013-04-04 19:01:16 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13105002/100001
7 years, 8 months ago (2013-04-04 19:34:54 UTC) #38
commit-bot: I haz the power
Step "update" is always a major failure. Look at the try server FAQ for more ...
7 years, 8 months ago (2013-04-04 19:37:06 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13105002/100001
7 years, 8 months ago (2013-04-04 23:20:23 UTC) #40
commit-bot: I haz the power
7 years, 8 months ago (2013-04-05 03:31:45 UTC) #41
Message was sent while issue was closed.
Change committed as 192472

Powered by Google App Engine
This is Rietveld 408576698