Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1809073004:
Fix CPDFSDK_Widget::OnXFAAAction() (Closed)
|
Created:
4 years, 9 months ago by Wei Li Modified:
4 years, 9 months ago CC:
pdfium-reviews_googlegroups.com Base URL:
https://pdfium.googlesource.com/pdfium.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionFix CPDFSDK_Widget::OnXFAAAction()
Fix a couple issues with OnXFAAAction():
-- radio button widget should not call ProcessEvent() twice;
-- UpdateDocView should be called before returning;
R=jun_fang@foxitsoftware.com
Committed: https://pdfium.googlesource.com/pdfium/+/42d1c1cca78f9cd2f64136dcf99fad207c9f4099
Patch Set 1 : #
Total comments: 12
Patch Set 2 : address comments #Messages
Total messages: 10 (4 generated)
weili@chromium.org changed reviewers: + jun_fang@foxitsoftware.com, tsepez@chromium.org
@jun, I am not very certain about this. Please check. thanks!
Patchset #1 (id:1) has been deleted
Description was changed from ========== Fix CPDFSDK_Widget::OnXFAAAction() Fix a couple issues with OnXFAAAction(): -- radio button widget will call ProcessEvent() twice; -- UpdateDocView will never be called; ========== to ========== Fix CPDFSDK_Widget::OnXFAAAction() Fix a couple issues with OnXFAAAction(): -- radio button widget should not call ProcessEvent() twice; -- UpdateDocView should be called before returning; ==========
https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.cpp File fpdfsdk/fsdk_baseform.cpp (left): https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... fpdfsdk/fsdk_baseform.cpp:236: if (IXFA_Widget* hGroupWidget = GetGroupMixXFAWidget()) { When users click a radio button, the clicking event is passed to its group widget first. This gives its group widget a chance to handle the event. Later, this event will be passed to the radio button itself. https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... fpdfsdk/fsdk_baseform.cpp:239: pXFAWidgetHandler->ProcessEvent(pAcc, ¶m); Pass the event to a group widget of radio button. https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... fpdfsdk/fsdk_baseform.cpp:243: CXFA_WidgetAcc* pAcc = pXFAWidgetHandler->GetDataAcc(hWidget); hWidget here is different with hGroupWidget. A group widget may have more than one widget. https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... fpdfsdk/fsdk_baseform.cpp:245: int32_t nRet = pXFAWidgetHandler->ProcessEvent(pAcc, ¶m); Pass the event to radio button itself. https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.cpp File fpdfsdk/fsdk_baseform.cpp (right): https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... fpdfsdk/fsdk_baseform.cpp:239: nRet = pXFAWidgetHandler->ProcessEvent(pAcc, ¶m); pAcc should be updated to param.m_pTarget in your patch. https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... fpdfsdk/fsdk_baseform.cpp:242: nRet = pXFAWidgetHandler->ProcessEvent(pAcc, ¶m); The same as above.
Thanks for the review and clarification. PTAL. https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.cpp File fpdfsdk/fsdk_baseform.cpp (left): https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... fpdfsdk/fsdk_baseform.cpp:236: if (IXFA_Widget* hGroupWidget = GetGroupMixXFAWidget()) { On 2016/03/21 06:21:46, jun_fang wrote: > When users click a radio button, the clicking event is passed to its group > widget first. This gives its group widget a chance to handle the event. Later, > this event will be passed to the radio button itself. Thank you for clarifying this! https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... fpdfsdk/fsdk_baseform.cpp:239: pXFAWidgetHandler->ProcessEvent(pAcc, ¶m); On 2016/03/21 06:21:46, jun_fang wrote: > Pass the event to a group widget of radio button. Acknowledged. https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... fpdfsdk/fsdk_baseform.cpp:243: CXFA_WidgetAcc* pAcc = pXFAWidgetHandler->GetDataAcc(hWidget); On 2016/03/21 06:21:46, jun_fang wrote: > hWidget here is different with hGroupWidget. A group widget may have more than > one widget. Acknowledged. https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... fpdfsdk/fsdk_baseform.cpp:245: int32_t nRet = pXFAWidgetHandler->ProcessEvent(pAcc, ¶m); On 2016/03/21 06:21:46, jun_fang wrote: > Pass the event to radio button itself. Acknowledged. https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.cpp File fpdfsdk/fsdk_baseform.cpp (right): https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... fpdfsdk/fsdk_baseform.cpp:239: nRet = pXFAWidgetHandler->ProcessEvent(pAcc, ¶m); On 2016/03/21 06:21:46, jun_fang wrote: > pAcc should be updated to param.m_pTarget in your patch. Done. https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... fpdfsdk/fsdk_baseform.cpp:242: nRet = pXFAWidgetHandler->ProcessEvent(pAcc, ¶m); On 2016/03/21 06:21:46, jun_fang wrote: > The same as above. Done.
On 2016/03/21 20:24:53, Wei Li wrote: > Thanks for the review and clarification. PTAL. > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.cpp > File fpdfsdk/fsdk_baseform.cpp (left): > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... > fpdfsdk/fsdk_baseform.cpp:236: if (IXFA_Widget* hGroupWidget = > GetGroupMixXFAWidget()) { > On 2016/03/21 06:21:46, jun_fang wrote: > > When users click a radio button, the clicking event is passed to its group > > widget first. This gives its group widget a chance to handle the event. Later, > > this event will be passed to the radio button itself. > > Thank you for clarifying this! > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... > fpdfsdk/fsdk_baseform.cpp:239: pXFAWidgetHandler->ProcessEvent(pAcc, ¶m); > On 2016/03/21 06:21:46, jun_fang wrote: > > Pass the event to a group widget of radio button. > > Acknowledged. > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... > fpdfsdk/fsdk_baseform.cpp:243: CXFA_WidgetAcc* pAcc = > pXFAWidgetHandler->GetDataAcc(hWidget); > On 2016/03/21 06:21:46, jun_fang wrote: > > hWidget here is different with hGroupWidget. A group widget may have more than > > one widget. > > Acknowledged. > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... > fpdfsdk/fsdk_baseform.cpp:245: int32_t nRet = > pXFAWidgetHandler->ProcessEvent(pAcc, ¶m); > On 2016/03/21 06:21:46, jun_fang wrote: > > Pass the event to radio button itself. > > Acknowledged. > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.cpp > File fpdfsdk/fsdk_baseform.cpp (right): > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... > fpdfsdk/fsdk_baseform.cpp:239: nRet = pXFAWidgetHandler->ProcessEvent(pAcc, > ¶m); > On 2016/03/21 06:21:46, jun_fang wrote: > > pAcc should be updated to param.m_pTarget in your patch. > > Done. > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... > fpdfsdk/fsdk_baseform.cpp:242: nRet = pXFAWidgetHandler->ProcessEvent(pAcc, > ¶m); > On 2016/03/21 06:21:46, jun_fang wrote: > > The same as above. > > Done. LGTM. Thanks!
On 2016/03/21 20:24:53, Wei Li wrote: > Thanks for the review and clarification. PTAL. > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.cpp > File fpdfsdk/fsdk_baseform.cpp (left): > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... > fpdfsdk/fsdk_baseform.cpp:236: if (IXFA_Widget* hGroupWidget = > GetGroupMixXFAWidget()) { > On 2016/03/21 06:21:46, jun_fang wrote: > > When users click a radio button, the clicking event is passed to its group > > widget first. This gives its group widget a chance to handle the event. Later, > > this event will be passed to the radio button itself. > > Thank you for clarifying this! > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... > fpdfsdk/fsdk_baseform.cpp:239: pXFAWidgetHandler->ProcessEvent(pAcc, ¶m); > On 2016/03/21 06:21:46, jun_fang wrote: > > Pass the event to a group widget of radio button. > > Acknowledged. > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... > fpdfsdk/fsdk_baseform.cpp:243: CXFA_WidgetAcc* pAcc = > pXFAWidgetHandler->GetDataAcc(hWidget); > On 2016/03/21 06:21:46, jun_fang wrote: > > hWidget here is different with hGroupWidget. A group widget may have more than > > one widget. > > Acknowledged. > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... > fpdfsdk/fsdk_baseform.cpp:245: int32_t nRet = > pXFAWidgetHandler->ProcessEvent(pAcc, ¶m); > On 2016/03/21 06:21:46, jun_fang wrote: > > Pass the event to radio button itself. > > Acknowledged. > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.cpp > File fpdfsdk/fsdk_baseform.cpp (right): > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... > fpdfsdk/fsdk_baseform.cpp:239: nRet = pXFAWidgetHandler->ProcessEvent(pAcc, > ¶m); > On 2016/03/21 06:21:46, jun_fang wrote: > > pAcc should be updated to param.m_pTarget in your patch. > > Done. > > https://codereview.chromium.org/1809073004/diff/20001/fpdfsdk/fsdk_baseform.c... > fpdfsdk/fsdk_baseform.cpp:242: nRet = pXFAWidgetHandler->ProcessEvent(pAcc, > ¶m); > On 2016/03/21 06:21:46, jun_fang wrote: > > The same as above. > > Done. LGTM. Thanks!
Description was changed from ========== Fix CPDFSDK_Widget::OnXFAAAction() Fix a couple issues with OnXFAAAction(): -- radio button widget should not call ProcessEvent() twice; -- UpdateDocView should be called before returning; ========== to ========== Fix CPDFSDK_Widget::OnXFAAAction() Fix a couple issues with OnXFAAAction(): -- radio button widget should not call ProcessEvent() twice; -- UpdateDocView should be called before returning; R=jun_fang@foxitsoftware.com Committed: https://pdfium.googlesource.com/pdfium/+/42d1c1cca78f9cd2f64136dcf99fad207c9f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) manually as 42d1c1cca78f9cd2f64136dcf99fad207c9f4099 (presubmit successful). |
