Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(56)

Issue 196573035: Do null check when setting ScrollClipLayer (Closed)

Created:
6 years, 9 months ago by qinmin
Modified:
6 years, 9 months ago
CC:
chromium-reviews, cc-bugs_chromium.org, darin-cc_chromium.org, wjmaclean, bokan
Visibility:
Public.

Description

Do null check when setting ScrollClipLayer WebLayerImpl::setScrollClipLayer() doesn't seem to handle null case correctly on the layer we passed in. It calls cc_clip_layer->id() even if cc_clip_layer is set to null. We want to pass a null clipping layer to stop the layer from scrolling for the RenderFullscreen, as suggested by aelias in https://codereview.chromium.org/200943002/ BUG=352183 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257926

Patch Set 1 #

Total comments: 2

Patch Set 2 : remove the previous null check #

Total comments: 2

Patch Set 3 : removing useless static cast #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -3 lines) Patch
M webkit/renderer/compositor_bindings/web_layer_impl.cc View 1 2 1 chunk +5 lines, -3 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
qinmin
PTAL
6 years, 9 months ago (2014-03-18 22:19:01 UTC) #1
aelias_OOO_until_Jul13
https://codereview.chromium.org/196573035/diff/1/webkit/renderer/compositor_bindings/web_layer_impl.cc File webkit/renderer/compositor_bindings/web_layer_impl.cc (right): https://codereview.chromium.org/196573035/diff/1/webkit/renderer/compositor_bindings/web_layer_impl.cc#newcode266 webkit/renderer/compositor_bindings/web_layer_impl.cc:266: clip_layer ? static_cast<WebLayerImpl*>(clip_layer)->layer() : 0; Please also get rid ...
6 years, 9 months ago (2014-03-18 23:17:10 UTC) #2
qinmin
https://codereview.chromium.org/196573035/diff/1/webkit/renderer/compositor_bindings/web_layer_impl.cc File webkit/renderer/compositor_bindings/web_layer_impl.cc (right): https://codereview.chromium.org/196573035/diff/1/webkit/renderer/compositor_bindings/web_layer_impl.cc#newcode266 webkit/renderer/compositor_bindings/web_layer_impl.cc:266: clip_layer ? static_cast<WebLayerImpl*>(clip_layer)->layer() : 0; missed it, done On ...
6 years, 9 months ago (2014-03-18 23:20:43 UTC) #3
enne (OOO)
Can you also format your description to 72 cols max to be more git-friendly? https://codereview.chromium.org/196573035/diff/20001/webkit/renderer/compositor_bindings/web_layer_impl.cc ...
6 years, 9 months ago (2014-03-18 23:39:05 UTC) #4
qinmin
modified the CL description to be git-friendly https://codereview.chromium.org/196573035/diff/20001/webkit/renderer/compositor_bindings/web_layer_impl.cc File webkit/renderer/compositor_bindings/web_layer_impl.cc (right): https://codereview.chromium.org/196573035/diff/20001/webkit/renderer/compositor_bindings/web_layer_impl.cc#newcode265 webkit/renderer/compositor_bindings/web_layer_impl.cc:265: cc::Layer* cc_clip_layer ...
6 years, 9 months ago (2014-03-18 23:57:40 UTC) #5
enne (OOO)
lgtm
6 years, 9 months ago (2014-03-19 00:00:22 UTC) #6
qinmin
The CQ bit was checked by qinmin@chromium.org
6 years, 9 months ago (2014-03-19 00:16:42 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/196573035/30001
6 years, 9 months ago (2014-03-19 00:17:43 UTC) #8
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-19 00:26:57 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg tryserver.chromium on linux_chromium_rel
6 years, 9 months ago (2014-03-19 00:26:57 UTC) #10
qinmin
The CQ bit was checked by qinmin@chromium.org
6 years, 9 months ago (2014-03-19 02:04:54 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/196573035/30001
6 years, 9 months ago (2014-03-19 02:05:51 UTC) #12
commit-bot: I haz the power
6 years, 9 months ago (2014-03-19 11:23:13 UTC) #13
Message was sent while issue was closed.
Change committed as 257926

Powered by Google App Engine
This is Rietveld 408576698