Cleanup some CPDFSDK_PageView annotation code.

Cleanup some CPDFSDK_PageView annotation code.

This Cl cleans up the code regarding CPDFSDK_Annots in CPDFSDK_PageView.
This includes:
  * Makes DeleteAnnot XFA only and wraps at the call site.
  * Removes unused methods
  * Replaces use of CountAnnots and GetAnnot with vector iteration
  * Removes {Set|Kill}FocusAnnot from CPDFSDK_PageView
  * Renames m_fxAnnotArray to m_SDKAnnotArray

Committed: https://pdfium.googlesource.com/pdfium/+/8afe15a019c78a24168ebc42306342c25358ccdf

[dsinclair, 2016-10-05 14:06:31]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 14:06:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dsinclair, 2016-10-05 14:06:46]

dsinclair@chromium.org changed reviewers:
+ thestig@chromium.org, tsepez@chromium.org

[dsinclair, 2016-10-05 14:06:47]

PTAL.

https://codereview.chromium.org/2384323005/diff/1/fpdfsdk/cba_annotiterator.cpp
File fpdfsdk/cba_annotiterator.cpp (right):

https://codereview.chromium.org/2384323005/diff/1/fpdfsdk/cba_annotiterator.c...
fpdfsdk/cba_annotiterator.cpp:80: std::vector<CPDFSDK_Annot*> sa;
This chunk of code was the same between ROW and COLUMN so wanted to factor it
out.

https://codereview.chromium.org/2384323005/diff/1/fpdfsdk/cpdfsdk_pageview.h
File fpdfsdk/cpdfsdk_pageview.h (right):

https://codereview.chromium.org/2384323005/diff/1/fpdfsdk/cpdfsdk_pageview.h#...
fpdfsdk/cpdfsdk_pageview.h:104: std::vector<CPDFSDK_Annot*> m_SDKAnnotArray;
Trying to make the difference between m_fxAnnotArray and m_pAnnotList a bit
clearer by calling out they are SDK annots.

[commit-bot: I haz the power, 2016-10-05 14:18:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 14:18:07]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-10-05 17:08:28]

... and so forth.

https://codereview.chromium.org/2384323005/diff/1/fpdfsdk/cba_annotiterator.cpp
File fpdfsdk/cba_annotiterator.cpp (right):

https://codereview.chromium.org/2384323005/diff/1/fpdfsdk/cba_annotiterator.c...
fpdfsdk/cba_annotiterator.cpp:70: if (m_eTabOrder == STRUCTURE) {
Dunno, I kinda like the switch and pulling the common stuff into helper methods.
 There's still a lot of duplication even after the refactor.

https://codereview.chromium.org/2384323005/diff/1/fpdfsdk/cba_annotiterator.c...
fpdfsdk/cba_annotiterator.cpp:80: std::vector<CPDFSDK_Annot*> sa;
Can we have a helper method like

void CBA_AnnotIter::CollectAnnots(std::vector<CPDFDSK_Annot*>* pArray {

  for (auto pAnnot : m_pPageView->GetAnnotList()) {
    if (pAnnot->GetAnnotSubtype() == m_nAnnotSubtype &&
        !pAnnot->IsSignatureWidget()) {
      pArray->push_back(pAnnot);
    }
  }
}

then line 71 is
  CollectAnnots(&m_Annots);
  return;

and the code here and for the other case statement is
  CollectAnnots(&sa);

https://codereview.chromium.org/2384323005/diff/1/fpdfsdk/cba_annotiterator.c...
fpdfsdk/cba_annotiterator.cpp:93: int nLeftTopIndex = -1;
Maybe code from 93-100 becomes a function GetLeftTopIndex(const
std::vector<CPDFDSK_Annot*>& sa) ?

[dsinclair, 2016-10-05 17:52:51]

https://codereview.chromium.org/2384323005/diff/1/fpdfsdk/cba_annotiterator.cpp
File fpdfsdk/cba_annotiterator.cpp (right):

https://codereview.chromium.org/2384323005/diff/1/fpdfsdk/cba_annotiterator.c...
fpdfsdk/cba_annotiterator.cpp:70: if (m_eTabOrder == STRUCTURE) {
On 2016/10/05 17:08:28, Tom Sepez wrote:
> Dunno, I kinda like the switch and pulling the common stuff into helper
methods.
>  There's still a lot of duplication even after the refactor.

Done.

https://codereview.chromium.org/2384323005/diff/1/fpdfsdk/cba_annotiterator.c...
fpdfsdk/cba_annotiterator.cpp:80: std::vector<CPDFSDK_Annot*> sa;
On 2016/10/05 17:08:28, Tom Sepez wrote:
> Can we have a helper method like
> 
> void CBA_AnnotIter::CollectAnnots(std::vector<CPDFDSK_Annot*>* pArray {
> 
>   for (auto pAnnot : m_pPageView->GetAnnotList()) {
>     if (pAnnot->GetAnnotSubtype() == m_nAnnotSubtype &&
>         !pAnnot->IsSignatureWidget()) {
>       pArray->push_back(pAnnot);
>     }
>   }
> }
> 
> then line 71 is
>   CollectAnnots(&m_Annots);
>   return;
> 
> and the code here and for the other case statement is
>   CollectAnnots(&sa);

Done.

https://codereview.chromium.org/2384323005/diff/1/fpdfsdk/cba_annotiterator.c...
fpdfsdk/cba_annotiterator.cpp:93: int nLeftTopIndex = -1;
On 2016/10/05 17:08:28, Tom Sepez wrote:
> Maybe code from 93-100 becomes a function GetLeftTopIndex(const
> std::vector<CPDFDSK_Annot*>& sa) ?

The actual comparisons done at ~97 are different between the two blocks.

[Tom Sepez, 2016-10-05 17:59:53]

lgtm

https://codereview.chromium.org/2384323005/diff/1/fpdfsdk/cba_annotiterator.cpp
File fpdfsdk/cba_annotiterator.cpp (right):

https://codereview.chromium.org/2384323005/diff/1/fpdfsdk/cba_annotiterator.c...
fpdfsdk/cba_annotiterator.cpp:93: int nLeftTopIndex = -1;
On 2016/10/05 17:52:51, dsinclair wrote:
> On 2016/10/05 17:08:28, Tom Sepez wrote:
> > Maybe code from 93-100 becomes a function GetLeftTopIndex(const
> > std::vector<CPDFDSK_Annot*>& sa) ?
> 
> The actual comparisons done at ~97 are different between the two blocks.
Ah, ok.  nevermind.

https://codereview.chromium.org/2384323005/diff/20001/fpdfsdk/cba_annotiterat...
File fpdfsdk/cba_annotiterator.cpp (right):

https://codereview.chromium.org/2384323005/diff/20001/fpdfsdk/cba_annotiterat...
fpdfsdk/cba_annotiterator.cpp:107: std::sort(sa.begin(), sa.end(),
CompareByLeftAscending);
super-super-super-nit:  I'd put the blank line after this so that the collect
and sort appear on adjacent lines.

https://codereview.chromium.org/2384323005/diff/20001/fpdfsdk/cba_annotiterat...
File fpdfsdk/cba_annotiterator.h (right):

https://codereview.chromium.org/2384323005/diff/20001/fpdfsdk/cba_annotiterat...
fpdfsdk/cba_annotiterator.h:33: static CFX_FloatRect GetAnnotRect(const
CPDFSDK_Annot* pAnnot);
nit: I'd just put this in namespace { } in the .cpp file.

https://codereview.chromium.org/2384323005/diff/20001/fpdfsdk/cba_annotiterat...
fpdfsdk/cba_annotiterator.h:37: CFX_FloatRect
AddToAnnotsList(std::vector<CPDFSDK_Annot*>& sa, size_t idx);
nit: can we pass |sa| as pointer rather than adding a non-const ref argument?

https://codereview.chromium.org/2384323005/diff/20001/fpdfsdk/cba_annotiterat...
fpdfsdk/cba_annotiterator.h:39: std::vector<size_t>& aSelect);
nit: ditto with aselect

[dsinclair, 2016-10-05 18:26:56]

https://codereview.chromium.org/2384323005/diff/20001/fpdfsdk/cba_annotiterat...
File fpdfsdk/cba_annotiterator.cpp (right):

https://codereview.chromium.org/2384323005/diff/20001/fpdfsdk/cba_annotiterat...
fpdfsdk/cba_annotiterator.cpp:107: std::sort(sa.begin(), sa.end(),
CompareByLeftAscending);
On 2016/10/05 17:59:52, Tom Sepez wrote:
> super-super-super-nit:  I'd put the blank line after this so that the collect
> and sort appear on adjacent lines.

Done.

https://codereview.chromium.org/2384323005/diff/20001/fpdfsdk/cba_annotiterat...
File fpdfsdk/cba_annotiterator.h (right):

https://codereview.chromium.org/2384323005/diff/20001/fpdfsdk/cba_annotiterat...
fpdfsdk/cba_annotiterator.h:33: static CFX_FloatRect GetAnnotRect(const
CPDFSDK_Annot* pAnnot);
On 2016/10/05 17:59:52, Tom Sepez wrote:
> nit: I'd just put this in namespace { } in the .cpp file.

Done.

https://codereview.chromium.org/2384323005/diff/20001/fpdfsdk/cba_annotiterat...
fpdfsdk/cba_annotiterator.h:37: CFX_FloatRect
AddToAnnotsList(std::vector<CPDFSDK_Annot*>& sa, size_t idx);
On 2016/10/05 17:59:52, Tom Sepez wrote:
> nit: can we pass |sa| as pointer rather than adding a non-const ref argument?

Done.

https://codereview.chromium.org/2384323005/diff/20001/fpdfsdk/cba_annotiterat...
fpdfsdk/cba_annotiterator.h:39: std::vector<size_t>& aSelect);
On 2016/10/05 17:59:52, Tom Sepez wrote:
> nit: ditto with aselect

Done.

[dsinclair, 2016-10-05 18:27:32]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-10-05 18:27:32]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2384323005/#ps40001
(title: "Review feedback")

[commit-bot: I haz the power, 2016-10-05 18:27:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 19:00:37]

Description was changed from

==========
Cleanup some CPDFSDK_PageView annotation code.

This Cl cleans up the code regarding CPDFSDK_Annots in CPDFSDK_PageView.
This includes:
  * Makes DeleteAnnot XFA only and wraps at the call site.
  * Removes unused methods
  * Replaces use of CountAnnots and GetAnnot with vector iteration
  * Removes {Set|Kill}FocusAnnot from CPDFSDK_PageView
  * Renames m_fxAnnotArray to m_SDKAnnotArray
==========

to

==========
Cleanup some CPDFSDK_PageView annotation code.

This Cl cleans up the code regarding CPDFSDK_Annots in CPDFSDK_PageView.
This includes:
  * Makes DeleteAnnot XFA only and wraps at the call site.
  * Removes unused methods
  * Replaces use of CountAnnots and GetAnnot with vector iteration
  * Removes {Set|Kill}FocusAnnot from CPDFSDK_PageView
  * Renames m_fxAnnotArray to m_SDKAnnotArray

Committed:
https://pdfium.googlesource.com/pdfium/+/8afe15a019c78a24168ebc42306342c25358...
==========

[commit-bot: I haz the power, 2016-10-05 19:00:39]

Committed patchset #3 (id:40001) as
https://pdfium.googlesource.com/pdfium/+/8afe15a019c78a24168ebc42306342c25358...