Cleanup CPDFXFA_Document.

fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp

Index: fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp
diff --git a/fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp b/fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp
index 2944ed6c2ea58a3c9e8326b190956d49629e368f..709cf058170743aa8bc061bd7f9efcdfa34e74cf 100644
--- a/fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp
+++ b/fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp
@@ -45,6 +45,15 @@ extern void SetLastError(int err);
 extern int GetLastError();
 #endif
 
+namespace {
+
+CFX_FloatRect FXRect2PDFRect(const CFX_RectF& fxRectF) {
+  return CFX_FloatRect(fxRectF.left, fxRectF.bottom(), fxRectF.right(),

[Tom Sepez, 2016/09/08 16:53:36]

The code at old line 220 flipped top and bottom. Ditto with 314.

nit: Maybe there should be a CFX_FloatRect::FromCFXRectF() static method in the
CFX_FloatRect class.

[dsinclair, 2016/09/12 13:25:49]

Done.

+                       fxRectF.top);
+}
+
+}  // namespace
+
 CPDFXFA_Document::CPDFXFA_Document(std::unique_ptr<CPDF_Document> pPDFDoc,
                                    CPDFXFA_App* pProvider)
     : m_iDocType(DOCTYPE_PDF),
@@ -62,9 +71,8 @@ CPDFXFA_Document::~CPDFXFA_Document() {
     CXFA_FFApp* pApp = m_pApp->GetXFAApp();
     if (pApp) {
       CXFA_FFDocHandler* pDocHandler = pApp->GetDocHandler();
-      if (pDocHandler) {
+      if (pDocHandler)
         CloseXFADoc(pDocHandler);
-      }
     }
     m_pXFADoc.reset();
   }
@@ -217,18 +225,9 @@ CPDFSDK_Document* CPDFXFA_Document::GetSDKDocument(
   return m_pSDKDoc.get();
 }
 
-void CPDFXFA_Document::FXRect2PDFRect(const CFX_RectF& fxRectF,
-                                      CFX_FloatRect& pdfRect) {
-  pdfRect.left = fxRectF.left;
-  pdfRect.top = fxRectF.bottom();
-  pdfRect.right = fxRectF.right();
-  pdfRect.bottom = fxRectF.top;
-}
-
 void CPDFXFA_Document::SetChangeMark(CXFA_FFDoc* hDoc) {
-  if (hDoc == m_pXFADoc.get() && m_pSDKDoc) {
+  if (hDoc == m_pXFADoc.get() && m_pSDKDoc)
     m_pSDKDoc->SetChangeMark();
-  }
 }
 
 void CPDFXFA_Document::InvalidateRect(CXFA_FFPageView* pPageView,
@@ -240,9 +239,6 @@ void CPDFXFA_Document::InvalidateRect(CXFA_FFPageView* pPageView,
   if (m_iDocType != DOCTYPE_DYNAMIC_XFA)
     return;
 
-  CFX_FloatRect rcPage;
-  FXRect2PDFRect(rt, rcPage);
-
   CPDFXFA_Page* pPage = GetPage(pPageView);
   if (!pPage)
     return;
@@ -251,6 +247,7 @@ void CPDFXFA_Document::InvalidateRect(CXFA_FFPageView* pPageView,
   if (!pEnv)
     return;
 
+  CFX_FloatRect rcPage = FXRect2PDFRect(rt);
   pEnv->FFI_Invalidate((FPDF_PAGE)pPage, rcPage.left, rcPage.bottom,
                        rcPage.right, rcPage.top);
 }
@@ -276,13 +273,11 @@ void CPDFXFA_Document::DisplayCaret(CXFA_FFWidget* hWidget,
   if (!pPage)
     return;
 
-  CFX_FloatRect rcCaret;
-  FXRect2PDFRect(*pRtAnchor, rcCaret);
-
   CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
   if (!pEnv)
     return;
 
+  CFX_FloatRect rcCaret = FXRect2PDFRect(*pRtAnchor);
   pEnv->FFI_DisplayCaret((FPDF_PAGE)pPage, bVisible, rcCaret.left, rcCaret.top,
                          rcCaret.right, rcCaret.bottom);
 }
@@ -308,60 +303,49 @@ FX_BOOL CPDFXFA_Document::GetPopupPos(CXFA_FFWidget* hWidget,
   CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
   if (!pEnv)
     return FALSE;
+
   FS_RECTF pageViewRect = {0.0f, 0.0f, 0.0f, 0.0f};
   pEnv->FFI_GetPageViewRect(pPage, pageViewRect);
 
-  CFX_FloatRect rcAnchor;
-  rcAnchor.left = rtAnchor.left;
-  rcAnchor.top = rtAnchor.bottom();
-  rcAnchor.right = rtAnchor.right();
-  rcAnchor.bottom = rtAnchor.top;
-
-  int t1, t2, t;
-  uint32_t dwPos;
-  FX_FLOAT fPoupHeight;
+  int t1;
+  int t2;
+  CFX_FloatRect rcAnchor = FXRect2PDFRect(rtAnchor);
   switch (nRotate) {
     case 90: {
       t1 = (int)(pageViewRect.right - rcAnchor.right);
       t2 = (int)(rcAnchor.left - pageViewRect.left);
-      if (rcAnchor.bottom < pageViewRect.bottom) {
+      if (rcAnchor.bottom < pageViewRect.bottom)
         rtPopup.left += rcAnchor.bottom - pageViewRect.bottom;
-      }
-
       break;
     }
-
     case 180: {
       t2 = (int)(pageViewRect.top - rcAnchor.top);
       t1 = (int)(rcAnchor.bottom - pageViewRect.bottom);
-      if (rcAnchor.left < pageViewRect.left) {
+      if (rcAnchor.left < pageViewRect.left)
         rtPopup.left += rcAnchor.left - pageViewRect.left;
-      }
       break;
     }
     case 270: {
       t1 = (int)(rcAnchor.left - pageViewRect.left);
       t2 = (int)(pageViewRect.right - rcAnchor.right);
-
-      if (rcAnchor.top > pageViewRect.top) {
+      if (rcAnchor.top > pageViewRect.top)
         rtPopup.left -= rcAnchor.top - pageViewRect.top;
-      }
       break;
     }
     case 0:
     default: {
       t1 = (int)(pageViewRect.top - rcAnchor.top);
       t2 = (int)(rcAnchor.bottom - pageViewRect.bottom);
-      if (rcAnchor.right > pageViewRect.right) {
+      if (rcAnchor.right > pageViewRect.right)
         rtPopup.left -= rcAnchor.right - pageViewRect.right;
-      }
       break;
     }
   }
 
-  if (t1 <= 0 && t2 <= 0) {
+  int t;
+  uint32_t dwPos;
+  if (t1 <= 0 && t2 <= 0)
     return FALSE;
-  }
   if (t1 <= 0) {
     t = t2;
     dwPos = 1;
@@ -375,23 +359,24 @@ FX_BOOL CPDFXFA_Document::GetPopupPos(CXFA_FFWidget* hWidget,
     t = t2;
     dwPos = 1;
   }
-  if (t < fMinPopup) {
-    fPoupHeight = fMinPopup;
-  } else if (t > fMaxPopup) {
-    fPoupHeight = fMaxPopup;
-  } else {
-    fPoupHeight = (FX_FLOAT)t;
-  }
+
+  FX_FLOAT fPopupHeight;
+  if (t < fMinPopup)
+    fPopupHeight = fMinPopup;
+  else if (t > fMaxPopup)
+    fPopupHeight = fMaxPopup;
+  else
+    fPopupHeight = static_cast<FX_FLOAT>(t);
 
   switch (nRotate) {
     case 0:
     case 180: {
       if (dwPos == 0) {
         rtPopup.top = rtAnchor.height;
-        rtPopup.height = fPoupHeight;
+        rtPopup.height = fPopupHeight;
       } else {
-        rtPopup.top = -fPoupHeight;
-        rtPopup.height = fPoupHeight;
+        rtPopup.top = -fPopupHeight;
+        rtPopup.height = fPopupHeight;
       }
       break;
     }
@@ -399,10 +384,10 @@ FX_BOOL CPDFXFA_Document::GetPopupPos(CXFA_FFWidget* hWidget,
     case 270: {
       if (dwPos == 0) {
         rtPopup.top = rtAnchor.width;
-        rtPopup.height = fPoupHeight;
+        rtPopup.height = fPopupHeight;
       } else {
-        rtPopup.top = -fPoupHeight;
-        rtPopup.height = fPoupHeight;
+        rtPopup.top = -fPopupHeight;
+        rtPopup.height = fPopupHeight;
       }
       break;
     }
@@ -414,8 +399,7 @@ FX_BOOL CPDFXFA_Document::GetPopupPos(CXFA_FFWidget* hWidget,
 }
 
 FX_BOOL CPDFXFA_Document::PopupMenu(CXFA_FFWidget* hWidget,
-                                    CFX_PointF ptPopup,
-                                    const CFX_RectF* pRectExclude) {
+                                    CFX_PointF ptPopup) {
   if (!hWidget)
     return FALSE;
 
@@ -427,8 +411,11 @@ FX_BOOL CPDFXFA_Document::PopupMenu(CXFA_FFWidget* hWidget,
   if (!pPage)
     return FALSE;
 
-  int menuFlag = 0;
+  CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
+  if (!pEnv)
+    return FALSE;
 
+  int menuFlag = 0;
   if (hWidget->CanUndo())
     menuFlag |= FXFA_MEMU_UNDO;

[Tom Sepez, 2016/09/08 16:53:36]

nit: MEMU is apparently a train in India, and is not a menu.  We should fix
this, even though its a public constant.  

[dsinclair, 2016/09/12 13:25:48]

Done.

   if (hWidget->CanRedo())
@@ -442,9 +429,7 @@ FX_BOOL CPDFXFA_Document::PopupMenu(CXFA_FFWidget* hWidget,
   if (hWidget->CanSelectAll())
     menuFlag |= FXFA_MEMU_SELECTALL;
 
-  CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
-  return pEnv &&
-         pEnv->FFI_PopupMenu(pPage, hWidget, menuFlag, ptPopup, nullptr);
+  return pEnv->FFI_PopupMenu(pPage, hWidget, menuFlag, ptPopup, nullptr);
 }
 
 void CPDFXFA_Document::PageViewEvent(CXFA_FFPageView* pPageView,
@@ -453,32 +438,34 @@ void CPDFXFA_Document::PageViewEvent(CXFA_FFPageView* pPageView,
   if (!pEnv)
     return;
 
-  if (m_nLoadStatus != FXFA_LOADSTATUS_LOADING &&
-      m_nLoadStatus != FXFA_LOADSTATUS_CLOSING &&
-      XFA_PAGEVIEWEVENT_StopLayout == dwFlags) {
-    int nNewCount = GetPageCount();
-    if (nNewCount == m_nPageCount)
-      return;
+  if (m_nLoadStatus == FXFA_LOADSTATUS_LOADING ||
+      m_nLoadStatus == FXFA_LOADSTATUS_CLOSING ||
+      XFA_PAGEVIEWEVENT_StopLayout != dwFlags)
+    return;
 
-    CXFA_FFDocView* pXFADocView = GetXFADocView();
-    if (!pXFADocView)
-      return;
-    for (int iPageIter = 0; iPageIter < m_nPageCount; iPageIter++) {
-      CPDFXFA_Page* pPage = m_XFAPageList.GetAt(iPageIter);
-      if (!pPage)
-        continue;
-      m_pSDKDoc->RemovePageView(pPage);
-      CXFA_FFPageView* pXFAPageView = pXFADocView->GetPageView(iPageIter);
-      pPage->SetXFAPageView(pXFAPageView);
-    }
+  int nNewCount = GetPageCount();
+  if (nNewCount == m_nPageCount)
+    return;
+
+  CXFA_FFDocView* pXFADocView = GetXFADocView();
+  if (!pXFADocView)
+    return;
 
-    int flag = (nNewCount < m_nPageCount) ? FXFA_PAGEVIEWEVENT_POSTREMOVED
-                                          : FXFA_PAGEVIEWEVENT_POSTADDED;
-    int count = FXSYS_abs(nNewCount - m_nPageCount);
-    m_nPageCount = nNewCount;
-    m_XFAPageList.SetSize(nNewCount);
-    pEnv->FFI_PageEvent(count, flag);
+  for (int iPageIter = 0; iPageIter < m_nPageCount; iPageIter++) {
+    CPDFXFA_Page* pPage = m_XFAPageList.GetAt(iPageIter);
+    if (!pPage)
+      continue;
+
+    m_pSDKDoc->RemovePageView(pPage);
+    pPage->SetXFAPageView(pXFADocView->GetPageView(iPageIter));
   }
+
+  int flag = (nNewCount < m_nPageCount) ? FXFA_PAGEVIEWEVENT_POSTREMOVED
+                                        : FXFA_PAGEVIEWEVENT_POSTADDED;
+  int count = FXSYS_abs(nNewCount - m_nPageCount);
+  m_nPageCount = nNewCount;
+  m_XFAPageList.SetSize(nNewCount);
+  pEnv->FFI_PageEvent(count, flag);
 }
 
 void CPDFXFA_Document::WidgetPostAdd(CXFA_FFWidget* hWidget,
@@ -540,6 +527,7 @@ void CPDFXFA_Document::SetCurrentPage(CXFA_FFDoc* hDoc, int32_t iCurPage) {
       iCurPage >= m_pSDKDoc->GetPageCount()) {
     return;
   }
+
   CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
   if (!pEnv)
     return;
@@ -551,7 +539,6 @@ FX_BOOL CPDFXFA_Document::IsCalculationsEnabled(CXFA_FFDoc* hDoc) {
     return FALSE;
   if (m_pSDKDoc->GetInterForm())
     return m_pSDKDoc->GetInterForm()->IsXfaCalculateEnabled();
-
   return FALSE;
 }
 
@@ -603,14 +590,15 @@ void CPDFXFA_Document::ExportData(CXFA_FFDoc* hDoc,
   if (wsFilePath.IsEmpty()) {
     if (!pEnv->GetFormFillInfo() || !pEnv->GetFormFillInfo()->m_pJsPlatform)
       return;
+
     CFX_WideString filepath = pEnv->JS_fieldBrowse();
     bs = filepath.UTF16LE_Encode();
   }
-  int len = bs.GetLength() / sizeof(unsigned short);

[dsinclair, 2016/09/08 15:13:51]

This pattern is repeated a few times in this file. len is len/sizeof(short) but
then every time we use len we do len * sizeof(short).

The len is a int, so there will be some precision lost on rounding, but I don't
see why we'd want that?

[Tom Sepez, 2016/09/08 16:53:36]

Maybe we don't want to pass an odd number of bytes as the lengh to the callback
for a buffer that is supposed to contain shorts?

Maybe call this byteLen in that case, and lets trunc downwards once here.

[Tom Sepez, 2016/09/08 16:55:07]

No, actually if it's smakller, getbuffer just leaves it alone if its already big
enough (which it is).

[dsinclair, 2016/09/12 13:25:48]

So, is there anything to be done here?

-  FPDF_FILEHANDLER* pFileHandler = pEnv->FFI_OpenFile(
-      bXDP ? FXFA_SAVEAS_XDP : FXFA_SAVEAS_XML,
-      (FPDF_WIDESTRING)bs.GetBuffer(len * sizeof(unsigned short)), "wb");
-  bs.ReleaseBuffer(len * sizeof(unsigned short));
+  int len = bs.GetLength();
+  FPDF_FILEHANDLER* pFileHandler =
+      pEnv->FFI_OpenFile(bXDP ? FXFA_SAVEAS_XDP : FXFA_SAVEAS_XML,
+                         (FPDF_WIDESTRING)bs.GetBuffer(len), "wb");
+  bs.ReleaseBuffer(len);
   if (!pFileHandler)
     return;
 
@@ -625,12 +613,15 @@ void CPDFXFA_Document::ExportData(CXFA_FFDoc* hDoc,
   } else if (fileType == FXFA_SAVEAS_XDP) {
     if (!m_pPDFDoc)
       return;
+
     CPDF_Dictionary* pRoot = m_pPDFDoc->GetRoot();
     if (!pRoot)
       return;
+
     CPDF_Dictionary* pAcroForm = pRoot->GetDictBy("AcroForm");
     if (!pAcroForm)
       return;
+
     CPDF_Array* pArray = ToArray(pAcroForm->GetObjectBy("XFA"));
     if (!pArray)
       return;
@@ -643,32 +634,35 @@ void CPDFXFA_Document::ExportData(CXFA_FFDoc* hDoc,
         continue;
       if (!pPDFObj->IsReference())
         continue;
+
       CPDF_Stream* pStream = ToStream(pPDFObj->GetDirect());
       if (!pStream)
         continue;
       if (pPrePDFObj->GetString() == "form") {
         m_pXFADocView->GetDoc()->SavePackage(XFA_HASHCODE_Form, &fileWrite,
                                              nullptr);
-      } else if (pPrePDFObj->GetString() == "datasets") {
+        continue;
+      }
+      if (pPrePDFObj->GetString() == "datasets") {
         m_pXFADocView->GetDoc()->SavePackage(XFA_HASHCODE_Datasets, &fileWrite,
                                              nullptr);
-      } else {
-        if (i == size - 1) {
-          CFX_WideString wPath = CFX_WideString::FromUTF16LE(
-              reinterpret_cast<const unsigned short*>(bs.c_str()),
-              bs.GetLength() / sizeof(unsigned short));
-          CFX_ByteString bPath = wPath.UTF8Encode();
-          const char* szFormat =
-              "\n<pdf href=\"%s\" xmlns=\"http://ns.adobe.com/xdp/pdf/\"/>";
-          content.Format(szFormat, bPath.c_str());
-          fileWrite.WriteBlock(content.c_str(), fileWrite.GetSize(),
-                               content.GetLength());
-        }
-        std::unique_ptr<CPDF_StreamAcc> pAcc(new CPDF_StreamAcc);
-        pAcc->LoadAllData(pStream);
-        fileWrite.WriteBlock(pAcc->GetData(), fileWrite.GetSize(),
-                             pAcc->GetSize());
+        continue;
       }
+      if (i == size - 1) {
+        CFX_WideString wPath = CFX_WideString::FromUTF16LE(
+            reinterpret_cast<const unsigned short*>(bs.c_str()),
+            bs.GetLength() / sizeof(unsigned short));
+        CFX_ByteString bPath = wPath.UTF8Encode();
+        const char* szFormat =
+            "\n<pdf href=\"%s\" xmlns=\"http://ns.adobe.com/xdp/pdf/\"/>";
+        content.Format(szFormat, bPath.c_str());
+        fileWrite.WriteBlock(content.c_str(), fileWrite.GetSize(),
+                             content.GetLength());
+      }
+      std::unique_ptr<CPDF_StreamAcc> pAcc(new CPDF_StreamAcc);
+      pAcc->LoadAllData(pStream);
+      fileWrite.WriteBlock(pAcc->GetData(), fileWrite.GetSize(),
+                           pAcc->GetSize());
     }
   }
   if (!fileWrite.Flush()) {
@@ -699,7 +693,6 @@ FX_BOOL CPDFXFA_Document::IsValidationsEnabled(CXFA_FFDoc* hDoc) {
     return FALSE;
   if (m_pSDKDoc->GetInterForm())
     return m_pSDKDoc->GetInterForm()->IsXfaValidationsEnabled();
-
   return TRUE;
 }
 
@@ -726,6 +719,7 @@ void CPDFXFA_Document::SetFocusWidget(CXFA_FFDoc* hDoc,
     CPDFSDK_PageView* pPageView = m_pSDKDoc->GetPageView(i);
     if (!pPageView)
       continue;
+
     CPDFSDK_Annot* pAnnot = pPageView->GetAnnotByXFAWidget(hWidget);
     if (pAnnot) {
       m_pSDKDoc->SetFocusAnnot(pAnnot);
@@ -756,29 +750,20 @@ void CPDFXFA_Document::Print(CXFA_FFDoc* hDoc,
       dwOptions & XFA_PRINTOPT_PrintAnnot);
 }
 
-void CPDFXFA_Document::GetURL(CXFA_FFDoc* hDoc, CFX_WideString& wsDocURL) {
-  if (hDoc != m_pXFADoc.get())
-    return;
-
-  CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
-  if (!pEnv)
-    return;
-
-  pEnv->FFI_GetURL(this, wsDocURL);
-}
-
 FX_ARGB CPDFXFA_Document::GetHighlightColor(CXFA_FFDoc* hDoc) {
   if (hDoc != m_pXFADoc.get())
     return 0;
-  if (m_pSDKDoc) {
-    if (CPDFSDK_InterForm* pInterForm = m_pSDKDoc->GetInterForm()) {
-      FX_COLORREF color = pInterForm->GetHighlightColor(FPDF_FORMFIELD_XFA);
-      uint8_t alpha = pInterForm->GetHighlightAlpha();
-      FX_ARGB argb = ArgbEncode((int)alpha, color);
-      return argb;
-    }
-  }
-  return 0;
+  if (!m_pSDKDoc)

[Tom Sepez, 2016/09/08 16:53:36]

nit: maybe combine with 754

[dsinclair, 2016/09/12 13:25:49]

Done.

+    return 0;
+
+  CPDFSDK_InterForm* pInterForm = m_pSDKDoc->GetInterForm();
+  if (!pInterForm)
+    return 0;
+
+  FX_COLORREF color = pInterForm->GetHighlightColor(FPDF_FORMFIELD_XFA);
+  uint8_t alpha = pInterForm->GetHighlightAlpha();
+  FX_ARGB argb = ArgbEncode((int)alpha, color);

[Tom Sepez, 2016/09/08 16:53:36]

nit: local argb not needed (nor are the others, but it gets kinda ugly when you
write it without alpha&color locals, but maybe that way too).
(int) cast not needed to pass uint8 to int.

[dsinclair, 2016/09/12 13:25:48]

Done.

+  return argb;
 }
 
 FX_BOOL CPDFXFA_Document::NotifySubmit(FX_BOOL bPrevOrPost) {
@@ -810,29 +795,31 @@ FX_BOOL CPDFXFA_Document::OnBeforeNotifySubmit() {
   }
 
   pWidgetAccIterator.reset(m_pXFADocView->CreateWidgetAccIterator());
-  if (pWidgetAccIterator) {
-    CXFA_WidgetAcc* pWidgetAcc = pWidgetAccIterator->MoveToNext();
-    pWidgetAcc = pWidgetAccIterator->MoveToNext();
-    while (pWidgetAcc) {
-      int fRet = pWidgetAcc->ProcessValidate(-1);
-      if (fRet == XFA_EVENTERROR_Error) {
-        CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
-        if (!pEnv)
-          return FALSE;
-        CFX_WideString ws;
-        ws.FromLocal(IDS_XFA_Validate_Input);
-        CFX_ByteString bs = ws.UTF16LE_Encode();
-        int len = bs.GetLength() / sizeof(unsigned short);
-        pEnv->FFI_Alert(
-            (FPDF_WIDESTRING)bs.GetBuffer(len * sizeof(unsigned short)),
-            (FPDF_WIDESTRING)L"", 0, 1);
-        bs.ReleaseBuffer(len * sizeof(unsigned short));
+  if (!pWidgetAccIterator)
+    return TRUE;
+
+  CXFA_WidgetAcc* pWidgetAcc = pWidgetAccIterator->MoveToNext();
+  pWidgetAcc = pWidgetAccIterator->MoveToNext();
+  while (pWidgetAcc) {
+    int fRet = pWidgetAcc->ProcessValidate(-1);
+    if (fRet == XFA_EVENTERROR_Error) {
+      CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
+      if (!pEnv)
         return FALSE;
-      }
-      pWidgetAcc = pWidgetAccIterator->MoveToNext();
+
+      CFX_WideString ws;
+      ws.FromLocal(IDS_XFA_Validate_Input);
+      CFX_ByteString bs = ws.UTF16LE_Encode();
+      int len = bs.GetLength();
+      pEnv->FFI_Alert((FPDF_WIDESTRING)bs.GetBuffer(len), (FPDF_WIDESTRING)L"",
+                      0, 1);
+      bs.ReleaseBuffer(len);
+      return FALSE;
     }
-    m_pXFADocView->UpdateDocView();
+    pWidgetAcc = pWidgetAccIterator->MoveToNext();
   }
+  m_pXFADocView->UpdateDocView();
+
   return TRUE;
 }
 
@@ -865,8 +852,8 @@ void CPDFXFA_Document::OnAfterNotifySubmit() {
 FX_BOOL CPDFXFA_Document::SubmitData(CXFA_FFDoc* hDoc, CXFA_Submit submit) {
   if (!NotifySubmit(TRUE) || !m_pXFADocView)
     return FALSE;
-  m_pXFADocView->UpdateDocView();
 
+  m_pXFADocView->UpdateDocView();
   FX_BOOL ret = SubmitDataInternal(hDoc, submit);
   NotifySubmit(FALSE);
   return ret;
@@ -877,11 +864,12 @@ IFX_FileRead* CPDFXFA_Document::OpenLinkedFile(CXFA_FFDoc* hDoc,
   CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
   if (!pEnv)
     return FALSE;
+
   CFX_ByteString bs = wsLink.UTF16LE_Encode();
-  int len = bs.GetLength() / sizeof(unsigned short);
-  FPDF_FILEHANDLER* pFileHandler = pEnv->FFI_OpenFile(
-      0, (FPDF_WIDESTRING)bs.GetBuffer(len * sizeof(unsigned short)), "rb");
-  bs.ReleaseBuffer(len * sizeof(unsigned short));
+  int len = bs.GetLength();
+  FPDF_FILEHANDLER* pFileHandler =
+      pEnv->FFI_OpenFile(0, (FPDF_WIDESTRING)bs.GetBuffer(len), "rb");

[Tom Sepez, 2016/09/08 16:53:36]

pre-existing:  casting a buffer of bytestring to widestring seems wrong, except
that it is 16 bit le encoded.  See below

[dsinclair, 2016/09/12 13:25:49]

Acknowledged.

+  bs.ReleaseBuffer(len);
 
   if (!pFileHandler)
     return nullptr;
@@ -901,7 +889,6 @@ FX_BOOL CPDFXFA_Document::ExportSubmitFile(FPDF_FILEHANDLER* pFileHandler,
     return FALSE;
 
   CFPDF_FileStream fileStream(pFileHandler);
-
   if (fileType == FXFA_SAVEAS_XML) {
     const char kContent[] = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\r\n";
     fileStream.WriteBlock(kContent, 0, strlen(kContent));
@@ -920,16 +907,19 @@ FX_BOOL CPDFXFA_Document::ExportSubmitFile(FPDF_FILEHANDLER* pFileHandler,
     fileStream.Flush();
     return FALSE;
   }
+
   CPDF_Dictionary* pRoot = m_pPDFDoc->GetRoot();
   if (!pRoot) {
     fileStream.Flush();
     return FALSE;
   }
+
   CPDF_Dictionary* pAcroForm = pRoot->GetDictBy("AcroForm");
   if (!pAcroForm) {
     fileStream.Flush();
     return FALSE;
   }
+
   CPDF_Array* pArray = ToArray(pAcroForm->GetObjectBy("XFA"));
   if (!pArray) {
     fileStream.Flush();
@@ -944,6 +934,7 @@ FX_BOOL CPDFXFA_Document::ExportSubmitFile(FPDF_FILEHANDLER* pFileHandler,
       continue;
     if (!pPDFObj->IsReference())
       continue;
+
     CPDF_Object* pDirectObj = pPDFObj->GetDirect();
     if (!pDirectObj->IsStream())
       continue;
@@ -993,9 +984,10 @@ void CPDFXFA_Document::ToXFAContentFlags(CFX_WideString csSrcContent,
     flag |= FXFA_XFDF;
   if (csSrcContent.Find(L" form ", 0) != -1)
     flag |= FXFA_FORM;
-  if (flag == 0)
+  if (flag == 0) {
     flag = FXFA_CONFIG | FXFA_TEMPLATE | FXFA_LOCALESET | FXFA_DATASETS |
            FXFA_XMPMETA | FXFA_XFDF | FXFA_FORM;
+  }
 }
 
 FX_BOOL CPDFXFA_Document::MailToInfo(CFX_WideString& csURL,
@@ -1006,8 +998,9 @@ FX_BOOL CPDFXFA_Document::MailToInfo(CFX_WideString& csURL,
                                      CFX_WideString& csMsg) {
   CFX_WideString srcURL = csURL;
   srcURL.TrimLeft();
-  if (0 != srcURL.Left(7).CompareNoCase(L"mailto:"))
+  if (srcURL.Left(7).CompareNoCase(L"mailto:") != 0)
     return FALSE;
+
   int pos = srcURL.Find(L'?', 0);
   CFX_WideString tmp;
   if (pos == -1) {
@@ -1016,14 +1009,12 @@ FX_BOOL CPDFXFA_Document::MailToInfo(CFX_WideString& csURL,
       return FALSE;
 
     tmp = srcURL.Right(csURL.GetLength() - 7);
-    tmp.TrimLeft();
-    tmp.TrimRight();
   } else {
     tmp = srcURL.Left(pos);
     tmp = tmp.Right(tmp.GetLength() - 7);
-    tmp.TrimLeft();
-    tmp.TrimRight();
   }
+  tmp.TrimLeft();
+  tmp.TrimRight();
 
   csToAddress = tmp;
 
@@ -1032,38 +1023,31 @@ FX_BOOL CPDFXFA_Document::MailToInfo(CFX_WideString& csURL,
     srcURL.TrimLeft();
     srcURL.TrimRight();
     pos = srcURL.Find(L'&', 0);
-    if (pos == -1)
-      tmp = srcURL;
-    else
-      tmp = srcURL.Left(pos);
 
+    tmp = (pos == -1) ? srcURL : srcURL.Left(pos);

[dsinclair, 2016/09/08 15:13:51]

()'s aren't strictly needed but seems a bit easier to read.

     tmp.TrimLeft();
     tmp.TrimRight();
-    if (tmp.GetLength() >= 3 && 0 == tmp.Left(3).CompareNoCase(L"cc=")) {
+    if (tmp.GetLength() >= 3 && tmp.Left(3).CompareNoCase(L"cc=") == 0) {
       tmp = tmp.Right(tmp.GetLength() - 3);
       if (!csCCAddress.IsEmpty())
         csCCAddress += L';';
       csCCAddress += tmp;
-
     } else if (tmp.GetLength() >= 4 &&
-               0 == tmp.Left(4).CompareNoCase(L"bcc=")) {
+               tmp.Left(4).CompareNoCase(L"bcc=") == 0) {
       tmp = tmp.Right(tmp.GetLength() - 4);
       if (!csBCCAddress.IsEmpty())
         csBCCAddress += L';';
       csBCCAddress += tmp;
     } else if (tmp.GetLength() >= 8 &&
-               0 == tmp.Left(8).CompareNoCase(L"subject=")) {
+               tmp.Left(8).CompareNoCase(L"subject=") == 0) {
       tmp = tmp.Right(tmp.GetLength() - 8);
       csSubject += tmp;
     } else if (tmp.GetLength() >= 5 &&
-               0 == tmp.Left(5).CompareNoCase(L"body=")) {
+               tmp.Left(5).CompareNoCase(L"body=") == 0) {
       tmp = tmp.Right(tmp.GetLength() - 5);
       csMsg += tmp;
     }
-    if (pos == -1)
-      srcURL = L"";
-    else
-      srcURL = srcURL.Right(csURL.GetLength() - (pos + 1));
+    srcURL = (pos == -1) ? L"" : srcURL.Right(csURL.GetLength() - (pos + 1));
   }
   csToAddress.Replace(L",", L";");
   csCCAddress.Replace(L",", L";");
@@ -1076,6 +1060,7 @@ FX_BOOL CPDFXFA_Document::SubmitDataInternal(CXFA_FFDoc* hDoc,
   CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
   if (!pEnv)
     return FALSE;
+
   CFX_WideStringC csURLC;
   submit.GetSubmitTarget(csURLC);
   CFX_WideString csURL(csURLC);
@@ -1083,12 +1068,13 @@ FX_BOOL CPDFXFA_Document::SubmitDataInternal(CXFA_FFDoc* hDoc,
     CFX_WideString ws;
     ws.FromLocal("Submit cancelled.");
     CFX_ByteString bs = ws.UTF16LE_Encode();
-    int len = bs.GetLength() / sizeof(unsigned short);
-    pEnv->FFI_Alert((FPDF_WIDESTRING)bs.GetBuffer(len * sizeof(unsigned short)),
-                    (FPDF_WIDESTRING)L"", 0, 4);
-    bs.ReleaseBuffer(len * sizeof(unsigned short));
+    int len = bs.GetLength();
+    pEnv->FFI_Alert((FPDF_WIDESTRING)bs.GetBuffer(len), (FPDF_WIDESTRING)L"", 0,

[dsinclair, 2016/09/08 15:13:51]

These can't static_cast because it's a unsigned short to a wide char which
static_cast doesn't like.

[Tom Sepez, 2016/09/08 16:53:36]

ok, pre-existing, but casting bs to FPDF_Widestring (public, always short) seems
wrong again.  L"" is either 2 or 4 bytes of terminating 0, and passing that as a
short seems fine as its covered in either case.

I thought I made some helpers for these,
https://cs.chromium.org/chromium/src/third_party/pdfium/fpdfsdk/fsdk_mgr.cpp?...
but it seems private.  Maybe it needs to be made visible.

[dsinclair, 2016/09/12 13:25:49]

Followup.

+                    4);
+    bs.ReleaseBuffer(len);

[Tom Sepez, 2016/09/08 16:53:36]

ReleaseBuffer isn't required as |bs| still owns the GetBuffer() result.  It's
about to go out of scope and will be freed automatically.  This just adds some
more work.  If we remove this, we don't need to know len if we use the helper
above.

Several other places.

[dsinclair, 2016/09/12 13:25:49]

Done.

     return FALSE;
   }
+
   FPDF_BOOL bRet = TRUE;
   FPDF_FILEHANDLER* pFileHandler = nullptr;
   int fileFlag = -1;
@@ -1106,6 +1092,7 @@ FX_BOOL CPDFXFA_Document::SubmitDataInternal(CXFA_FFDoc* hDoc,
       FPDF_DWORD flag = 0;
       if (submit.IsSubmitEmbedPDF())
         flag |= FXFA_PDF;
+
       ToXFAContentFlags(csContent, flag);
       pFileHandler = pEnv->FFI_OpenFile(FXFA_SAVEAS_XDP, nullptr, "wb");
       fileFlag = FXFA_SAVEAS_XDP;
@@ -1129,7 +1116,7 @@ FX_BOOL CPDFXFA_Document::SubmitDataInternal(CXFA_FFDoc* hDoc,
   }
   if (!pFileHandler)
     return FALSE;
-  if (0 == csURL.Left(7).CompareNoCase(L"mailto:")) {
+  if (csURL.Left(7).CompareNoCase(L"mailto:") == 0) {
     CFX_WideString csToAddress;
     CFX_WideString csCCAddress;
     CFX_WideString csBCCAddress;
@@ -1139,6 +1126,7 @@ FX_BOOL CPDFXFA_Document::SubmitDataInternal(CXFA_FFDoc* hDoc,
                       csMsg);
     if (!bRet)
       return FALSE;
+
     CFX_ByteString bsTo = CFX_WideString(csToAddress).UTF16LE_Encode();
     CFX_ByteString bsCC = CFX_WideString(csCCAddress).UTF16LE_Encode();
     CFX_ByteString bsBcc = CFX_WideString(csBCCAddress).UTF16LE_Encode();
@@ -1160,11 +1148,10 @@ FX_BOOL CPDFXFA_Document::SubmitDataInternal(CXFA_FFDoc* hDoc,
     // HTTP or FTP
     CFX_WideString ws;
     CFX_ByteString bs = csURL.UTF16LE_Encode();
-    int len = bs.GetLength() / sizeof(unsigned short);
-    pEnv->FFI_UploadTo(
-        pFileHandler, fileFlag,
-        (FPDF_WIDESTRING)bs.GetBuffer(len * sizeof(unsigned short)));
-    bs.ReleaseBuffer(len * sizeof(unsigned short));
+    int len = bs.GetLength();
+    pEnv->FFI_UploadTo(pFileHandler, fileFlag,
+                       (FPDF_WIDESTRING)bs.GetBuffer(len));
+    bs.ReleaseBuffer(len);
   }
   return bRet;
 }
@@ -1180,22 +1167,7 @@ FX_BOOL CPDFXFA_Document::SetGlobalProperty(CXFA_FFDoc* hDoc,
                                                                pValue);
   return FALSE;
 }
-FX_BOOL CPDFXFA_Document::GetPDFScriptObject(CXFA_FFDoc* hDoc,
-                                             const CFX_ByteStringC& utf8Name,
-                                             CFXJSE_Value* pValue) {
-  if (hDoc != m_pXFADoc.get())
-    return FALSE;
-
-  if (!m_pSDKDoc || !m_pSDKDoc->GetEnv()->GetJSRuntime())
-    return FALSE;
 
-  if (!m_pJSContext) {
-    m_pSDKDoc->GetEnv()->GetJSRuntime()->SetReaderDocument(m_pSDKDoc.get());
-    m_pJSContext = m_pSDKDoc->GetEnv()->GetJSRuntime()->NewContext();
-  }
-
-  return m_pSDKDoc->GetEnv()->GetJSRuntime()->GetValueByName(utf8Name, pValue);
-}
 FX_BOOL CPDFXFA_Document::GetGlobalProperty(CXFA_FFDoc* hDoc,
                                             const CFX_ByteStringC& szPropName,
                                             CFXJSE_Value* pValue) {