Truncate the Javascript alert message if the length is too long on Mac port

Set the maximum lines number to 32 for Javascript alert message on Mac port.

BUG=331219
R=jeremy@chromium.org
TEST=Manual test please: Launch chrome and load the popup-bug.html in BUG331219

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246457

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247093

[yi, 2014-01-20 22:55:07]

Jeremy, could you please review this for me. On linux port, the maximum lines
number sets to 32, so I added same logic for Mac.

[jeremy, 2014-01-21 21:24:39]

https://codereview.chromium.org/140323005/diff/1/chrome/browser/ui/cocoa/java...
File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right):

https://codereview.chromium.org/140323005/diff/1/chrome/browser/ui/cocoa/java...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:20: const int
kMessageTextMaxRows = 32;
This is a cross-platform limit right, WDYT about using one constant across
platforms?

https://codereview.chromium.org/140323005/diff/1/chrome/browser/ui/cocoa/java...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:152: NSArray *
informative_text_array =
Add a comment with a link to the bug - something like:
// Truncate long JS alerts - crbug.com/331219

https://codereview.chromium.org/140323005/diff/1/chrome/browser/ui/cocoa/java...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:158:
componentsJoinedByString:@"\n"] stringByAppendingString:@"\n..."];
Can you split this up into 3 or 4 statements so this is a little more readable?

[yi, 2014-01-21 21:57:39]

Thanks for review! Please check the comments below and I will provide a new
patch ASAP.
On 2014/01/21 21:24:39, jeremy wrote:
>
https://codereview.chromium.org/140323005/diff/1/chrome/browser/ui/cocoa/java...
> File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right):
> 
>
https://codereview.chromium.org/140323005/diff/1/chrome/browser/ui/cocoa/java...
> chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:20: const int
> kMessageTextMaxRows = 32;
> This is a cross-platform limit right, WDYT about using one constant across
> platforms?
There is a constant for the max rows defined in a .cc file, which only can be
used by POSIX (exclude MACOS). For Windows platform, there is a characters limit
which is set to 3000. I don't think it's a general cross-platform limit, so I
would suggest to add a separated constant here.
> 
>
https://codereview.chromium.org/140323005/diff/1/chrome/browser/ui/cocoa/java...
> chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:152: NSArray *
> informative_text_array =
> Add a comment with a link to the bug - something like:
> // Truncate long JS alerts - crbug.com/331219
> 
Sure, will add it.
>
https://codereview.chromium.org/140323005/diff/1/chrome/browser/ui/cocoa/java...
> chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:158:
> componentsJoinedByString:@"\n"] stringByAppendingString:@"\n..."];
> Can you split this up into 3 or 4 statements so this is a little more
readable?
Sure, will do.

[jeremy, 2014-01-22 20:16:02]

https://codereview.chromium.org/140323005/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right):

https://codereview.chromium.org/140323005/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:159: info_array =
[info_array valueForKey:@"description"];
What is this meant to do?

[yi, 2014-01-22 22:00:26]

You are right. The valueForKey:@"description" is no need here. Removed from the
patch.

[jeremy, 2014-01-22 22:18:44]

LGTM

[yi, 2014-01-22 22:28:59]

On 2014/01/22 22:18:44, jeremy wrote:
> LGTM

Many thanks, Jeremy. Could you please commit this patch for me since this is my
first patch?

[commit-bot: I haz the power, 2014-01-22 22:34:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yi.shen@samsung.com/140323005/140001

[commit-bot: I haz the power, 2014-01-23 00:39:13]

Change committed as 246457

[Avi (use Gerrit), 2014-01-23 02:58:30]

I don't see this as usefully addressing the problem.

Fire an alert with a meg of random text but text that has no newlines. You have
the same problem.

When someone said that other platforms cut off dialogs after "32 lines", I
imagine that they mean lines as laid-out, not 32 literal lines as provided by
the hostile data.

If I were going to fix this myself, I would truncate it after a certain length
that is "long enough". Anyone who puts that much info into an alert deserves
what they get.

https://codereview.chromium.org/140323005/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right):

https://codereview.chromium.org/140323005/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:153: NSArray *
info_array =
style nit: no space before the *.

[Avi (use Gerrit), 2014-01-23 03:00:04]

Oh. Not LGTM.

[jeremy, 2014-01-23 03:04:53]

A revert of this CL has been created in
https://codereview.chromium.org/145023010/ by jeremy@chromium.org.

The reason for reverting is: Avi is absolutely right, reverting..

[yi, 2014-01-24 01:36:07]

Avi is right - the previous patch doesn't handle the case when a long string has
no new line characters, however, there is no trivial way to limit the number of
the 'visual' lines as long as we use NSAlert to display the alert message.
Truncating the message to a certain length seems to be a reasonable solution,
but it is unable to handle the case when a string contains many new line
characters.

As a compromise solution, my new patch translates the string characters and
empty space in each line to 'slots'. Each character counts as 1 slot and each
empty line (newline) counts as 50 slots. The maximum slots number sets to 2000.
All the string characters beyond 2000 slots would be truncated. This is not a
perfect solution, but it can handle most cases.

[Avi (use Gerrit), 2014-01-24 01:53:23]

I re-opened this CL for you.

https://codereview.chromium.org/140323005/diff/420001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right):

https://codereview.chromium.org/140323005/diff/420001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:157:
informative_text = [[NSString alloc] init];
This leaks. You can do

informative_text = @"";

https://codereview.chromium.org/140323005/diff/420001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:161:
info_text.length < kSlotsPerLine ? kSlotsPerLine : info_text.length;
std::max?

https://codereview.chromium.org/140323005/diff/420001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:162: int free_slots
= kMessageTextMaxSlots - occupied_slots;
You recompute free_slots each time; easier is to make line 158:

int free_slots = kMessageTextMaxSlots;

drop this line, and then below on line 166 do:

free_slots -= new_slots;

https://codereview.chromium.org/140323005/diff/420001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:168: if
((int)info_text.length > free_slots)
Alleviate the need for a cast if you make free_slots a size_t.

https://codereview.chromium.org/140323005/diff/420001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:169: info_text =
[info_text substringToIndex:free_slots];
Indent 2 for an if().

https://codereview.chromium.org/140323005/diff/420001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:171:
informative_text = [informative_text stringByAppendingString:@"\n..."];
What bums me out is that we have gfx::TruncateString to do exactly this kind of
truncation. Can we be clever in counting to re-use it?

[yi, 2014-01-24 02:12:42]

On 2014/01/24 01:53:23, Avi wrote:
> I re-opened this CL for you.
> 
Thank you Avi, I will check the gfx::TruncateString.

[yi, 2014-01-24 19:59:25]

Simplified the truncation algorithm for the long JS alert message. The
gfx::TruncateString is not proper here is because I want to count the newline
characters as 50 instead of 1.

[Avi (use Gerrit), 2014-01-24 20:08:01]

https://codereview.chromium.org/140323005/diff/460001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right):

https://codereview.chromium.org/140323005/diff/460001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:163:
informative_text = [informative_text substringToIndex:index - 1];
> The gfx::TruncateString is not proper here is because I want to count the
newline
> characters as 50 instead of 1.

But that's what you're doing here with -substringToIndex:! It, as well as
TruncateString, takes a length to truncate to. You could easily put in
TruncateString here with i as the length.

[yi, 2014-01-24 20:41:48]

You are right Avi. I just updated the patch which is using TruncateString to
truncate long message.

[Avi (use Gerrit), 2014-01-24 21:08:30]

LGTM

[yi, 2014-01-24 21:14:15]

On 2014/01/24 21:08:30, Avi wrote:
> LGTM
Thanks Avi! Could you please help me commit it -- I believe I have no
permission.

[Avi (use Gerrit), 2014-01-24 21:18:29]

On 2014/01/24 21:14:15, yi wrote:
> On 2014/01/24 21:08:30, Avi wrote:
> > LGTM
> Thanks Avi! Could you please help me commit it -- I believe I have no
> permission.

I clicked the "Commit" checkbox, though I'm surprised that you don't have
permissions to do so yourself.

[commit-bot: I haz the power, 2014-01-24 21:21:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yi.shen@samsung.com/140323005/490001

[yi, 2014-01-24 21:32:08]

On 2014/01/24 21:18:29, Avi wrote:
> On 2014/01/24 21:14:15, yi wrote:
> > On 2014/01/24 21:08:30, Avi wrote:
> > > LGTM
> > Thanks Avi! Could you please help me commit it -- I believe I have no
> > permission.
> 
> I clicked the "Commit" checkbox, though I'm surprised that you don't have
> permissions to do so yourself.
Thanks, I think that's because this is my first patch. BTW, is there a script
like 'check-webkit-style' to help check the coding style?

[commit-bot: I haz the power, 2014-01-24 22:26:41]

Retried try job too often on win_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-25 02:16:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yi.shen@samsung.com/140323005/490001

[commit-bot: I haz the power, 2014-01-25 07:43:19]

Change committed as 247093