Trigger page view event when re-layout is finished

Trigger page view event when re-layout is finished

BUG=pdfium:401
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/a1cef70c08a16e2b9d7ec14987a8b20660d83534

[Jim Wang, 2016-03-02 05:28:07]

jinming_wang@foxitsoftware.com changed reviewers:
+ thestig@chromium.org

[Jim Wang, 2016-03-02 05:28:08]

Hello Lei, please help to review it.

[Lei Zhang, 2016-03-16 23:06:08]

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

[Lei Zhang, 2016-03-16 23:06:09]

Deferring to Tom.

[Tom Sepez, 2016-03-17 00:23:58]

Just a few small comments, otherwise good.

https://codereview.chromium.org/1758553003/diff/1/fpdfsdk/src/fpdfxfa/fpdfxfa...
File fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp (right):

https://codereview.chromium.org/1758553003/diff/1/fpdfsdk/src/fpdfxfa/fpdfxfa...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:30: #define FXFA_LOADSTATUS_PRELOAD 0
can we make this an enum nested in the class?

https://codereview.chromium.org/1758553003/diff/1/fpdfsdk/src/fpdfxfa/fpdfxfa...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:492: CPDFXFA_Page* pPage = nullptr;
This gets shadowed at line 505.  We don't need this declaration.

https://codereview.chromium.org/1758553003/diff/1/fpdfsdk/src/fpdfxfa/fpdfxfa...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:494: if (FXFA_LOADSTATUS_LOADING !=
m_nLoadStatus &&
Nit: prefer m_nLoadStatus != XFA_LOADSTATUS_LOADING

https://codereview.chromium.org/1758553003/diff/1/public/fpdf_formfill.h
File public/fpdf_formfill.h (right):

https://codereview.chromium.org/1758553003/diff/1/public/fpdf_formfill.h#newc...
public/fpdf_formfill.h:824: *document.
Nit: tidy the indentation, but the comment is good.

https://codereview.chromium.org/1758553003/diff/1/public/fpdf_formfill.h#newc...
public/fpdf_formfill.h:839: *           event type is
FXFA_PAGEVIEWEVENT_POSTADDED, 2 new pages had been
nit: have

https://codereview.chromium.org/1758553003/diff/1/public/fpdf_formfill.h#newc...
public/fpdf_formfill.h:840: *appended to the tailor of document; If page_count
is 2 and
nit: tail

https://codereview.chromium.org/1758553003/diff/1/public/fpdf_formfill.h#newc...
public/fpdf_formfill.h:841: *           event type is
FXFA_PAGEVIEWEVENT_POSTREMOVED, the last 2 pages had
nit: have.

[Jim Wang, 2016-03-17 05:00:43]

https://codereview.chromium.org/1758553003/diff/1/fpdfsdk/src/fpdfxfa/fpdfxfa...
File fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp (right):

https://codereview.chromium.org/1758553003/diff/1/fpdfsdk/src/fpdfxfa/fpdfxfa...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:30: #define FXFA_LOADSTATUS_PRELOAD 0
On 2016/03/17 00:23:57, Tom Sepez wrote:
> can we make this an enum nested in the class?

Done.

https://codereview.chromium.org/1758553003/diff/1/fpdfsdk/src/fpdfxfa/fpdfxfa...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:492: CPDFXFA_Page* pPage = nullptr;
On 2016/03/17 00:23:57, Tom Sepez wrote:
> This gets shadowed at line 505.  We don't need this declaration.

Done.

https://codereview.chromium.org/1758553003/diff/1/fpdfsdk/src/fpdfxfa/fpdfxfa...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:494: if (FXFA_LOADSTATUS_LOADING !=
m_nLoadStatus &&
On 2016/03/17 00:23:57, Tom Sepez wrote:
> Nit: prefer m_nLoadStatus != XFA_LOADSTATUS_LOADING

Done.

https://codereview.chromium.org/1758553003/diff/1/public/fpdf_formfill.h
File public/fpdf_formfill.h (right):

https://codereview.chromium.org/1758553003/diff/1/public/fpdf_formfill.h#newc...
public/fpdf_formfill.h:824: *document.
On 2016/03/17 00:23:57, Tom Sepez wrote:
> Nit: tidy the indentation, but the comment is good.

Done.

https://codereview.chromium.org/1758553003/diff/1/public/fpdf_formfill.h#newc...
public/fpdf_formfill.h:839: *           event type is
FXFA_PAGEVIEWEVENT_POSTADDED, 2 new pages had been
On 2016/03/17 00:23:58, Tom Sepez wrote:
> nit: have

Done.

https://codereview.chromium.org/1758553003/diff/1/public/fpdf_formfill.h#newc...
public/fpdf_formfill.h:840: *appended to the tailor of document; If page_count
is 2 and
On 2016/03/17 00:23:57, Tom Sepez wrote:
> nit: tail

Done.

https://codereview.chromium.org/1758553003/diff/1/public/fpdf_formfill.h#newc...
public/fpdf_formfill.h:841: *           event type is
FXFA_PAGEVIEWEVENT_POSTREMOVED, the last 2 pages had
On 2016/03/17 00:23:58, Tom Sepez wrote:
> nit: have.

Done.

[Tom Sepez, 2016-03-17 16:16:43]

Almost. Just a few little changes and you're good to go.

https://codereview.chromium.org/1758553003/diff/20001/fpdfsdk/include/fpdfxfa...
File fpdfsdk/include/fpdfxfa/fpdfxfa_doc.h (right):

https://codereview.chromium.org/1758553003/diff/20001/fpdfsdk/include/fpdfxfa...
fpdfsdk/include/fpdfxfa/fpdfxfa_doc.h:211: enum LOADSTATUS {
nit: I hate to be picky, but let's call this LoadStatus.  The values can remain
in ALL_CAPS though. This matches the style in use in many other places.

https://codereview.chromium.org/1758553003/diff/20001/fpdfsdk/include/fpdfxfa...
fpdfsdk/include/fpdfxfa/fpdfxfa_doc.h:235: int m_nLoadStatus;
This should be LoadStatus m_nLoadStatus, not int.

https://codereview.chromium.org/1758553003/diff/20001/public/fpdf_formfill.h
File public/fpdf_formfill.h (right):

https://codereview.chromium.org/1758553003/diff/20001/public/fpdf_formfill.h#...
public/fpdf_formfill.h:832: *document.
nit: indentation.

https://codereview.chromium.org/1758553003/diff/20001/public/fpdf_formfill.h#...
public/fpdf_formfill.h:838: *of document. This means that if parameter
page_count is 2 and
nit: indentation (also line 840 and 842).

[Jim Wang, 2016-03-18 01:12:26]

https://codereview.chromium.org/1758553003/diff/20001/fpdfsdk/include/fpdfxfa...
File fpdfsdk/include/fpdfxfa/fpdfxfa_doc.h (right):

https://codereview.chromium.org/1758553003/diff/20001/fpdfsdk/include/fpdfxfa...
fpdfsdk/include/fpdfxfa/fpdfxfa_doc.h:211: enum LOADSTATUS {
On 2016/03/17 16:16:43, Tom Sepez wrote:
> nit: I hate to be picky, but let's call this LoadStatus.  The values can
remain
> in ALL_CAPS though. This matches the style in use in many other places.

Done.

https://codereview.chromium.org/1758553003/diff/20001/fpdfsdk/include/fpdfxfa...
fpdfsdk/include/fpdfxfa/fpdfxfa_doc.h:235: int m_nLoadStatus;
On 2016/03/17 16:16:42, Tom Sepez wrote:
> This should be LoadStatus m_nLoadStatus, not int.

Done.

https://codereview.chromium.org/1758553003/diff/20001/public/fpdf_formfill.h
File public/fpdf_formfill.h (right):

https://codereview.chromium.org/1758553003/diff/20001/public/fpdf_formfill.h#...
public/fpdf_formfill.h:832: *document.
On 2016/03/17 16:16:43, Tom Sepez wrote:
> nit: indentation.

Done.

https://codereview.chromium.org/1758553003/diff/20001/public/fpdf_formfill.h#...
public/fpdf_formfill.h:838: *of document. This means that if parameter
page_count is 2 and
On 2016/03/17 16:16:43, Tom Sepez wrote:
> nit: indentation (also line 840 and 842).

Done.

[Tom Sepez, 2016-03-18 02:56:43]

lgtm

[Jim Wang, 2016-03-18 08:35:58]

Description was changed from

==========
Trigger page view event when re-layout is finished

BUG=pdfium:401
==========

to

==========
Trigger page view event when re-layout is finished

BUG=pdfium:401
R=tsepez@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/a1cef70c08a16e2b9d7ec14987a8b20660d8...
==========

[Jim Wang, 2016-03-18 08:36:02]

Committed patchset #3 (id:40001) manually as
a1cef70c08a16e2b9d7ec14987a8b20660d83534 (presubmit successful).