Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2881013003:
Move FrameSelection::CurrentForm() to ContextMenuClientImpl (Closed)
DescriptionMove FrameSelection::CurrentForm() to ContextMenuClientImpl
This CL moved CurrentForm logic from FrameSelection to ContextMenuClientImpl.
Currently CurrentForm() was used only by ContextMenuClientImpl in
ContextMenuClientImpl::ShowContextMenu().
BUG=721653
Review-Url: https://codereview.chromium.org/2881013003
Cr-Commit-Position: refs/heads/master@{#473158}
Committed: https://chromium.googlesource.com/chromium/src/+/a34d0750b0cd0db8bdfef1e6a5f5b6be02e984ca
Patch Set 1 #
Total comments: 2
Patch Set 2 : updated patch #
Total comments: 13
Patch Set 3 : updated Patch #Patch Set 4 : updated PS #
Messages
Total messages: 33 (22 generated)
The CQ bit was checked by tanvir.rizvi@samsung.com 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== Move FrameSelection::CurrentForm() to ContextMenuClientImpl::CurrentForm This CL moved CurrentForm logic from FrameSelection to ContextMenuClientImpl. Currently CurrentForm() was used only by ContextMenuClientImpl in ContextMenuClientImpl::ShowContextMenu(). BUG=721653 ========== to ========== Move FrameSelection::CurrentForm() to ContextMenuClientImpl::CurrentForm This CL moved CurrentForm logic from FrameSelection to ContextMenuClientImpl. Currently CurrentForm() was used only by ContextMenuClientImpl in ContextMenuClientImpl::ShowContextMenu(). BUG=721653 ==========
tanvir.rizvi@samsung.com changed reviewers: + shanmuga.m@samsung.com, srirama.m@samsung.com
https://codereview.chromium.org/2881013003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/2881013003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:231: HTMLFormElement* ContextMenuClientImpl::CurrentForm( Should CurrentForm be member of ContextMenuClientImpl ? https://codereview.chromium.org/2881013003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/ContextMenuClientImpl.h (right): https://codereview.chromium.org/2881013003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ContextMenuClientImpl.h:41: class FrameSelection; Keep it in alphabetical order.
Remove extra space between "moved" and "CurrentForm" in description.
Description was changed from ========== Move FrameSelection::CurrentForm() to ContextMenuClientImpl::CurrentForm This CL moved CurrentForm logic from FrameSelection to ContextMenuClientImpl. Currently CurrentForm() was used only by ContextMenuClientImpl in ContextMenuClientImpl::ShowContextMenu(). BUG=721653 ========== to ========== Move FrameSelection::CurrentForm() to ContextMenuClientImpl This CL moved CurrentForm logic from FrameSelection to ContextMenuClientImpl. Currently CurrentForm() was used only by ContextMenuClientImpl in ContextMenuClientImpl::ShowContextMenu(). BUG=721653 ==========
tanvir.rizvi@samsung.com changed reviewers: + yosin@chromium.org
PTAL!!! Thanks
The CQ bit was checked by tanvir.rizvi@samsung.com 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
Thanks for working this! Almost good with nits. I would like to cleanup old style codes, e.g. 0 as nullptr, range-for for ancestors. https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:203: static inline HTMLFormElement* AssociatedFormElement(HTMLElement& element) { nit: s/inline // https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:212: return 0; nit: s/0/nullptr/ https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:226: return 0; nit: s/0/nullptr/ https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:234: Node* start = current_selection.GetDocument().FocusedElement(); nit: s/Node*/Node* const/ https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:241: return 0; nit: s/0/nullptr/ https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:244: for (HTMLElement* element = Let's use NodeTraversal::InclusiveAncestorsOf() to use range-for. We could write: for (Node& node : NodeTraversal::InclusiveAncestorsOf(*start)) { if (!isHTMLElement(node)) break; const HTMLElement& element = toHTMLElement(node); .... }
Patch updated. PTAL! https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:203: static inline HTMLFormElement* AssociatedFormElement(HTMLElement& element) { On 2017/05/16 09:57:33, yosin_UTC9 wrote: > nit: s/inline // Done. https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:212: return 0; On 2017/05/16 09:57:33, yosin_UTC9 wrote: > nit: s/0/nullptr/ Done. https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:226: return 0; On 2017/05/16 09:57:33, yosin_UTC9 wrote: > nit: s/0/nullptr/ Done. https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:226: return 0; On 2017/05/16 09:57:33, yosin_UTC9 wrote: > nit: s/0/nullptr/ Done. https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:234: Node* start = current_selection.GetDocument().FocusedElement(); On 2017/05/16 09:57:33, yosin_UTC9 wrote: > nit: s/Node*/Node* const/ We are assigning value below to start when !start. https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:241: return 0; On 2017/05/16 09:57:33, yosin_UTC9 wrote: > nit: s/0/nullptr/ Done. https://codereview.chromium.org/2881013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:244: for (HTMLElement* element = On 2017/05/16 09:57:33, yosin_UTC9 wrote: > Let's use NodeTraversal::InclusiveAncestorsOf() to use range-for. > We could write: > > for (Node& node : NodeTraversal::InclusiveAncestorsOf(*start)) { > if (!isHTMLElement(node)) > break; > const HTMLElement& element = toHTMLElement(node); > .... > } done. here also const not added as because of line num 205, "return &toHTMLFormElement(element);"
The CQ bit was checked by yosin@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
yosin@chromium.org changed reviewers: + tkent@chromium.org
lgtm +tkent@ for OWNERS review. Trybots, linux, mac, and win, failures are: * mojo/module-loading-manual-deps-loading.html * mojo/module-loading.html They are not related to this patch.
lgtm
The CQ bit was checked by shanmuga.m@samsung.com
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by yosin@chromium.org
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": 60001, "attempt_start_ts": 1495190045288970,
"parent_rev": "e66c783498c98f236876f2c70f7688fce46eb96d", "commit_rev":
"a34d0750b0cd0db8bdfef1e6a5f5b6be02e984ca"}
Message was sent while issue was closed.
Description was changed from ========== Move FrameSelection::CurrentForm() to ContextMenuClientImpl This CL moved CurrentForm logic from FrameSelection to ContextMenuClientImpl. Currently CurrentForm() was used only by ContextMenuClientImpl in ContextMenuClientImpl::ShowContextMenu(). BUG=721653 ========== to ========== Move FrameSelection::CurrentForm() to ContextMenuClientImpl This CL moved CurrentForm logic from FrameSelection to ContextMenuClientImpl. Currently CurrentForm() was used only by ContextMenuClientImpl in ContextMenuClientImpl::ShowContextMenu(). BUG=721653 Review-Url: https://codereview.chromium.org/2881013003 Cr-Commit-Position: refs/heads/master@{#473158} Committed: https://chromium.googlesource.com/chromium/src/+/a34d0750b0cd0db8bdfef1e6a5f5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a34d0750b0cd0db8bdfef1e6a5f5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
