Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2354893002:
Introduce VisiblePosition::isValid (Closed)
|
Created:
4 years, 3 months ago by Xiaocheng Modified:
4 years, 3 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce VisiblePosition::isValid
In general, functions taking VisiblePosition parameters must be called
in clean layout and before any DOM mutation. This patch introduces
VisiblePosition::isValid to check this.
This patch is a preparation for pruning createVisiblePositionDeprecated.
BUG=647219
Committed: https://crrev.com/b18b9565248f6074c188ee3eb3e50528ccc89ed4
Cr-Commit-Position: refs/heads/master@{#420024}
Patch Set 1 #Patch Set 2 : Revise tests #Patch Set 3 : Fix compile error... #Patch Set 4 : Further fix compile... #
Total comments: 10
Patch Set 5 : Add style change unit test, which currently fails... #Patch Set 6 : Use Document::styleVersion #Patch Set 7 : Remove TODO from unit test.. #
Total comments: 4
Patch Set 8 : 201609211817 #
Messages
Total messages: 44 (31 generated)
The CQ bit was checked by xiaochengh@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 checked by xiaochengh@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 ========== Introduce VisiblePosition::isValid BUG= ========== to ========== Introduce VisiblePosition::isValid In general, functions taking VisiblePosition parameters must be called in clean layout and before any DOM mutation. This patch introduces VisiblePosition::isValid to check this. This patch is a preparation for pruning createVisiblePositionDeprecated. BUG=647219 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by xiaochengh@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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by xiaochengh@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.
xiaochengh@chromium.org changed reviewers: + yosin@chromium.org
PTAL.
https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisiblePosition.cpp (right): https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisiblePosition.cpp:49: : m_domTreeVersion(0) We also need to track |Document::styleTreeVersion()|, e.g. display:none changes visible position. See "Selection.{cpp,h}" in http://crrev.com/1958093002 https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisiblePosition.cpp:230: return m_domTreeVersion == document.domTreeVersion() && !document.needsLayoutTreeUpdate(); Fumm... If we check |Document::needsLayoutTreeUpdate()| here, do we need to check dom tree version and style tree version? Is checking |Document::needsLayoutTreeUpdate()| enough? https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisiblePosition.h (right): https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisiblePosition.h:90: PositionTemplate<Strategy> deepEquivalent() const { return m_positionWithAffinity.position(); } Should we have |DCHECK(isValid())| in |deepEquivalent()|? Is it too early to do so? If so, let's add |TODO| for |deepEquivalent()|. https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisiblePositionTest.cpp (right): https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisiblePositionTest.cpp:71: #endif Let's check "display" property change. Not sure about "color", is changing "color" turns |Document::needsLayoutTreeUpdare()| to true? https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1340: DCHECK(visiblePosition.isValid()); We should add |DCHECK(vp.isValid())| all functions take VP. Since, we have gTest for |VP::isValid()|, is it better to add |DCHECK(vp.isValid()| in another gigantic patch"?
The CQ bit was checked by xiaochengh@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisiblePosition.cpp (right): https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisiblePosition.cpp:49: : m_domTreeVersion(0) On 2016/09/20 at 13:55:51, Yosi_UTC9 wrote: > We also need to track |Document::styleTreeVersion()|, e.g. display:none changes visible position. > See "Selection.{cpp,h}" in http://crrev.com/1958093002 I can't find |Document::styleTreeVersion()| in the code base... I searched "TreeVersion" but found only dom tree version: https://cs.chromium.org/search/?q=TreeVersion+case:yes&p=2&sq=package:chromiu... https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisiblePosition.cpp:230: return m_domTreeVersion == document.domTreeVersion() && !document.needsLayoutTreeUpdate(); On 2016/09/20 at 13:55:51, Yosi_UTC9 wrote: > Fumm... If we check |Document::needsLayoutTreeUpdate()| here, do we need to check dom tree version and style tree version? > Is checking |Document::needsLayoutTreeUpdate()| enough? I think both dom tree version and |Document::needsLayoutTreeUpdate| need to be checked. We don't want to see an invalid VP becoming valid again after layout update. We also have to check style tree version, as we don't want to see a VP invalidated due to style change becoming valid again. Unfortunately, style tree version is not available in the code base (or at least I didn't find it). I'm wondering if we have both dom and style tree versions, do we still need to check |Document::needsLayoutTreeUpdate|? Or perhaps it simply doesn't harm to have more checks... https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisiblePosition.h (right): https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisiblePosition.h:90: PositionTemplate<Strategy> deepEquivalent() const { return m_positionWithAffinity.position(); } On 2016/09/20 at 13:55:51, Yosi_UTC9 wrote: > Should we have |DCHECK(isValid())| in |deepEquivalent()|? Is it too early to do so? > If so, let's add |TODO| for |deepEquivalent()|. I think we can't do that, at least for now. Some clients store a VP and call deepEquivalent() after mutation. An example is DragCaretController::setCaretPosition. https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisiblePositionTest.cpp (right): https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisiblePositionTest.cpp:71: #endif On 2016/09/20 at 13:55:51, Yosi_UTC9 wrote: > Let's check "display" property change. > Not sure about "color", is changing "color" turns |Document::needsLayoutTreeUpdare()| to true? Done. https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1340: DCHECK(visiblePosition.isValid()); On 2016/09/20 at 13:55:51, Yosi_UTC9 wrote: > We should add |DCHECK(vp.isValid())| all functions take VP. > Since, we have gTest for |VP::isValid()|, is it better to add |DCHECK(vp.isValid()| in another gigantic patch"? Unfortunately we are not having such a gigantic patch... I tried adding a |DCHECK(vp.isValid())| to all functions in VisibleUnits.cpp and got massive hitting. We may create a crbug for it and gradually add DCHECKs to all functions, though.
On 2016/09/21 at 06:31:49, xiaochengh wrote: > https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/VisiblePosition.cpp (right): > > https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/VisiblePosition.cpp:49: : m_domTreeVersion(0) > On 2016/09/20 at 13:55:51, Yosi_UTC9 wrote: > > We also need to track |Document::styleTreeVersion()|, e.g. display:none changes visible position. > > See "Selection.{cpp,h}" in http://crrev.com/1958093002 > > I can't find |Document::styleTreeVersion()| in the code base... > > I searched "TreeVersion" but found only dom tree version: > > https://cs.chromium.org/search/?q=TreeVersion+case:yes&p=2&sq=package:chromiu... > Sorry, it is |Document::styleVersion()|. It is used in "SelectionEditor.cpp" in http://crrev.com/1958093002
The CQ bit was checked by xiaochengh@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 checked by xiaochengh@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...
On 2016/09/21 at 07:37:50, yosin wrote: > On 2016/09/21 at 06:31:49, xiaochengh wrote: > > https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/editing/VisiblePosition.cpp (right): > > > > https://codereview.chromium.org/2354893002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/VisiblePosition.cpp:49: : m_domTreeVersion(0) > > On 2016/09/20 at 13:55:51, Yosi_UTC9 wrote: > > > We also need to track |Document::styleTreeVersion()|, e.g. display:none changes visible position. > > > See "Selection.{cpp,h}" in http://crrev.com/1958093002 > > > > I can't find |Document::styleTreeVersion()| in the code base... > > > > I searched "TreeVersion" but found only dom tree version: > > > > https://cs.chromium.org/search/?q=TreeVersion+case:yes&p=2&sq=package:chromiu... > > > Sorry, it is |Document::styleVersion()|. > It is used in "SelectionEditor.cpp" in http://crrev.com/1958093002 Thanks. Updated.
lgtm w/ small nits Thanks for this patch! This is the first step toward deprecating VisiblePosition usage. https://codereview.chromium.org/2354893002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisiblePosition.h (right): https://codereview.chromium.org/2354893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisiblePosition.h:85: bool isNull() const { return m_positionWithAffinity.isNull(); } Could you add TODO about we should have |isValid()| for |isNull()|, |isNotNull()| and |isOrphan()|? https://codereview.chromium.org/2354893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisiblePosition.h:90: PositionTemplate<Strategy> deepEquivalent() const { return m_positionWithAffinity.position(); } Could you add TODO about we should have |isValid()| in |deepEquivalent()| function body?
https://codereview.chromium.org/2354893002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisiblePosition.h (right): https://codereview.chromium.org/2354893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisiblePosition.h:85: bool isNull() const { return m_positionWithAffinity.isNull(); } On 2016/09/21 at 08:48:42, Yosi_UTC9 wrote: > Could you add TODO about we should have |isValid()| for |isNull()|, |isNotNull()| and |isOrphan()|? We can't do that now because there are some clients storing a VP and then perform these checks after mutation. Or maybe that's not the correct usage and should be fixed? https://codereview.chromium.org/2354893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisiblePosition.h:90: PositionTemplate<Strategy> deepEquivalent() const { return m_positionWithAffinity.position(); } On 2016/09/21 at 08:48:42, Yosi_UTC9 wrote: > Could you add TODO about we should have |isValid()| in |deepEquivalent()| function body? Similar to the above checks.
On 2016/09/21 at 09:04:24, xiaochengh wrote: > https://codereview.chromium.org/2354893002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/VisiblePosition.h (right): > > https://codereview.chromium.org/2354893002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/VisiblePosition.h:85: bool isNull() const { return m_positionWithAffinity.isNull(); } > On 2016/09/21 at 08:48:42, Yosi_UTC9 wrote: > > Could you add TODO about we should have |isValid()| for |isNull()|, |isNotNull()| and |isOrphan()|? > > We can't do that now because there are some clients storing a VP and then perform these checks after mutation. > > Or maybe that's not the correct usage and should be fixed? > Using invalid VP is bad thing. We should fix such usage. But, there are too many places. That's why we would like to have TODO. > https://codereview.chromium.org/2354893002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/VisiblePosition.h:90: PositionTemplate<Strategy> deepEquivalent() const { return m_positionWithAffinity.position(); } > On 2016/09/21 at 08:48:42, Yosi_UTC9 wrote: > > Could you add TODO about we should have |isValid()| in |deepEquivalent()| function body? > > Similar to the above checks.
The CQ bit was checked by xiaochengh@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...
On 2016/09/21 at 09:10:01, yosin wrote: > On 2016/09/21 at 09:04:24, xiaochengh wrote: > > https://codereview.chromium.org/2354893002/diff/120001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/editing/VisiblePosition.h (right): > > > > https://codereview.chromium.org/2354893002/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/editing/VisiblePosition.h:85: bool isNull() const { return m_positionWithAffinity.isNull(); } > > On 2016/09/21 at 08:48:42, Yosi_UTC9 wrote: > > > Could you add TODO about we should have |isValid()| for |isNull()|, |isNotNull()| and |isOrphan()|? > > > > We can't do that now because there are some clients storing a VP and then perform these checks after mutation. > > > > Or maybe that's not the correct usage and should be fixed? > > > Using invalid VP is bad thing. We should fix such usage. But, there are too many places. > That's why we would like to have TODO. > > > https://codereview.chromium.org/2354893002/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/editing/VisiblePosition.h:90: PositionTemplate<Strategy> deepEquivalent() const { return m_positionWithAffinity.position(); } > > On 2016/09/21 at 08:48:42, Yosi_UTC9 wrote: > > > Could you add TODO about we should have |isValid()| in |deepEquivalent()| function body? > > > > Similar to the above checks. TODO added. Also (drive-by) replaced the DCHECKs in FrameSelection.cpp.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaochengh@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/2354893002/#ps140001 (title: "201609211817")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Introduce VisiblePosition::isValid In general, functions taking VisiblePosition parameters must be called in clean layout and before any DOM mutation. This patch introduces VisiblePosition::isValid to check this. This patch is a preparation for pruning createVisiblePositionDeprecated. BUG=647219 ========== to ========== Introduce VisiblePosition::isValid In general, functions taking VisiblePosition parameters must be called in clean layout and before any DOM mutation. This patch introduces VisiblePosition::isValid to check this. This patch is a preparation for pruning createVisiblePositionDeprecated. BUG=647219 Committed: https://crrev.com/b18b9565248f6074c188ee3eb3e50528ccc89ed4 Cr-Commit-Position: refs/heads/master@{#420024} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b18b9565248f6074c188ee3eb3e50528ccc89ed4 Cr-Commit-Position: refs/heads/master@{#420024}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2357143003/ by ymalik@chromium.org. The reason for reverting is: I suspect that this is breaking the Webkit Mac10.11 builder https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.11%20(db... crbug.com/648990). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
