Provide user friendly messages for OTS parsing of fonts

Provide user friendly messages for OTS parsing of fonts

Add feature to provide user (font-developer) friendly messages for
used web font parsing errors.

Few new error/warnings related to font parsing and tables are added 
in https://codereview.chromium.org/966703002/.

BUG=97467
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197551

[h.joshi, 2015-05-04 07:06:40]

Patchset #3 (id:40001) has been deleted

[h.joshi, 2015-05-12 04:21:11]

h.joshi@samsung.com changed reviewers:
+ eae@chromium.org, jshin@chromium.org

[h.joshi, 2015-05-12 04:21:33]

Pls review

[h.joshi, 2015-05-12 04:21:46]

Patchset #2 (id:20001) has been deleted

[jungshik at Google, 2015-05-12 20:22:48]

Just had a brief look. A couple of questions. 

BTW, I have another question about the console message: Do we always use English
(i.e. no localization)?

https://codereview.chromium.org/983973004/diff/120001/Source/platform/fonts/F...
File Source/platform/fonts/FontCustomPlatformData.cpp (right):

https://codereview.chromium.org/983973004/diff/120001/Source/platform/fonts/F...
Source/platform/fonts/FontCustomPlatformData.cpp:96: if
(sanitizer.isParsingError())
I wonder if there's a case when transcodeBuffer==0 but isParsingError() = false.
 If not, we can have just a single if-stmnt.

https://codereview.chromium.org/983973004/diff/120001/Source/platform/fonts/o...
File Source/platform/fonts/opentype/OpenTypeSanitizer.h (right):

https://codereview.chromium.org/983973004/diff/120001/Source/platform/fonts/o...
Source/platform/fonts/opentype/OpenTypeSanitizer.h:73: virtual void Message(int
level, const char *format, ...)
Just wondering if there's a reason to have this inlined in *h file. Not a big
deal.

[h.joshi, 2015-05-14 04:52:16]

@Jungshik: Made suggested changes
BTW, I have another question about the console message: Do we always use English
(i.e. no localization)?

Himanshu >> OTS is providing error string in English only. I am not aware of any
method or way to convert English strings to localization form, pls guide me if
we have any such method. It will be good to provide localized string to user.

https://codereview.chromium.org/983973004/diff/120001/Source/platform/fonts/F...
File Source/platform/fonts/FontCustomPlatformData.cpp (right):

https://codereview.chromium.org/983973004/diff/120001/Source/platform/fonts/F...
Source/platform/fonts/FontCustomPlatformData.cpp:96: if
(sanitizer.isParsingError())
Yes, two checks are not required. thank you.
Made changes.

https://codereview.chromium.org/983973004/diff/120001/Source/platform/fonts/o...
File Source/platform/fonts/opentype/OpenTypeSanitizer.h (right):

https://codereview.chromium.org/983973004/diff/120001/Source/platform/fonts/o...
Source/platform/fonts/opentype/OpenTypeSanitizer.h:73: virtual void Message(int
level, const char *format, ...)
Moved methods to CPP file, normally all methods were in header file to kept the
same way.

[h.joshi, 2015-05-14 08:46:12]

Patchset #6 (id:140001) has been deleted

[jungshik at Google, 2015-05-15 19:17:21]

Thank you for the update. A couple of nit-like comments below. 

As for the localization, it's just out of curiosity. (there's a way to localize
Blink strings, but I'm not sure if we want to do that in OTS. Anyway, it's a
separate issue and should not block this CL).

https://codereview.chromium.org/983973004/diff/160001/Source/platform/fonts/F...
File Source/platform/fonts/FontCustomPlatformData.cpp (right):

https://codereview.chromium.org/983973004/diff/160001/Source/platform/fonts/F...
Source/platform/fonts/FontCustomPlatformData.cpp:95: if (!transcodeBuffer) {
I took a look at sanitize() to find that there are two  cases where
|transcodeBuffer == nullptr} but |m_parsingError| is uninitialized. So checking
|!transcodeBuffer| makes sense.

https://codereview.chromium.org/983973004/diff/160001/Source/platform/fonts/o...
File Source/platform/fonts/opentype/OpenTypeSanitizer.h (right):

https://codereview.chromium.org/983973004/diff/160001/Source/platform/fonts/o...
Source/platform/fonts/opentype/OpenTypeSanitizer.h:63: bool m_parsingError;
Do you need |m_parsingError|? Nobody uses it other than |sanitize()|, but no
customer of OpenTypeSanitizer checks for that.

[h.joshi, 2015-05-18 05:50:14]

Patchset #7 (id:180001) has been deleted

[h.joshi, 2015-05-18 05:58:28]

Correcting method "setParsingError" call due to which in few places
uninitialized variable was present.

https://codereview.chromium.org/983973004/diff/160001/Source/platform/fonts/F...
File Source/platform/fonts/FontCustomPlatformData.cpp (right):

https://codereview.chromium.org/983973004/diff/160001/Source/platform/fonts/F...
Source/platform/fonts/FontCustomPlatformData.cpp:95: if (!transcodeBuffer) {
Method "setParsingError" was not called at right place. Corrected that in new
patch.

https://codereview.chromium.org/983973004/diff/160001/Source/platform/fonts/o...
File Source/platform/fonts/opentype/OpenTypeSanitizer.h (right):

https://codereview.chromium.org/983973004/diff/160001/Source/platform/fonts/o...
Source/platform/fonts/opentype/OpenTypeSanitizer.h:63: bool m_parsingError;
Method "setParsingError" was not called at right place. Corrected that in new
patch.

[jungshik at Google, 2015-06-01 23:22:19]

Sorry that I had a few draft comments that I thought I had sent your way (but I
have NOT. They've remained in draft.). Could you take care of them? 
Sorry again for the delay and your patience.

https://codereview.chromium.org/983973004/diff/200001/Source/platform/fonts/F...
File Source/platform/fonts/FontCustomPlatformData.cpp (right):

https://codereview.chromium.org/983973004/diff/200001/Source/platform/fonts/F...
Source/platform/fonts/FontCustomPlatformData.cpp:98: otsParseMessage =
sanitizer.getErrorString();
If you set |errorString| whenever sanitize() returns empty buffer, you don't
need a separate isParsingError() check  (and m_ParsingError in OTSSanitizer is
NOT necessary, either).  

Alternatively, you can set 'parsingError' to true and set |errorString| 
whenever you return 'empty' buffer, but it seems redundant to do that way. 

Either way, I don't see a reason to use two if-statements here.

https://codereview.chromium.org/983973004/diff/200001/Source/platform/fonts/o...
File Source/platform/fonts/opentype/OpenTypeSanitizer.cpp (right):

https://codereview.chromium.org/983973004/diff/200001/Source/platform/fonts/o...
Source/platform/fonts/opentype/OpenTypeSanitizer.cpp:62: setParsingError(false);
You don't need to call an accessor. Why don't you just set it directly?

https://codereview.chromium.org/983973004/diff/200001/Source/platform/fonts/o...
Source/platform/fonts/opentype/OpenTypeSanitizer.cpp:63: if (!m_buffer)
Set the error message to 'Empty Buffer' or something like that. 

Otherwise, otsParseError in callers would be uninitialized. (or fix the caller
to initialize it)

https://codereview.chromium.org/983973004/diff/200001/Source/platform/fonts/o...
Source/platform/fonts/opentype/OpenTypeSanitizer.cpp:69: return nullptr;
IMHO, you'd better set a custom error string here, too. (e.g. 'Web font is
larger than 30MB').

https://codereview.chromium.org/983973004/diff/200001/Source/platform/fonts/o...
Source/platform/fonts/opentype/OpenTypeSanitizer.cpp:84:
setErrorString(otsContext.getErrorString());
again, you don't need accessors to set member variables here.

[h.joshi, 2015-06-04 15:19:17]

removal of "isParsingError" method and related variables were done already in
previous patch.
Not sure why comment links are pointing to old patch which I deleted due to one
issue while uploading
(sorry for the inconvenience)

 In this new patch, added error strings for other NULL return values.

https://codereview.chromium.org/983973004/diff/200001/Source/platform/fonts/F...
File Source/platform/fonts/FontCustomPlatformData.cpp (right):

https://codereview.chromium.org/983973004/diff/200001/Source/platform/fonts/F...
Source/platform/fonts/FontCustomPlatformData.cpp:98: otsParseMessage =
sanitizer.getErrorString();
On 2015/06/01 23:22:19, jungshik at google wrote:
> If you set |errorString| whenever sanitize() returns empty buffer, you don't
> need a separate isParsingError() check  (and m_ParsingError in OTSSanitizer is
> NOT necessary, either).  
> 
> Alternatively, you can set 'parsingError' to true and set |errorString| 
> whenever you return 'empty' buffer, but it seems redundant to do that way. 
> 
> Either way, I don't see a reason to use two if-statements here. 

Done.

https://codereview.chromium.org/983973004/diff/200001/Source/platform/fonts/o...
File Source/platform/fonts/opentype/OpenTypeSanitizer.cpp (right):

https://codereview.chromium.org/983973004/diff/200001/Source/platform/fonts/o...
Source/platform/fonts/opentype/OpenTypeSanitizer.cpp:62: setParsingError(false);
On 2015/06/01 23:22:19, jungshik at google wrote:
> You don't need to call an accessor. Why don't you just set it directly? 

Done.

https://codereview.chromium.org/983973004/diff/200001/Source/platform/fonts/o...
Source/platform/fonts/opentype/OpenTypeSanitizer.cpp:63: if (!m_buffer)
On 2015/06/01 23:22:19, jungshik at google wrote:
> Set the error message to 'Empty Buffer' or something like that. 
> 
> Otherwise, otsParseError in callers would be uninitialized. (or fix the caller
> to initialize it)

Done.

https://codereview.chromium.org/983973004/diff/200001/Source/platform/fonts/o...
Source/platform/fonts/opentype/OpenTypeSanitizer.cpp:69: return nullptr;
On 2015/06/01 23:22:19, jungshik at google wrote:
> IMHO, you'd better set a custom error string here, too. (e.g. 'Web font is
> larger than 30MB'). 

Done.

https://codereview.chromium.org/983973004/diff/200001/Source/platform/fonts/o...
Source/platform/fonts/opentype/OpenTypeSanitizer.cpp:84:
setErrorString(otsContext.getErrorString());
On 2015/06/01 23:22:19, jungshik at google wrote:
> again, you don't need accessors to set member variables here. 

Done.

[jungshik at Google, 2015-06-08 18:57:42]

Thanks. LGTM

[h.joshi, 2015-06-10 03:30:16]

@Jungshik: Thank you

[h.joshi, 2015-06-10 03:30:23]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2015-06-10 03:30:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/983973004/240001

[commit-bot: I haz the power, 2015-06-10 03:37:21]

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

[commit-bot: I haz the power, 2015-06-10 03:37:22]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[h.joshi, 2015-06-10 03:48:32]

@Emil: Pls review
Getting presubmit OWNER LGTM error for "Source/core" file.

[h.joshi, 2015-06-12 01:42:06]

@Emil: Pls review

[h.joshi, 2015-06-17 02:25:52]

@Emil: Pls review

[eae, 2015-06-20 00:25:53]

LGTM

[h.joshi, 2015-06-20 02:05:27]

Thank you Emil

[h.joshi, 2015-06-20 02:05:51]

The CQ bit was checked by h.joshi@samsung.com

[h.joshi, 2015-06-20 02:05:51]

The patchset sent to the CQ was uploaded after l-g-t-m from jshin@chromium.org
Link to the patchset: https://codereview.chromium.org/983973004/#ps260001
(title: "Updating Test Expectation")

[commit-bot: I haz the power, 2015-06-20 02:06:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/983973004/260001

[commit-bot: I haz the power, 2015-06-20 02:12:14]

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

[commit-bot: I haz the power, 2015-06-20 02:12:17]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[h.joshi, 2015-06-22 03:55:33]

The CQ bit was checked by h.joshi@samsung.com

[h.joshi, 2015-06-22 03:55:33]

The patchset sent to the CQ was uploaded after l-g-t-m from jshin@chromium.org,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/983973004/#ps280001
(title: "Rebase patch")

[commit-bot: I haz the power, 2015-06-22 03:55:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/983973004/280001

[commit-bot: I haz the power, 2015-06-22 05:16:45]

Committed patchset #11 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197551