[MD Bookmarks] Support elision of bookmark names in the bookmark toast.

[MD Bookmarks] Support elision of bookmark names in the bookmark toast.

This CL makes the translated string in the bookmark toast for deletion collapse
bookmark names when the bookmark name is too long, without affecting the
prior or subsequent parts of the string

BUG=725786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2917003003
Cr-Commit-Position: refs/heads/master@{#478227}
Committed: https://chromium.googlesource.com/chromium/src/+/e0917640f381ceb2e2822f9f697bf62447ad4034

[calamity, 2017-06-02 06:04:11]

Description was changed from

==========
[MD Bookmarks] Support elision of bookmark names in the bookmark toast.

BUG=725786
==========

to

==========
[MD Bookmarks] Support elision of bookmark names in the bookmark toast.

BUG=725786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2017-06-02 06:50:24]

Patchset #1 (id:1) has been deleted

[calamity, 2017-06-02 06:50:36]

calamity@chromium.org changed reviewers:
+ tsergeant@chromium.org

[calamity, 2017-06-02 07:13:13]

Description was changed from

==========
[MD Bookmarks] Support elision of bookmark names in the bookmark toast.

BUG=725786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Support elision of bookmark names in the bookmark toast.

This CL makes the translated string in the bookmark toast for deletion collapse
bookmark names when the bookmark name is too long, without affecting the
prior or subsequent parts of the string

BUG=725786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2017-06-05 03:33:54]

https://codereview.chromium.org/2917003003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/toast_manager.js (right):

https://codereview.chromium.org/2917003003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/toast_manager.js:64: this.$.toast.open =
true;
This looks like it was leftover from a rebase

https://codereview.chromium.org/2917003003/diff/20001/ui/webui/resources/js/l...
File ui/webui/resources/js/load_time_data.js (right):

https://codereview.chromium.org/2917003003/diff/20001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:97: * Returns a list of pieces of a
substituted string which has separate
This is a fairly complex function with a bunch of opaque regex operations, so it
would be nice to more fully explain what's going on here. Eg,

Returns a formatted string where $1 to $9 are replaced by the second to tenth
argument, split apart into a list of pieces describing how the substitution was
performed.

https://codereview.chromium.org/2917003003/diff/20001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:99: * @param {string} label
Add a comment here to explicitly say that this is a string and not a string ID.

https://codereview.chromium.org/2917003003/diff/20001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:107: var pieces =
label.split(/(?=[^$]?)(\$[1-9])/)
Why is the positive lookahead here?

It says something like "optionally match something that is not a dollar sign,
but don't consume it if you do match it", which seems to amount to...never
actually doing anything

https://codereview.chromium.org/2917003003/diff/20001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:115: assert((p.match(/\$/g) ||
[]).length % 2 == 0);
This assertion makes getSubstitutedStringPieces() more restrictive than
getStringF() above.

Where this function requires every $ to be escaped, getStringF() only requires
escaping for things like $1.

It would be nice if we could converge the two checks so that both functions
behave the same (I prefer the getStringF() version, but there's not much
difference).

[calamity, 2017-06-06 03:47:02]

Patchset #2 (id:40001) has been deleted

[calamity, 2017-06-06 03:54:00]

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

[commit-bot: I haz the power, 2017-06-06 03:54:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 03:57:15]

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

[commit-bot: I haz the power, 2017-06-06 03:57:16]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[calamity, 2017-06-06 05:32:37]

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

[commit-bot: I haz the power, 2017-06-06 05:32:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[calamity, 2017-06-06 05:36:03]

https://codereview.chromium.org/2917003003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/toast_manager.js (right):

https://codereview.chromium.org/2917003003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/toast_manager.js:64: this.$.toast.open =
true;
On 2017/06/05 03:33:54, tsergeant wrote:
> This looks like it was leftover from a rebase

Yeah, oops, the last CL has been wreaking havoc on this one.

https://codereview.chromium.org/2917003003/diff/20001/ui/webui/resources/js/l...
File ui/webui/resources/js/load_time_data.js (right):

https://codereview.chromium.org/2917003003/diff/20001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:97: * Returns a list of pieces of a
substituted string which has separate
On 2017/06/05 03:33:54, tsergeant wrote:
> This is a fairly complex function with a bunch of opaque regex operations, so
it
> would be nice to more fully explain what's going on here. Eg,
> 
> Returns a formatted string where $1 to $9 are replaced by the second to tenth
> argument, split apart into a list of pieces describing how the substitution
was
> performed.

Done.

https://codereview.chromium.org/2917003003/diff/20001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:99: * @param {string} label
On 2017/06/05 03:33:54, tsergeant wrote:
> Add a comment here to explicitly say that this is a string and not a string
ID.

Done.

https://codereview.chromium.org/2917003003/diff/20001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:107: var pieces =
label.split(/(?=[^$]?)(\$[1-9])/)
On 2017/06/05 03:33:54, tsergeant wrote:
> Why is the positive lookahead here?
> 
> It says something like "optionally match something that is not a dollar sign,
> but don't consume it if you do match it", which seems to amount to...never
> actually doing anything

Yeah, good point. Okay. I've revised this. We need to catch cases such as $$60
which should render as $60. Tests also updated to catch this case.

The current regex says:
Match $1-9
(\$[1-9])

or
|

Match 1 or more (non $ characters or ($ followed by non 1-9 or EOS))
(([^$]|\$([^1-9]|$))+)

https://codereview.chromium.org/2917003003/diff/20001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:115: assert((p.match(/\$/g) ||
[]).length % 2 == 0);
On 2017/06/05 03:33:54, tsergeant wrote:
> This assertion makes getSubstitutedStringPieces() more restrictive than
> getStringF() above.
> 
> Where this function requires every $ to be escaped, getStringF() only requires
> escaping for things like $1.
> 
> It would be nice if we could converge the two checks so that both functions
> behave the same (I prefer the getStringF() version, but there's not much
> difference).

Done. Went with 'all $ must be escaped' and added tests. I think the consistency
of escaping $ is nice and asserting will help catch mistakes. In any case,
AFAIK, this escaping never comes up (there are no floating $ signs in chrome).

Also added testing for ensuring that getStringF and this function return the
same string when joined.

[commit-bot: I haz the power, 2017-06-06 06:19:47]

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

[commit-bot: I haz the power, 2017-06-06 06:19:48]

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

[tsergeant, 2017-06-06 07:15:56]

lg, but tests are currently failing -- looks like the CommandManager tests
require a ToastManager to exist.

[tsergeant, 2017-06-06 07:17:17]

Oh, that failure is due to the upstream CL. lgtm.

[calamity, 2017-06-07 04:37:35]

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

[commit-bot: I haz the power, 2017-06-07 04:37:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 05:38:19]

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

[commit-bot: I haz the power, 2017-06-07 05:38:20]

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

[calamity, 2017-06-08 03:16:58]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2017-06-08 03:17:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 04:59:16]

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

[commit-bot: I haz the power, 2017-06-08 04:59:17]

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

[calamity, 2017-06-08 05:46:19]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2017-06-08 05:46:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 07:22:47]

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

[commit-bot: I haz the power, 2017-06-08 07:22:49]

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

[calamity, 2017-06-08 07:46:44]

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

[commit-bot: I haz the power, 2017-06-08 07:46:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[calamity, 2017-06-08 08:35:31]

calamity@chromium.org changed reviewers:
+ hirono@chromium.org

[calamity, 2017-06-08 08:35:43]

Description was changed from

==========
[MD Bookmarks] Support elision of bookmark names in the bookmark toast.

This CL makes the translated string in the bookmark toast for deletion collapse
bookmark names when the bookmark name is too long, without affecting the
prior or subsequent parts of the string

BUG=725786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Support elision of bookmark names in the bookmark toast.

This CL makes the translated string in the bookmark toast for deletion collapse
bookmark names when the bookmark name is too long, without affecting the
prior or subsequent parts of the string

BUG=725786
TBR=hirono@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2017-06-08 08:36:10]

+hirono for ui/file_manager OWNERS.

[commit-bot: I haz the power, 2017-06-08 09:20:21]

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

[commit-bot: I haz the power, 2017-06-08 09:20:22]

Dry run: This issue passed the CQ dry run.

[hirono, 2017-06-09 01:02:21]

https://codereview.chromium.org/2917003003/diff/90001/ui/webui/resources/js/l...
File ui/webui/resources/js/load_time_data.js (right):

https://codereview.chromium.org/2917003003/diff/90001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:107: return label.replace(/\$(.|$)/g,
function(m) {
/\$(.|$)/g does not match something like "$\nHi!".
Maybe /\$(.|$|\n)/g or /\$(.|$)/gm ?

https://codereview.chromium.org/2917003003/diff/90001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:128: var pieces =
(label.match(/(\$[1-9])|(([^$]|\$([^1-9]|$))+)/g) ||
nit & optional: (label ? label.split(/(\$[1-9])/) : []).map(...) may be simpler.

[calamity, 2017-06-09 05:55:23]

https://codereview.chromium.org/2917003003/diff/90001/ui/webui/resources/js/l...
File ui/webui/resources/js/load_time_data.js (right):

https://codereview.chromium.org/2917003003/diff/90001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:107: return label.replace(/\$(.|$)/g,
function(m) {
On 2017/06/09 01:02:21, hirono wrote:
> /\$(.|$)/g does not match something like "$\nHi!".
> Maybe /\$(.|$|\n)/g or /\$(.|$)/gm ?

I'm not aware of any multiline strings.

Either way, fixed.

https://codereview.chromium.org/2917003003/diff/90001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:128: var pieces =
(label.match(/(\$[1-9])|(([^$]|\$([^1-9]|$))+)/g) ||
On 2017/06/09 01:02:21, hirono wrote:
> nit & optional: (label ? label.split(/(\$[1-9])/) : []).map(...) may be
simpler.

This won't work for $$60 which I'd expect to render as $60, rather than $, $6,
0. The tests check this case.

[hirono, 2017-06-09 06:01:25]

lgtm

https://codereview.chromium.org/2917003003/diff/90001/ui/webui/resources/js/l...
File ui/webui/resources/js/load_time_data.js (right):

https://codereview.chromium.org/2917003003/diff/90001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:128: var pieces =
(label.match(/(\$[1-9])|(([^$]|\$([^1-9]|$))+)/g) ||
On 2017/06/09 05:55:23, calamity wrote:
> On 2017/06/09 01:02:21, hirono wrote:
> > nit & optional: (label ? label.split(/(\$[1-9])/) : []).map(...) may be
> simpler.
> 
> This won't work for $$60 which I'd expect to render as $60, rather than $, $6,
> 0. The tests check this case.

That's true!

[calamity, 2017-06-09 06:11:30]

Description was changed from

==========
[MD Bookmarks] Support elision of bookmark names in the bookmark toast.

This CL makes the translated string in the bookmark toast for deletion collapse
bookmark names when the bookmark name is too long, without affecting the
prior or subsequent parts of the string

BUG=725786
TBR=hirono@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Support elision of bookmark names in the bookmark toast.

This CL makes the translated string in the bookmark toast for deletion collapse
bookmark names when the bookmark name is too long, without affecting the
prior or subsequent parts of the string

BUG=725786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2017-06-09 06:11:37]

The CQ bit was checked by calamity@chromium.org

[calamity, 2017-06-09 06:11:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org
Link to the patchset: https://codereview.chromium.org/2917003003/#ps110001
(title: "fix multiline")

[commit-bot: I haz the power, 2017-06-09 06:11:55]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 07:34:40]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1496988697546010,
"parent_rev": "9ed3f43a6cc49b844d769498283b2e1e0be6cb94", "commit_rev":
"e0917640f381ceb2e2822f9f697bf62447ad4034"}

[commit-bot: I haz the power, 2017-06-09 07:34:50]

Description was changed from

==========
[MD Bookmarks] Support elision of bookmark names in the bookmark toast.

This CL makes the translated string in the bookmark toast for deletion collapse
bookmark names when the bookmark name is too long, without affecting the
prior or subsequent parts of the string

BUG=725786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Support elision of bookmark names in the bookmark toast.

This CL makes the translated string in the bookmark toast for deletion collapse
bookmark names when the bookmark name is too long, without affecting the
prior or subsequent parts of the string

BUG=725786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2917003003
Cr-Commit-Position: refs/heads/master@{#478227}
Committed:
https://chromium.googlesource.com/chromium/src/+/e0917640f381ceb2e2822f9f697b...
==========

[commit-bot: I haz the power, 2017-06-09 07:34:52]

Committed patchset #5 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/e0917640f381ceb2e2822f9f697b...