Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2641053005:
Brush up localSelectionRectOfPositionTemplate (Closed)
DescriptionBrush up localSelectionRectOfPositionTemplate
This CL does:
1. Import |localCaretRectOfPositionTemplate()| partially to avoid calling
|computeInlineBox()| twice(L2562-L2586).
In |localCaretRectOfPositionTemplate()|, we call |computeInlineBox()|, which already
called at L2568. Unified it.
Lines from 2562 to 2586 is virtually localCaretRectOfPositionTemplate except
|boxPosition.inlineBox| null-nonnull condition because we need non-null inlineBox
to get valid LayoutRect.
2. Get rid of redundant and unsafe cast(L2586).
We cast InlineBoxPosition.inlineBox to InlineTextBox at L2570.
However, computeInlineBoxPosition returns InlineBoxPosition which inlineBox
is not InlineTextBox when position.node.layoutObject is not Text and under few
conditions.
Thus the cast at L2570 fails.
BTW, we don't need to use InlineTextBox following lines because no special
functions on InlineTextBox but only InlineBox are used.
Then we can use boxPosition.inlineBox just as InlineBox.
BUG=680428
TEST=run_webkit_unittests
--gtest_filter=VisibleUnitsTest.localSelectionRectOfPositionTemplateNotCrash
Review-Url: https://codereview.chromium.org/2641053005
Cr-Commit-Position: refs/heads/master@{#445026}
Committed: https://chromium.googlesource.com/chromium/src/+/2ac0c19603e70ef75f156ee8409162bbff94de74
Patch Set 1 #
Total comments: 6
Patch Set 2 : update #
Total comments: 4
Patch Set 3 : update #
Messages
Total messages: 21 (14 generated)
The CQ bit was checked by yoichio@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 ========== Not cast in localSelectionRectOfpositiontemplate BUG= ========== to ========== Not cast to InlineTextBox in localSelectionRectOfPositionTemplate In the function, we cast InlineBoxPosition.inlineBox to InlineTextBox at L2570. However, computeInlineBoxPosition returns InlineBoxPosition which inlineBox is not InlineTextBox when positoin.node.layoutObject is not Text and under few conditions. Thus the cast at L2570 fails. BTW, we don't need to use InlineTextBox following lines because no special functions on InlineTextBox but only InlineBox are used. Then just we can use boxPosition.inlineBox just as InlineBox. BUG=680428 ==========
yoichio@chromium.org changed reviewers: + yosin@chromium.org
yoichio@chromium.org changed reviewers: + joone.hur@intel.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2641053005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2641053005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2570: InlineBox* const box = boxPosition.inlineBox; Please see also https://codereview.chromium.org/2636623002 If we can get base line for non-InlineTextBox, it is good! https://codereview.chromium.org/2641053005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right): https://codereview.chromium.org/2641053005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2002: const char* bodyContent = "<div>foo<img /></div>"; No need to have |bodyContent|. s"<img />"<img>" No need to have " /" for IMG. https://codereview.chromium.org/2641053005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2006: IntRect r = absoluteSelectionBoundsOf(VisiblePosition::create( nit: Please avoid to use one letter variable name. https://codereview.chromium.org/2641053005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2008: } EXPECT_EQ(IntRect(), r) ?
lgtm w/ nit Please add TEST=run_webkit_unittests --gtest_filter=VisibleUnitsTest.localSelectionRectOfPositionTemplateNotCrash https://codereview.chromium.org/2641053005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right): https://codereview.chromium.org/2641053005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2002: const char* bodyContent = "<div>foo<img></div>"; nit: no need to use |bodyContent|. https://codereview.chromium.org/2641053005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2008: EXPECT_EQ(IntRect(9, 9, 1, 1), rect); I recommend to use EXPECT_FALSE(rect.isEmpty()) because font size is varied in platform.
Description was changed from ========== Not cast to InlineTextBox in localSelectionRectOfPositionTemplate In the function, we cast InlineBoxPosition.inlineBox to InlineTextBox at L2570. However, computeInlineBoxPosition returns InlineBoxPosition which inlineBox is not InlineTextBox when positoin.node.layoutObject is not Text and under few conditions. Thus the cast at L2570 fails. BTW, we don't need to use InlineTextBox following lines because no special functions on InlineTextBox but only InlineBox are used. Then just we can use boxPosition.inlineBox just as InlineBox. BUG=680428 ========== to ========== Not cast to InlineTextBox in localSelectionRectOfPositionTemplate In the function, we cast InlineBoxPosition.inlineBox to InlineTextBox at L2570. However, computeInlineBoxPosition returns InlineBoxPosition which inlineBox is not InlineTextBox when positoin.node.layoutObject is not Text and under few conditions. Thus the cast at L2570 fails. BTW, we don't need to use InlineTextBox following lines because no special functions on InlineTextBox but only InlineBox are used. Then just we can use boxPosition.inlineBox just as InlineBox. BUG=680428 TEST=run_webkit_unittests --gtest_filter=VisibleUnitsTest.localSelectionRectOfPositionTemplateNotCrash ==========
Description was changed from ========== Not cast to InlineTextBox in localSelectionRectOfPositionTemplate In the function, we cast InlineBoxPosition.inlineBox to InlineTextBox at L2570. However, computeInlineBoxPosition returns InlineBoxPosition which inlineBox is not InlineTextBox when positoin.node.layoutObject is not Text and under few conditions. Thus the cast at L2570 fails. BTW, we don't need to use InlineTextBox following lines because no special functions on InlineTextBox but only InlineBox are used. Then just we can use boxPosition.inlineBox just as InlineBox. BUG=680428 TEST=run_webkit_unittests --gtest_filter=VisibleUnitsTest.localSelectionRectOfPositionTemplateNotCrash ========== to ========== Not cast to InlineTextBox in localSelectionRectOfPositionTemplate In the function, we cast InlineBoxPosition.inlineBox to InlineTextBox at L2570. However, computeInlineBoxPosition returns InlineBoxPosition which inlineBox is not InlineTextBox when position.node.layoutObject is not Text and under few conditions. Thus the cast at L2570 fails. BTW, we don't need to use InlineTextBox following lines because no special functions on InlineTextBox but only InlineBox are used. Then we can use boxPosition.inlineBox just as InlineBox. BUG=680428 TEST=run_webkit_unittests --gtest_filter=VisibleUnitsTest.localSelectionRectOfPositionTemplateNotCrash ==========
Title and description don't explain about this change. This change makes |localSelectionRectOfPositionTemplate()| 1. not to call |localCartRectOfPositionTemplate()| to avoid calling |computeInlineBox()| twice. 2. Check return value of |computeInlineBox()| 3. Get rid of redundant and unsafe cast. Please update title and description before committing.
Description was changed from ========== Not cast to InlineTextBox in localSelectionRectOfPositionTemplate In the function, we cast InlineBoxPosition.inlineBox to InlineTextBox at L2570. However, computeInlineBoxPosition returns InlineBoxPosition which inlineBox is not InlineTextBox when position.node.layoutObject is not Text and under few conditions. Thus the cast at L2570 fails. BTW, we don't need to use InlineTextBox following lines because no special functions on InlineTextBox but only InlineBox are used. Then we can use boxPosition.inlineBox just as InlineBox. BUG=680428 TEST=run_webkit_unittests --gtest_filter=VisibleUnitsTest.localSelectionRectOfPositionTemplateNotCrash ========== to ========== Brush up localSelectionRectOfPositionTemplate This CL does: 1. Import |localCaretRectOfPositionTemplate()| partially to avoid calling |computeInlineBox()| twice(L2562-L2586). In |localCaretRectOfPositionTemplate()|, we call |computeInlineBox()|, which already called at L2568. Unified it. Lines from 2562 to 2586 is virtually localCaretRectOfPositionTemplate except |boxPosition.inlineBox| null-nonnull condition because we need non-null inlineBox to get valid LayoutRect. 2. Get rid of redundant and unsafe cast(L2586). We cast InlineBoxPosition.inlineBox to InlineTextBox at L2570. However, computeInlineBoxPosition returns InlineBoxPosition which inlineBox is not InlineTextBox when position.node.layoutObject is not Text and under few conditions. Thus the cast at L2570 fails. BTW, we don't need to use InlineTextBox following lines because no special functions on InlineTextBox but only InlineBox are used. Then we can use boxPosition.inlineBox just as InlineBox. BUG=680428 TEST=run_webkit_unittests --gtest_filter=VisibleUnitsTest.localSelectionRectOfPositionTemplateNotCrash ==========
The CQ bit was checked by yoichio@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2641053005/#ps40001 (title: "update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Update title and description. https://codereview.chromium.org/2641053005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right): https://codereview.chromium.org/2641053005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2002: const char* bodyContent = "<div>foo<img /></div>"; On 2017/01/20 05:59:53, Yosi_UTC9 wrote: > No need to have |bodyContent|. > > s"<img />"<img>" > > No need to have " /" for IMG. Done. https://codereview.chromium.org/2641053005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2006: IntRect r = absoluteSelectionBoundsOf(VisiblePosition::create( On 2017/01/20 05:59:53, Yosi_UTC9 wrote: > nit: Please avoid to use one letter variable name. Done. https://codereview.chromium.org/2641053005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right): https://codereview.chromium.org/2641053005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2002: const char* bodyContent = "<div>foo<img></div>"; On 2017/01/20 08:00:02, Yosi_UTC9 wrote: > nit: no need to use |bodyContent|. Done. https://codereview.chromium.org/2641053005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2008: EXPECT_EQ(IntRect(9, 9, 1, 1), rect); On 2017/01/20 08:00:03, Yosi_UTC9 wrote: > I recommend to use EXPECT_FALSE(rect.isEmpty()) because font size is varied in > platform. Done.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484901003336260,
"parent_rev": "1a8ddf2e5a1ee482848599ee5b4d4faf8ac7ad19", "commit_rev":
"2ac0c19603e70ef75f156ee8409162bbff94de74"}
Message was sent while issue was closed.
Description was changed from ========== Brush up localSelectionRectOfPositionTemplate This CL does: 1. Import |localCaretRectOfPositionTemplate()| partially to avoid calling |computeInlineBox()| twice(L2562-L2586). In |localCaretRectOfPositionTemplate()|, we call |computeInlineBox()|, which already called at L2568. Unified it. Lines from 2562 to 2586 is virtually localCaretRectOfPositionTemplate except |boxPosition.inlineBox| null-nonnull condition because we need non-null inlineBox to get valid LayoutRect. 2. Get rid of redundant and unsafe cast(L2586). We cast InlineBoxPosition.inlineBox to InlineTextBox at L2570. However, computeInlineBoxPosition returns InlineBoxPosition which inlineBox is not InlineTextBox when position.node.layoutObject is not Text and under few conditions. Thus the cast at L2570 fails. BTW, we don't need to use InlineTextBox following lines because no special functions on InlineTextBox but only InlineBox are used. Then we can use boxPosition.inlineBox just as InlineBox. BUG=680428 TEST=run_webkit_unittests --gtest_filter=VisibleUnitsTest.localSelectionRectOfPositionTemplateNotCrash ========== to ========== Brush up localSelectionRectOfPositionTemplate This CL does: 1. Import |localCaretRectOfPositionTemplate()| partially to avoid calling |computeInlineBox()| twice(L2562-L2586). In |localCaretRectOfPositionTemplate()|, we call |computeInlineBox()|, which already called at L2568. Unified it. Lines from 2562 to 2586 is virtually localCaretRectOfPositionTemplate except |boxPosition.inlineBox| null-nonnull condition because we need non-null inlineBox to get valid LayoutRect. 2. Get rid of redundant and unsafe cast(L2586). We cast InlineBoxPosition.inlineBox to InlineTextBox at L2570. However, computeInlineBoxPosition returns InlineBoxPosition which inlineBox is not InlineTextBox when position.node.layoutObject is not Text and under few conditions. Thus the cast at L2570 fails. BTW, we don't need to use InlineTextBox following lines because no special functions on InlineTextBox but only InlineBox are used. Then we can use boxPosition.inlineBox just as InlineBox. BUG=680428 TEST=run_webkit_unittests --gtest_filter=VisibleUnitsTest.localSelectionRectOfPositionTemplateNotCrash Review-Url: https://codereview.chromium.org/2641053005 Cr-Commit-Position: refs/heads/master@{#445026} Committed: https://chromium.googlesource.com/chromium/src/+/2ac0c19603e70ef75f156ee84091... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2ac0c19603e70ef75f156ee84091... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
