[webkit-reviews] review denied: [Bug 67640] ReadAV(NULL) @ chrome.dll!ubrk_setText_46 #3c121a35, 7beced32, 3f8c5464 : [Attachment 128825] patch (fix style nits)

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Jungshik Shin <jshin at chromium.org>'s request for
review:
Bug 67640: ReadAV(NULL) @ chrome.dll!ubrk_setText_46
#3c121a35,7beced32,3f8c5464
https://bugs.webkit.org/show_bug.cgi?id=67640

Attachment 128825: patch (fix style nits)
https://bugs.webkit.org/attachment.cgi?id=128825&action=review

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=128825&action=review


Looks good, r- because I think you’re not handling an error case that was
previously handled.

> Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:64
> +	       bool isLocaleEmpty = locale.isEmpty();

I think the question phrase “isLocaleEmpty” isn’t a great name for this
variable. You could call it “localeIsEmpty”.

> Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:67
> +	       // locale comes from a web page and it can be invalid leading
ICU
> +	       // to fail, in which case we fall back to the default locale.

You’re missing a comma after “invalid”.

> Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:75
> +		   if (U_FAILURE(openStatus)) {
> +		       LOG_ERROR("ubrk_open failed with status %d",
openStatus);
> +		       return 0;
> +		   }

This should go outside the “if (!isLocalEmpty && U_FAILURE(openStatus))” block,
otherwise we’re not handling the case of ubrk_open failing with the
currentTextBreakLocaleID() when locale *is* empty.