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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2001083002: Explicit management of FrameSelection availability (Closed)

Created:
4 years, 7 months ago by yosin_UTC9
Modified:
3 years, 8 months ago
Reviewers:
tkent, yoichio
CC:
blink-reviews, chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Explicit management of FrameSelection availability This patch makes |FrameSelection| availability explicitly by observing document attach/detach with introducing |FrameSelection::isAvailable| and checking it before using |FrameSelection| to avoid using unavailable |FrameSelection| for improving code health. The lifetime of |FrameSelection| is as same as |LocalFrame|, but it is available during associated |Document| object is attached to |LayoutView|. Before this patch, Blink uses unavailable |FrameSeleciton| in some code path, e.g. callbacks invoked by |FrameSelection::prepareDestructor| via setting null positions to selection for resetting, and |SelectionController| attempts to use |FrameSelection| associated to detached |Document|. After this patch, |FrameSelection| holds pointer to attached |Document| or |nullptr| to indicate availability of |FrameSelection|. Note: I considered to make |DOMWindow| to own |FrameSelection| for avoiding to observe document attach/detach, but I didn't. Because lifetime of |DOMWindow| isn't as same as |Document|, e.g. |DOMWindow| is constructed before |Document| and lifetime of |DOMWindow| is longer than |LayoutView| of |Document|. Given that, I need to observer attach/detach and it isn't simpler than this patch. BUG=n/a TEST=n/a; no behavior changes Committed: https://crrev.com/9c38a83b780e6cbd388579067b137ef12a39ff0c Cr-Commit-Position: refs/heads/master@{#398519}

Patch Set 1 #

Patch Set 2 : 2016-05-24T18:21:56 #

Patch Set 3 : 2016-05-25T11:03:53 #

Patch Set 4 : 2016-05-25T16:05:55 #

Patch Set 5 : 2016-05-26T17:39:59 #

Patch Set 6 : 2016-05-27T17:09:07 #

Patch Set 7 : 2016-05-30T16:28:47 #

Patch Set 8 : 2016-05-31T11:02:12 #

Patch Set 9 : 2016-06-01T14:02:04 #

Patch Set 10 : 2016-06-06T17:09:04 #

Total comments: 13

Patch Set 11 : 2016-06-08T12:48:46 #

Patch Set 12 : 2016-06-08T18:08:39 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+224 lines, -87 lines) Patch
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/editing/DOMSelection.cpp View 1 2 3 4 2 chunks +6 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/FrameCaret.h View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/FrameCaret.cpp View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/FrameSelection.h View 1 2 3 4 5 6 7 8 9 10 4 chunks +7 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/FrameSelection.cpp View 1 2 3 4 5 6 7 8 9 10 22 chunks +68 lines, -52 lines 0 comments Download
M third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -19 lines 0 comments Download
M third_party/WebKit/Source/core/editing/SelectionController.cpp View 1 2 3 4 5 6 7 8 9 10 8 chunks +33 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/SelectionEditor.h View 1 2 3 4 5 6 7 8 9 2 chunks +8 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/SelectionEditor.cpp View 1 2 3 4 5 6 7 8 9 10 5 chunks +57 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoader.cpp View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/page/DragController.cpp View 1 2 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebViewImpl.cpp View 1 2 3 4 5 6 7 8 9 5 chunks +25 lines, -3 lines 0 comments Download

Messages

Total messages: 45 (30 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001083002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001083002/40001
4 years, 7 months ago (2016-05-25 02:27:30 UTC) #6
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/233432)
4 years, 7 months ago (2016-05-25 03:18:11 UTC) #8
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001083002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001083002/140001
4 years, 6 months ago (2016-05-31 09:12:56 UTC) #23
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 6 months ago (2016-05-31 13:39:01 UTC) #25
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001083002/180001
4 years, 6 months ago (2016-06-06 09:00:27 UTC) #27
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 6 months ago (2016-06-06 10:43:38 UTC) #29
yosin_UTC9
PTAL
4 years, 6 months ago (2016-06-07 04:32:12 UTC) #32
tkent
https://codereview.chromium.org/2001083002/diff/180001/third_party/WebKit/Source/core/editing/FrameSelection.cpp File third_party/WebKit/Source/core/editing/FrameSelection.cpp (left): https://codereview.chromium.org/2001083002/diff/180001/third_party/WebKit/Source/core/editing/FrameSelection.cpp#oldcode104 third_party/WebKit/Source/core/editing/FrameSelection.cpp:104: if (shouldAlwaysUseDirectionalSelection(m_frame)) Why can we remove this code block? ...
4 years, 6 months ago (2016-06-07 23:41:59 UTC) #33
yosin_UTC9
PTAL https://codereview.chromium.org/2001083002/diff/180001/third_party/WebKit/Source/core/editing/FrameSelection.cpp File third_party/WebKit/Source/core/editing/FrameSelection.cpp (left): https://codereview.chromium.org/2001083002/diff/180001/third_party/WebKit/Source/core/editing/FrameSelection.cpp#oldcode104 third_party/WebKit/Source/core/editing/FrameSelection.cpp:104: if (shouldAlwaysUseDirectionalSelection(m_frame)) On 2016/06/07 at 23:41:59, tkent wrote: ...
4 years, 6 months ago (2016-06-08 04:06:10 UTC) #34
tkent
lgtm
4 years, 6 months ago (2016-06-08 04:11:24 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001083002/200001
4 years, 6 months ago (2016-06-08 05:12:09 UTC) #37
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/240838)
4 years, 6 months ago (2016-06-08 06:04:35 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001083002/220001
4 years, 6 months ago (2016-06-08 09:10:05 UTC) #42
commit-bot: I haz the power
Committed patchset #12 (id:220001)
4 years, 6 months ago (2016-06-08 11:12:31 UTC) #43
commit-bot: I haz the power
4 years, 6 months ago (2016-06-08 11:14:05 UTC) #45
Message was sent while issue was closed. 
Patchset 12 (id:??) landed as
https://crrev.com/9c38a83b780e6cbd388579067b137ef12a39ff0c
Cr-Commit-Position: refs/heads/master@{#398519}

Powered by Google App Engine
This is Rietveld 408576698