Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2709283002:
Avoid to change layer type in InkDropHostView::AddInkDropLayer (Closed)
DescriptionAvoid to change layer type in InkDropHostView::AddInkDropLayer
Driving by the fact that the layer type of a view can be changed after
landing cl https://codereview.chromium.org/2639203007.
It's possible that classes that extends InkDropHostView (ex: Button)
created a LAYER_SOLID_COLOR S as its layer type, then by calling
InkDropHostView::AddInkDropLayer, S is destroyed and a new LAYER_TEXTURED
T is created by calling "SetPaintToLayer();". Add check such that no new
layer is created if the view has a layer already.
BUG=680536
Review-Url: https://codereview.chromium.org/2709283002
Cr-Commit-Position: refs/heads/master@{#453067}
Committed: https://chromium.googlesource.com/chromium/src/+/49f6290b07b97c200fbf8737be29107ca99f923e
Patch Set 1 #
Total comments: 2
Patch Set 2 : add comments #
Total comments: 2
Patch Set 3 : change approach #Messages
Total messages: 22 (9 generated)
Description was changed from ========== add dchecks BUG=680536 ========== to ========== Add DCHECK on layer type after allowing layer to change its type Driving by the fact that the layer type of a view can be changed after landing this cl, https://codereview.chromium.org/2639203007, it's possible that InkDropHostView::RemoveInkDropLayer() won't be restoring the correct layer type (comment from @varkha) BUG=680536 ==========
Description was changed from ========== Add DCHECK on layer type after allowing layer to change its type Driving by the fact that the layer type of a view can be changed after landing this cl, https://codereview.chromium.org/2639203007, it's possible that InkDropHostView::RemoveInkDropLayer() won't be restoring the correct layer type (comment from @varkha) BUG=680536 ========== to ========== Add DCHECK on layer type after allowing layer to change its type Driving by the fact that the layer type of a view can be changed after landing this cl, https://codereview.chromium.org/2639203007, it's possible that InkDropHostView::RemoveInkDropLayer() won't be restoring the correct layer type (comment from @varkha) BUG=680536 ==========
yiyix@chromium.org changed reviewers: + sky@chromium.org
@sky, could you please review this change?
https://codereview.chromium.org/2709283002/diff/1/ui/views/animation/ink_drop... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2709283002/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host_view.cc:129: DCHECK(!layer() || layer()->type() == ui::LAYER_TEXTURED); Please add a comment as to why this expects ui::LAYER_TEXTURED. Do you know that this may happen? If so, when?
Description was changed from ========== Add DCHECK on layer type after allowing layer to change its type Driving by the fact that the layer type of a view can be changed after landing this cl, https://codereview.chromium.org/2639203007, it's possible that InkDropHostView::RemoveInkDropLayer() won't be restoring the correct layer type (comment from @varkha) BUG=680536 ========== to ========== Add DCHECK on layer type after allowing layer to change its type Driving by the fact that the layer type of a view can be changed after landing cl https://codereview.chromium.org/2639203007. It's possible that classes that extends InkDropHostView (ex: Button) created a LAYER_SOLID_COLOR S as its layer type, then by calling InkDropHostView::AddInkDropLayer, S is destroyed and a new LAYER_TEXTURED T is created. To prevent this from happening a DCHECK on layer type is added in InkDropHostView::AddInkDropLayer. BUG=680536 ==========
@sky, could you please take another look? Thank you very much https://codereview.chromium.org/2709283002/diff/1/ui/views/animation/ink_drop... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2709283002/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host_view.cc:129: DCHECK(!layer() || layer()->type() == ui::LAYER_TEXTURED); On 2017/02/22 21:29:49, sky wrote: > Please add a comment as to why this expects ui::LAYER_TEXTURED. Do you know that > this may happen? If so, when? I tried to answer your questions in the description of this cl. Is it more clear now?
https://codereview.chromium.org/2709283002/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2709283002/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:132: SetPaintToLayer(); Is this the problematic line? If we only do this then can we nuke the DCHECK?
On 2017/02/23 00:58:35, sky wrote: > https://codereview.chromium.org/2709283002/diff/20001/ui/views/animation/ink_... > File ui/views/animation/ink_drop_host_view.cc (right): > > https://codereview.chromium.org/2709283002/diff/20001/ui/views/animation/ink_... > ui/views/animation/ink_drop_host_view.cc:132: SetPaintToLayer(); > Is this the problematic line? If we only do this then can we nuke the DCHECK? Yes, the problematic line is "SetPaintToLayer();". It is a prevention against that its subclasses creates a solid color layer.
https://codereview.chromium.org/2709283002/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2709283002/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:132: SetPaintToLayer(); On 2017/02/23 00:58:35, sky wrote: > Is this the problematic line? If we only do this then can we nuke the DCHECK? What goes wrong if a solid color layer was created?
On 2017/02/24 00:58:44, sky wrote: > https://codereview.chromium.org/2709283002/diff/20001/ui/views/animation/ink_... > File ui/views/animation/ink_drop_host_view.cc (right): > > https://codereview.chromium.org/2709283002/diff/20001/ui/views/animation/ink_... > ui/views/animation/ink_drop_host_view.cc:132: SetPaintToLayer(); > On 2017/02/23 00:58:35, sky wrote: > > Is this the problematic line? If we only do this then can we nuke the DCHECK? > > What goes wrong if a solid color layer was created? My comments here are to help clarify why we have the DCHECK and if it's really necessary. The comment you are adding: // Prevent its subclass from created a non textured layer on this view. Documents the DCHECK, which isn't the important part. The important part of comments is to document *why*. Imagine you're the person that starts hitting this DCHECK. Does the comment help to identify what you are doing wrong and why?
On 2017/02/24 00:58:44, sky wrote: > https://codereview.chromium.org/2709283002/diff/20001/ui/views/animation/ink_... > File ui/views/animation/ink_drop_host_view.cc (right): > > https://codereview.chromium.org/2709283002/diff/20001/ui/views/animation/ink_... > ui/views/animation/ink_drop_host_view.cc:132: SetPaintToLayer(); > On 2017/02/23 00:58:35, sky wrote: > > Is this the problematic line? If we only do this then can we nuke the DCHECK? > > What goes wrong if a solid color layer was created? Functions in texture_layer and solid_color layer are different, which can cause unexpected failures. Ex: if the subclass A of InkDropHostView created a solid color layer and it's unaware that IDHV changed the layer to texture_layer in AddInkDropLayer, it could call some function unique to solid_color layer, ex: SetBackgroundColor. This function SetBackgroundColor does not exist in texture layer, so the call would fail. I also believe this can happen in real life. In MD, we introduced that the background of buttons changes as native theme changes (basically, OnNativeThemeChanged is called after a theme change and changing the background of button and color of label on button are needed.) Therefore, I believe that adding this DCHECK is necessary.
On 2017/02/24 01:02:56, sky wrote: > On 2017/02/24 00:58:44, sky wrote: > > > https://codereview.chromium.org/2709283002/diff/20001/ui/views/animation/ink_... > > File ui/views/animation/ink_drop_host_view.cc (right): > > > > > https://codereview.chromium.org/2709283002/diff/20001/ui/views/animation/ink_... > > ui/views/animation/ink_drop_host_view.cc:132: SetPaintToLayer(); > > On 2017/02/23 00:58:35, sky wrote: > > > Is this the problematic line? If we only do this then can we nuke the > DCHECK? > > > > What goes wrong if a solid color layer was created? > > My comments here are to help clarify why we have the DCHECK and if it's really > necessary. The comment you are adding: > > // Prevent its subclass from created a non textured layer on this view. > > Documents the DCHECK, which isn't the important part. The important part of > comments is to document *why*. Imagine you're the person that starts hitting > this DCHECK. Does the comment help to identify what you are doing wrong and why? Thank you so much for the clarification, I finally understand what you want. I will upload a new version.
Description was changed from ========== Add DCHECK on layer type after allowing layer to change its type Driving by the fact that the layer type of a view can be changed after landing cl https://codereview.chromium.org/2639203007. It's possible that classes that extends InkDropHostView (ex: Button) created a LAYER_SOLID_COLOR S as its layer type, then by calling InkDropHostView::AddInkDropLayer, S is destroyed and a new LAYER_TEXTURED T is created. To prevent this from happening a DCHECK on layer type is added in InkDropHostView::AddInkDropLayer. BUG=680536 ========== to ========== Add DCHECK on layer type after allowing layer to change its type Driving by the fact that the layer type of a view can be changed after landing cl https://codereview.chromium.org/2639203007. It's possible that classes that extends InkDropHostView (ex: Button) created a LAYER_SOLID_COLOR S as its layer type, then by calling InkDropHostView::AddInkDropLayer, S is destroyed and a new LAYER_TEXTURED T is created by calling "SetPaintToLayer();". Add check such that no new layer is created if the view has a layer already. BUG=680536 ==========
Description was changed from ========== Add DCHECK on layer type after allowing layer to change its type Driving by the fact that the layer type of a view can be changed after landing cl https://codereview.chromium.org/2639203007. It's possible that classes that extends InkDropHostView (ex: Button) created a LAYER_SOLID_COLOR S as its layer type, then by calling InkDropHostView::AddInkDropLayer, S is destroyed and a new LAYER_TEXTURED T is created by calling "SetPaintToLayer();". Add check such that no new layer is created if the view has a layer already. BUG=680536 ========== to ========== Avoid to change layer type in InkDropHostView::AddInkDropLayer Driving by the fact that the layer type of a view can be changed after landing cl https://codereview.chromium.org/2639203007. It's possible that classes that extends InkDropHostView (ex: Button) created a LAYER_SOLID_COLOR S as its layer type, then by calling InkDropHostView::AddInkDropLayer, S is destroyed and a new LAYER_TEXTURED T is created by calling "SetPaintToLayer();". Add check such that no new layer is created if the view has a layer already. BUG=680536 ==========
@sky, could you take another look? Thank you
LGTM
The CQ bit was checked by yiyix@chromium.org
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": 40001, "attempt_start_ts": 1487994274501440,
"parent_rev": "311516d81989d2307f5211860653c5fc5d5636f4", "commit_rev":
"49f6290b07b97c200fbf8737be29107ca99f923e"}
Message was sent while issue was closed.
Description was changed from ========== Avoid to change layer type in InkDropHostView::AddInkDropLayer Driving by the fact that the layer type of a view can be changed after landing cl https://codereview.chromium.org/2639203007. It's possible that classes that extends InkDropHostView (ex: Button) created a LAYER_SOLID_COLOR S as its layer type, then by calling InkDropHostView::AddInkDropLayer, S is destroyed and a new LAYER_TEXTURED T is created by calling "SetPaintToLayer();". Add check such that no new layer is created if the view has a layer already. BUG=680536 ========== to ========== Avoid to change layer type in InkDropHostView::AddInkDropLayer Driving by the fact that the layer type of a view can be changed after landing cl https://codereview.chromium.org/2639203007. It's possible that classes that extends InkDropHostView (ex: Button) created a LAYER_SOLID_COLOR S as its layer type, then by calling InkDropHostView::AddInkDropLayer, S is destroyed and a new LAYER_TEXTURED T is created by calling "SetPaintToLayer();". Add check such that no new layer is created if the view has a layer already. BUG=680536 Review-Url: https://codereview.chromium.org/2709283002 Cr-Commit-Position: refs/heads/master@{#453067} Committed: https://chromium.googlesource.com/chromium/src/+/49f6290b07b97c200fbf8737be29... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/49f6290b07b97c200fbf8737be29... |
