Cleanup parts of CPDFSDK_AnnotIterator and CPDFSDK_PageView.

Cleanup parts of CPDFSDK_AnnotIterator and CPDFSDK_PageView.

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/9b2741829b8a8c511ef0f2f313ff53d77ac96307

[Lei Zhang, 2015-10-21 14:24:15]

thestig@chromium.org changed reviewers:
+ tsepez@google.com

[Lei Zhang, 2015-10-21 14:24:16]

Some of the diffs don't line up well, so it's harder to read side by side.

https://codereview.chromium.org/1411203007/diff/1/fpdfsdk/include/fsdk_annoth...
File fpdfsdk/include/fsdk_annothandler.h (left):

https://codereview.chromium.org/1411203007/diff/1/fpdfsdk/include/fsdk_annoth...
fpdfsdk/include/fsdk_annothandler.h:312: FX_BOOL bIgnoreTopmost = FALSE,
Never explicitly set, thus there's dead code in the impl.

https://codereview.chromium.org/1411203007/diff/1/fpdfsdk/include/fsdk_annoth...
File fpdfsdk/include/fsdk_annothandler.h (right):

https://codereview.chromium.org/1411203007/diff/1/fpdfsdk/include/fsdk_annoth...
fpdfsdk/include/fsdk_annothandler.h:312: CPDFSDK_Annot* Prev(int* index);
Also dead, but leaving it in for completeness.

[Lei Zhang, 2015-10-21 18:17:31]

thestig@chromium.org changed reviewers:
+ tsepez@chromium.org
- tsepez@google.com

[Lei Zhang, 2015-10-21 18:17:31]

Whoops, wrong acct.

[Lei Zhang, 2015-10-21 18:18:04]

And I managed to break something. Please hold off on the review.

[Lei Zhang, 2015-10-21 18:29:02]

... and ready for review.

[Tom Sepez, 2015-10-21 19:32:59]

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/include/fsdk_an...
File fpdfsdk/include/fsdk_annothandler.h (right):

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/include/fsdk_an...
fpdfsdk/include/fsdk_annothandler.h:307: class CPDFSDK_AnnotIterator {
Can we just kill this class entirely, and have a method to get a sorted
std::vector<> of annots, and make the callers deal with traversal using sane
method?

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/include/fsdk_an...
fpdfsdk/include/fsdk_annothandler.h:312: CPDFSDK_Annot* Next(size_t* index);
Shouldn't an iterator maintain position on its own without relying on the
callers, for crying out loud?

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
File fpdfsdk/src/fsdk_annothandler.cpp (left):

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:671: for (int i = 0; i < nCount; i++) {
Yeah, lets make traversal O(n^2) just for grins.

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
File fpdfsdk/src/fsdk_annothandler.cpp (right):

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:15: if (p1->GetLayoutOrder() ==
p2->GetLayoutOrder())
just return p1->GetLayouOrder() - p2->GetLayoutOrder() ?

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:666: for (auto it = annots.rbegin(); it !=
annots.rend(); ++it)
Do we care what order we traverse the annots if we are going to sort them
afterwards?

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:674: for (auto it =
m_iteratorAnnotList.begin(); it != m_iteratorAnnotList.end();
can we use std::find or some such?

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:689: const size_t oldIndex = *index;
there's got to be a simpler way to write this.

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:698: if (m_iteratorAnnotList.empty() || *index
>= m_iteratorAnnotList.size())
Wouldn't the empty() check be redundant if size() == 0 for empty lists?

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:714: void CPDFSDK_AnnotIterator::InsertSort()
{
WHY???

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_mgr.cpp
File fpdfsdk/src/fsdk_mgr.cpp (right):

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_mgr.cp...
fpdfsdk/src/fsdk_mgr.cpp:616: m_bEnterWidget = FALSE;
Can we make these part of the initialization list, esp since we're passing
|this| to some method above?

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_mgr.cp...
fpdfsdk/src/fsdk_mgr.cpp:628: for (CPDFSDK_Annot* pAnnot : m_fxAnnotArray) {
find() of some sort?

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_mgr.cp...
fpdfsdk/src/fsdk_mgr.cpp:954: FX_BOOL CPDFSDK_PageView::IsValidAnnot(void* p)
const {
Why does this take a void?

[Lei Zhang, 2015-10-22 06:51:17]

Patchset #7 (id:120001) has been deleted

[Lei Zhang, 2015-10-22 06:56:22]

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/include/fsdk_an...
File fpdfsdk/include/fsdk_annothandler.h (right):

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/include/fsdk_an...
fpdfsdk/include/fsdk_annothandler.h:307: class CPDFSDK_AnnotIterator {
On 2015/10/21 19:32:58, Tom Sepez wrote:
> Can we just kill this class entirely, and have a method to get a sorted
> std::vector<> of annots, and make the callers deal with traversal using sane
> method?

The sorting is not trivial. I changed it to just Next().

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/include/fsdk_an...
fpdfsdk/include/fsdk_annothandler.h:312: CPDFSDK_Annot* Next(size_t* index);
On 2015/10/21 19:32:58, Tom Sepez wrote:
> Shouldn't an iterator maintain position on its own without relying on the
> callers, for crying out loud? 

Mmmhmm.

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
File fpdfsdk/src/fsdk_annothandler.cpp (left):

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:671: for (int i = 0; i < nCount; i++) {
On 2015/10/21 19:32:58, Tom Sepez wrote:
> Yeah, lets make traversal O(n^2) just for grins.

But it's dead code, so that's O(0) right?

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
File fpdfsdk/src/fsdk_annothandler.cpp (right):

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:15: if (p1->GetLayoutOrder() ==
p2->GetLayoutOrder())
On 2015/10/21 19:32:59, Tom Sepez wrote:
> just return p1->GetLayouOrder() - p2->GetLayoutOrder() ?

Oh yes, it's 2 vs 5!

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:666: for (auto it = annots.rbegin(); it !=
annots.rend(); ++it)
On 2015/10/21 19:32:58, Tom Sepez wrote:
> Do we care what order we traverse the annots if we are going to sort them
> afterwards?

Apparently we do. Probably for z-ordering. If I flip it around, tests fail.

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:674: for (auto it =
m_iteratorAnnotList.begin(); it != m_iteratorAnnotList.end();
On 2015/10/21 19:32:58, Tom Sepez wrote:
> can we use std::find or some such?

Done.

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:689: const size_t oldIndex = *index;
On 2015/10/21 19:32:58, Tom Sepez wrote:
> there's got to be a simpler way to write this.

Done.

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:698: if (m_iteratorAnnotList.empty() || *index
>= m_iteratorAnnotList.size())
On 2015/10/21 19:32:58, Tom Sepez wrote:
> Wouldn't the empty() check be redundant if size() == 0 for empty lists?

Done.

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:714: void CPDFSDK_AnnotIterator::InsertSort()
{
On 2015/10/21 19:32:58, Tom Sepez wrote:
> WHY???

I was lazy and didn't really want to deal with it. We need a stable sort.

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_mgr.cpp
File fpdfsdk/src/fsdk_mgr.cpp (right):

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_mgr.cp...
fpdfsdk/src/fsdk_mgr.cpp:616: m_bEnterWidget = FALSE;
On 2015/10/21 19:32:59, Tom Sepez wrote:
> Can we make these part of the initialization list, esp since we're passing
> |this| to some method above?

Done.

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_mgr.cp...
fpdfsdk/src/fsdk_mgr.cpp:628: for (CPDFSDK_Annot* pAnnot : m_fxAnnotArray) {
On 2015/10/21 19:32:59, Tom Sepez wrote:
> find() of some sort?

Done.

https://codereview.chromium.org/1411203007/diff/80001/fpdfsdk/src/fsdk_mgr.cp...
fpdfsdk/src/fsdk_mgr.cpp:954: FX_BOOL CPDFSDK_PageView::IsValidAnnot(void* p)
const {
On 2015/10/21 19:32:59, Tom Sepez wrote:
> Why does this take a void?

Dunno, fixed.

[Tom Sepez, 2015-10-23 15:54:49]

So much better. Lots of nits.  LGTM otherwise I think.

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_annot...
File fpdfsdk/src/fsdk_annothandler.cpp (right):

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_annot...
fpdfsdk/src/fsdk_annothandler.cpp:16: bool LayoutOrderCompare(CPDFSDK_Annot* p1,
CPDFSDK_Annot* p2) {
nit: This is actually LayoutOrderLessThan(), xxxCompare returns {-1, 0, 1} etc
in my tiny mind at least. Alternatively, do we use this in more than one place?
If not, maybe its a [](CPDFSDK_Annot* p1, CPDFSDK_Annot* p2) {return 
p1->GetLayoutOrder() < p2->GetLayoutOrder()} at the call site.

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_annot...
fpdfsdk/src/fsdk_annothandler.cpp:688: if (m_pos >= m_iteratorAnnotList.size())
nit: could you flip this around?  My small brain understands x < size much
better. Thanks.

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_annot...
fpdfsdk/src/fsdk_annothandler.cpp:699: ++m_pos;
nit: maybe combine with previous line as - m_pos++ - 1? and then eliminate
oldindex temporary.  And then this becomes a single line, so we don't introcude
braces if we invert the if above.

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_mgr.cpp
File fpdfsdk/src/fsdk_mgr.cpp (right):

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_mgr.c...
fpdfsdk/src/fsdk_mgr.cpp:695: CPDFSDK_AnnotIterator annotIterator(this, false);
nit: can we move this down to just before line 698.

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_mgr.c...
fpdfsdk/src/fsdk_mgr.cpp:698: CPDFSDK_Annot* pSDKAnnot = NULL;
nit: move declaration to condition of while, lose (()).

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_mgr.c...
fpdfsdk/src/fsdk_mgr.cpp:710: CPDFSDK_AnnotIterator annotIterator(this, false);
nit: can we move this down to just before line 713?

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_mgr.c...
fpdfsdk/src/fsdk_mgr.cpp:713: CPDFSDK_Annot* pSDKAnnot = NULL;
nit: move declaration to condition of while, lose (()).

[Tom Sepez, 2015-10-23 16:05:04]

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_annot...
File fpdfsdk/src/fsdk_annothandler.cpp (right):

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_annot...
fpdfsdk/src/fsdk_annothandler.cpp:699: ++m_pos;
On 2015/10/23 15:54:48, Tom Sepez wrote:
> nit: maybe combine with previous line as - m_pos++ - 1? and then eliminate
> oldindex temporary.  And then this becomes a single line, so we don't
introcude
> braces if we invert the if above.

And isn't x - m_pos++ - 1 == x - ++m_pos ? So maybe (and doublecheck):

CPDFSDK_Annot* CPDFSDK_AnnotIterator::PrevAnnot() {
  if (m_pos < m_iteratorAnnotList.size())
    return m_iteratorAnnotList[m_iteratorAnnotList.size() - ++m_pos];
  return nullptr;
}

[Lei Zhang, 2015-10-24 01:01:24]

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_annot...
File fpdfsdk/src/fsdk_annothandler.cpp (right):

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_annot...
fpdfsdk/src/fsdk_annothandler.cpp:16: bool LayoutOrderCompare(CPDFSDK_Annot* p1,
CPDFSDK_Annot* p2) {
On 2015/10/23 15:54:48, Tom Sepez wrote:
> nit: This is actually LayoutOrderLessThan(), xxxCompare returns {-1, 0, 1} etc
> in my tiny mind at least. Alternatively, do we use this in more than one
place?
> If not, maybe its a [](CPDFSDK_Annot* p1, CPDFSDK_Annot* p2) {return 
> p1->GetLayoutOrder() < p2->GetLayoutOrder()} at the call site.

Done.

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_annot...
fpdfsdk/src/fsdk_annothandler.cpp:688: if (m_pos >= m_iteratorAnnotList.size())
On 2015/10/23 15:54:48, Tom Sepez wrote:
> nit: could you flip this around?  My small brain understands x < size much
> better. Thanks.

Done.

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_annot...
fpdfsdk/src/fsdk_annothandler.cpp:699: ++m_pos;
On 2015/10/23 15:54:48, Tom Sepez wrote:
> nit: maybe combine with previous line as - m_pos++ - 1? and then eliminate
> oldindex temporary.  And then this becomes a single line, so we don't
introcude
> braces if we invert the if above.

Acknowledged.

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_annot...
fpdfsdk/src/fsdk_annothandler.cpp:699: ++m_pos;
On 2015/10/23 16:05:04, Tom Sepez wrote:
> On 2015/10/23 15:54:48, Tom Sepez wrote:
> > nit: maybe combine with previous line as - m_pos++ - 1? and then eliminate
> > oldindex temporary.  And then this becomes a single line, so we don't
> introcude
> > braces if we invert the if above.
> 
> And isn't x - m_pos++ - 1 == x - ++m_pos ? So maybe (and doublecheck):
> 
> CPDFSDK_Annot* CPDFSDK_AnnotIterator::PrevAnnot() {
>   if (m_pos < m_iteratorAnnotList.size())
>     return m_iteratorAnnotList[m_iteratorAnnotList.size() - ++m_pos];
>   return nullptr;
> }

Done.

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_mgr.cpp
File fpdfsdk/src/fsdk_mgr.cpp (right):

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_mgr.c...
fpdfsdk/src/fsdk_mgr.cpp:695: CPDFSDK_AnnotIterator annotIterator(this, false);
On 2015/10/23 15:54:48, Tom Sepez wrote:
> nit: can we move this down to just before line 698.

Done.

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_mgr.c...
fpdfsdk/src/fsdk_mgr.cpp:698: CPDFSDK_Annot* pSDKAnnot = NULL;
On 2015/10/23 15:54:48, Tom Sepez wrote:
> nit: move declaration to condition of while, lose (()).

Done.

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_mgr.c...
fpdfsdk/src/fsdk_mgr.cpp:710: CPDFSDK_AnnotIterator annotIterator(this, false);
On 2015/10/23 15:54:48, Tom Sepez wrote:
> nit: can we move this down to just before line 713?

Done.

https://codereview.chromium.org/1411203007/diff/140001/fpdfsdk/src/fsdk_mgr.c...
fpdfsdk/src/fsdk_mgr.cpp:713: CPDFSDK_Annot* pSDKAnnot = NULL;
On 2015/10/23 15:54:48, Tom Sepez wrote:
> nit: move declaration to condition of while, lose (()).

Done.

[Lei Zhang, 2015-10-24 01:06:24]

Committed patchset #9 (id:180001) manually as
9b2741829b8a8c511ef0f2f313ff53d77ac96307 (presubmit successful).