Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1867703002:
DevTools: allow builds with asserions when using DevTools front-end. (Closed)
DescriptionDevTools: allow builds with asserions when using DevTools front-end.
BUG=595100
Committed: https://crrev.com/a53caae5ed9ec598cfa1851bbc72bbc886ed04d2
Cr-Commit-Position: refs/heads/master@{#385858}
Patch Set 1 #
Total comments: 1
Messages
Total messages: 14 (4 generated)
pfeldman@chromium.org changed reviewers: + kojii@chromium.org
kojii@chromium.org changed reviewers: + yosin@chromium.org
Don't really remember, it's been while since last time I worked on editing, but the change looks at least harmless or good. In the original CL[1], PS3 had ASSERT here but then removed assuming we can catch the crash in Range::create(). non-owner LGTM. yosin@? [1] https://codereview.chromium.org/800633004
https://codereview.chromium.org/1867703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right): https://codereview.chromium.org/1867703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/DOMSelection.cpp:387: if (!node) // crbug.com/595100 Could you add a layout test for this case and for avoiding regression in future? It seems we should change |shadowAdjustedNode()| not to return |nullptr|, in theory, |shadowAdjustedNode()| should not be |nullptr|.
On 2016/04/07 01:25:39, Yosi_UTC9 wrote: > https://codereview.chromium.org/1867703002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right): > > https://codereview.chromium.org/1867703002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/DOMSelection.cpp:387: if (!node) // > crbug.com/595100 > Could you add a layout test for this case and for avoiding regression in future? > It seems we should change |shadowAdjustedNode()| not to return |nullptr|, > in theory, |shadowAdjustedNode()| should not be |nullptr|. I know too little about the shadowAdjustedNode to write a test. I can assist with the repro though if you want to take over!
On 2016/04/07 at 01:39:22, pfeldman wrote: > On 2016/04/07 01:25:39, Yosi_UTC9 wrote: > > https://codereview.chromium.org/1867703002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right): > > > > https://codereview.chromium.org/1867703002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/editing/DOMSelection.cpp:387: if (!node) // > > crbug.com/595100 > > Could you add a layout test for this case and for avoiding regression in future? > > It seems we should change |shadowAdjustedNode()| not to return |nullptr|, > > in theory, |shadowAdjustedNode()| should not be |nullptr|. > > I know too little about the shadowAdjustedNode to write a test. I can assist with the repro though if you want to take over! Could you provide minimal-HTML to reproduce this case?
No, I still don't get the nature of what is going on. I can repro it within few seconds though. If I knew what the expectaions were wrt return values, I'd fix it myself. But methods return 0 left and right in that flow and it is unclear which of those are not legit.
sorry, but i can not work with it crashing all the time. I'll land this, but ping me if you need a repro or something.
The CQ bit was checked by pfeldman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1867703002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1867703002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== DevTools: allow builds with asserions when using DevTools front-end. BUG=595100 ========== to ========== DevTools: allow builds with asserions when using DevTools front-end. BUG=595100 Committed: https://crrev.com/a53caae5ed9ec598cfa1851bbc72bbc886ed04d2 Cr-Commit-Position: refs/heads/master@{#385858} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/a53caae5ed9ec598cfa1851bbc72bbc886ed04d2 Cr-Commit-Position: refs/heads/master@{#385858} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
