Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1815453002:
Handle empty objects in IsSameTextObject() and free from warning (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. |
DescriptionHandle empty objects in IsSameTextObject() and free from warning
When both text objects have no items, directly return TRUE;
The last if statement moved inside loop to make the intent more
obvious and free from warning (msvc warns using potentially
uninitialized var itemPer)
R=jun_fang@foxitsoftware.com
Committed: https://pdfium.googlesource.com/pdfium/+/34fa8d90ae2f60fae219e4dbeff14c053d2e8eef
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #Messages
Total messages: 11 (5 generated)
Description was changed from ========== Handle empty objects in IsSameTextObject() and free from warning ========== to ========== Handle empty objects in IsSameTextObject() and free from warning When both text objects have no items, directly return TRUE; The last if statement moved inside loop to make the intent more obvious and free from warning (msvc warns using potentially uninitialized var itemPer) ==========
weili@chromium.org changed reviewers: + jun_fang@foxitsoftware.com, tsepez@chromium.org
Description was changed from ========== Handle empty objects in IsSameTextObject() and free from warning When both text objects have no items, directly return TRUE; The last if statement moved inside loop to make the intent more obvious and free from warning (msvc warns using potentially uninitialized var itemPer) ========== to ========== Handle empty objects in IsSameTextObject() and free from warning When both text objects have no items, directly return TRUE; The last if statement moved inside loop to make the intent more obvious and free from warning (msvc warns using potentially uninitialized var itemPer) ==========
@jun, please take a look whether the last if stmt is intended to be run only for the last char. Empty objects are not a problem for now since looks like we won't create text objects when there is no content. But handling it to make the function complete.
https://codereview.chromium.org/1815453002/diff/1/core/fpdftext/fpdf_text_int... File core/fpdftext/fpdf_text_int.cpp (left): https://codereview.chromium.org/1815453002/diff/1/core/fpdftext/fpdf_text_int... core/fpdftext/fpdf_text_int.cpp:1813: if (FXSYS_fabs(pTextObj1->GetPosX() - pTextObj2->GetPosX()) > Code here is used to check whether the distance of two text objects exceeds a specific range (calculated by char width and font size). If two text objects are close enough and them have the same content, they are considered as the same object. Otherwise, they are different objects. Putting code inside of loop may make readers a little bit confused. I think we should keep it outside of loop. https://codereview.chromium.org/1815453002/diff/1/core/fpdftext/fpdf_text_int... File core/fpdftext/fpdf_text_int.cpp (right): https://codereview.chromium.org/1815453002/diff/1/core/fpdftext/fpdf_text_int... core/fpdftext/fpdf_text_int.cpp:1814: if (i == nPreCount - 1) { If I work on this issue, I may do code change like: CPDF_TextPage::IsSameTextObject(CPDF_TextObject* pTextObj1, if (nPreCount != nCurCount) { return FALSE; } - CPDF_TextObjectItem itemPer, itemCur; + CPDF_TextObjectItem itemPer = {0, 0.0f, 0.0f}; + CPDF_TextObjectItem itemCur = {0, 0.0f, 0.0f}; for (int i = 0; i < nPreCount; i++) { pTextObj2->GetItemInfo(i, &itemPer); pTextObj1->GetItemInfo(i, &itemCur); @@ -1810,6 +1811,8 @@ FX_BOOL CPDF_TextPage::IsSameTextObject(CPDF_TextObject* pTextObj1, return FALSE; } } + if (!itemPer.m_CharCode) + return TRUE; if (FXSYS_fabs(pTextObj1->GetPosX() - pTextObj2->GetPosX()) > GetCharWidth(itemPer.m_CharCode, pTextObj2->GetFont()) * pTextObj2->GetFontSize() / 1000 * 0.9 ||
https://codereview.chromium.org/1815453002/diff/1/core/fpdftext/fpdf_text_int... File core/fpdftext/fpdf_text_int.cpp (right): https://codereview.chromium.org/1815453002/diff/1/core/fpdftext/fpdf_text_int... core/fpdftext/fpdf_text_int.cpp:1814: if (i == nPreCount - 1) { On 2016/03/18 05:31:48, jun_fang wrote: > If I work on this issue, I may do code change like: > > CPDF_TextPage::IsSameTextObject(CPDF_TextObject* pTextObj1, > if (nPreCount != nCurCount) { > return FALSE; > } > - CPDF_TextObjectItem itemPer, itemCur; > + CPDF_TextObjectItem itemPer = {0, 0.0f, 0.0f}; > + CPDF_TextObjectItem itemCur = {0, 0.0f, 0.0f}; > for (int i = 0; i < nPreCount; i++) { > pTextObj2->GetItemInfo(i, &itemPer); > pTextObj1->GetItemInfo(i, &itemCur); > @@ -1810,6 +1811,8 @@ FX_BOOL CPDF_TextPage::IsSameTextObject(CPDF_TextObject* > pTextObj1, > return FALSE; > } > } > + if (!itemPer.m_CharCode) > + return TRUE; > if (FXSYS_fabs(pTextObj1->GetPosX() - pTextObj2->GetPosX()) > > GetCharWidth(itemPer.m_CharCode, pTextObj2->GetFont()) * > pTextObj2->GetFontSize() / 1000 * 0.9 || Ok, this is close to what Wei originally suggested. Lets take this, then. Thanks for clarifiying.
Patchset #2 (id:20001) has been deleted
thanks https://codereview.chromium.org/1815453002/diff/1/core/fpdftext/fpdf_text_int... File core/fpdftext/fpdf_text_int.cpp (right): https://codereview.chromium.org/1815453002/diff/1/core/fpdftext/fpdf_text_int... core/fpdftext/fpdf_text_int.cpp:1814: if (i == nPreCount - 1) { On 2016/03/18 05:31:48, jun_fang wrote: > If I work on this issue, I may do code change like: > > CPDF_TextPage::IsSameTextObject(CPDF_TextObject* pTextObj1, > if (nPreCount != nCurCount) { > return FALSE; > } > - CPDF_TextObjectItem itemPer, itemCur; > + CPDF_TextObjectItem itemPer = {0, 0.0f, 0.0f}; > + CPDF_TextObjectItem itemCur = {0, 0.0f, 0.0f}; > for (int i = 0; i < nPreCount; i++) { > pTextObj2->GetItemInfo(i, &itemPer); > pTextObj1->GetItemInfo(i, &itemCur); > @@ -1810,6 +1811,8 @@ FX_BOOL CPDF_TextPage::IsSameTextObject(CPDF_TextObject* > pTextObj1, > return FALSE; > } > } > + if (!itemPer.m_CharCode) > + return TRUE; > if (FXSYS_fabs(pTextObj1->GetPosX() - pTextObj2->GetPosX()) > > GetCharWidth(itemPer.m_CharCode, pTextObj2->GetFont()) * > pTextObj2->GetFontSize() / 1000 * 0.9 || Thank you for the clarification and suggestion. I put the condition back outside loop now. For empty objects, I think it is better to return early based on the item count since 0 is still a valid charcode.
On 2016/03/18 16:59:44, Wei Li wrote: > thanks > > https://codereview.chromium.org/1815453002/diff/1/core/fpdftext/fpdf_text_int... > File core/fpdftext/fpdf_text_int.cpp (right): > > https://codereview.chromium.org/1815453002/diff/1/core/fpdftext/fpdf_text_int... > core/fpdftext/fpdf_text_int.cpp:1814: if (i == nPreCount - 1) { > On 2016/03/18 05:31:48, jun_fang wrote: > > If I work on this issue, I may do code change like: > > > > CPDF_TextPage::IsSameTextObject(CPDF_TextObject* pTextObj1, > > if (nPreCount != nCurCount) { > > return FALSE; > > } > > - CPDF_TextObjectItem itemPer, itemCur; > > + CPDF_TextObjectItem itemPer = {0, 0.0f, 0.0f}; > > + CPDF_TextObjectItem itemCur = {0, 0.0f, 0.0f}; > > for (int i = 0; i < nPreCount; i++) { > > pTextObj2->GetItemInfo(i, &itemPer); > > pTextObj1->GetItemInfo(i, &itemCur); > > @@ -1810,6 +1811,8 @@ FX_BOOL CPDF_TextPage::IsSameTextObject(CPDF_TextObject* > > pTextObj1, > > return FALSE; > > } > > } > > + if (!itemPer.m_CharCode) > > + return TRUE; > > if (FXSYS_fabs(pTextObj1->GetPosX() - pTextObj2->GetPosX()) > > > GetCharWidth(itemPer.m_CharCode, pTextObj2->GetFont()) * > > pTextObj2->GetFontSize() / 1000 * 0.9 || > > Thank you for the clarification and suggestion. I put the condition back outside > loop now. For empty objects, I think it is better to return early based on the > item count since 0 is still a valid charcode. Thanks! LGTM
Description was changed from ========== Handle empty objects in IsSameTextObject() and free from warning When both text objects have no items, directly return TRUE; The last if statement moved inside loop to make the intent more obvious and free from warning (msvc warns using potentially uninitialized var itemPer) ========== to ========== Handle empty objects in IsSameTextObject() and free from warning When both text objects have no items, directly return TRUE; The last if statement moved inside loop to make the intent more obvious and free from warning (msvc warns using potentially uninitialized var itemPer) R=jun_fang@foxitsoftware.com Committed: https://pdfium.googlesource.com/pdfium/+/34fa8d90ae2f60fae219e4dbeff14c053d2e... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) manually as 34fa8d90ae2f60fae219e4dbeff14c053d2e8eef (presubmit successful). |
