Correctly extracting email addresses

Correctly extracting email addresses

An email address contains user name part and host name part.
User name allows dash or underscore, but not leading/ending/double
period. Host name doesn't allow leading/ending/double
period either.

BUG=489107
R=jun_fang@foxitsoftware.com, thestig@chromium.org

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

[Wei Li, 2015-12-17 03:08:43]

Patchset #1 (id:1) has been deleted

[Wei Li, 2015-12-17 03:42:15]

Description was changed from

==========
Correctly handle some valid email addresses

Email addresses with dash or underscore should be valid. Add such links
as they appear.

BUG=489107
==========

to

==========
Correctly extracting email addresses

An email address contains user name part and host name part.
User name allows dash or underscore, but not leading/ending/double
period. Host name doesn't allow leading/ending/double
period either.

BUG=489107
==========

[Wei Li, 2015-12-17 03:46:46]

Patchset #1 (id:20001) has been deleted

[Wei Li, 2015-12-17 06:57:30]

Patchset #1 (id:40001) has been deleted

[Wei Li, 2015-12-17 07:01:08]

weili@chromium.org changed reviewers:
+ jun_fang@foxitsoftware.com, thestig@chromium.org

[Wei Li, 2015-12-17 07:01:08]

PTAL, thanks

[Wei Li, 2015-12-17 07:11:38]

Forgot to mention, the original code chose to do more strict checking. For
example, not allowing '#$%?' etc. special chars. This seems to be a common
requirement for email system although the spec allows it. I still keep it this
way.

[jun_fang, 2015-12-17 13:30:45]

https://codereview.chromium.org/1530763005/diff/60001/core/src/fpdftext/fpdf_...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1530763005/diff/60001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2617: int i;
nit: prefer "for (int i = aPos - 1; i >= 0; i--)"

https://codereview.chromium.org/1530763005/diff/60001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2621: if (ch == L'_' || ch == L'-' || (ch >=
L'a' && ch <= L'z') ||
Should we handle upper-case letters in email address?

https://codereview.chromium.org/1530763005/diff/60001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2656: for (i = aPos + 1; i < nLen; i++) {
nit: for (int i = aPos + 1; i < nLen; i++).

https://codereview.chromium.org/1530763005/diff/60001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2658: if (wch == L'-' || (wch >= L'a' && wch
<= L'z') ||
How about upper-case letters?

[Wei Li, 2015-12-17 17:41:41]

https://codereview.chromium.org/1530763005/diff/60001/core/src/fpdftext/fpdf_...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1530763005/diff/60001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2617: int i;
On 2015/12/17 13:30:45, jun_fang wrote:
> nit: prefer "for (int i = aPos - 1; i >= 0; i--)"

Done.

I was not sure what style I should follow when I changed the original code. But
glad to change this one. :)

https://codereview.chromium.org/1530763005/diff/60001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2621: if (ch == L'_' || ch == L'-' || (ch >=
L'a' && ch <= L'z') ||
On 2015/12/17 13:30:45, jun_fang wrote:
> Should we handle upper-case letters in email address?

Done.

I don't know why the original code chose to be case insensitive. Since it seems
no harm, I kept it. But keeping the case is better.

https://codereview.chromium.org/1530763005/diff/60001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2656: for (i = aPos + 1; i < nLen; i++) {
On 2015/12/17 13:30:45, jun_fang wrote:
> nit: for (int i = aPos + 1; i < nLen; i++).

Done.

https://codereview.chromium.org/1530763005/diff/60001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2658: if (wch == L'-' || (wch >= L'a' && wch
<= L'z') ||
On 2015/12/17 13:30:45, jun_fang wrote:
> How about upper-case letters?

See above.

[Lei Zhang, 2015-12-17 19:13:30]

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2616: 
Maybe add a comment to say checking the local part?

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2634: } else {
no need for else after a break.

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2657: if (wch == L'-' || (wch >= L'A' && wch
<= L'Z') ||
Maybe add some helpers like IsLetter() and IsNumber() ?

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2670: } else {
ditto

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
File core/src/fpdftext/fpdf_text_int_unittest.cpp (right):

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int_unittest.cpp:20: const wchar_t* invalid_strs[] =
{
const wchar_t* const, ditto with |valid_strings|

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int_unittest.cpp:23: L"abc@server",      // Host
name needs at least one '.'.
Host names without a dot can still be valid. Best to check and see if Acrobat
Reader recognizes them. We can try to fix this in a separate CL.

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int_unittest.cpp:39: {L"abc_@gmail.com",
L"abc_@gmail.com"},  // '_' is ok before '@'.
nit: align the comment with the rest.

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int_unittest.cpp:53: EXPECT_EQ(TRUE,
extractor.CheckMailLink(text_str));
You may want to convert CheckMailLink() to return a bool and just use
EXPECT_TRUE().

[Wei Li, 2015-12-17 19:42:27]

PTAL, thanks

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2616: 
On 2015/12/17 19:13:29, Lei Zhang wrote:
> Maybe add a comment to say checking the local part?

Done.

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2634: } else {
On 2015/12/17 19:13:29, Lei Zhang wrote:
> no need for else after a break.

Done.

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2657: if (wch == L'-' || (wch >= L'A' && wch
<= L'Z') ||
On 2015/12/17 19:13:29, Lei Zhang wrote:
> Maybe add some helpers like IsLetter() and IsNumber() ?

Done.

https://codereview.chromium.org/1530763005/diff/80001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2670: } else {
On 2015/12/17 19:13:29, Lei Zhang wrote:
> ditto

Done.

[Wei Li, 2015-12-17 23:07:06]

Patchset #4 (id:120001) has been deleted

[Wei Li, 2015-12-17 23:11:44]

defined two Macros, PTAL.

[jun_fang, 2015-12-18 00:08:55]

On 2015/12/17 23:11:44, Wei wrote:
> defined two Macros, PTAL.

LGTM. Thanks!

[Lei Zhang, 2015-12-18 00:20:56]

https://codereview.chromium.org/1530763005/diff/140001/core/include/fxcrt/fx_...
File core/include/fxcrt/fx_basic.h (right):

https://codereview.chromium.org/1530763005/diff/140001/core/include/fxcrt/fx_...
core/include/fxcrt/fx_basic.h:33: #define FX_ISWALPHA(x) \
We already have FXSYS_islower() and friends in core/include/fxcrt/fx_ext.h.
Let's put it there, and not as a #define.

[Lei Zhang, 2015-12-18 00:24:09]

https://codereview.chromium.org/1530763005/diff/140001/core/src/fpdftext/fpdf...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1530763005/diff/140001/core/src/fpdftext/fpdf...
core/src/fpdftext/fpdf_text_int.cpp:2645: CFX_WideString strtemp = str;
Not used?

https://codereview.chromium.org/1530763005/diff/140001/core/src/fpdftext/fpdf...
File core/src/fpdftext/fpdf_text_int_unittest.cpp (right):

https://codereview.chromium.org/1530763005/diff/140001/core/src/fpdftext/fpdf...
core/src/fpdftext/fpdf_text_int_unittest.cpp:23: L"abc@server",      // Host
name needs at least one '.'.
As I mentioned previously, we need to investigate whether this is actually valid
or not. Please add a TODO.

https://codereview.chromium.org/1530763005/diff/140001/core/src/fpdftext/fpdf...
core/src/fpdftext/fpdf_text_int_unittest.cpp:31: EXPECT_EQ(FALSE,
extractor.CheckMailLink(text_str));
It should be trivial to convert CheckMailLink() to return a bool. Can we do that
and use EXPECT_FALSE() here, and EXPECT_TRUE() below?

[Wei Li, 2015-12-18 01:12:21]

https://codereview.chromium.org/1530763005/diff/140001/core/include/fxcrt/fx_...
File core/include/fxcrt/fx_basic.h (right):

https://codereview.chromium.org/1530763005/diff/140001/core/include/fxcrt/fx_...
core/include/fxcrt/fx_basic.h:33: #define FX_ISWALPHA(x) \
On 2015/12/18 00:20:56, Lei Zhang wrote:
> We already have FXSYS_islower() and friends in core/include/fxcrt/fx_ext.h.
> Let's put it there, and not as a #define.

Done.

https://codereview.chromium.org/1530763005/diff/140001/core/src/fpdftext/fpdf...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1530763005/diff/140001/core/src/fpdftext/fpdf...
core/src/fpdftext/fpdf_text_int.cpp:2645: CFX_WideString strtemp = str;
On 2015/12/18 00:24:09, Lei Zhang wrote:
> Not used?

Done, thank you for your keen eyes. :)

https://codereview.chromium.org/1530763005/diff/140001/core/src/fpdftext/fpdf...
File core/src/fpdftext/fpdf_text_int_unittest.cpp (right):

https://codereview.chromium.org/1530763005/diff/140001/core/src/fpdftext/fpdf...
core/src/fpdftext/fpdf_text_int_unittest.cpp:23: L"abc@server",      // Host
name needs at least one '.'.
On 2015/12/18 00:24:09, Lei Zhang wrote:
> As I mentioned previously, we need to investigate whether this is actually
valid
> or not. Please add a TODO.

Added in cpp file.

https://codereview.chromium.org/1530763005/diff/140001/core/src/fpdftext/fpdf...
core/src/fpdftext/fpdf_text_int_unittest.cpp:31: EXPECT_EQ(FALSE,
extractor.CheckMailLink(text_str));
On 2015/12/18 00:24:09, Lei Zhang wrote:
> It should be trivial to convert CheckMailLink() to return a bool. Can we do
that
> and use EXPECT_FALSE() here, and EXPECT_TRUE() below?

Done.

[Lei Zhang, 2015-12-18 01:15:43]

lgtm

[Wei Li, 2015-12-18 01:47:03]

Description was changed from

==========
Correctly extracting email addresses

An email address contains user name part and host name part.
User name allows dash or underscore, but not leading/ending/double
period. Host name doesn't allow leading/ending/double
period either.

BUG=489107
==========

to

==========
Correctly extracting email addresses

An email address contains user name part and host name part.
User name allows dash or underscore, but not leading/ending/double
period. Host name doesn't allow leading/ending/double
period either.

BUG=489107
R=jun_fang@foxitsoftware.com, thestig@chromium.org

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

[Wei Li, 2015-12-18 01:47:05]

Committed patchset #5 (id:160001) manually as
cc70b7b55c9edcd0ff038f59080699060fbbede1 (presubmit successful).

[Wei Li, 2016-01-05 23:54:01]

Patchset #6 (id:180001) has been deleted