Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2854543002:
Block incognito browser windows for voice interaction. (Closed)
|
Created:
3 years, 7 months ago by Muyuan Modified:
3 years, 7 months ago CC:
chromium-reviews, danakj+watch_chromium.org, davemoore+watch_chromium.org, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, jbauman+watch_chromium.org, kalyank, lhchavez+watch_chromium.org, oshima+watch_chromium.org, piman+watch_chromium.org, victorhsieh+watch_chromium.org, Ian Vollick, yusukes+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRecreates the layer hierarchy modulo all layers that
belong to incognito windows, replacing them with a solid black
layer.
BUG=b/37755798
TEST=wm_unittests --gtest_filter=WindowUtilTest.RecreateLayersWithClosure
Review-Url: https://codereview.chromium.org/2854543002
Cr-Commit-Position: refs/heads/master@{#472678}
Committed: https://chromium.googlesource.com/chromium/src/+/f134db33d0941e7d89ab862455bf17e0b490fc88
Patch Set 1 #Patch Set 2 : replace layer instead of tweaking layer properties #Patch Set 3 : syntax optimization #Patch Set 4 : clean up unnecessary includes #
Total comments: 11
Patch Set 5 : address review comments #
Total comments: 6
Patch Set 6 : address review comment #
Total comments: 2
Patch Set 7 : address review comment #Patch Set 8 : minor modification to snapshot_aura #
Total comments: 10
Patch Set 9 : WIP: Block incognito browser windows for voice interaction. #
Total comments: 4
Patch Set 10 : address review comments #
Messages
Total messages: 39 (16 generated)
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...
Description was changed from ========== WIP: Block incognito browser windows for voice interaction. NOTE: Current CL is not ready for commit for: 1. approach review 2. dirty hacks 3. fix include violations BUG=b/37755798 ========== to ========== This approach recreates the layer hierarchy modulo all layers that belong to incognito windows, replacing them with a solid black layer. BUG=b/37755798 ==========
Description was changed from ========== This approach recreates the layer hierarchy modulo all layers that belong to incognito windows, replacing them with a solid black layer. BUG=b/37755798 ========== to ========== Recreates the layer hierarchy modulo all layers that belong to incognito windows, replacing them with a solid black layer. BUG=b/37755798 ==========
muyuanli@chromium.org changed reviewers: + lhchavez@chromium.org, reveman@chromium.org, sky@chromium.org
reveman@: could you please take a look to see if this whole idea looks good to you? lhchavez@ and sky@: everything plz :p.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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.
Isn't this going to be super expensive?
https://codereview.chromium.org/2854543002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2854543002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:56: auto blocked_layers = LayerSet(); Why not LayerSet blocked_layers; ? It's shorter and more readable. https://codereview.chromium.org/2854543002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:58: for (auto* browser : *browser_list) { nit: BrowserList itself does for (auto* browser : *BrowserList::GetInstance()) https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_list.cc?l=31 https://codereview.chromium.org/2854543002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:82: if (layer_tree_owner) { nit: use the guard clause pattern: if (!layer_tree_owner) return layer_tree_owner; // rest https://codereview.chromium.org/2854543002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:190: old_layer_owner_ = BlockIncognitoWindows(window); It doesn't seem like you *need* to store this as a member. How about sending it with base::Passed(...) to ArcVoiceInteractionFrameworkService::EncodeAndReturnImage? https://codereview.chromium.org/2854543002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:191: DCHECK(old_layer_owner_); I don't think you can DCHECK here since it seems like it's legit that you get a nullptr from BlockIncognitoWindows. If it's not legit, you should add more comments/DCHECKs there mentioning why it should "never happen". https://codereview.chromium.org/2854543002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:258: base::TaskTraits({base::MayBlock(), base::TaskPriority::USER_BLOCKING}), nit: base::TaskTraits{base::MayBlock(), base::TaskPriority::USER_BLOCKING} otherwise you're building a base::TaskTraits with the {...}, and then building another one with base::TaskTraits(...) and passing the original in.
On 2017/05/15 13:41:20, sky wrote: > Isn't this going to be super expensive? It shouldn't.. it only copies the layer meta structure not the textures the layer owns. The screen rotation animation is done in this way as well: https://cs.chromium.org/chromium/src/ash/rotator/screen_rotation_animator.cc?...
On Mon, May 15, 2017 at 1:49 PM, <muyuanli@chromium.org> wrote: > On 2017/05/15 13:41:20, sky wrote: > > Isn't this going to be super expensive? > > It shouldn't.. it only copies the layer meta structure not the textures the > layer owns. > Is this true? I'd expect any PictureLayers to require being recorded, rastered, uploaded, using new textures. Can you verify this? > The screen rotation animation is done in this way as well: > > https://cs.chromium.org/chromium/src/ash/rotator/ > screen_rotation_animator.cc?rcl=40f898c48cccf321bc48fe2f58b03a > 84b167a4e5&l=323 > Notably that is the slow fallback path. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/15 17:53:25, chromium-reviews wrote: > On Mon, May 15, 2017 at 1:49 PM, <mailto:muyuanli@chromium.org> wrote: > > > On 2017/05/15 13:41:20, sky wrote: > > > Isn't this going to be super expensive? > > > > It shouldn't.. it only copies the layer meta structure not the textures the > > layer owns. > > > > Is this true? I'd expect any PictureLayers to require being recorded, > rastered, uploaded, using new textures. Can you verify this? > > > > The screen rotation animation is done in this way as well: > > > > https://cs.chromium.org/chromium/src/ash/rotator/ > > screen_rotation_animator.cc?rcl=40f898c48cccf321bc48fe2f58b03a > > 84b167a4e5&l=323 > > > > Notably that is the slow fallback path. > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Dana, could you please point me to the point where PictureLayer is re-rasterized? I can't seem to find related code.. And according to the comment to ui::Layer::Clone, only surface layer and solid color layer is copied: https://cs.chromium.org/chromium/src/ui/compositor/layer.h?rcl=d0602af69e19b4...
https://codereview.chromium.org/2854543002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2854543002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:56: auto blocked_layers = LayerSet(); On 2017/05/15 16:47:52, Luis Héctor Chávez wrote: > Why not > > LayerSet blocked_layers; > > ? It's shorter and more readable. Done. https://codereview.chromium.org/2854543002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:58: for (auto* browser : *browser_list) { On 2017/05/15 16:47:51, Luis Héctor Chávez wrote: > nit: BrowserList itself does > > for (auto* browser : *BrowserList::GetInstance()) > > https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_list.cc?l=31 Done. https://codereview.chromium.org/2854543002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:82: if (layer_tree_owner) { On 2017/05/15 16:47:52, Luis Héctor Chávez wrote: > nit: use the guard clause pattern: > > if (!layer_tree_owner) > return layer_tree_owner; > > // rest Done. https://codereview.chromium.org/2854543002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:191: DCHECK(old_layer_owner_); On 2017/05/15 16:47:52, Luis Héctor Chávez wrote: > I don't think you can DCHECK here since it seems like it's legit that you get a > nullptr from BlockIncognitoWindows. > > If it's not legit, you should add more comments/DCHECKs there mentioning why it > should "never happen". Done. https://codereview.chromium.org/2854543002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:258: base::TaskTraits({base::MayBlock(), base::TaskPriority::USER_BLOCKING}), On 2017/05/15 16:47:51, Luis Héctor Chávez wrote: > nit: > > base::TaskTraits{base::MayBlock(), base::TaskPriority::USER_BLOCKING} > > otherwise you're building a base::TaskTraits with the {...}, and then building > another one with base::TaskTraits(...) and passing the original in. Done.
On Mon, May 15, 2017 at 2:08 PM, <muyuanli@chromium.org> wrote: > On 2017/05/15 17:53:25, chromium-reviews wrote: > > On Mon, May 15, 2017 at 1:49 PM, <mailto:muyuanli@chromium.org> wrote: > > > > > On 2017/05/15 13:41:20, sky wrote: > > > > Isn't this going to be super expensive? > > > > > > It shouldn't.. it only copies the layer meta structure not the > textures the > > > layer owns. > > > > > > > Is this true? I'd expect any PictureLayers to require being recorded, > > rastered, uploaded, using new textures. Can you verify this? > > > > > > > The screen rotation animation is done in this way as well: > > > > > > https://cs.chromium.org/chromium/src/ash/rotator/ > > > screen_rotation_animator.cc?rcl=40f898c48cccf321bc48fe2f58b03a > > > 84b167a4e5&l=323 > > > > > > > Notably that is the slow fallback path. > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Dana, could you please point me to the point where PictureLayer is > re-rasterized? I can't seem to find related code.. > And according to the comment to ui::Layer::Clone, only surface layer and > solid > color layer is copied: > https://cs.chromium.org/chromium/src/ui/compositor/layer.h?r > cl=d0602af69e19b44bbbbc64165766098d9c9e36a6&l=76 > You're using RecreateLayer() which does much more than Clone(), including setting the delegate, which is what would paint for a PictureLayer. https://cs.chromium.org/chromium/src/ui/compositor/layer_owner.cc?rcl=7afec4 2068281c82d60ba40ade02f86d40b28e0e&l=65 PictureLayerImpl rasterization happens in the raster buffer providers, such as OneCopyRasterBufferProvider, that's trickier to attribute to a layer. But if you see a new PictureLayerImpl, which has new a PictureLayerTiling, etc, the tiles are each a new texture. You can also see raster done in a frame viewer trace (chrome://tracing) on the raster worker thread. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2854543002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2854543002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:53: std::unique_ptr<ui::LayerTreeOwner> BlockIncognitoWindows( Is blocking incognito windows the only thing this function does? I thought it would also avoid to include the overlay. In that case maybe CreateLayerTreeForSnapshot is a more appropriate name. https://codereview.chromium.org/2854543002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:55: using LayerSet = std::set<const ui::Layer*>; flat_set more appropriate? https://codereview.chromium.org/2854543002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:62: auto layer_tree_owner = ::wm::MapLayers( Would it not be easier to re-implement the CloneChildren functionality here for this specific use case instead complicating the RecreateLayers code in window_util.cc? Maybe you can even avoid the blocked_layer set that way..
Had offline discussion with danakj, we agree that this operation could be expensive for picture layers but given current UX requirement and limitation of current system design, we don't have a better solution for now. https://codereview.chromium.org/2854543002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2854543002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:53: std::unique_ptr<ui::LayerTreeOwner> BlockIncognitoWindows( On 2017/05/15 18:55:07, reveman wrote: > Is blocking incognito windows the only thing this function does? I thought it > would also avoid to include the overlay. In that case maybe > CreateLayerTreeForSnapshot is a more appropriate name. Done. https://codereview.chromium.org/2854543002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:55: using LayerSet = std::set<const ui::Layer*>; On 2017/05/15 18:55:07, reveman wrote: > flat_set more appropriate? Done. https://codereview.chromium.org/2854543002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:62: auto layer_tree_owner = ::wm::MapLayers( On 2017/05/15 18:55:07, reveman wrote: > Would it not be easier to re-implement the CloneChildren functionality here for > this specific use case instead complicating the RecreateLayers code in > window_util.cc? Maybe you can even avoid the blocked_layer set that way.. I feel that the logic used here is almost identical to CloneChildren and that's why I generalized the code in RecreateLayers in window_utils to allow providing a customized traversal function instead of copying pasting code of CloneChildren back here.
On Mon, May 15, 2017 at 3:09 PM, <muyuanli@chromium.org> wrote: > Had offline discussion with danakj, we agree that this operation could be > expensive for picture layers but given current UX requirement and > limitation of > current system design, we don't have a better solution for now. > I would suggest that instead of taking a screenshot of the entire desktop, taking screenshots of each individual android app contents, and tab contents would be better, as you can simply skip over incognito or other things you don't want, and you don't need to reproduce the chrome/ash UI. Any additional metadata such as position of the apps could be sent in better format than via pixel representation. > > > > https://codereview.chromium.org/2854543002/diff/80001/ > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > interaction_framework_service.cc > File > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > interaction_framework_service.cc > (right): > > https://codereview.chromium.org/2854543002/diff/80001/ > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > interaction_framework_service.cc#newcode53 > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > interaction_framework_service.cc:53: > std::unique_ptr<ui::LayerTreeOwner> BlockIncognitoWindows( > On 2017/05/15 18:55:07, reveman wrote: > > Is blocking incognito windows the only thing this function does? I > thought it > > would also avoid to include the overlay. In that case maybe > > CreateLayerTreeForSnapshot is a more appropriate name. > > Done. > > https://codereview.chromium.org/2854543002/diff/80001/ > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > interaction_framework_service.cc#newcode55 > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > interaction_framework_service.cc:55: > using LayerSet = std::set<const ui::Layer*>; > On 2017/05/15 18:55:07, reveman wrote: > > flat_set more appropriate? > > Done. > > https://codereview.chromium.org/2854543002/diff/80001/ > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > interaction_framework_service.cc#newcode62 > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > interaction_framework_service.cc:62: > auto layer_tree_owner = ::wm::MapLayers( > On 2017/05/15 18:55:07, reveman wrote: > > Would it not be easier to re-implement the CloneChildren functionality > here for > > this specific use case instead complicating the RecreateLayers code in > > window_util.cc? Maybe you can even avoid the blocked_layer set that > way.. > > I feel that the logic used here is almost identical to CloneChildren and > that's why I generalized the code in RecreateLayers in window_utils to > allow providing a customized traversal function instead of copying > pasting code of CloneChildren back here. > > https://codereview.chromium.org/2854543002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/15 19:24:54, danakj wrote: > On Mon, May 15, 2017 at 3:09 PM, <mailto:muyuanli@chromium.org> wrote: > > > Had offline discussion with danakj, we agree that this operation could be > > expensive for picture layers but given current UX requirement and > > limitation of > > current system design, we don't have a better solution for now. > > > > I would suggest that instead of taking a screenshot of the entire desktop, > taking screenshots of each individual android app contents, and tab > contents would be better, as you can simply skip over incognito or other > things you don't want, and you don't need to reproduce the chrome/ash UI. > Any additional metadata such as position of the apps could be sent in > better format than via pixel representation. > > I agree that the approach used in this CL introduces additional cost for rasterizing layers, for which I don't know about a better solution. Taking and sending screenshot of entire desktop is not a functionality added by this CL. This CL only targets to enhance privacy and usability. Whether we should have a pixel representation of the desktop, I believe, is outside the scope of this CL and should be discussed with PM, UX and security personals. > > > > > > > > https://codereview.chromium.org/2854543002/diff/80001/ > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > interaction_framework_service.cc > > File > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > interaction_framework_service.cc > > (right): > > > > https://codereview.chromium.org/2854543002/diff/80001/ > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > interaction_framework_service.cc#newcode53 > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > interaction_framework_service.cc:53: > > std::unique_ptr<ui::LayerTreeOwner> BlockIncognitoWindows( > > On 2017/05/15 18:55:07, reveman wrote: > > > Is blocking incognito windows the only thing this function does? I > > thought it > > > would also avoid to include the overlay. In that case maybe > > > CreateLayerTreeForSnapshot is a more appropriate name. > > > > Done. > > > > https://codereview.chromium.org/2854543002/diff/80001/ > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > interaction_framework_service.cc#newcode55 > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > interaction_framework_service.cc:55: > > using LayerSet = std::set<const ui::Layer*>; > > On 2017/05/15 18:55:07, reveman wrote: > > > flat_set more appropriate? > > > > Done. > > > > https://codereview.chromium.org/2854543002/diff/80001/ > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > interaction_framework_service.cc#newcode62 > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > interaction_framework_service.cc:62: > > auto layer_tree_owner = ::wm::MapLayers( > > On 2017/05/15 18:55:07, reveman wrote: > > > Would it not be easier to re-implement the CloneChildren functionality > > here for > > > this specific use case instead complicating the RecreateLayers code in > > > window_util.cc? Maybe you can even avoid the blocked_layer set that > > way.. > > > > I feel that the logic used here is almost identical to CloneChildren and > > that's why I generalized the code in RecreateLayers in window_utils to > > allow providing a customized traversal function instead of copying > > pasting code of CloneChildren back here. > > > > https://codereview.chromium.org/2854543002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2017/05/15 19:46:32, Muyuan wrote: > On 2017/05/15 19:24:54, danakj wrote: > > On Mon, May 15, 2017 at 3:09 PM, <mailto:muyuanli@chromium.org> wrote: > > > > > Had offline discussion with danakj, we agree that this operation could be > > > expensive for picture layers but given current UX requirement and > > > limitation of > > > current system design, we don't have a better solution for now. > > > > > > > I would suggest that instead of taking a screenshot of the entire desktop, > > taking screenshots of each individual android app contents, and tab > > contents would be better, as you can simply skip over incognito or other > > things you don't want, and you don't need to reproduce the chrome/ash UI. > > Any additional metadata such as position of the apps could be sent in > > better format than via pixel representation. > > > > > > I agree that the approach used in this CL introduces additional cost for > rasterizing layers, for which I don't know about a better solution. > > Taking and sending screenshot of entire desktop is not a functionality added > by this CL. This CL only targets to enhance privacy and usability. > Whether we should have a pixel representation of the desktop, I believe, is > outside the scope of this CL and should be discussed with PM, UX and security > personals. I have discussed with reveman about the option to take screenshot of each individual window, the problem would be: 1. Additional CPU cost 2. Double memory cost 3. Corner cases such as tricky to handle transparent overlays More over, doing so essentially repeats the functionality of the compositor, all in CPU, which I don't feel to be a better option. > > > > > > > > > > > > > https://codereview.chromium.org/2854543002/diff/80001/ > > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > > interaction_framework_service.cc > > > File > > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > > interaction_framework_service.cc > > > (right): > > > > > > https://codereview.chromium.org/2854543002/diff/80001/ > > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > > interaction_framework_service.cc#newcode53 > > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > > interaction_framework_service.cc:53: > > > std::unique_ptr<ui::LayerTreeOwner> BlockIncognitoWindows( > > > On 2017/05/15 18:55:07, reveman wrote: > > > > Is blocking incognito windows the only thing this function does? I > > > thought it > > > > would also avoid to include the overlay. In that case maybe > > > > CreateLayerTreeForSnapshot is a more appropriate name. > > > > > > Done. > > > > > > https://codereview.chromium.org/2854543002/diff/80001/ > > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > > interaction_framework_service.cc#newcode55 > > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > > interaction_framework_service.cc:55: > > > using LayerSet = std::set<const ui::Layer*>; > > > On 2017/05/15 18:55:07, reveman wrote: > > > > flat_set more appropriate? > > > > > > Done. > > > > > > https://codereview.chromium.org/2854543002/diff/80001/ > > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > > interaction_framework_service.cc#newcode62 > > > chrome/browser/chromeos/arc/voice_interaction/arc_voice_ > > > interaction_framework_service.cc:62: > > > auto layer_tree_owner = ::wm::MapLayers( > > > On 2017/05/15 18:55:07, reveman wrote: > > > > Would it not be easier to re-implement the CloneChildren functionality > > > here for > > > > this specific use case instead complicating the RecreateLayers code in > > > > window_util.cc? Maybe you can even avoid the blocked_layer set that > > > way.. > > > > > > I feel that the logic used here is almost identical to CloneChildren and > > > that's why I generalized the code in RecreateLayers in window_utils to > > > allow providing a customized traversal function instead of copying > > > pasting code of CloneChildren back here. > > > > > > https://codereview.chromium.org/2854543002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org.
lgtm with one more nit. https://codereview.chromium.org/2854543002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2854543002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:64: [](LayerSet layers, nit: s/layers/blocked_layers/
https://codereview.chromium.org/2854543002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2854543002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:64: [](LayerSet layers, On 2017/05/15 22:42:21, Luis Héctor Chávez wrote: > nit: s/layers/blocked_layers/ Done.
https://codereview.chromium.org/2854543002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2854543002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:78: return owner->RecreateLayer(); Is it possible to use Clone in some way to achieve the effect you are after? Again, having to recreate all the layers is super expensive. How often is this going to happen? https://codereview.chromium.org/2854543002/diff/140001/ui/snapshot/snapshot_a... File ui/snapshot/snapshot_aura.cc (right): https://codereview.chromium.org/2854543002/diff/140001/ui/snapshot/snapshot_a... ui/snapshot/snapshot_aura.cc:136: void GrabLayerSnapshotAsync(ui::Layer* layer, How come this doesn't need the logic in FinishedAsyncCopyRequest? https://codereview.chromium.org/2854543002/diff/140001/ui/snapshot/snapshot_a... File ui/snapshot/snapshot_aura.h (right): https://codereview.chromium.org/2854543002/diff/140001/ui/snapshot/snapshot_a... ui/snapshot/snapshot_aura.h:33: SNAPSHOT_EXPORT void GrabLayerSnapshotAsync( Please add description and use OnceCallback. https://codereview.chromium.org/2854543002/diff/140001/ui/wm/core/window_util.h File ui/wm/core/window_util.h (right): https://codereview.chromium.org/2854543002/diff/140001/ui/wm/core/window_util... ui/wm/core/window_util.h:52: base::Callback<std::unique_ptr<ui::Layer>(ui::LayerOwner*)>; RepeatingCallback. https://codereview.chromium.org/2854543002/diff/140001/ui/wm/core/window_util... ui/wm/core/window_util.h:58: WM_EXPORT std::unique_ptr<ui::LayerTreeOwner> MapLayers( Add test coverage of this. How about naming this RecreateLayersUsingClosure() or something?
On the topic of taking snapshots of each individual window instead: I was suggesting this earlier and I still think that would be a cleaner solution. However, I'm OK with something like the approach in the current patch if that's easier for now when we need to send a composited image that match what is displayed on screen except for this incognito special case. Maybe not useful today but it seems like sending the contents of each window and not just what's visible can allow for a more powerful feature.
On 2017/05/17 16:58:14, reveman wrote: > On the topic of taking snapshots of each individual window instead: > > I was suggesting this earlier and I still think that would be a cleaner > solution. However, I'm OK with something like the approach in the current patch > if that's easier for now when we need to send a composited image that match what > is displayed on screen except for this incognito special case. > > Maybe not useful today but it seems like sending the contents of each window and > not just what's visible can allow for a more powerful feature. Yes, this is a viable (and potentially desirable) solution in the future.
Description was changed from ========== Recreates the layer hierarchy modulo all layers that belong to incognito windows, replacing them with a solid black layer. BUG=b/37755798 ========== to ========== Recreates the layer hierarchy modulo all layers that belong to incognito windows, replacing them with a solid black layer. BUG=b/37755798 TEST=wm_unittests --gtest_filter=WindowUtilTest.RecreateLayersWithClosure ==========
https://codereview.chromium.org/2854543002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2854543002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:78: return owner->RecreateLayer(); On 2017/05/17 14:48:30, sky wrote: > Is it possible to use Clone in some way to achieve the effect you are after? > Again, having to recreate all the layers is super expensive. How often is this > going to happen? Layer::Clone may not be sufficient for picture layers. That being said, this is a low frequency operation. A screenshot is only taken when the voice interaction session is initiated. https://codereview.chromium.org/2854543002/diff/140001/ui/snapshot/snapshot_a... File ui/snapshot/snapshot_aura.cc (right): https://codereview.chromium.org/2854543002/diff/140001/ui/snapshot/snapshot_a... ui/snapshot/snapshot_aura.cc:136: void GrabLayerSnapshotAsync(ui::Layer* layer, On 2017/05/17 14:48:30, sky wrote: > How come this doesn't need the logic in FinishedAsyncCopyRequest? A window tracker used in FinishedAsyncCopyRequest is not applicable to layers.. and what would be the reason for a failed CopyRequest? https://codereview.chromium.org/2854543002/diff/140001/ui/snapshot/snapshot_a... File ui/snapshot/snapshot_aura.h (right): https://codereview.chromium.org/2854543002/diff/140001/ui/snapshot/snapshot_a... ui/snapshot/snapshot_aura.h:33: SNAPSHOT_EXPORT void GrabLayerSnapshotAsync( On 2017/05/17 14:48:30, sky wrote: > Please add description and use OnceCallback. I'm reusing the function in MakeAsyncCopyRequest, which could take something other than OnceCallback.. https://codereview.chromium.org/2854543002/diff/140001/ui/wm/core/window_util.h File ui/wm/core/window_util.h (right): https://codereview.chromium.org/2854543002/diff/140001/ui/wm/core/window_util... ui/wm/core/window_util.h:52: base::Callback<std::unique_ptr<ui::Layer>(ui::LayerOwner*)>; On 2017/05/17 14:48:30, sky wrote: > RepeatingCallback. Done. https://codereview.chromium.org/2854543002/diff/140001/ui/wm/core/window_util... ui/wm/core/window_util.h:58: WM_EXPORT std::unique_ptr<ui::LayerTreeOwner> MapLayers( On 2017/05/17 14:48:30, sky wrote: > Add test coverage of this. > How about naming this RecreateLayersUsingClosure() or something? Done.
LGTM with the following changes. https://codereview.chromium.org/2854543002/diff/160001/ui/wm/core/window_util... File ui/wm/core/window_util_unittest.cc (right): https://codereview.chromium.org/2854543002/diff/160001/ui/wm/core/window_util... ui/wm/core/window_util_unittest.cc:77: ASSERT_TRUE(!tree_empty); The not makes this slightly hard to parse, use ASSERT_FALSE(tree_empty). https://codereview.chromium.org/2854543002/diff/160001/ui/wm/core/window_util... ui/wm/core/window_util_unittest.cc:90: // window12 is filtered out in the above recreation logic. ASSERT_TRUE(tree) ?
https://codereview.chromium.org/2854543002/diff/160001/ui/wm/core/window_util... File ui/wm/core/window_util_unittest.cc (right): https://codereview.chromium.org/2854543002/diff/160001/ui/wm/core/window_util... ui/wm/core/window_util_unittest.cc:77: ASSERT_TRUE(!tree_empty); On 2017/05/18 02:50:15, sky wrote: > The not makes this slightly hard to parse, use ASSERT_FALSE(tree_empty). Done. https://codereview.chromium.org/2854543002/diff/160001/ui/wm/core/window_util... ui/wm/core/window_util_unittest.cc:90: // window12 is filtered out in the above recreation logic. On 2017/05/18 02:50:15, sky wrote: > ASSERT_TRUE(tree) ? Done.
The CQ bit was checked by muyuanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2854543002/#ps180001 (title: "address review comments")
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": 180001, "attempt_start_ts": 1495076320990140,
"parent_rev": "3b1e029f111a9c24ecef1cc497c263ac1a1d731b", "commit_rev":
"f134db33d0941e7d89ab862455bf17e0b490fc88"}
Message was sent while issue was closed.
Description was changed from ========== Recreates the layer hierarchy modulo all layers that belong to incognito windows, replacing them with a solid black layer. BUG=b/37755798 TEST=wm_unittests --gtest_filter=WindowUtilTest.RecreateLayersWithClosure ========== to ========== Recreates the layer hierarchy modulo all layers that belong to incognito windows, replacing them with a solid black layer. BUG=b/37755798 TEST=wm_unittests --gtest_filter=WindowUtilTest.RecreateLayersWithClosure Review-Url: https://codereview.chromium.org/2854543002 Cr-Commit-Position: refs/heads/master@{#472678} Committed: https://chromium.googlesource.com/chromium/src/+/f134db33d0941e7d89ab862455bf... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/f134db33d0941e7d89ab862455bf... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
