Restrict JavaScript dialogs to 100 characters.

Restrict JavaScript dialogs to 100 characters.

BUG=587962
TEST=none; welcome to the new reality

[Avi (use Gerrit), 2016-02-18 20:42:32]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 20:49:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1711863004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1711863004/1

[Avi (use Gerrit), 2016-02-18 21:20:30]

avi@chromium.org changed reviewers:
+ palmer@chromium.org

[palmer, 2016-02-18 21:38:19]

The new reality LGTM.

[Avi (use Gerrit), 2016-02-18 21:47:01]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 21:48:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1711863004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1711863004/20001

[palmer, 2016-02-18 21:51:10]

https://codereview.chromium.org/1711863004/diff/20001/components/app_modal/ja...
File components/app_modal/javascript_dialog_manager.cc (right):

https://codereview.chromium.org/1711863004/diff/20001/components/app_modal/ja...
components/app_modal/javascript_dialog_manager.cc:193:
gfx::TruncateString(message_text, 100, gfx::CHARACTER_BREAK) +
Rather than repeat the code and comment, abstract it into its own little
function?

Also, calling a truncated string |full_message| looks a bit odd.

[Avi (use Gerrit), 2016-02-18 22:03:07]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[Avi (use Gerrit), 2016-02-18 22:05:41]

Chris, ptal.

[commit-bot: I haz the power, 2016-02-18 22:10:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1711863004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1711863004/40001

[esprehn, 2016-02-18 22:53:49]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-02-18 22:53:49]

Not lgtm, this is not the right layer for this. The truncation should be inside
blink and should warn in the console.

[commit-bot: I haz the power, 2016-02-18 22:56:13]

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

[commit-bot: I haz the power, 2016-02-18 22:56:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[palmer, 2016-02-18 22:57:11]

|TruncateMessage| LGTM.

> Not lgtm, this is not the right layer for this. The truncation should be
inside
> blink and should warn in the console.

I disagree about the layering: This is a UX policy choice. Other Blink and
content/ embedders should be allowed to set their own policies.

Logging on the console is a reasonable idea, of course.

[Avi (use Gerrit), 2016-02-18 23:00:24]

On 2016/02/18 22:57:11, palmer wrote:
> |TruncateMessage| LGTM.
> 
> > Not lgtm, this is not the right layer for this. The truncation should be
> inside
> > blink and should warn in the console.
> 
> I disagree about the layering: This is a UX policy choice. Other Blink and
> content/ embedders should be allowed to set their own policies.
> 
> Logging on the console is a reasonable idea, of course.

As suggested by Dmitri, I am going to turn this into an intent-to-ship. Coming
soon to blink-dev.

[esprehn, 2016-02-18 23:05:04]

On 2016/02/18 at 23:00:24, avi wrote:
> On 2016/02/18 22:57:11, palmer wrote:
> > |TruncateMessage| LGTM.
> > 
> > > Not lgtm, this is not the right layer for this. The truncation should be
> > inside
> > > blink and should warn in the console.
> > 
> > I disagree about the layering: This is a UX policy choice. Other Blink and
> > content/ embedders should be allowed to set their own policies.
> > 
> > Logging on the console is a reasonable idea, of course.
> 
> As suggested by Dmitri, I am going to turn this into an intent-to-ship. Coming
soon to blink-dev.

There are no other embedders of Blink except Chrome, it's not a generic
rendering engine, it's Chrome's rendering engine. :)

This is also web facing, and will impact content all over the web. Authors will
need to know, potentially billions of pages will need to change. Did you use
count how often the string is longer than 100 characters? Do you know how often
this triggers? How does an author find out what happened and what the limit is
without the console warning?

This should be in blink.

[Avi (use Gerrit), 2016-02-18 23:26:13]

On 2016/02/18 23:00:24, Avi wrote:
> On 2016/02/18 22:57:11, palmer wrote:
> > |TruncateMessage| LGTM.
> > 
> > > Not lgtm, this is not the right layer for this. The truncation should be
> > inside
> > > blink and should warn in the console.
> > 
> > I disagree about the layering: This is a UX policy choice. Other Blink and
> > content/ embedders should be allowed to set their own policies.
> > 
> > Logging on the console is a reasonable idea, of course.
> 
> As suggested by Dmitri, I am going to turn this into an intent-to-ship. Coming
> soon to blink-dev.

https://groups.google.com/a/chromium.org/d/msg/blink-dev/cFtsKXKd4F0/BOH61FjX...

[Peter Kasting, 2016-02-19 02:18:33]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2016-02-19 02:18:34]

https://codereview.chromium.org/1711863004/diff/40001/components/app_modal/ja...
File components/app_modal/javascript_dialog_manager.cc (right):

https://codereview.chromium.org/1711863004/diff/40001/components/app_modal/ja...
components/app_modal/javascript_dialog_manager.cc:64: return
gfx::TruncateString(message, 100, gfx::CHARACTER_BREAK);
Please use an eliding method rather than a truncation method so that at least
the message looks like we've elided it instead of just looking like a bug.

[Avi (use Gerrit), 2016-02-19 02:24:21]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[Avi (use Gerrit), 2016-02-19 02:24:42]

https://codereview.chromium.org/1711863004/diff/40001/components/app_modal/ja...
File components/app_modal/javascript_dialog_manager.cc (right):

https://codereview.chromium.org/1711863004/diff/40001/components/app_modal/ja...
components/app_modal/javascript_dialog_manager.cc:64: return
gfx::TruncateString(message, 100, gfx::CHARACTER_BREAK);
On 2016/02/19 02:18:34, Peter Kasting wrote:
> Please use an eliding method rather than a truncation method so that at least
> the message looks like we've elided it instead of just looking like a bug.

This was a bad implementation; doing it in a new way in the new patchset.

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1711863004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1711863004/60001

[commit-bot: I haz the power, 2016-02-19 03:28:35]

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

[commit-bot: I haz the power, 2016-02-19 03:28:36]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-02-19 16:39:19]

On 2016/02/19 03:28:36, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

I'm going to close this out for now; I have metrics coming to help us make this
decision.