Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2795693005:
Taking screenshot of current focused window. (Closed)
|
Created:
3 years, 8 months ago by Muyuan Modified:
3 years, 8 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionTaking screenshot of current focused window.
BUG=704684
BUG=b:35643583
TEST=screenshot sent to voice interaction session
in ARC.
Review-Url: https://codereview.chromium.org/2795693005
Cr-Commit-Position: refs/heads/master@{#461950}
Committed: https://chromium.googlesource.com/chromium/src/+/bac4a9787174ce5e4bb7c4dded78f60550d667ba
Patch Set 1 #
Total comments: 4
Patch Set 2 : address review comment #
Total comments: 10
Patch Set 3 : address review comments #
Total comments: 4
Patch Set 4 : check null for screenshot data #Messages
Total messages: 28 (14 generated)
Description was changed from ========== [WIP] taking screenshot of current focused window. BUG=704684 BUG=b:35643583 ========== to ========== Taking screenshot of current focused window. BUG=704684 BUG=b:35643583 TEST=screenshot sent to voice interaction session in ARC. ==========
muyuanli@chromium.org changed reviewers: + dcheng@chromium.org, lhchavez@chromium.org, xiyuan@chromium.org
https://codereview.chromium.org/2795693005/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2795693005/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:36: std::vector<uint8_t> result(png_data->front(), Do you need to check if |png_data->front()| is nullptr?
https://codereview.chromium.org/2795693005/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2795693005/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:104: window, window->bounds(), window->bounds() is not correct here. This should be a source rect in the underlying layer's (i.e. the window's for this call) coordinates. But bounds() is relative to the window's parent. Think we need to adjust this to gfx::Rect(window->bounds().size()) to make it relative to the window layer.
The CQ bit was checked by muyuanli@chromium.org to run a CQ dry run
https://codereview.chromium.org/2795693005/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2795693005/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:36: std::vector<uint8_t> result(png_data->front(), On 2017/04/04 15:54:59, Luis Héctor Chávez wrote: > Do you need to check if |png_data->front()| is nullptr? Done. https://codereview.chromium.org/2795693005/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:104: window, window->bounds(), On 2017/04/04 16:08:13, xiyuan wrote: > window->bounds() is not correct here. This should be a source rect in the > underlying layer's (i.e. the window's for this call) coordinates. But bounds() > is relative to the window's parent. > > Think we need to adjust this to gfx::Rect(window->bounds().size()) to make it > relative to the window layer. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2795693005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2795693005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:19: #include "chrome/browser/ui/tabs/tab_strip_model.h" nit: Are the includes in line 16-19 used ? https://codereview.chromium.org/2795693005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:100: LOG(ERROR) << "Getting screenshot"; nit: Is this a debugging log? https://codereview.chromium.org/2795693005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:111: content::BrowserThread::FILE_USER_BLOCKING), nit: I was about to ask to add browser_thread.h then found that this way of using FILE_USER_BLOCKING is deprecated [1]. The comment says the preferred way is to: base::CreateSequencedTaskRunnerWithTraits( TaskTraits().MayBlock() .WithPriority(TaskPriority::USER_BLOCKING)) and don't forget to include "base/task_scheduler/post_task.h". [1]: https://cs.chromium.org/chromium/src/content/public/browser/browser_thread.h?...
mojo security review lgtm, but two thoughts https://codereview.chromium.org/2795693005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2795693005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:41: png_data->front() + png_data->size()); Btw, this is pretty sad: we actually already have a vector internally in the RefCountedMemory (as it's always a RefCountedBytes), so having to copy this here is unfortunate. I'll throw together a patch to fix this I guess. https://codereview.chromium.org/2795693005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:42: callback.Run(std::move(result)); I'm not sure if this std::move() is actually doing anything -- I suspect the argument is passed by const ref. So maybe just remove it.
https://codereview.chromium.org/2795693005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2795693005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:19: #include "chrome/browser/ui/tabs/tab_strip_model.h" On 2017/04/04 19:36:12, xiyuan wrote: > nit: Are the includes in line 16-19 used ? Done. https://codereview.chromium.org/2795693005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:41: png_data->front() + png_data->size()); On 2017/04/04 20:17:41, dcheng wrote: > Btw, this is pretty sad: we actually already have a vector internally in the > RefCountedMemory (as it's always a RefCountedBytes), so having to copy this here > is unfortunate. I'll throw together a patch to fix this I guess. Acknowledged. https://codereview.chromium.org/2795693005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:42: callback.Run(std::move(result)); On 2017/04/04 20:17:41, dcheng wrote: > I'm not sure if this std::move() is actually doing anything -- I suspect the > argument is passed by const ref. So maybe just remove it. Done. https://codereview.chromium.org/2795693005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:100: LOG(ERROR) << "Getting screenshot"; On 2017/04/04 19:36:12, xiyuan wrote: > nit: Is this a debugging log? Done. https://codereview.chromium.org/2795693005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:111: content::BrowserThread::FILE_USER_BLOCKING), On 2017/04/04 19:36:12, xiyuan wrote: > nit: I was about to ask to add browser_thread.h then found that this way of > using FILE_USER_BLOCKING is deprecated [1]. The comment says the preferred way > is to: > base::CreateSequencedTaskRunnerWithTraits( > TaskTraits().MayBlock() > .WithPriority(TaskPriority::USER_BLOCKING)) > > and don't forget to include "base/task_scheduler/post_task.h". > > [1]: > https://cs.chromium.org/chromium/src/content/public/browser/browser_thread.h?... Done.
https://codereview.chromium.org/2795693005/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2795693005/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:33: if (png_data->size() == 0) { Maybe mention that |png_data->front()| can be nullptr when |png_data->size() == 0|, otherwise it looks redundant (or do the actual nullptr-check I suggested).
lgtm
https://codereview.chromium.org/2795693005/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2795693005/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:33: if (png_data->size() == 0) { On 2017/04/04 20:44:56, Luis Héctor Chávez wrote: > Maybe mention that |png_data->front()| can be nullptr when |png_data->size() == > 0|, otherwise it looks redundant (or do the actual nullptr-check I suggested). Hmm.. I'm not sure if this could really be null.. judging by the code: https://cs.chromium.org/chromium/src/ui/snapshot/screenshot_grabber.cc?gsn=Re... It seems that grabbing screenshot could not fail?
https://codereview.chromium.org/2795693005/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2795693005/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:33: if (png_data->size() == 0) { On 2017/04/04 23:01:13, Muyuan wrote: > On 2017/04/04 20:44:56, Luis Héctor Chávez wrote: > > Maybe mention that |png_data->front()| can be nullptr when |png_data->size() > == > > 0|, otherwise it looks redundant (or do the actual nullptr-check I suggested). > > Hmm.. I'm not sure if this could really be null.. judging by the code: > https://cs.chromium.org/chromium/src/ui/snapshot/screenshot_grabber.cc?gsn=Re... > > > It seems that grabbing screenshot could not fail? It most definitely can: https://cs.chromium.org/chromium/src/ui/snapshot/screenshot_grabber.cc?l=208 This is being called from https://cs.chromium.org/chromium/src/ui/snapshot/screenshot_grabber.cc?l=171 , which is exactly the same thing you're doing.
https://codereview.chromium.org/2795693005/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2795693005/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:33: if (png_data->size() == 0) { On 2017/04/04 23:17:00, Luis Héctor Chávez wrote: > On 2017/04/04 23:01:13, Muyuan wrote: > > On 2017/04/04 20:44:56, Luis Héctor Chávez wrote: > > > Maybe mention that |png_data->front()| can be nullptr when |png_data->size() > > == > > > 0|, otherwise it looks redundant (or do the actual nullptr-check I > suggested). > > > > Hmm.. I'm not sure if this could really be null.. judging by the code: > > > https://cs.chromium.org/chromium/src/ui/snapshot/screenshot_grabber.cc?gsn=Re... > > > > > > It seems that grabbing screenshot could not fail? > > It most definitely can: > https://cs.chromium.org/chromium/src/ui/snapshot/screenshot_grabber.cc?l=208 > This is being called from > https://cs.chromium.org/chromium/src/ui/snapshot/screenshot_grabber.cc?l=171 , > which is exactly the same thing you're doing. Acknowledged.
The CQ bit was checked by muyuanli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm thanks!
The CQ bit was checked by muyuanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2795693005/#ps60001 (title: "check null for screenshot data")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491359635391200,
"parent_rev": "c112e8db945886afb876c3a116aa3b7dabb149dd", "commit_rev":
"bac4a9787174ce5e4bb7c4dded78f60550d667ba"}
Message was sent while issue was closed.
Description was changed from ========== Taking screenshot of current focused window. BUG=704684 BUG=b:35643583 TEST=screenshot sent to voice interaction session in ARC. ========== to ========== Taking screenshot of current focused window. BUG=704684 BUG=b:35643583 TEST=screenshot sent to voice interaction session in ARC. Review-Url: https://codereview.chromium.org/2795693005 Cr-Commit-Position: refs/heads/master@{#461950} Committed: https://chromium.googlesource.com/chromium/src/+/bac4a9787174ce5e4bb7c4dded78... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/bac4a9787174ce5e4bb7c4dded78... |
