Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2849883002:
Cleanup determining ibeam for node. (Closed)
DescriptionCleanup determining ibeam for node.
Change
https://chromium.googlesource.com/chromium/src/+/d892f9592860691ae9a782c12260c94ed6bd1a63%5E%21/#F30
caused the layout object to possibly be invalidated. Collapse the code
so that LayoutObject is correclty scoped so we don't run this risk.
BUG=712459
Review-Url: https://codereview.chromium.org/2849883002
Cr-Commit-Position: refs/heads/master@{#468045}
Committed: https://chromium.googlesource.com/chromium/src/+/9d6b6c40b933564260a0a70567b90ac30656222a
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix nits #Messages
Total messages: 18 (12 generated)
Description was changed from ========== Cleanup determining ibeam for node. Change https://chromium.googlesource.com/chromium/src/+/d892f9592860691ae9a782c12260... caused the layout object to possibly be invalidated. Collapse the code so that LayoutObject is correclty scoped so we don't run this risk. BUG=712459 ========== to ========== Cleanup determining ibeam for node. Change https://chromium.googlesource.com/chromium/src/+/d892f9592860691ae9a782c12260... caused the layout object to possibly be invalidated. Collapse the code so that LayoutObject is correclty scoped so we don't run this risk. BUG=712459 ==========
dtapuska@chromium.org changed reviewers: + jbroman@chromium.org
dtapuska@chromium.org changed reviewers: + yosin@google.com - jbroman@chromium.org
On 2017/04/28 15:07:41, dtapuska wrote: Reviewer changed to yosin@ since he has the context.
jbroman@chromium.org changed reviewers: + jbroman@chromium.org, yosin@chromium.org - yosin@google.com
Alright; I think I understand this well enough to give it an LGTM, since yosin is OOO for the next week. It might be a good idea to have him take a second look (e.g. maybe this layout causes other surprises elsewhere in editing). https://codereview.chromium.org/2849883002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2849883002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:121: bool ShowIBeamForNode(const Node* node, const HitTestResult& result) { nit: not clear if this function shows it or is simply a policy method; consider "ShouldShowIBeamForNode" or similar? https://codereview.chromium.org/2849883002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:122: if (!node || result.GetScrollbar()) nit: the caller already checks for GetScrollbar, I think? https://codereview.chromium.org/2849883002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:569: if (mouse_event_manager_->MousePressed() && nit: consider comment that this causes layout to avoid reintroducing the issue (but maybe want to be suitably vague)?
The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run
https://codereview.chromium.org/2849883002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2849883002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:121: bool ShowIBeamForNode(const Node* node, const HitTestResult& result) { On 2017/04/28 15:35:48, jbroman wrote: > nit: not clear if this function shows it or is simply a policy method; consider > "ShouldShowIBeamForNode" or similar? Done. https://codereview.chromium.org/2849883002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:122: if (!node || result.GetScrollbar()) On 2017/04/28 15:35:48, jbroman wrote: > nit: the caller already checks for GetScrollbar, I think? Done. https://codereview.chromium.org/2849883002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:569: if (mouse_event_manager_->MousePressed() && On 2017/04/28 15:35:49, jbroman wrote: > nit: consider comment that this causes layout to avoid reintroducing the issue > (but maybe want to be suitably vague)? Done.
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.
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/2849883002/#ps20001 (title: "Fix nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493400271766880,
"parent_rev": "0d25864a9695b631e4c6da7037ba0c6cb0854f30", "commit_rev":
"9d6b6c40b933564260a0a70567b90ac30656222a"}
Message was sent while issue was closed.
Description was changed from ========== Cleanup determining ibeam for node. Change https://chromium.googlesource.com/chromium/src/+/d892f9592860691ae9a782c12260... caused the layout object to possibly be invalidated. Collapse the code so that LayoutObject is correclty scoped so we don't run this risk. BUG=712459 ========== to ========== Cleanup determining ibeam for node. Change https://chromium.googlesource.com/chromium/src/+/d892f9592860691ae9a782c12260... caused the layout object to possibly be invalidated. Collapse the code so that LayoutObject is correclty scoped so we don't run this risk. BUG=712459 Review-Url: https://codereview.chromium.org/2849883002 Cr-Commit-Position: refs/heads/master@{#468045} Committed: https://chromium.googlesource.com/chromium/src/+/9d6b6c40b933564260a0a70567b9... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/9d6b6c40b933564260a0a70567b9... |
