Display content of the annotation when mouse hover.

Display content of the annotation when mouse hover.

Each annotation has its contents, and users should be able to see the
contents. In this patch, PDFium creates a Popup annotation for each
annotation and stores the author and the content. When a user mouse
hover on the annotation, PDFium draws the corresponding Popup annotation
and displays the content.

Also, roll DEPS for testing/corpus to 5867fa6.

BUG=62625
Committed: https://pdfium.googlesource.com/pdfium/+/35512aa7e4acc3ceb9c6aef5d61eebfb4ae802af

[jaepark, 2016-08-23 22:30:50]

jaepark@google.com changed reviewers:
+ dsinclair@chromium.org, thestig@chromium.org

[jaepark, 2016-08-23 22:30:51]

This is a work in progress patch for displaying content of the annotation when
mouse hover. The visual output of this patch is in
https://codereview.chromium.org/2269953003 .

[dsinclair, 2016-08-24 14:00:21]

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
File core/fpdfdoc/cpdf_annot.cpp (right):

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
core/fpdfdoc/cpdf_annot.cpp:33: else if (m_sSubtype == "Popup")
Should we switch m_sSubtype to be an enum class? We're getting a bunch of
comparisons with it.

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
File core/fpdfdoc/cpdf_annotlist.cpp (right):

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
core/fpdfdoc/cpdf_annotlist.cpp:21: CPDF_Annot* CreatePopupAnnot(CPDF_Annot*
pAnnot, CPDF_Document* pDocument) {
Can this return a std::unique_ptr<CPDF_Annot> to show the ownership transfer?

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
core/fpdfdoc/cpdf_annotlist.cpp:33: CFX_FloatRect popupRect(0, 0, 200, 200);
Where does 200 come from? Is it spec, or just a number?

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
core/fpdfdoc/cpdf_annotlist.cpp:34: popupRect.Translate((rect.left + rect.right)
/ 2,
Why left + right and not left + popupRect.width? if left + right is bigger then
200 won't this get placed strange?

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpvt_gener...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:572: CPDF_Dictionary*
GenerateResourceFontDict(CPDF_Document* pDoc,
std::unique_ptr?

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:578: pFontDict->SetAtName("Encoding",
"WinAnsiEncoding");
Are these settings spec, or just created? If created, why "WinAnsiEncoding"?

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:588: CPDF_Dictionary*
GenerateResourceDict(CPDF_Dictionary* pExtGStateDict,
Pass in a std::unique_ptr to show the transfer? (and return a unique_ptr?)

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/include/cp...
File core/fpdfdoc/include/cpdf_annot.h (right):

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/include/cp...
core/fpdfdoc/include/cpdf_annot.h:74: CPDF_Annot* m_pPopupAnnot;
Add a // not owned if this doesn't take possession of the pointer.

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_annotha...
File fpdfsdk/cpdfsdk_annothandlermgr.cpp (left):

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_annotha...
fpdfsdk/cpdfsdk_annothandlermgr.cpp:109: return
GetAnnotHandler(pPDFAnnot->GetSubType());
This used to operate on the internal annotation and now operates on the parent?

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_annotha...
fpdfsdk/cpdfsdk_annothandlermgr.cpp:134: if (pAnnot->IsXFAField())
We no longer need to bail early on XFA fields?

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_annotha...
File fpdfsdk/cpdfsdk_annothandlermgr.cpp (right):

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_annotha...
fpdfsdk/cpdfsdk_annothandlermgr.cpp:54: IPDFSDK_AnnotHandler* pAnnotHandler =
GetAnnotHandler(pAnnot);
So, previously if we didnt' find the annotation hander this would delete the
annotation. We're now forcing it to always find m_pBAAnnotHandler. Similar with
the methods below, is this change intentional? Do we handle more annotation
types through BAAnnotHandler now?

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_baannot...
File fpdfsdk/cpdfsdk_baannot.cpp (right):

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_baannot...
fpdfsdk/cpdfsdk_baannot.cpp:413: 
if (CPDF_Annot* pAnnot = m_pAnnot->GetPopupAnnot())
  pAnnot->SetOpenState(bOpenState);

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_baannot...
File fpdfsdk/cpdfsdk_baannothandler.cpp (right):

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_baannot...
fpdfsdk/cpdfsdk_baannothandler.cpp:68: ASSERT(pAnnot);
I believe this assert isn't needed as you can call delete on a nullptr.

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/include/cpdfsdk...
File fpdfsdk/include/cpdfsdk_annothandlermgr.h (right):

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/include/cpdfsdk...
fpdfsdk/include/cpdfsdk_annothandlermgr.h:120:
std::unique_ptr<CPDFSDK_BAAnnotHandler> m_pBAAnnotHandler;
Put this one above m_pBFAnnotHandler. It's a bit hard to read when split over
the #ifdef.

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/include/fsdk_mgr.h
File fpdfsdk/include/fsdk_mgr.h (right):

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/include/fsdk_mg...
fpdfsdk/include/fsdk_mgr.h:623: CPDFSDK_Annot* m_CaptureWidget;
This appears to live in both sides of the #ifdef can it be move before and this
simplified to a ifndef for m_bTakeOverPage?

[jaepark, 2016-08-24 18:37:49]

I've also updated the offset of text and position of the popup. New result of
the popup note is in https://codereview.chromium.org/2269953003 .

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
File core/fpdfdoc/cpdf_annot.cpp (right):

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
core/fpdfdoc/cpdf_annot.cpp:33: else if (m_sSubtype == "Popup")
On 2016/08/24 14:00:20, dsinclair wrote:
> Should we switch m_sSubtype to be an enum class? We're getting a bunch of
> comparisons with it.

I'd like to do this in another CL if that's OK since this CL is already pretty
big.

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
File core/fpdfdoc/cpdf_annotlist.cpp (right):

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
core/fpdfdoc/cpdf_annotlist.cpp:21: CPDF_Annot* CreatePopupAnnot(CPDF_Annot*
pAnnot, CPDF_Document* pDocument) {
On 2016/08/24 14:00:20, dsinclair wrote:
> Can this return a std::unique_ptr<CPDF_Annot> to show the ownership transfer? 

Done.

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
core/fpdfdoc/cpdf_annotlist.cpp:33: CFX_FloatRect popupRect(0, 0, 200, 200);
On 2016/08/24 14:00:20, dsinclair wrote:
> Where does 200 come from? Is it spec, or just a number?

This is just a number. This represents the rectangle of the popup note that'll
be shown.

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
core/fpdfdoc/cpdf_annotlist.cpp:34: popupRect.Translate((rect.left + rect.right)
/ 2,
On 2016/08/24 14:00:20, dsinclair wrote:
> Why left + right and not left + popupRect.width? if left + right is bigger
then
> 200 won't this get placed strange?

I wanted to place the popup note in the middle of the original annotation, which
is (rect.left + rect.right) / 2. But I now think that it's better to place the
popup at the left side of annotation so that it aligns with  the original
annotation. Fixed accordingly.

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpvt_gener...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:572: CPDF_Dictionary*
GenerateResourceFontDict(CPDF_Document* pDoc,
On 2016/08/24 14:00:20, dsinclair wrote:
> std::unique_ptr?

This is a dictionary that should be set as a key of another dictionary. When
setting a CPDF_Dictionary, it take s raw pointer, and CPDF_Dictionary manages
the life cycle of it.

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:578: pFontDict->SetAtName("Encoding",
"WinAnsiEncoding");
On 2016/08/24 14:00:20, dsinclair wrote:
> Are these settings spec, or just created? If created, why "WinAnsiEncoding"?

These are just created. I referred to line 69~77 of this file. There are
actually several encodings in the PDF reference Appendix D, but I'm not quite
sure which one to use. Any suggestions?

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:588: CPDF_Dictionary*
GenerateResourceDict(CPDF_Dictionary* pExtGStateDict,
On 2016/08/24 14:00:20, dsinclair wrote:
> Pass in a std::unique_ptr to show the transfer? (and return a unique_ptr?)

Same as above. CPDF_Dictionary takes a raw pointer when using ::SetAt.

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/include/cp...
File core/fpdfdoc/include/cpdf_annot.h (right):

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/include/cp...
core/fpdfdoc/include/cpdf_annot.h:74: CPDF_Annot* m_pPopupAnnot;
On 2016/08/24 14:00:20, dsinclair wrote:
> Add a // not owned if this doesn't take possession of the pointer.

Done.

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_annotha...
File fpdfsdk/cpdfsdk_annothandlermgr.cpp (left):

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_annotha...
fpdfsdk/cpdfsdk_annothandlermgr.cpp:109: return
GetAnnotHandler(pPDFAnnot->GetSubType());
On 2016/08/24 14:00:20, dsinclair wrote:
> This used to operate on the internal annotation and now operates on the
parent?

The logic of GetAnnotHandler is the same.
CPDFSDK_Annot::GetType() can only return 3 values: "Widget", "XFAWidget"
(FSDK_XFAWIDGET_TYPENAME), and "". So, it returns CPDFSDK_BFAnnotHandler,
CPDFSDK_XFAAnnotHandler, and CPDFSDK_BAAnnotHandler respectively.

Previously, if CPDF_Annot->GetSubType() (which can return all annotation type)
returns "Widget", it returns CPDFSDK_BFAnnotHandler, if the
CPDFSDK_Annot::GetXFAWidget() is not null (which is means
CPDFSDK_Annot::GetType() == FSDK_XFAWIDGET_TYPENAME) it returns
CPDFSDK_XFAAnnotHandler. Otherwise return nullptr, because there's no handler
for all other types of annotations.

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_annotha...
fpdfsdk/cpdfsdk_annothandlermgr.cpp:134: if (pAnnot->IsXFAField())
On 2016/08/24 14:00:20, dsinclair wrote:
> We no longer need to bail early on XFA fields?

We handle this in BAAnnotHandler.

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_annotha...
File fpdfsdk/cpdfsdk_annothandlermgr.cpp (right):

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_annotha...
fpdfsdk/cpdfsdk_annothandlermgr.cpp:54: IPDFSDK_AnnotHandler* pAnnotHandler =
GetAnnotHandler(pAnnot);
On 2016/08/24 14:00:20, dsinclair wrote:
> So, previously if we didnt' find the annotation hander this would delete the
> annotation. We're now forcing it to always find m_pBAAnnotHandler. Similar
with
> the methods below, is this change intentional? Do we handle more annotation
> types through BAAnnotHandler now?

In BAAnnotHandler, it does the same thing as before, delete the annotation.
Previously we only handled Widget and XFAWidget annotations through
IPDFSDK_AnnotHandler, and CPDFSDK_BAAnnots were handled in
CPDFSDK_AnnotHandlerMgr. Now, we're now handling everything through
IPDFSDK_AnnotHandler. This change allows other types of annotations to deal with
user interaction. Also, it makes the name of CPDFSDK_AnnotHandlerMgr more sense
as it doesn't deal with CPDFSDK_Annot directly.

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_baannot...
File fpdfsdk/cpdfsdk_baannot.cpp (right):

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_baannot...
fpdfsdk/cpdfsdk_baannot.cpp:413: 
On 2016/08/24 14:00:20, dsinclair wrote:
> if (CPDF_Annot* pAnnot = m_pAnnot->GetPopupAnnot())
>   pAnnot->SetOpenState(bOpenState);

Done.

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_baannot...
File fpdfsdk/cpdfsdk_baannothandler.cpp (right):

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/cpdfsdk_baannot...
fpdfsdk/cpdfsdk_baannothandler.cpp:68: ASSERT(pAnnot);
On 2016/08/24 14:00:21, dsinclair wrote:
> I believe this assert isn't needed as you can call delete on a nullptr.

Done.

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/include/cpdfsdk...
File fpdfsdk/include/cpdfsdk_annothandlermgr.h (right):

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/include/cpdfsdk...
fpdfsdk/include/cpdfsdk_annothandlermgr.h:120:
std::unique_ptr<CPDFSDK_BAAnnotHandler> m_pBAAnnotHandler;
On 2016/08/24 14:00:21, dsinclair wrote:
> Put this one above m_pBFAnnotHandler. It's a bit hard to read when split over
> the #ifdef.

Done.

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/include/fsdk_mgr.h
File fpdfsdk/include/fsdk_mgr.h (right):

https://codereview.chromium.org/2273893002/diff/20001/fpdfsdk/include/fsdk_mg...
fpdfsdk/include/fsdk_mgr.h:623: CPDFSDK_Annot* m_CaptureWidget;
On 2016/08/24 14:00:21, dsinclair wrote:
> This appears to live in both sides of the #ifdef can it be move before and
this
> simplified to a ifndef for m_bTakeOverPage?

Done.

[dsinclair, 2016-08-24 18:56:27]

lgtm w/ nit.

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
File core/fpdfdoc/cpdf_annot.cpp (right):

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
core/fpdfdoc/cpdf_annot.cpp:33: else if (m_sSubtype == "Popup")
On 2016/08/24 18:37:48, jaepark wrote:
> On 2016/08/24 14:00:20, dsinclair wrote:
> > Should we switch m_sSubtype to be an enum class? We're getting a bunch of
> > comparisons with it.
> 
> I'd like to do this in another CL if that's OK since this CL is already pretty
> big.


As a followup Cl is fine.

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/include/cp...
File core/fpdfdoc/include/cpdf_annot.h (right):

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/include/cp...
core/fpdfdoc/include/cpdf_annot.h:74: CPDF_Annot* m_pPopupAnnot;
On 2016/08/24 18:37:48, jaepark wrote:
> On 2016/08/24 14:00:20, dsinclair wrote:
> > Add a // not owned if this doesn't take possession of the pointer.
> 
> Done.


Sorry, wasn't clear in my comment. Add a:

// not owned.

if this doesnt' take possession of the pointer.

[jaepark, 2016-08-24 19:02:03]

The CQ bit was checked by jaepark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 19:02:08]

Dry run: 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-08-24 19:04:38]

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

[commit-bot: I haz the power, 2016-08-24 19:04:39]

Dry run: Try jobs failed on following builders:
  linux on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux/builds/1777)
  linux_xfa on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa/build...)

[jaepark, 2016-08-24 19:29:07]

The CQ bit was checked by jaepark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 19:29:18]

Dry run: 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-08-24 19:45:30]

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

[commit-bot: I haz the power, 2016-08-24 19:45:32]

Dry run: This issue passed the CQ dry run.

[jaepark, 2016-08-26 02:37:29]

Popup should be drawn on top of other annotations so that other annotations are
not drawn on top of the comment note.

Also, mouse events shouldn't be propagated to Popup annotations as they will
prevent from selecting other annotation.

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
File core/fpdfdoc/cpdf_annot.cpp (right):

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/cpdf_annot...
core/fpdfdoc/cpdf_annot.cpp:33: else if (m_sSubtype == "Popup")
On 2016/08/24 18:56:26, dsinclair wrote:
> On 2016/08/24 18:37:48, jaepark wrote:
> > On 2016/08/24 14:00:20, dsinclair wrote:
> > > Should we switch m_sSubtype to be an enum class? We're getting a bunch of
> > > comparisons with it.
> > 
> > I'd like to do this in another CL if that's OK since this CL is already
pretty
> > big.
> 
> 
> As a followup Cl is fine.

Acknowledged.

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/include/cp...
File core/fpdfdoc/include/cpdf_annot.h (right):

https://codereview.chromium.org/2273893002/diff/20001/core/fpdfdoc/include/cp...
core/fpdfdoc/include/cpdf_annot.h:74: CPDF_Annot* m_pPopupAnnot;
On 2016/08/24 18:56:27, dsinclair wrote:
> On 2016/08/24 18:37:48, jaepark wrote:
> > On 2016/08/24 14:00:20, dsinclair wrote:
> > > Add a // not owned if this doesn't take possession of the pointer.
> > 
> > Done.
> 
> 
> Sorry, wasn't clear in my comment. Add a:
> 
> // not owned.
> 
> if this doesnt' take possession of the pointer.

Done.

[Lei Zhang, 2016-08-27 01:36:10]

Finally getting around to take a look. Please rebase. e.g. pdfium.gyp is gone.

[Lei Zhang, 2016-08-27 02:16:53]

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpdf_annot...
File core/fpdfdoc/cpdf_annotlist.cpp (right):

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpdf_annot...
core/fpdfdoc/cpdf_annotlist.cpp:85: size_t nAnnotListCount = m_AnnotList.size();
no need for variable.

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpvt_gener...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:517: swValue +=
pAnnotDict.GetUnicodeTextBy("T");
Do you care if GetUnicodeTextBy() returns an empty string?

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:519: swValue +=
pAnnotDict.GetUnicodeTextBy("Contents");
Similarly, if there's no content, do you still want the carriage return from the
line above?

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:524: vt.SetProvider(&prd);
The objects are destroyed in the reverse order of their declaration. So if |vt|
has a pointer to |prd|, then you should declare |prd| first. Otherwise, |prd|
gets destroyed first and |vt| is left with a dangling pointer.

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:525: CFX_FloatRect rect =
pAnnotDict.GetRectBy("Rect");
Drop the variable and merge with the next line?

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:970: << "b\n";
merge into previous line? i.e. << " re b\n";

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/include/cp...
File core/fpdfdoc/include/cpdf_annot.h (right):

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/include/cp...
core/fpdfdoc/include/cpdf_annot.h:75: bool m_bOpenState;
|m_bOpenState| is only ever set for popups, right?

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/include/cp...
core/fpdfdoc/include/cpdf_annot.h:77: CPDF_Annot* m_pPopupAnnot;
And the annot with a valid pointer to the popup is never itself a popup, right?
Might be good to document these properties.

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

https://codereview.chromium.org/2273893002/diff/80001/fpdfsdk/fsdk_mgr.cpp#ne...
fpdfsdk/fsdk_mgr.cpp:767: 
nit: extra blank line?

[jaepark, 2016-08-29 20:58:40]

The CQ bit was checked by jaepark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 20:58:59]

Dry run: 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-08-29 21:17:57]

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

[commit-bot: I haz the power, 2016-08-29 21:17:57]

Dry run: This issue passed the CQ dry run.

[jaepark, 2016-08-29 21:19:25]

Rebased.
Also, test cases are updated in https://codereview.chromium.org/2269953003 .

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpdf_annot...
File core/fpdfdoc/cpdf_annotlist.cpp (right):

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpdf_annot...
core/fpdfdoc/cpdf_annotlist.cpp:85: size_t nAnnotListCount = m_AnnotList.size();
On 2016/08/27 02:16:53, Lei Zhang wrote:
> no need for variable.

Actually, the below for loop is pushing back to the same m_AnnotList, so I think
it will cause infinite forloop if we use m_AnnotList.size() as condition.

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpvt_gener...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:516: CFX_WideString swValue(L"Author: ");
I've deleted this line because "T" field stands for title of the annotation. As
a convention, it has the author of the annotation, but it may contain other
values.

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:517: swValue +=
pAnnotDict.GetUnicodeTextBy("T");
On 2016/08/27 02:16:53, Lei Zhang wrote:
> Do you care if GetUnicodeTextBy() returns an empty string?

if GetUnicodeTextBy() returns empty string, we will have a blank line at the
top, and start displaying the contents from the next line. This is what Acrobat
Reader does.

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:519: swValue +=
pAnnotDict.GetUnicodeTextBy("Contents");
On 2016/08/27 02:16:53, Lei Zhang wrote:
> Similarly, if there's no content, do you still want the carriage return from
the
> line above?

If there is no content, PopupAnnot should not be created. I've updated the CL.

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:524: vt.SetProvider(&prd);
On 2016/08/27 02:16:53, Lei Zhang wrote:
> The objects are destroyed in the reverse order of their declaration. So if
|vt|
> has a pointer to |prd|, then you should declare |prd| first. Otherwise, |prd|
> gets destroyed first and |vt| is left with a dangling pointer.

Done.

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:525: CFX_FloatRect rect =
pAnnotDict.GetRectBy("Rect");
On 2016/08/27 02:16:53, Lei Zhang wrote:
> Drop the variable and merge with the next line?

Done.

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:970: << "b\n";
On 2016/08/27 02:16:53, Lei Zhang wrote:
> merge into previous line? i.e. << " re b\n";

Done.

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/include/cp...
File core/fpdfdoc/include/cpdf_annot.h (right):

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/include/cp...
core/fpdfdoc/include/cpdf_annot.h:75: bool m_bOpenState;
On 2016/08/27 02:16:53, Lei Zhang wrote:
> |m_bOpenState| is only ever set for popups, right?

Yes, it is only set for popups.

https://codereview.chromium.org/2273893002/diff/80001/core/fpdfdoc/include/cp...
core/fpdfdoc/include/cpdf_annot.h:77: CPDF_Annot* m_pPopupAnnot;
On 2016/08/27 02:16:53, Lei Zhang wrote:
> And the annot with a valid pointer to the popup is never itself a popup,
right?
> Might be good to document these properties.

Done.

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

https://codereview.chromium.org/2273893002/diff/80001/fpdfsdk/fsdk_mgr.cpp#ne...
fpdfsdk/fsdk_mgr.cpp:767: 
On 2016/08/27 02:16:53, Lei Zhang wrote:
> nit: extra blank line?

Done.

[Lei Zhang, 2016-08-29 22:41:29]

lgtm

https://codereview.chromium.org/2273893002/diff/100001/core/fpdfdoc/cpvt_gene...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2273893002/diff/100001/core/fpdfdoc/cpvt_gene...
core/fpdfdoc/cpvt_generateap.cpp:963: FX_FLOAT fBorderWidth = 1;
const?

[jaepark, 2016-08-29 23:15:35]

Description was changed from

==========
Display content of the annotation when mouse hover.

Each annotation has its contents, and users should be able to see the
contents. In this patch, PDFium creates a Popup annotation for each
annotation and stores the author and the content. When a user mouse
hover on the annotation, PDFium draws the corresponding Popup annotation
and displays the content.

BUG=62625
==========

to

==========
Display content of the annotation when mouse hover.

Each annotation has its contents, and users should be able to see the
contents. In this patch, PDFium creates a Popup annotation for each
annotation and stores the author and the content. When a user mouse
hover on the annotation, PDFium draws the corresponding Popup annotation
and displays the content.

Also, roll DEPS for testing/corpus to 5867fa6.

BUG=62625
==========

[jaepark, 2016-08-29 23:16:00]

The CQ bit was checked by jaepark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 23:16:16]

Dry run: 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-08-29 23:25:39]

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

[commit-bot: I haz the power, 2016-08-29 23:25:40]

Dry run: Try jobs failed on following builders:
  mac on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/mac/builds/1844)

[jaepark, 2016-08-29 23:30:19]

The CQ bit was checked by jaepark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 23:30:29]

Dry run: 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-08-29 23:56:10]

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

[commit-bot: I haz the power, 2016-08-29 23:56:11]

Dry run: This issue passed the CQ dry run.

[jaepark, 2016-08-29 23:59:37]

I think the mac trybots were failing for some corpus test cases due to font
problem. So I've added them to SUPRESSION.

https://codereview.chromium.org/2273893002/diff/100001/core/fpdfdoc/cpvt_gene...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2273893002/diff/100001/core/fpdfdoc/cpvt_gene...
core/fpdfdoc/cpvt_generateap.cpp:963: FX_FLOAT fBorderWidth = 1;
On 2016/08/29 22:41:28, Lei Zhang wrote:
> const?

Done.

[jaepark, 2016-08-30 00:14:37]

The CQ bit was checked by jaepark@google.com

[jaepark, 2016-08-30 00:14:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2273893002/#ps140001
(title: "Display content of the annotation when mouse hover.")

[commit-bot: I haz the power, 2016-08-30 00:14:41]

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-08-30 00:15:13]

Description was changed from

==========
Display content of the annotation when mouse hover.

Each annotation has its contents, and users should be able to see the
contents. In this patch, PDFium creates a Popup annotation for each
annotation and stores the author and the content. When a user mouse
hover on the annotation, PDFium draws the corresponding Popup annotation
and displays the content.

Also, roll DEPS for testing/corpus to 5867fa6.

BUG=62625
==========

to

==========
Display content of the annotation when mouse hover.

Each annotation has its contents, and users should be able to see the
contents. In this patch, PDFium creates a Popup annotation for each
annotation and stores the author and the content. When a user mouse
hover on the annotation, PDFium draws the corresponding Popup annotation
and displays the content.

Also, roll DEPS for testing/corpus to 5867fa6.

BUG=62625

Committed:
https://pdfium.googlesource.com/pdfium/+/35512aa7e4acc3ceb9c6aef5d61eebfb4ae8...
==========

[commit-bot: I haz the power, 2016-08-30 00:15:14]

Committed patchset #8 (id:140001) as
https://pdfium.googlesource.com/pdfium/+/35512aa7e4acc3ceb9c6aef5d61eebfb4ae8...