Chromium Code Reviews
Help | Chromium Project | Sign in
(17)

Issue 13105002: Screenshot effect non-obvious (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 1 month ago by sschmitz
Modified:
2 years, 1 month 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
Commit: CQ not working?

Messages

Total messages: 41 (0 generated)
sschmitz
2 years, 1 month 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? ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 = ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month ago (2013-03-29 23:40:44 UTC) #15
James Cook
lgtm
2 years, 1 month 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, ...
2 years, 1 month 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? ...
2 years, 1 month 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): ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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, ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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, ...
2 years, 1 month 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 ...
2 years, 1 month 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: ...
2 years, 1 month ago (2013-04-04 16:49:47 UTC) #33
I haz the power - commit-bot
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13105002/100001
2 years, 1 month ago (2013-04-04 17:04:34 UTC) #34
I haz the power - commit-bot
Step "update" is always a major failure. Look at the try server FAQ for more ...
2 years, 1 month ago (2013-04-04 17:13:30 UTC) #35
I haz the power - commit-bot
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13105002/100001
2 years, 1 month ago (2013-04-04 18:59:19 UTC) #36
I haz the power - commit-bot
Step "update" is always a major failure. Look at the try server FAQ for more ...
2 years, 1 month ago (2013-04-04 19:01:16 UTC) #37
I haz the power - commit-bot
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13105002/100001
2 years, 1 month ago (2013-04-04 19:34:54 UTC) #38
I haz the power - commit-bot
Step "update" is always a major failure. Look at the try server FAQ for more ...
2 years, 1 month ago (2013-04-04 19:37:06 UTC) #39
I haz the power - commit-bot
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13105002/100001
2 years, 1 month ago (2013-04-04 23:20:23 UTC) #40
I haz the power - commit-bot
2 years, 1 month ago (2013-04-05 03:31:45 UTC) #41
Message was sent while issue was closed.
Change committed as 192472
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld ec887be