Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9430041:
Makes the visual feedback for taking screenshot. (Closed)
|
Created:
8 years, 10 months ago by Jun Mukai Modified:
8 years, 9 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionMakes the visual feedback for taking screenshot.
The original implementation is in platform/window_manager/screenshot.cc
of CrOS source tree.
BUG=113467
TEST=manually
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124203
Patch Set 1 #
Total comments: 12
Patch Set 2 : use ui::Layer instead of custom view #
Total comments: 12
Patch Set 3 : Fix based on comments #
Total comments: 8
Patch Set 4 : add visual_feedback_layer_ as a scoped_ptr #
Total comments: 4
Patch Set 5 : Fix code based on comments #Patch Set 6 : rebase #
Messages
Total messages: 13 (0 generated)
http://codereview.chromium.org/9430041/diff/1/chrome/browser/ui/views/aura/sc... File chrome/browser/ui/views/aura/screenshot_taker.cc (right): http://codereview.chromium.org/9430041/diff/1/chrome/browser/ui/views/aura/sc... chrome/browser/ui/views/aura/screenshot_taker.cc:16: #include "ash/shell.h" alphabetize these http://codereview.chromium.org/9430041/diff/1/chrome/browser/ui/views/aura/sc... chrome/browser/ui/views/aura/screenshot_taker.cc:79: // The view of making visual feedback for screenshot, i.e.: flashing the screen. // Flashes the screen to provide visual feedback that a screenshot has been taken. http://codereview.chromium.org/9430041/diff/1/chrome/browser/ui/views/aura/sc... chrome/browser/ui/views/aura/screenshot_taker.cc:80: class VisualFeedbackView : public views::WidgetDelegateView { not sure how much to care about this, but it'd be more efficient to just create a ui::Layer of type LAYER_SOLID_COLOR http://codereview.chromium.org/9430041/diff/1/chrome/browser/ui/views/aura/sc... chrome/browser/ui/views/aura/screenshot_taker.cc:91: VisualFeedbackView::VisualFeedbackView() { define this inline in the class declaration? http://codereview.chromium.org/9430041/diff/1/chrome/browser/ui/views/aura/sc... chrome/browser/ui/views/aura/screenshot_taker.cc:95: // All white. delete unnecessary comment http://codereview.chromium.org/9430041/diff/1/chrome/browser/ui/views/aura/sc... chrome/browser/ui/views/aura/screenshot_taker.cc:108: void MakeVisualFeedback(const gfx::Rect& rect) { nit: s/MakeVisualFeedback/DisplayVisualFeedback/
https://chromiumcodereview.appspot.com/9430041/diff/1/chrome/browser/ui/views... File chrome/browser/ui/views/aura/screenshot_taker.cc (right): https://chromiumcodereview.appspot.com/9430041/diff/1/chrome/browser/ui/views... chrome/browser/ui/views/aura/screenshot_taker.cc:16: #include "ash/shell.h" On 2012/02/22 17:22:06, Daniel Erat wrote: > alphabetize these Done. https://chromiumcodereview.appspot.com/9430041/diff/1/chrome/browser/ui/views... chrome/browser/ui/views/aura/screenshot_taker.cc:79: // The view of making visual feedback for screenshot, i.e.: flashing the screen. On 2012/02/22 17:22:06, Daniel Erat wrote: > // Flashes the screen to provide visual feedback that a screenshot has been > taken. Done. https://chromiumcodereview.appspot.com/9430041/diff/1/chrome/browser/ui/views... chrome/browser/ui/views/aura/screenshot_taker.cc:80: class VisualFeedbackView : public views::WidgetDelegateView { On 2012/02/22 17:22:06, Daniel Erat wrote: > not sure how much to care about this, but it'd be more efficient to just create > a ui::Layer of type LAYER_SOLID_COLOR Done. https://chromiumcodereview.appspot.com/9430041/diff/1/chrome/browser/ui/views... chrome/browser/ui/views/aura/screenshot_taker.cc:91: VisualFeedbackView::VisualFeedbackView() { On 2012/02/22 17:22:06, Daniel Erat wrote: > define this inline in the class declaration? removed the class itself and use ui::Layer instead. https://chromiumcodereview.appspot.com/9430041/diff/1/chrome/browser/ui/views... chrome/browser/ui/views/aura/screenshot_taker.cc:95: // All white. On 2012/02/22 17:22:06, Daniel Erat wrote: > delete unnecessary comment removed the class itself. https://chromiumcodereview.appspot.com/9430041/diff/1/chrome/browser/ui/views... chrome/browser/ui/views/aura/screenshot_taker.cc:108: void MakeVisualFeedback(const gfx::Rect& rect) { On 2012/02/22 17:22:06, Daniel Erat wrote: > nit: s/MakeVisualFeedback/DisplayVisualFeedback/ Done.
https://chromiumcodereview.appspot.com/9430041/diff/6001/chrome/browser/ui/vi... File chrome/browser/ui/views/aura/screenshot_taker.cc (right): https://chromiumcodereview.appspot.com/9430041/diff/6001/chrome/browser/ui/vi... chrome/browser/ui/views/aura/screenshot_taker.cc:24: #include "ui/views/widget/widget_delegate.h" you don't need these includes anymore; you should include ui/gfx/compositor/layer.h instead, though https://chromiumcodereview.appspot.com/9430041/diff/6001/chrome/browser/ui/vi... chrome/browser/ui/views/aura/screenshot_taker.cc:72: // How opaque should the window that we flash onscreen to provide visual nit: s/window/layer here and in the const variable names https://chromiumcodereview.appspot.com/9430041/diff/6001/chrome/browser/ui/vi... chrome/browser/ui/views/aura/screenshot_taker.cc:79: void CleanupFeedbackView(ui::Layer* layer) { s/CleanupFeedbackView/CloseFeedbackLayer/ ? https://chromiumcodereview.appspot.com/9430041/diff/6001/chrome/browser/ui/vi... chrome/browser/ui/views/aura/screenshot_taker.cc:82: MessageLoopForUI::current()->DeleteSoon(FROM_HERE, layer); you can delete it immediately instead of setting it invisible and posting this task, i think https://chromiumcodereview.appspot.com/9430041/diff/6001/chrome/browser/ui/vi... chrome/browser/ui/views/aura/screenshot_taker.cc:88: ui::Layer* layer = new ui::Layer(ui::Layer::LAYER_SOLID_COLOR); put this in a scoped_ptr in ScreenshotTaker so it doesn't get leaked if the object is deleted before the delayed task runs? https://chromiumcodereview.appspot.com/9430041/diff/6001/chrome/browser/ui/vi... chrome/browser/ui/views/aura/screenshot_taker.cc:95: parent->StackAtTop(layer); i guess it doesn't hurt to be explicit, but i'm pretty sure that children are already added at the top of the stack
http://codereview.chromium.org/9430041/diff/6001/chrome/browser/ui/views/aura... File chrome/browser/ui/views/aura/screenshot_taker.cc (right): http://codereview.chromium.org/9430041/diff/6001/chrome/browser/ui/views/aura... chrome/browser/ui/views/aura/screenshot_taker.cc:24: #include "ui/views/widget/widget_delegate.h" On 2012/02/23 14:32:13, Daniel Erat wrote: > you don't need these includes anymore; you should include > ui/gfx/compositor/layer.h instead, though Done. http://codereview.chromium.org/9430041/diff/6001/chrome/browser/ui/views/aura... chrome/browser/ui/views/aura/screenshot_taker.cc:72: // How opaque should the window that we flash onscreen to provide visual On 2012/02/23 14:32:13, Daniel Erat wrote: > nit: s/window/layer here and in the const variable names Done. http://codereview.chromium.org/9430041/diff/6001/chrome/browser/ui/views/aura... chrome/browser/ui/views/aura/screenshot_taker.cc:79: void CleanupFeedbackView(ui::Layer* layer) { On 2012/02/23 14:32:13, Daniel Erat wrote: > s/CleanupFeedbackView/CloseFeedbackLayer/ ? Done. http://codereview.chromium.org/9430041/diff/6001/chrome/browser/ui/views/aura... chrome/browser/ui/views/aura/screenshot_taker.cc:82: MessageLoopForUI::current()->DeleteSoon(FROM_HERE, layer); On 2012/02/23 14:32:13, Daniel Erat wrote: > you can delete it immediately instead of setting it invisible and posting this > task, i think Done. http://codereview.chromium.org/9430041/diff/6001/chrome/browser/ui/views/aura... chrome/browser/ui/views/aura/screenshot_taker.cc:88: ui::Layer* layer = new ui::Layer(ui::Layer::LAYER_SOLID_COLOR); On 2012/02/23 14:32:13, Daniel Erat wrote: > put this in a scoped_ptr in ScreenshotTaker so it doesn't get leaked if the > object is deleted before the delayed task runs? Done. http://codereview.chromium.org/9430041/diff/6001/chrome/browser/ui/views/aura... chrome/browser/ui/views/aura/screenshot_taker.cc:95: parent->StackAtTop(layer); On 2012/02/23 14:32:13, Daniel Erat wrote: > i guess it doesn't hurt to be explicit, but i'm pretty sure that children are > already added at the top of the stack Done.
http://codereview.chromium.org/9430041/diff/8002/chrome/browser/ui/views/aura... File chrome/browser/ui/views/aura/screenshot_taker.cc (right): http://codereview.chromium.org/9430041/diff/8002/chrome/browser/ui/views/aura... chrome/browser/ui/views/aura/screenshot_taker.cc:72: const unsigned char kVisualFeedbackWindowOpacity = 64; nit: s/Window/Layer/ in variable name http://codereview.chromium.org/9430041/diff/8002/chrome/browser/ui/views/aura... chrome/browser/ui/views/aura/screenshot_taker.cc:75: const uint64_t kVisualFeedbackWindowDisplayTimeMs = 100; s/Window/Layer/ http://codereview.chromium.org/9430041/diff/8002/chrome/browser/ui/views/aura... chrome/browser/ui/views/aura/screenshot_taker.cc:79: layer->SetVisible(false); nit: unneeded; deleting it makes it invisible :-) http://codereview.chromium.org/9430041/diff/8002/chrome/browser/ui/views/aura... chrome/browser/ui/views/aura/screenshot_taker.cc:97: base::Bind(&CloseFeedbackLayer, base::Unretained(layer.release())), i meant that you could make these functions instead be private methods of ScreenshotTaker. create a scoped_ptr<ui::Layer> named something like ScreenshotTaker::visual_feedback_layer_ and make CloseFeedbackLayer() take no parameters and just call visual_feedback_layer_.reset();
http://codereview.chromium.org/9430041/diff/8002/chrome/browser/ui/views/aura... File chrome/browser/ui/views/aura/screenshot_taker.cc (right): http://codereview.chromium.org/9430041/diff/8002/chrome/browser/ui/views/aura... chrome/browser/ui/views/aura/screenshot_taker.cc:72: const unsigned char kVisualFeedbackWindowOpacity = 64; On 2012/02/24 00:48:46, Daniel Erat wrote: > nit: s/Window/Layer/ in variable name Done. http://codereview.chromium.org/9430041/diff/8002/chrome/browser/ui/views/aura... chrome/browser/ui/views/aura/screenshot_taker.cc:75: const uint64_t kVisualFeedbackWindowDisplayTimeMs = 100; On 2012/02/24 00:48:46, Daniel Erat wrote: > s/Window/Layer/ Done. http://codereview.chromium.org/9430041/diff/8002/chrome/browser/ui/views/aura... chrome/browser/ui/views/aura/screenshot_taker.cc:79: layer->SetVisible(false); On 2012/02/24 00:48:46, Daniel Erat wrote: > nit: unneeded; deleting it makes it invisible :-) Done. http://codereview.chromium.org/9430041/diff/8002/chrome/browser/ui/views/aura... chrome/browser/ui/views/aura/screenshot_taker.cc:97: base::Bind(&CloseFeedbackLayer, base::Unretained(layer.release())), On 2012/02/24 00:48:46, Daniel Erat wrote: > i meant that you could make these functions instead be private methods of > ScreenshotTaker. create a scoped_ptr<ui::Layer> named something like > ScreenshotTaker::visual_feedback_layer_ and make CloseFeedbackLayer() take no > parameters and just call visual_feedback_layer_.reset(); Ah, thank you for the explanation. Done.
LGTM with nits. Thanks! http://codereview.chromium.org/9430041/diff/11001/chrome/browser/ui/views/aur... File chrome/browser/ui/views/aura/screenshot_taker.cc (right): http://codereview.chromium.org/9430041/diff/11001/chrome/browser/ui/views/aur... chrome/browser/ui/views/aura/screenshot_taker.cc:74: const uint64_t kVisualFeedbackLayerDisplayTimeMs = 100; nit: s/uint64_t/int64/ to match the type from PostDelayedTask() and FromMilliseconds() http://codereview.chromium.org/9430041/diff/11001/chrome/browser/ui/views/aur... chrome/browser/ui/views/aura/screenshot_taker.cc:125: kVisualFeedbackLayerDisplayTimeMs); nit: mind changing this to instead be base::TimeDelta::FromMilliseconds(kVisualFeedbackLayerDisplayTimeMs)?
http://codereview.chromium.org/9430041/diff/11001/chrome/browser/ui/views/aur... File chrome/browser/ui/views/aura/screenshot_taker.cc (right): http://codereview.chromium.org/9430041/diff/11001/chrome/browser/ui/views/aur... chrome/browser/ui/views/aura/screenshot_taker.cc:74: const uint64_t kVisualFeedbackLayerDisplayTimeMs = 100; On 2012/02/24 17:23:39, Daniel Erat wrote: > nit: s/uint64_t/int64/ to match the type from PostDelayedTask() and > FromMilliseconds() Done. http://codereview.chromium.org/9430041/diff/11001/chrome/browser/ui/views/aur... chrome/browser/ui/views/aura/screenshot_taker.cc:125: kVisualFeedbackLayerDisplayTimeMs); On 2012/02/24 17:23:39, Daniel Erat wrote: > nit: mind changing this to instead be > base::TimeDelta::FromMilliseconds(kVisualFeedbackLayerDisplayTimeMs)? Done.
Ben, could you take a look at this CL too as the owner?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/9430041/15001
Change committed as 124203 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
