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

Issue 13105002: Screenshot effect non-obvious (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year ago by sschmitz
Modified:
1 year ago
Reviewers:
James Cook, Jun Mukai, sky
CC:
chromium-reviews_chromium.org, 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) Lint Patch
M ash/ash_strings.grd View 1 chunk +12 lines, -0 lines 0 comments ? errors 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 ? errors 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 ? errors 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 ? errors 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 1 errors 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 2 errors 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 1 errors 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 ? errors Download
Commit:

Messages

Total messages: 41
sschmitz
1 year ago #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? ...
1 year ago #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 ...
1 year ago #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 ...
1 year ago #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 = ...
1 year ago #5
James Cook
The static id is fine, though I might comment that it's only being used to ...
1 year ago #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 ...
1 year ago #7
James Cook
Also, technically our browser tests aren't really "unit" tests, since they don't test individual code ...
1 year ago #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 ...
1 year ago #9
sschmitz
Rebased Added ScreenshotTakerObserver for test Added result codes so the test indicates the type of ...
1 year ago #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 ...
1 year ago #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 ...
1 year ago #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 ...
1 year ago #13
Jun Mukai
sorry for drive-by comments at the last minutes. I didn't check with the previous reviews/feedback ...
1 year ago #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 ...
1 year ago #15
James Cook
lgtm
1 year ago #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, ...
1 year ago #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? ...
1 year ago #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): ...
1 year ago #19
James Cook
Almost there. I like this better as a unit test. Sky can comment more on ...
1 year ago #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 ...
1 year ago #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, ...
1 year ago #22
sschmitz
An issue arose. I believe the action on clicking the notification, namely to open the ...
1 year ago #23
James Cook
For win7_aura I would be OK with either "Done" or just the empty string. I ...
1 year ago #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 ...
1 year ago #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 ...
1 year ago #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 ...
1 year ago #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 ...
1 year ago #28
sschmitz
Thanks again. I ifdef-ed out sending the Notification on Windows. Otherwise it fails with NOTIMPLEMENTED ...
1 year ago #29
James Cook
Despite my earlier L G T M please don't commit this until you talk to ...
1 year ago #30
James Cook
Never mind. This is why I shouldn't review code after hours. base::Bind() handles weak pointers, ...
1 year ago #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 ...
1 year ago #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: ...
1 year ago #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
1 year ago #34
I haz the power (commit-bot)
Step "update" is always a major failure. Look at the try server FAQ for more ...
1 year ago #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
1 year ago #36
I haz the power (commit-bot)
Step "update" is always a major failure. Look at the try server FAQ for more ...
1 year ago #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
1 year ago #38
I haz the power (commit-bot)
Step "update" is always a major failure. Look at the try server FAQ for more ...
1 year ago #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
1 year ago #40
I haz the power (commit-bot)
1 year ago #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 1280:2d3e6564b7b6