|
|
Created:
7 years, 9 months ago by sschmitz Modified:
7 years, 8 months ago 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. |
DescriptionScreenshot 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 #
Messages
Total messages: 41 (0 generated)
https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.cc:47: const float kVisualFeedbackLayerOpacity = 0.25f; Is this still needed? https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.cc:50: const int64 kVisualFeedbackLayerDisplayTimeMs = 100; Ditto https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.cc:285: namespace { Anonymous namespace goes at top of file. https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.cc:286: class Delegate : public NotificationDelegate { Call this something other than "Delegate", maybe ScreenshotDelegate or ScreenshotNotification or something? https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.cc:324: void ScreenshotTaker::ShowNotification( Could you pass all the data you need as parameters to this function? Then it could be an anonymous function and you wouldn't need weak pointers to the class. https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.h:39: // WeakPtr factory. Better to explain why you need weak pointers at all. https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.h:45: // Notification icon. This comment doesn't add anything - the variable name is enough. https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.h:51: // Notification title text when successful. Likewise, none of these comments add anything. I might just use a single comment like "// Text appearing in notifications." for all 4 members.
https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.cc:47: const float kVisualFeedbackLayerOpacity = 0.25f; On 2013/03/26 23:54:25, James Cook (Chromium) wrote: > Is this still needed? no; removed https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.cc:50: const int64 kVisualFeedbackLayerDisplayTimeMs = 100; On 2013/03/26 23:54:25, James Cook (Chromium) wrote: > Ditto Done. https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.cc:285: namespace { On 2013/03/26 23:54:25, James Cook (Chromium) wrote: > Anonymous namespace goes at top of file. Done. https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.cc:286: class Delegate : public NotificationDelegate { On 2013/03/26 23:54:25, James Cook (Chromium) wrote: > Call this something other than "Delegate", maybe ScreenshotDelegate or > ScreenshotNotification or something? Done. https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.cc:324: void ScreenshotTaker::ShowNotification( On 2013/03/26 23:54:25, James Cook (Chromium) wrote: > Could you pass all the data you need as parameters to this function? Then it > could be an anonymous function and you wouldn't need weak pointers to the class. Good point. I found a similar solution. The more or less constant notification data is split into a struct with static members. ShowNotification only needs a bool and file-path. No more need to pass a weak ptr or data around. Done https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.h:39: // WeakPtr factory. On 2013/03/26 23:54:25, James Cook (Chromium) wrote: > Better to explain why you need weak pointers at all. Removed. https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.h:45: // Notification icon. On 2013/03/26 23:54:25, James Cook (Chromium) wrote: > This comment doesn't add anything - the variable name is enough. Removed https://codereview.chromium.org/13105002/diff/1/chrome/browser/ui/ash/screens... chrome/browser/ui/ash/screenshot_taker.h:51: // Notification title text when successful. On 2013/03/26 23:54:25, James Cook (Chromium) wrote: > Likewise, none of these comments add anything. I might just use a single comment > like "// Text appearing in notifications." for all 4 members. Removed
And we need some kind of test, maybe a browser_test https://codereview.chromium.org/13105002/diff/6001/chrome/browser/ui/ash/scre... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/6001/chrome/browser/ui/ash/scre... chrome/browser/ui/ash/screenshot_taker.cc:60: notification_icon = ui::ResourceBundle::GetSharedInstance().GetImageNamed( Why not just look up these images and resources every time a notification is shown? This is a rarely-used feature - the performance penalty is unlikely to matter. https://codereview.chromium.org/13105002/diff/6001/chrome/browser/ui/ash/scre... chrome/browser/ui/ash/screenshot_taker.cc:122: void ShowNotification(bool success, const base::FilePath& screenshot_path) { If you pass notification_id in here and look up the strings every time I think you don't need the struct at all.
Will look into the unit test next. https://codereview.chromium.org/13105002/diff/6001/chrome/browser/ui/ash/scre... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/6001/chrome/browser/ui/ash/scre... chrome/browser/ui/ash/screenshot_taker.cc:60: notification_icon = ui::ResourceBundle::GetSharedInstance().GetImageNamed( On 2013/03/27 16:49:17, James Cook (Chromium) wrote: > Why not just look up these images and resources every time a notification is > shown? This is a rarely-used feature - the performance penalty is unlikely to > matter. Great. Makes it much simpler. Done. https://codereview.chromium.org/13105002/diff/6001/chrome/browser/ui/ash/scre... chrome/browser/ui/ash/screenshot_taker.cc:122: void ShowNotification(bool success, const base::FilePath& screenshot_path) { On 2013/03/27 16:49:17, James Cook (Chromium) wrote: > If you pass notification_id in here and look up the strings every time I think > you don't need the struct at all. Decided to use static int id inside ShowNotification. That limits the scope and requires fewer changes to existing code. Otherwise, we would have to pass the id through many calls. Decided not to use the file-path as id because conversion to std::string is OS dependent. This is simpler.
The static id is fine, though I might comment that it's only being used to avoid generating duplicate notifications. https://codereview.chromium.org/13105002/diff/7006/chrome/browser/ui/ash/scre... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/7006/chrome/browser/ui/ash/scre... chrome/browser/ui/ash/screenshot_taker.cc:116: DCHECK(g_browser_process); nit: You don't need this DCHECK. If g_browser_process isn't around (a) things are really broken in the browser and (b) you'll crash immediately on the next line, which will be obvious. https://codereview.chromium.org/13105002/diff/7006/chrome/browser/ui/ash/scre... chrome/browser/ui/ash/screenshot_taker.cc:121: void PostNotification(bool success, const base::FilePath& screenshot_path) { Either comment or DCHECK which thread you expect to be on in this function. It looks me to like this can be called from multiple threads, which is confusing. Consider calling ShowNotification directly if you know you're on the UI thread.
Added unittest in browser_tests https://codereview.chromium.org/13105002/diff/7006/chrome/browser/ui/ash/scre... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/7006/chrome/browser/ui/ash/scre... chrome/browser/ui/ash/screenshot_taker.cc:116: DCHECK(g_browser_process); On 2013/03/27 22:40:29, James Cook (Chromium) wrote: > nit: You don't need this DCHECK. If g_browser_process isn't around (a) things > are really broken in the browser and (b) you'll crash immediately on the next > line, which will be obvious. Done. https://codereview.chromium.org/13105002/diff/7006/chrome/browser/ui/ash/scre... chrome/browser/ui/ash/screenshot_taker.cc:121: void PostNotification(bool success, const base::FilePath& screenshot_path) { On 2013/03/27 22:40:29, James Cook (Chromium) wrote: > Either comment or DCHECK which thread you expect to be on in this function. > > It looks me to like this can be called from multiple threads, which is > confusing. Consider calling ShowNotification directly if you know you're on the > UI thread. Done.
Also, technically our browser tests aren't really "unit" tests, since they don't test individual code modules. They're more like integration tests. https://codereview.chromium.org/13105002/diff/15001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/15001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:274: ShowNotification(false, screenshot_path); Isn't this called on the UI thread? If so, the DCHECK in ShowNotification will fail. https://codereview.chromium.org/13105002/diff/15001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:298: ShowNotification(false, screenshot_path); ditto, this is on the UI thread https://codereview.chromium.org/13105002/diff/15001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker_browsertest.cc (right): https://codereview.chromium.org/13105002/diff/15001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_browsertest.cc:37: // We repeatedly check for this notification to be present or exit You can't write browser tests in Chrome this way (polling for a result). The bots run in virtual machines that can have widely varying performance - even simple tests can sometimes take a very long time, and long tests can sometimes run very quickly. The pattern we often use is to wait for a global Chrome notification (not related to the rich notifications). For example, see chrome_notification_types.h for things like NOTIFICATION_BROWSER_CLOSED. There might already be one for rich notifications, or you might have to add one. Or you could investigate lower-level unit tests.
Great feedback. This is simpler. https://codereview.chromium.org/13105002/diff/15001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/15001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:274: ShowNotification(false, screenshot_path); On 2013/03/28 19:49:36, James Cook (Chromium) wrote: > Isn't this called on the UI thread? If so, the DCHECK in ShowNotification will > fail. Yes it is on the UI thread and No it works. I tested that. (I tested all ChromeOS code paths by temporarily changing the code for force the branch.) To make the distinction between UI and Blocking-Pool threads clearer I changed PostNotification to PostShowNotification. https://codereview.chromium.org/13105002/diff/15001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:298: ShowNotification(false, screenshot_path); On 2013/03/28 19:49:36, James Cook (Chromium) wrote: > ditto, this is on the UI thread same as above. https://codereview.chromium.org/13105002/diff/15001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker_browsertest.cc (right): https://codereview.chromium.org/13105002/diff/15001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_browsertest.cc:37: // We repeatedly check for this notification to be present or exit On 2013/03/28 19:49:36, James Cook (Chromium) wrote: > You can't write browser tests in Chrome this way (polling for a result). The > bots run in virtual machines that can have widely varying performance - even > simple tests can sometimes take a very long time, and long tests can sometimes > run very quickly. > > The pattern we often use is to wait for a global Chrome notification (not > related to the rich notifications). For example, see > chrome_notification_types.h for things like NOTIFICATION_BROWSER_CLOSED. There > might already be one for rich notifications, or you might have to add one. Or > you could investigate lower-level unit tests. > Thanks for the info... done.
Rebased Added ScreenshotTakerObserver for test Added result codes so the test indicates the type of failure in case the screenshot fails
https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:250: ScreenshotTakerObserver* ScreenshotTaker::s_observer_for_test = 0; I think we prefer NULL for null pointers. https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:23: enum Result { Move this into ScreenshotTakerObserver https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:32: SCREENSHOT_RESULT_COUNT // must be last Two spaces before comment and period at end (or just remove the comment). https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:69: static ScreenshotTakerObserver* s_observer_for_test; statics are observer_for_test_ like members. https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker_browsertest.cc (right): https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_browsertest.cc:13: #include "base/time.h" Do you need "time.h"? https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_browsertest.cc:46: screenshot_path_ = screenshot_path; Do you need screenshot_path_? https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_browsertest.cc:63: void RunTest() { We usually put most of this code in the IN_PROC_BROWSER_TEST section below. https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_browsertest.cc:81: base::StringPrintf("screenshot_%3.3d", 1))); Do you need StringPrintf here? Why not just provide the string you expect?
https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:250: ScreenshotTakerObserver* ScreenshotTaker::s_observer_for_test = 0; On 2013/03/29 20:00:32, James Cook (Chromium) wrote: > I think we prefer NULL for null pointers. Done. https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:23: enum Result { On 2013/03/29 20:00:32, James Cook (Chromium) wrote: > Move this into ScreenshotTakerObserver Done. https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:32: SCREENSHOT_RESULT_COUNT // must be last On 2013/03/29 20:00:32, James Cook (Chromium) wrote: > Two spaces before comment and period at end (or just remove the comment). Done. https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:69: static ScreenshotTakerObserver* s_observer_for_test; On 2013/03/29 20:00:32, James Cook (Chromium) wrote: > statics are observer_for_test_ like members. Done. https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker_browsertest.cc (right): https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_browsertest.cc:13: #include "base/time.h" On 2013/03/29 20:00:32, James Cook (Chromium) wrote: > Do you need "time.h"? Removed. https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_browsertest.cc:46: screenshot_path_ = screenshot_path; On 2013/03/29 20:00:32, James Cook (Chromium) wrote: > Do you need screenshot_path_? Added a test to verify that the file exists (if screenshot was successful). https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_browsertest.cc:63: void RunTest() { On 2013/03/29 20:00:32, James Cook (Chromium) wrote: > We usually put most of this code in the IN_PROC_BROWSER_TEST section below. Done. https://codereview.chromium.org/13105002/diff/29001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_browsertest.cc:81: base::StringPrintf("screenshot_%3.3d", 1))); On 2013/03/29 20:00:32, James Cook (Chromium) wrote: > Do you need StringPrintf here? Why not just provide the string you expect? Done.
LGTM with one final suggestion https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:64: scoped_ptr<base::FilePath> screenshot_directory_for_test_; I don't think this needs to be a pointer - you should just be able to use base::FilePath and check for empty.
sorry for drive-by comments at the last minutes. I didn't check with the previous reviews/feedback from others. If some of them conflict with his comments, please ignore mine. https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:92: // Notifications only on Chrome OS. I am not so sure the point here. If so, no feedback in other platforms? That doesn't sound quite well. IIRC this file itself is only for ChromeOS right now. And if we want to allow this to other platforms, probably we should consider the feedback on them. Anyway the guard doesn't make much sense to me. https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:58: static ScreenshotTakerObserver* GetObserverForTest(); move them to private methods and declare friend class the test class, for safety? https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:66: static ScreenshotTakerObserver* observer_for_test_; why static? I don't think your test needs to create multiple screenshot taker instance, so just a simple member would be fine.
minor updates https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:92: // Notifications only on Chrome OS. On 2013/03/29 23:01:37, Jun Mukai wrote: > I am not so sure the point here. > If so, no feedback in other platforms? That doesn't sound quite well. > IIRC this file itself is only for ChromeOS right now. And if we want to allow > this to other platforms, probably we should consider the feedback on them. > Anyway the guard doesn't make much sense to me. We had dicussions on this and decided non-ChromeOS should or do use other methods of screenshot taking. We ended up putting the icon in "cros", i.e. ChromeOS only, that necessitates the guard. We could reconsider in the future? I propose to leave it for now. https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:58: static ScreenshotTakerObserver* GetObserverForTest(); On 2013/03/29 23:01:37, Jun Mukai wrote: > move them to private methods and declare friend class the test class, for > safety? I think there would be a problem, because friendship is not inherited and the actual test is derived from ScreenshotTakerTest in the IN_PROC_BROWSER_TEST_F macro. And I'd rather not declare friendship to class ScreenshotTakerTest_TakeScreenshot_Test as well. https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:64: scoped_ptr<base::FilePath> screenshot_directory_for_test_; On 2013/03/29 22:49:34, James Cook (Chromium) wrote: > I don't think this needs to be a pointer - you should just be able to use > base::FilePath and check for empty. Done. https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:66: static ScreenshotTakerObserver* observer_for_test_; On 2013/03/29 23:01:37, Jun Mukai wrote: > why static? I don't think your test needs to create multiple screenshot taker > instance, so just a simple member would be fine. The reason for static is to avoid passing a weak pointer to the ScreenshotTaker through ~5 namespace (non-member) functions. (I had implemented that earlier but reverted to a simpler solution).
lgtm
https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:92: // Notifications only on Chrome OS. On 2013/03/29 23:40:44, sschmitz wrote: > On 2013/03/29 23:01:37, Jun Mukai wrote: > > I am not so sure the point here. > > If so, no feedback in other platforms? That doesn't sound quite well. > > IIRC this file itself is only for ChromeOS right now. And if we want to allow > > this to other platforms, probably we should consider the feedback on them. > > Anyway the guard doesn't make much sense to me. > We had dicussions on this and decided non-ChromeOS should or do use other > methods of screenshot taking. We ended up putting the icon in "cros", i.e. > ChromeOS only, that necessitates the guard. We could reconsider in the future? > I propose to leave it for now. > Ah, got it. thanks! https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:58: static ScreenshotTakerObserver* GetObserverForTest(); On 2013/03/29 23:40:44, sschmitz wrote: > On 2013/03/29 23:01:37, Jun Mukai wrote: > > move them to private methods and declare friend class the test class, for > > safety? > I think there would be a problem, because friendship is not inherited and the > actual test is derived from ScreenshotTakerTest in the IN_PROC_BROWSER_TEST_F > macro. And I'd rather not declare friendship to > class ScreenshotTakerTest_TakeScreenshot_Test as well. You can call the private methods from SetUp() of your friend class ScreenshotTakerTest. Or if you want to allow it in each IN_PROC_BROWSER_TEST_F, you can declare some utility methods in ScreenshotTakerTest to do so, and make them protected. https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:66: static ScreenshotTakerObserver* observer_for_test_; On 2013/03/29 23:40:44, sschmitz wrote: > On 2013/03/29 23:01:37, Jun Mukai wrote: > > why static? I don't think your test needs to create multiple screenshot taker > > instance, so just a simple member would be fine. > The reason for static is to avoid passing a weak pointer to the ScreenshotTaker > through ~5 namespace (non-member) functions. (I had implemented that earlier but > reverted to a simpler solution). I still personally prefer non-static solution but it's up to you.
https://codereview.chromium.org/13105002/diff/48001/chrome/app/theme/theme_re... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/13105002/diff/48001/chrome/app/theme/theme_re... chrome/app/theme/theme_resources.grd:683: <if expr="pp_ifdef('chromeos')"> Should this be ash and not chromeos? I assume the code will run win-ash, right? https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:81: std::string id_text_; Make all of these const if they can be. https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:91: // Notifications only on Chrome OS. Why is the notification only for chromeos? https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:116: Profile* profile = ProfileManager::GetDefaultProfileOrOffTheRecord(); Can ScreenshotTaker so that we minimize uses of GetDefaultPro... https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:120: if (ScreenshotTaker::GetObserverForTest()) { Why is this in the ifdef? https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:33: virtual ~ScreenshotTakerObserver() {} Move this to protected to make it clear it's not owned by ScreenShotTaker. https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:54: static void SetObserverForTest(ScreenshotTakerObserver* observer); If this is really only for a test, how about moving it to a separate header and having a friend set it. That said, it's rather unusual to have an observer only for tests. Is there a reason this needs to be SetObserverForTest rather than just set_observer? https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker_browsertest.cc (right): https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_browsertest.cc:5: #if defined(OS_CHROMEOS) This isn't the right way to have a chromeos specific browsertest. Again, I think this should be for windows too. If not, the right way is to name appropriately or excluded in gyp. https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_browsertest.cc:22: class ScreenshotTakerTest : public InProcessBrowserTest, Why does this need to be a browser test instead of a unit test?
Addressed all (I hope) comments. Changed from browser to unit test. https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:92: // Notifications only on Chrome OS. On 2013/03/30 00:37:17, Jun Mukai wrote: > On 2013/03/29 23:40:44, sschmitz wrote: > > On 2013/03/29 23:01:37, Jun Mukai wrote: > > > I am not so sure the point here. > > > If so, no feedback in other platforms? That doesn't sound quite well. > > > IIRC this file itself is only for ChromeOS right now. And if we want to > allow > > > this to other platforms, probably we should consider the feedback on them. > > > Anyway the guard doesn't make much sense to me. > > We had dicussions on this and decided non-ChromeOS should or do use other > > methods of screenshot taking. We ended up putting the icon in "cros", i.e. > > ChromeOS only, that necessitates the guard. We could reconsider in the future? > > I propose to leave it for now. > > > > Ah, got it. thanks! On second thought... and more discussions. I have removed the Chrome OS ifdef. It is now for Ash. https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:58: static ScreenshotTakerObserver* GetObserverForTest(); On 2013/03/30 00:37:17, Jun Mukai wrote: > On 2013/03/29 23:40:44, sschmitz wrote: > > On 2013/03/29 23:01:37, Jun Mukai wrote: > > > move them to private methods and declare friend class the test class, for > > > safety? > > I think there would be a problem, because friendship is not inherited and the > > actual test is derived from ScreenshotTakerTest in the IN_PROC_BROWSER_TEST_F > > macro. And I'd rather not declare friendship to > > class ScreenshotTakerTest_TakeScreenshot_Test as well. > > You can call the private methods from SetUp() of your friend class > ScreenshotTakerTest. Or if you want to allow it in each IN_PROC_BROWSER_TEST_F, > you can declare some utility methods in ScreenshotTakerTest to do so, and make > them protected. I did what you suggested. Made the override functions (renamed) private and the test a friend. I only access these functions in the test base class as you suggested. Done. https://codereview.chromium.org/13105002/diff/38001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:66: static ScreenshotTakerObserver* observer_for_test_; On 2013/03/30 00:37:17, Jun Mukai wrote: > On 2013/03/29 23:40:44, sschmitz wrote: > > On 2013/03/29 23:01:37, Jun Mukai wrote: > > > why static? I don't think your test needs to create multiple screenshot > taker > > > instance, so just a simple member would be fine. > > The reason for static is to avoid passing a weak pointer to the > ScreenshotTaker > > through ~5 namespace (non-member) functions. (I had implemented that earlier > but > > reverted to a simpler solution). > > I still personally prefer non-static solution but it's up to you. Yes. Good point. I removed all statics in favor of members. The namespace non-member fcts pass a weak ptr to the "final" function can be a member fct. Done. https://codereview.chromium.org/13105002/diff/48001/chrome/app/theme/theme_re... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/13105002/diff/48001/chrome/app/theme/theme_re... chrome/app/theme/theme_resources.grd:683: <if expr="pp_ifdef('chromeos')"> On 2013/03/31 01:16:03, sky wrote: > Should this be ash and not chromeos? I assume the code will run win-ash, right? Yes, removed the ifdef. https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:81: std::string id_text_; On 2013/03/31 01:16:03, sky wrote: > Make all of these const if they can be. Done. https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:91: // Notifications only on Chrome OS. On 2013/03/31 01:16:03, sky wrote: > Why is the notification only for chromeos? Removed. https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:116: Profile* profile = ProfileManager::GetDefaultProfileOrOffTheRecord(); On 2013/03/31 01:16:03, sky wrote: > Can ScreenshotTaker so that we minimize uses of GetDefaultPro... Changed: ScreenshotTake takes profile in ctor. https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:120: if (ScreenshotTaker::GetObserverForTest()) { On 2013/03/31 01:16:03, sky wrote: > Why is this in the ifdef? Removed. https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:33: virtual ~ScreenshotTakerObserver() {} On 2013/03/31 01:16:03, sky wrote: > Move this to protected to make it clear it's not owned by ScreenShotTaker. Done. https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:54: static void SetObserverForTest(ScreenshotTakerObserver* observer); On 2013/03/31 01:16:03, sky wrote: > If this is really only for a test, how about moving it to a separate header and > having a friend set it. That said, it's rather unusual to have an observer only > for tests. Is there a reason this needs to be SetObserverForTest rather than > just set_observer? Made it general; used standard Observer list. https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker_browsertest.cc (right): https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_browsertest.cc:5: #if defined(OS_CHROMEOS) On 2013/03/31 01:16:03, sky wrote: > This isn't the right way to have a chromeos specific browsertest. Again, I think > this should be for windows too. If not, the right way is to name appropriately > or excluded in gyp. Removed ifdef Chrome OS https://codereview.chromium.org/13105002/diff/48001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_browsertest.cc:22: class ScreenshotTakerTest : public InProcessBrowserTest, On 2013/03/31 01:16:03, sky wrote: > Why does this need to be a browser test instead of a unit test? Changed to unit test
Almost there. I like this better as a unit test. Sky can comment more on the use of profile, but given that screenshots are triggered from down inside ash I think it would be hard to wire the profile into where the screenshots are triggered. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:57: virtual void HandleTakeScreenshotForAllRootWindows() OVERRIDE; I wish there was a way to pass the profile in here, but I don't see one. :-( https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:72: void OverrideScreenshotDirectory(const base::FilePath& directory); We usually call these SetScreenshotDirectoryForTest(), etc. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:87: friend class ash::test::ScreenshotTakerTest; friends go at top of private section https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker_unittest.cc (right): https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:42: screenshot_taker_->RemoveObserver(this); Things you built in SetUp() should be cleaned up in TearDown(). https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:53: profile_.reset(new TestingProfile()); Can profile_, screenshot_taker_ and directory be moved into the TakeScreenshot test below? This might allow them to be local variables and eliminate some memory management. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:54: base::ScopedTempDir directory; I think this directory will be deleted when SetUp() completes, which is not what you want. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:89: base::ScopedTempDir temp_dir_; Is this used?
https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:48: class ScreenshotTakerNotificationDelegate : public NotificationDelegate { nit: add description of class. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:58: virtual void Display() OVERRIDE {} Generally we prefix overrides with the name of the class they're from and no spaces (see screenshot_taker.h for example). https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:91: base::Bind(&ScreenshotTaker::ShowNotification, Won't this crash if screenshot_taker is destroyed? https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:50: { { on previous line. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:57: virtual void HandleTakeScreenshotForAllRootWindows() OVERRIDE; On 2013/04/02 22:47:18, James Cook (Chromium) wrote: > I wish there was a way to pass the profile in here, but I don't see one. :-( This is invoked from ash, which doesn't know about profiles. Taking a Profile in the constructor seems best to me.
https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:48: class ScreenshotTakerNotificationDelegate : public NotificationDelegate { On 2013/04/03 04:24:46, sky wrote: > nit: add description of class. Done. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:58: virtual void Display() OVERRIDE {} On 2013/04/03 04:24:46, sky wrote: > Generally we prefix overrides with the name of the class they're from and no > spaces (see screenshot_taker.h for example). Done. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:91: base::Bind(&ScreenshotTaker::ShowNotification, On 2013/04/03 04:24:46, sky wrote: > Won't this crash if screenshot_taker is destroyed? Good point. Added one more level of indirection ;) ... to check that the weak ptr is still good. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:50: { On 2013/04/03 04:24:46, sky wrote: > { on previous line. old habits die hard;) Done. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:72: void OverrideScreenshotDirectory(const base::FilePath& directory); On 2013/04/02 22:47:18, James Cook (Chromium) wrote: > We usually call these SetScreenshotDirectoryForTest(), etc. Done. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:87: friend class ash::test::ScreenshotTakerTest; On 2013/04/02 22:47:18, James Cook (Chromium) wrote: > friends go at top of private section Done. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker_unittest.cc (right): https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:42: screenshot_taker_->RemoveObserver(this); On 2013/04/02 22:47:18, James Cook (Chromium) wrote: > Things you built in SetUp() should be cleaned up in TearDown(). Reorganized so overridden SetUp and TearDown not needed. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:53: profile_.reset(new TestingProfile()); On 2013/04/02 22:47:18, James Cook (Chromium) wrote: > Can profile_, screenshot_taker_ and directory be moved into the TakeScreenshot > test below? This might allow them to be local variables and eliminate some > memory management. Good Point. done. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:54: base::ScopedTempDir directory; On 2013/04/02 22:47:18, James Cook (Chromium) wrote: > I think this directory will be deleted when SetUp() completes, which is not what > you want. Of course, I can't believe I did this. Reorganized. Done. https://codereview.chromium.org/13105002/diff/58001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:89: base::ScopedTempDir temp_dir_; On 2013/04/02 22:47:18, James Cook (Chromium) wrote: > Is this used? Nope, removed.
An issue arose. I believe the action on clicking the notification, namely to open the filemanager with the screenshot selected, is for chrome os only. Link errors for win7_aura. I'm using: chrome/browser/chromeos/extensions/file_manager_util.cc Proposal: Make the click action depending on $ifdef OS_CHROMEOS, i.e. for win_aura do nothing and change the notification text, "Click to view.", to something else, like "Done." WDYT?
For win7_aura I would be OK with either "Done" or just the empty string. I would suggest leaving a TODO about Windows support. https://codereview.chromium.org/13105002/diff/68001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker_unittest.cc (right): https://codereview.chromium.org/13105002/diff/68001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:60: ScreenshotTaker& screenshot_taker, We usually pass pointers instead of non-const references. https://codereview.chromium.org/13105002/diff/68001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:65: ScreenshotTaker& screenshot_taker, ditto
Fixed Windows https://codereview.chromium.org/13105002/diff/68001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker_unittest.cc (right): https://codereview.chromium.org/13105002/diff/68001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:60: ScreenshotTaker& screenshot_taker, On 2013/04/03 19:50:19, James Cook (Chromium) wrote: > We usually pass pointers instead of non-const references. Done. https://codereview.chromium.org/13105002/diff/68001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:65: ScreenshotTaker& screenshot_taker, On 2013/04/03 19:50:19, James Cook (Chromium) wrote: > ditto Done.
LGTM with nit https://codereview.chromium.org/13105002/diff/77001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker_unittest.cc (right): https://codereview.chromium.org/13105002/diff/77001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:62: ASSERT_TRUE(screenshot_taker); nit: You don't need to assert these. Chromium-style is to not CHECK/DCHECK/ASSERT pointers if you immediately dereference them. A crash on the dereference line is just as good.
Oops... fixed #ifdef chrome os for windows https://codereview.chromium.org/13105002/diff/77001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker_unittest.cc (right): https://codereview.chromium.org/13105002/diff/77001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:62: ASSERT_TRUE(screenshot_taker); On 2013/04/03 21:25:04, James Cook (Chromium) wrote: > nit: You don't need to assert these. Chromium-style is to not > CHECK/DCHECK/ASSERT pointers if you immediately dereference them. A crash on the > dereference line is just as good. Done.
https://codereview.chromium.org/13105002/diff/83001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/83001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:89: void ShowNotificationCaller(base::WeakPtr<ScreenshotTaker> screenshot_taker, Is there a reason you went with WeakPtr here rather than WeakPtrFactory? Using WeakPtrFactory is more common in this situation as it means you don't need all the if checks, it's handled for you. https://codereview.chromium.org/13105002/diff/83001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/83001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:43: Result screenshot_result, const base::FilePath& screenshot_path) = 0; nit: when you wrap put each param on its own line.
Thanks again. I ifdef-ed out sending the Notification on Windows. Otherwise it fails with NOTIMPLEMENTED for win7_aura. https://codereview.chromium.org/13105002/diff/83001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/83001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:89: void ShowNotificationCaller(base::WeakPtr<ScreenshotTaker> screenshot_taker, On 2013/04/03 23:16:09, sky wrote: > Is there a reason you went with WeakPtr here rather than WeakPtrFactory? Using > WeakPtrFactory is more common in this situation as it means you don't need all > the if checks, it's handled for you. Using Callback instead. Done. https://codereview.chromium.org/13105002/diff/83001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.h (right): https://codereview.chromium.org/13105002/diff/83001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.h:43: Result screenshot_result, const base::FilePath& screenshot_path) = 0; On 2013/04/03 23:16:09, sky wrote: > nit: when you wrap put each param on its own line. Done.
Despite my earlier L G T M please don't commit this until you talk to sky. I think this code may crash if the ScreenshotTaker is deleted while there is a task/callback pending. I think there is a way to use WeakPtrFactory with posted tasks to automatically not run the task if the underlying object is deleted, but I am not familiar enough with that pattern to advise you without looking it up.
Never mind. This is why I shouldn't review code after hours. base::Bind() handles weak pointers, as you are using it. I would still suggest letting sky take one last look at this before committing.
LGTM https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:93: void PostShowNotification(const ShowNotificationCallback& callback, Calling this function takes as many lines as the implementation (minus the DCHECK). I would be inclined to inline it, but I'll leave that to you. https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker_unittest.cc (right): https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:14: #include "chrome/browser/ui/ash/screenshot_taker.h" This should be your first include (just like you have it in screenshot_taker.cc). https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:36: CommandLine::ForCurrentProcess()->AppendSwitch( I'm pretty sure we reset switches like this between tests, but please verify that. https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:38: ui_thread_.reset( Most tests declare ui_thread_ like this by value, but this certainly works too. https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:102: ash::Shell::GetPrimaryRootWindow(), gfx::Rect(0,0,100,100)); 0,0,100,100 -> 0, 0, 100, 100
https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker.cc (right): https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker.cc:93: void PostShowNotification(const ShowNotificationCallback& callback, On 2013/04/04 15:38:31, sky wrote: > Calling this function takes as many lines as the implementation (minus the > DCHECK). I would be inclined to inline it, but I'll leave that to you. I inlined it. Done. https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/scr... File chrome/browser/ui/ash/screenshot_taker_unittest.cc (right): https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:14: #include "chrome/browser/ui/ash/screenshot_taker.h" On 2013/04/04 15:38:31, sky wrote: > This should be your first include (just like you have it in > screenshot_taker.cc). Done. https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:36: CommandLine::ForCurrentProcess()->AppendSwitch( On 2013/04/04 15:38:31, sky wrote: > I'm pretty sure we reset switches like this between tests, but please verify > that. Yup, the command line gets reset between tests. But I don't need to set the switch... Removed it. Done. https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:38: ui_thread_.reset( On 2013/04/04 15:38:31, sky wrote: > Most tests declare ui_thread_ like this by value, but this certainly works too. Done. https://codereview.chromium.org/13105002/diff/92001/chrome/browser/ui/ash/scr... chrome/browser/ui/ash/screenshot_taker_unittest.cc:102: ash::Shell::GetPrimaryRootWindow(), gfx::Rect(0,0,100,100)); On 2013/04/04 15:38:31, sky wrote: > 0,0,100,100 -> 0, 0, 100, 100 Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13105002/100001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13105002/100001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13105002/100001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13105002/100001
Message was sent while issue was closed.
Change committed as 192472 |