Use ICU plural syntax in more place

Use ICU plural formatting in more places

1. Recently close tabs
2. Download cancel dialog
3. Cloud Printer discovery
4. extension: usb device selection

For #2, some strings are revised per the string review with ainslie@.
For both browser shutdown and incognito exit, the title is simply
'Download in progress' and the 'go-ahead' button is simply 'Cancel download'
(or their plural forms).

Besides, recently_closed.js is also removed because it's not used any more and removing it let us drop two strings (about closed_tabs) that has become unnecessary with this CL.

BUG=21407, 455005, 482092
TEST=Open a new window, open multiple tabs and close the windows. In another window, go to 'Recent tabs' menu. '# tabs' for the closed window is shown as expected
TEST=1. Go to http://www.thinkbroadband.com/download.html 2. Start downloading a single file 3. Try to quit chrome before the download is completed 4. Check the dialog popping up.
TEST=Do the above with multiple file downloads
TEST=Do the above two in the only open incognito window (close the only open incognito window)

Committed: https://crrev.com/ec68f03e4f4be911752b005fb82b9e49acafea51
Cr-Commit-Position: refs/heads/master@{#329747}

[jungshik at Google, 2015-05-01 20:18:46]

jshin@chromium.org changed reviewers:
+ avi@chromium.org, finnur@chromium.org, pkasting@chromium.org, sky@chromium.org

[jungshik at Google, 2015-05-01 20:18:47]

avi@chromium.org: Please review changes in mac UI

finnur@chromium.org: Please review changes in extensions/*

pkasting@chromium.org: Please review changes in chrome/browser
and strings

sky@chromium.org: Please review changes in chrome/browser and strings

[jungshik at Google, 2015-05-01 20:25:08]

jshin@chromium.org changed reviewers:
+ estade@chromium.org

[jungshik at Google, 2015-05-01 20:25:09]

estade@: Is recently_closed.js still used in NTP? More specifically, is this
function used? Didn't we remove 'recent tabs' from NTP and move it to 'Recent
Tabs' menu? 

 function formatTabsText(numTabs) {
    if (numTabs == 1)
      return loadTimeData.getString('closedwindowsingle');
    return loadTimeData.getStringF('closedwindowmultiple', numTabs);
  }

If we don't use formatTabsText any more (there's still a caller in
recently_closed.js), I wanna get rid of strings in generated_strings.grd. (see
my TODO comment there in this CL)

[jungshik at Google, 2015-05-01 20:28:34]

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/generated_re...
chrome/app/generated_resources.grd:11684: <message
name="IDS_NEW_TAB_RECENTLY_CLOSED_WINDOW_SINGLE"
IDS_NEW_TAB_RECENTLY_CLOSED_WINDOW_SINGLE and
IDS_NEW_TAB_RECENTLY_CLOSED_WINDOW_MULTIPLE are used in recently_closed.js, but
we got rid of 'recent tabs' menu in NTP in 2013. So, we may not need these
strings any more.

[jungshik at Google, 2015-05-01 20:30:08]

jshin@chromium.org changed reviewers:
+ shrike@chromium.org

[jungshik at Google, 2015-05-01 20:30:09]

shrike@, this CL fixes 'download progress' dialog (title, button, contents).
PTAL. Thanks

[shrike, 2015-05-01 20:58:51]

On 2015/05/01 20:30:09, Jungshik at google wrote:
> shrike@, this CL fixes 'download progress' dialog (title, button, contents).
> PTAL. Thanks

LGTM - thank you for working on this issue, and for letting me take a look at
the cl.

[Avi (use Gerrit), 2015-05-01 21:02:19]

It's all lovely to me. LGTM

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/generated_re...
chrome/app/generated_resources.grd:14950: +        <message
name="IDS_LOCAL_DISOCVERY_NOTIFICATION_CONTENTS_PRINTER" desc="Contents of
notification for one or more new printers showing up on your network. [ICU
Syntax]">
While you're in here can you fix all the "DISOCVERY" typos to "DISCOVERY"? Or
would that ripple all over the place?

(At least fix it for the messages we're altering.)

[Peter Kasting, 2015-05-01 21:11:29]

I assumed you wanted me to review the non-Mac parts of c/b/ui, as I'm not a
chrome/browser OWNER

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/chromium_str...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:649: <message
name="IDS_DOWNLOAD_REMOVE_CONFIRM_OK_BUTTON_LABEL" desc="In Title Case: Button
text for OKing to and close the browser when one or more downloads are in
progress. If necessary, add '#' for plural cases. [ICU Syntax]">
Nit: I think you mean "...for canceling the downloads and closing the
browser..." (2 places here, 2 places in google_chrome_strings.grd)

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:651: =1 {Cancel Download and Quit Chromium}
These more verbose strings are descriptive, but I'm not sure whether we want/can
have something this long here.  I'm a bit uncomfortable with changing the text
of the existing string significantly unless you're working with the UI leads
and/or tech writers; otherwise can you leave things close to how they were?

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/generated_re...
chrome/app/generated_resources.grd:2222: +        =1 {Cancel download and exit
incognito mode}
Again, I'm not sure we should change the string text to be noticeably longer.  I
wouldn't do this (or the "continue" case below) without UI OK.

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/google_chrom...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/google_chrom...
chrome/app/google_chrome_strings.grd:574: =1 {Cancel Download and Quit Google
Chrome}
Here we used to use "Chrome" instead of "Google Chrome" to reduce verbosity, and
I would expect us to want something similar even if my comment in the other file
about not changing these strings significantly is not applicable.

https://codereview.chromium.org/1103293004/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/global_menu_bar_x11.cc (right):

https://codereview.chromium.org/1103293004/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:765:
IDS_NEW_TAB_RECENTLY_CLOSED_WINDOW, tabs.size());
Nit: Strange indenting here

I'd probably just inline this into the next statement.

[shrike, 2015-05-01 21:25:35]

On 2015/05/01 21:11:29, Peter Kasting wrote:
>
https://codereview.chromium.org/1103293004/diff/80001/chrome/app/chromium_str...
> chrome/app/chromium_strings.grd:651: =1 {Cancel Download and Quit Chromium}
> These more verbose strings are descriptive, but I'm not sure whether we
want/can
> have something this long here.  I'm a bit uncomfortable with changing the text
> of the existing string significantly unless you're working with the UI leads
> and/or tech writers; otherwise can you leave things close to how they were?

The previous versions of the strings are problematic, which was the whole reason
I filed this bug in the first place. As I'm sure you know/agree, button titles
should completely describe what will happen when you click them (a title like
"Yes, ..." requires you read and understand all the text in the alert). The
longer titles will work on the Mac, so I would be happy if that were the only
place they are changed.

[Peter Kasting, 2015-05-01 21:31:54]

On 2015/05/01 21:25:35, shrike wrote:
> On 2015/05/01 21:11:29, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/1103293004/diff/80001/chrome/app/chromium_str...
> > chrome/app/chromium_strings.grd:651: =1 {Cancel Download and Quit Chromium}
> > These more verbose strings are descriptive, but I'm not sure whether we
> want/can
> > have something this long here.  I'm a bit uncomfortable with changing the
text
> > of the existing string significantly unless you're working with the UI leads
> > and/or tech writers; otherwise can you leave things close to how they were?
> 
> The previous versions of the strings are problematic, which was the whole
reason
> I filed this bug in the first place. As I'm sure you know/agree, button titles
> should completely describe what will happen when you click them (a title like
> "Yes, ..." requires you read and understand all the text in the alert). The
> longer titles will work on the Mac, so I would be happy if that were the only
> place they are changed.

(Please try to reply to my comment directly in the CL commenting interface
instead of separately here so that the discussion thread is all visible in one
place)

Button labels should be descriptive, but there's a limit to the verbosity we're
willing to accept to do that.  IMO, the proposed labels here are too long. 
Remember that these may get significantly longer when translated to Malayalam,
German, etc.  Take a look at this for a very relevant example of how downloads
strings explode in Malayalam: http://imgur.com/Q8jgHDI

This is why I'd like explicit UI/tech writing/localization signoff if we're not
only going to be changing the pluralization method for the strings but also
changing the string contents significantly.  Otherwise, if we just add plural
syntax and leave the existing strings largely unchanged, I can sign off on this.

[shrike, 2015-05-01 21:51:40]

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/chromium_str...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:651: =1 {Cancel Download and Quit Chromium}
On 2015/05/01 21:11:28, Peter Kasting wrote:
> (Please try to reply to my comment directly in the CL commenting interface
> instead of separately here so that the discussion thread is all visible in one
> place)

Sorry, and will do.

> Button labels should be descriptive, but there's a limit to the verbosity
we're
> willing to accept to do that.  IMO, the proposed labels here are too long. 
> Remember that these may get significantly longer when translated to Malayalam,
> German, etc.  Take a look at this for a very relevant example of how downloads
> strings explode in Malayalam: http://imgur.com/Q8jgHDI

It's not surprising that that panel isn't laid out correctly on
Windows/Linux/whatever that screenshot's from. That's why I was saying I'm happy
for the new titles to only appear on the Mac, because the Appkit will do the
right thing.

[Peter Kasting, 2015-05-01 21:59:16]

On 2015/05/01 21:51:40, shrike wrote:
> > Button labels should be descriptive, but there's a limit to the verbosity
> we're
> > willing to accept to do that.  IMO, the proposed labels here are too long. 
> > Remember that these may get significantly longer when translated to
Malayalam,
> > German, etc.  Take a look at this for a very relevant example of how
downloads
> > strings explode in Malayalam: http://imgur.com/Q8jgHDI
> 
> It's not surprising that that panel isn't laid out correctly on
> Windows/Linux/whatever that screenshot's from. That's why I was saying I'm
happy
> for the new titles to only appear on the Mac, because the Appkit will do the
> right thing.

We shouldn't have different strings on the two solely for technical reasons --
only if we actually want them to differ long term.  So it's not OK to change the
text of the string significantly for Mac in this change, but not for other
platforms.

Again, I'm not opposed to getting the text of these changed eventually when we
can do it with proper review and ensure all platforms will work correctly.  But
that doesn't need to happen in the same change as a technical change to modify
how we do pluralization in general.  So this CL should land without that
happening and a separate CL be posted for the substantive text changes, unless
we can verify now that these strings will work on all platforms and our what our
UI/l10n leadership have signed off on.

[jungshik at Google, 2015-05-01 22:25:58]

Thanks for the review. 

Peter, yeah, I meant chrome/browser/ui (I realized that after pressing 'send'
button.

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/chromium_str...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:649: <message
name="IDS_DOWNLOAD_REMOVE_CONFIRM_OK_BUTTON_LABEL" desc="In Title Case: Button
text for OKing to and close the browser when one or more downloads are in
progress. If necessary, add '#' for plural cases. [ICU Syntax]">
On 2015/05/01 21:11:28, Peter Kasting wrote:
> Nit: I think you mean "...for canceling the downloads and closing the
> browser..." (2 places here, 2 places in google_chrome_strings.grd)

Ick. Copy'n'paste .... Thanks

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:651: =1 {Cancel Download and Quit Chromium}
On 2015/05/01 21:11:28, Peter Kasting wrote:
> These more verbose strings are descriptive, but I'm not sure whether we
want/can
> have something this long here.  I'm a bit uncomfortable with changing the text
> of the existing string significantly unless you're working with the UI leads
> and/or tech writers; otherwise can you leave things close to how they were?

A good catch. I was worried about it and meant to do something. Then, I forgot
that. Anyway, http://crbug.com/455005#c7 (which was seconded by leer : UX
designer). 

I'll ask him to review the strings after updating the CL.

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/generated_re...
chrome/app/generated_resources.grd:2222: +        =1 {Cancel download and exit
incognito mode}
On 2015/05/01 21:11:28, Peter Kasting wrote:
> Again, I'm not sure we should change the string text to be noticeably longer. 
I
> wouldn't do this (or the "continue" case below) without UI OK.

I'll go back to strings 'seconded' by cleer@ (UX) and will ask for his approval.

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/generated_re...
chrome/app/generated_resources.grd:14950: +        <message
name="IDS_LOCAL_DISOCVERY_NOTIFICATION_CONTENTS_PRINTER" desc="Contents of
notification for one or more new printers showing up on your network. [ICU
Syntax]">
On 2015/05/01 21:02:18, Avi wrote:
> While you're in here can you fix all the "DISOCVERY" typos to "DISCOVERY"? Or
> would that ripple all over the place?
> 
> (At least fix it for the messages we're altering.)

Ok. I'll do.

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/google_chrom...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/1103293004/diff/80001/chrome/app/google_chrom...
chrome/app/google_chrome_strings.grd:574: =1 {Cancel Download and Quit Google
Chrome}
On 2015/05/01 21:11:29, Peter Kasting wrote:
> Here we used to use "Chrome" instead of "Google Chrome" to reduce verbosity,
and
> I would expect us to want something similar even if my comment in the other
file
> about not changing these strings significantly is not applicable.

I agree with you. Will update.

[Evan Stade, 2015-05-01 22:41:59]

On 2015/05/01 20:25:09, Jungshik at google wrote:
> estade@: Is recently_closed.js still used in NTP? More specifically, is this
> function used? Didn't we remove 'recent tabs' from NTP and move it to 'Recent
> Tabs' menu? 
> 
>  function formatTabsText(numTabs) {
>     if (numTabs == 1)
>       return loadTimeData.getString('closedwindowsingle');
>     return loadTimeData.getStringF('closedwindowmultiple', numTabs);
>   }
> 
> If we don't use formatTabsText any more (there's still a caller in
> recently_closed.js), I wanna get rid of strings in generated_strings.grd. (see
> my TODO comment there in this CL)

I believe recently_closed.js can be removed completely. I haven't looked at this
code in years but to my understanding, ntp4/new_tab.html is actually only used
for one purpose these days: the app launcher page on Linux. I think its
inclusion of recently_closed.js is vestigial. When I remove that include, I
don't get any additional errors in the JS console.

[jungshik at Google, 2015-05-01 23:10:58]

jshin@chromium.org changed reviewers:
+ cleer@chromium.orgpeter kasting
- pkasting@chromium.org

[jungshik at Google, 2015-05-01 23:10:59]

cleer@, can you review string changes? 

It's based on https://code.google.com/p/chromium/issues/detail?id=455005#c7
(which you liked in comment 9).

[jungshik at Google, 2015-05-01 23:18:51]

cleer@ : can you give your take on the questions below? 

estade@: thanks. I removed recently_closed.js

pkasting@: dropping <product name> in button labels allowed me to put common
strings in generated_strings.grd instead of two versions for Chrome and
Chromium. Thanks.

https://codereview.chromium.org/1103293004/diff/100001/chrome/app/chromium_st...
File chrome/app/chromium_strings.grd (left):

https://codereview.chromium.org/1103293004/diff/100001/chrome/app/chromium_st...
chrome/app/chromium_strings.grd:651: </message>
This string was moved to generated_strings.grd because we don't have 'Chromium /
Chrome' any more in the string.

https://codereview.chromium.org/1103293004/diff/100001/chrome/app/chromium_st...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/1103293004/diff/100001/chrome/app/chromium_st...
chrome/app/chromium_strings.grd:646: other {# downloads are currently in
progress. Do you want to cancel the downloads and exit Chromium?}}
I put 'cancel the downloads' before 'exit' (the opposite of the current string).
Is it ok or should I keep the current order?

https://codereview.chromium.org/1103293004/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1103293004/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:2227: +        other {Cancel downloads and
exit incognito mode}}
This string for button is still rather long for a button. We can consider
unifying this string with the corresponding non-incognito string
("Cancel download and exit")? 
The current string is 'Yes, exit incognito mode'.

[jungshik at Google, 2015-05-02 00:14:43]

jshin@chromium.org changed reviewers:
+ cleer@google.com, pkasting@chromium.org
- cleer@chromium.orgpeter kasting

[jungshik at Google, 2015-05-02 00:25:32]

Sending this (please read the quoted msg below) again because I messed up the
reviewer's list while adding cleer to the list. 

Avi, I fixed typos (DISCVO...) :-) Thanks

On 2015/05/01 23:18:51, Jungshik at google wrote:
> cleer@ : can you give your take on the questions below? 
> 
> estade@: thanks. I removed recently_closed.js
> 
> pkasting@: dropping <product name> in button labels allowed me to put common
> strings in generated_strings.grd instead of two versions for Chrome and
> Chromium. Thanks.
> 
>
https://codereview.chromium.org/1103293004/diff/100001/chrome/app/chromium_st...
> File chrome/app/chromium_strings.grd (left):
> 
>
https://codereview.chromium.org/1103293004/diff/100001/chrome/app/chromium_st...
> chrome/app/chromium_strings.grd:651: </message>
> This string was moved to generated_strings.grd because we don't have 'Chromium
/
> Chrome' any more in the string.
> 
>
https://codereview.chromium.org/1103293004/diff/100001/chrome/app/chromium_st...
> File chrome/app/chromium_strings.grd (right):
> 
>
https://codereview.chromium.org/1103293004/diff/100001/chrome/app/chromium_st...
> chrome/app/chromium_strings.grd:646: other {# downloads are currently in
> progress. Do you want to cancel the downloads and exit Chromium?}}
> I put 'cancel the downloads' before 'exit' (the opposite of the current
string).
> Is it ok or should I keep the current order?
> 
>
https://codereview.chromium.org/1103293004/diff/100001/chrome/app/generated_r...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/1103293004/diff/100001/chrome/app/generated_r...
> chrome/app/generated_resources.grd:2227: +        other {Cancel downloads and
> exit incognito mode}}
> This string for button is still rather long for a button. We can consider
> unifying this string with the corresponding non-incognito string
> ("Cancel download and exit")? 
> The current string is 'Yes, exit incognito mode'.

[jungshik at Google, 2015-05-02 01:56:39]

Will update to fix unittest

[Finnur, 2015-05-04 10:16:20]

extensions/* LGTM

[jungshik at Google, 2015-05-04 19:27:44]

https://codereview.chromium.org/1103293004/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1103293004/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:2227: +        other {Cancel downloads and
exit incognito mode}}
On 2015/05/01 23:18:51, Jungshik at google wrote:
> This string for button is still rather long for a button. We can consider
> unifying this string with the corresponding non-incognito string
> ("Cancel download and exit")? 
> The current string is 'Yes, exit incognito mode'. 
> 

Moreover, in some languages, 'Incognito mode' is rather long. Germanic
languages, Malayalam and Spanish (both) are ok. (I missed Peter's comments about
them 
when uploading this PS). However, French, pt-PT and Greek (el) are problematic
along with a few other languages. 

In those languages, both the current string ("Yes, exit incognito mode") and a
proposed string (Cancel downloads and exit incognito mode) would be
too long for a button. The latter is even longer than the former. 

I've filed a bug on this issue (too long translation for 'incognito mode') at
http://crbug.com/484322

Anyway, I might as well leave those strings alone in this CL as suggested by
Peter because the primary purpose of this CL is to handle plural better.

[jungshik at Google, 2015-05-05 18:41:52]

jshin@chromium.org changed reviewers:
+ ainslie@chromium.org

[jungshik at Google, 2015-05-05 18:52:05]

ainslie@ and cleer@: can you review the string changes for 'download in
progress'? 

Summary of changes for 'Download in progress'. 

1. Dialog title 
   - Plural support across languages
   - In English, it'll be either 'Download in progress'
     or 'Downloads in progress'
   - On Mac, titleCase is used

2. Button for cancelling download and quit chrome 

   - Current: Yes, exit Chrome
   - New
       Cancel dowload and exit

3. Button for continuing download (both regular and incognito)
    - Current: No
    - New : 
        Continue download
    - Plural support across languages
    - titlecase on Mac

4. Button for cancelling download and quit incognito mode

    - Current: Yes, exit incognito mode
    - New : 
        Cancel download and exit incognito mode
    - Pluralized
    - titlecase on Mac

 
#4 is problematic because in some languages, 'incognito mode' is trnalsated very
verbosely. I've filed a bug to change that. 

We can unify #2 and #4. What do you think of that? 

Thanks

If it's gonna hard to decide, I may as well postpone the string changes other
than pluralization in this CL as suggested by pkasting.

[chromium-reviews, 2015-05-06 15:27:23]

Those strings look good to me.

We should remove case #4 just use the shorter string from #2 for the
incognito case as well.


On Tue, May 5, 2015 at 11:52 AM, <jshin@chromium.org> wrote:

> ainslie@ and cleer@: can you review the string changes for 'download in
> progress'?
>
> Summary of changes for 'Download in progress'.
>
> 1. Dialog title
>    - Plural support across languages
>    - In English, it'll be either 'Download in progress'
>      or 'Downloads in progress'
>    - On Mac, titleCase is used
>
> 2. Button for cancelling download and quit chrome
>
>    - Current: Yes, exit Chrome
>    - New
>        Cancel dowload and exit
>
> 3. Button for continuing download (both regular and incognito)
>     - Current: No
>     - New :
>         Continue download
>     - Plural support across languages
>     - titlecase on Mac
>
> 4. Button for cancelling download and quit incognito mode
>
>     - Current: Yes, exit incognito mode
>     - New :
>         Cancel download and exit incognito mode
>     - Pluralized
>     - titlecase on Mac
>
>
> #4 is problematic because in some languages, 'incognito mode' is
> trnalsated very
> verbosely. I've filed a bug to change that.
>
> We can unify #2 and #4. What do you think of that?
>
> Thanks
>
> If it's gonna hard to decide, I may as well postpone the string changes
> other
> than pluralization in this CL as suggested by pkasting.
>
> https://codereview.chromium.org/1103293004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jungshik at Google, 2015-05-06 22:10:35]

On 2015/05/06 15:27:23, chromium-reviews wrote:
> Those strings look good to me.
> 
> We should remove case #4 just use the shorter string from #2 for the
> incognito case as well.

Thank you, ainslie, for the string review. 

I unified #4 and #2 on non-Mac in PS #8. 

On Mac, we have (before unification) these: 

2. Quitting Chrome while download is in progress 
  Cancel Download and Quit

4. Closing the last incognito Window
  Cancel Download and Exit Incognito Mode

Note that we're using 'Quit' for #2 and 'Exit Incognito Mode' for #4 on Mac
(while we use 'exit' in both #2 and #4 before the unification). 

In the latest PS (#8), I unified #2 and #4 to 'Cancel download and exit' on
non-Mac, but have two separate strings for #2 and #4 on Mac:

2. Cancel Download and Quit
4. Cancel Download and Exit 

What's your take on that? Do you think we have to unify #2 and #4 on Mac as
well? If so, which one, 'Quit' vs 'Exit'? 

An alternative would be to use 'close' for #4 (incognito) on all platforms
because a user is closing the last incognito window. 

2. Cancel download and exit (non-mac)
   Cancel Download and Quit (Mac)

4. Cancel download and close (non-mac) 
   Cancel Download and Close (Mac)

Thank you in advance for your advice. 

> 
> 
> On Tue, May 5, 2015 at 11:52 AM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=jshin@chromium.org> wrote:
> 
> > ainslie@ and cleer@: can you review the string changes for 'download in
> > progress'?
> >
> > Summary of changes for 'Download in progress'.
> >
> > 1. Dialog title
> >    - Plural support across languages
> >    - In English, it'll be either 'Download in progress'
> >      or 'Downloads in progress'
> >    - On Mac, titleCase is used
> >
> > 2. Button for cancelling download and quit chrome
> >
> >    - Current: Yes, exit Chrome
> >    - New
> >        Cancel dowload and exit
> >
> > 3. Button for continuing download (both regular and incognito)
> >     - Current: No
> >     - New :
> >         Continue download
> >     - Plural support across languages
> >     - titlecase on Mac
> >
> > 4. Button for cancelling download and quit incognito mode
> >
> >     - Current: Yes, exit incognito mode
> >     - New :
> >         Cancel download and exit incognito mode
> >     - Pluralized
> >     - titlecase on Mac
> >
> >
> > #4 is problematic because in some languages, 'incognito mode' is
> > trnalsated very
> > verbosely. I've filed a bug to change that.
> >
> > We can unify #2 and #4. What do you think of that?
> >
> > Thanks
> >
> > If it's gonna hard to decide, I may as well postpone the string changes
> > other
> > than pluralization in this CL as suggested by pkasting.
> >
> > https://codereview.chromium.org/1103293004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....

[chromium-reviews, 2015-05-07 00:19:27]

Hmm. Wait. Maybe I misunderstood :/

In general, on Windows we use "Exit" and Mac we use "Quit" to describe the
act of quitting/exiting the entire browser. A dialog that is catching you
from canceling a download by quitting/exiting Chrome shouldn't need to
distinguish between normal vs. incognito language. That's what I meant
about the merging of 2 and 4. (If you're exiting anyway it doesn't matter
that you're in incognito).

If you're talking about a case where we aren't exiting Chrome but are
closing the final incognito window, then it seems like we should use an
appropriately incognito-specific string.

Do you want to throw some time on the calendar next week?

Alex


On Wed, May 6, 2015 at 3:10 PM, <jshin@chromium.org> wrote:

> On 2015/05/06 15:27:23, chromium-reviews wrote:
>
>> Those strings look good to me.
>>
>
>  We should remove case #4 just use the shorter string from #2 for the
>> incognito case as well.
>>
>
> Thank you, ainslie, for the string review.
>
> I unified #4 and #2 on non-Mac in PS #8.
>
> On Mac, we have (before unification) these:
>
> 2. Quitting Chrome while download is in progress
>   Cancel Download and Quit
>
> 4. Closing the last incognito Window
>   Cancel Download and Exit Incognito Mode
>
> Note that we're using 'Quit' for #2 and 'Exit Incognito Mode' for #4 on Mac
> (while we use 'exit' in both #2 and #4 before the unification).
>
> In the latest PS (#8), I unified #2 and #4 to 'Cancel download and exit' on
> non-Mac, but have two separate strings for #2 and #4 on Mac:
>
> 2. Cancel Download and Quit
> 4. Cancel Download and Exit
>
> What's your take on that? Do you think we have to unify #2 and #4 on Mac as
> well? If so, which one, 'Quit' vs 'Exit'?
>
> An alternative would be to use 'close' for #4 (incognito) on all platforms
> because a user is closing the last incognito window.
>
> 2. Cancel download and exit (non-mac)
>    Cancel Download and Quit (Mac)
>
> 4. Cancel download and close (non-mac)
>    Cancel Download and Close (Mac)
>
> Thank you in advance for your advice.
>
>
>
>  On Tue, May 5, 2015 at 11:52 AM,
>>
> <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=jshin@chromium.org>
> wrote:
>
>  > ainslie@ and cleer@: can you review the string changes for 'download in
>> > progress'?
>> >
>> > Summary of changes for 'Download in progress'.
>> >
>> > 1. Dialog title
>> >    - Plural support across languages
>> >    - In English, it'll be either 'Download in progress'
>> >      or 'Downloads in progress'
>> >    - On Mac, titleCase is used
>> >
>> > 2. Button for cancelling download and quit chrome
>> >
>> >    - Current: Yes, exit Chrome
>> >    - New
>> >        Cancel dowload and exit
>> >
>> > 3. Button for continuing download (both regular and incognito)
>> >     - Current: No
>> >     - New :
>> >         Continue download
>> >     - Plural support across languages
>> >     - titlecase on Mac
>> >
>> > 4. Button for cancelling download and quit incognito mode
>> >
>> >     - Current: Yes, exit incognito mode
>> >     - New :
>> >         Cancel download and exit incognito mode
>> >     - Pluralized
>> >     - titlecase on Mac
>> >
>> >
>> > #4 is problematic because in some languages, 'incognito mode' is
>> > trnalsated very
>> > verbosely. I've filed a bug to change that.
>> >
>> > We can unify #2 and #4. What do you think of that?
>> >
>> > Thanks
>> >
>> > If it's gonna hard to decide, I may as well postpone the string changes
>> > other
>> > than pluralization in this CL as suggested by pkasting.
>> >
>> > https://codereview.chromium.org/1103293004/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to
>>
>
>
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri...
> .
>
>
> https://codereview.chromium.org/1103293004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jungshik at Google, 2015-05-07 22:26:32]

On 2015/05/07 00:19:27, chromium-reviews wrote:
> Hmm. Wait. Maybe I misunderstood :/
> 
> In general, on Windows we use "Exit" and Mac we use "Quit" to describe the
> act of quitting/exiting the entire browser. A dialog that is catching you
> from canceling a download by quitting/exiting Chrome shouldn't need to
> distinguish between normal vs. incognito language. That's what I meant
> about the merging of 2 and 4. (If you're exiting anyway it doesn't matter
> that you're in incognito).
> 
> If you're talking about a case where we aren't exiting Chrome but are
> closing the final incognito window, then it seems like we should use an
> appropriately incognito-specific string.

Yes, that's the case. So, in my previous email, I suggested the following:

2. Shutting down the browser 

   Cancel download and exit (non-mac)
   Cancel Download and Quit (Mac)

4. Closing the last remaining incognito window, but not shutting down the
browser

   Cancel download and close (non-mac) 
   Cancel Download and Close (Mac)

In both cases, download(s) in progress will be cancelled. I wonder if we can
just say 'Cancel download' in both cases. 

> Do you want to throw some time on the calendar next week?

I've just set up a meeting next week. If you like the above proposal, we don't
need to use that time.

If not, we can meet to come up with something better. 

Thanks, 

Jungshik 
> Alex
> 
> 
> On Wed, May 6, 2015 at 3:10 PM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=jshin@chromium.org> wrote:
> 
> > On 2015/05/06 15:27:23, chromium-reviews wrote:
> >
> >> Those strings look good to me.
> >>
> >
> >  We should remove case #4 just use the shorter string from #2 for the
> >> incognito case as well.
> >>
> >
> > Thank you, ainslie, for the string review.
> >
> > I unified #4 and #2 on non-Mac in PS #8.
> >
> > On Mac, we have (before unification) these:
> >
> > 2. Quitting Chrome while download is in progress
> >   Cancel Download and Quit
> >
> > 4. Closing the last incognito Window
> >   Cancel Download and Exit Incognito Mode
> >
> > Note that we're using 'Quit' for #2 and 'Exit Incognito Mode' for #4 on Mac
> > (while we use 'exit' in both #2 and #4 before the unification).
> >
> > In the latest PS (#8), I unified #2 and #4 to 'Cancel download and exit' on
> > non-Mac, but have two separate strings for #2 and #4 on Mac:
> >
> > 2. Cancel Download and Quit
> > 4. Cancel Download and Exit
> >
> > What's your take on that? Do you think we have to unify #2 and #4 on Mac as
> > well? If so, which one, 'Quit' vs 'Exit'?
> >
> > An alternative would be to use 'close' for #4 (incognito) on all platforms
> > because a user is closing the last incognito window.
> >
> > 2. Cancel download and exit (non-mac)
> >    Cancel Download and Quit (Mac)
> >
> > 4. Cancel download and close (non-mac)
> >    Cancel Download and Close (Mac)
> >
> > Thank you in advance for your advice.

[jungshik at Google, 2015-05-12 21:43:27]

jshin@chromium.org changed reviewers:
+ gene@chromium.org

[jungshik at Google, 2015-05-12 21:43:28]

Alex has just reviewed string changes with me. The latest CL reflects what he
suggested in the review (see the CL description). 


gene@ : can you take a look at chrome/browser/local_discovery? 

estade@: can you approve ntp4 changes (dropping recently_closed.js) in
chrome/browser/ui/webui/ntp and chrome/browser/resources/ntp4 ?

pkasting@: can you review/approve chrome/browser/ui ?

[Peter Kasting, 2015-05-12 21:52:03]

LGTM

https://codereview.chromium.org/1103293004/diff/240001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/history_menu_bridge.mm (right):

https://codereview.chromium.org/1103293004/diff/240001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/history_menu_bridge.mm:188: item->title
=l10n_util::GetPluralStringFUTF16(
Nit: Space after =

https://codereview.chromium.org/1103293004/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/download/download_in_progress_dialog_view.cc
(right):

https://codereview.chromium.org/1103293004/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_in_progress_dialog_view.cc:41: switch
(dialog_type) {
Nit: Simpler:

  DCHECK_NE(Browser::DOWNLOAD_CLOSE_OK, dialog_type);
  base::string16 explanation_text(l10n_util::GetPluralStringFUTF16(
      (dialog_type == Browser::DOWNLOAD_CLOSE_BROWSER_SHUTDOWN)
          ? IDS_DOWNLOAD_REMOVE_CONFIRM_EXPLANATION
          : IDS_INCOGNITO_DOWNLOAD_REMOVE_CONFIRM_EXPLANATION,
      download_count));

https://codereview.chromium.org/1103293004/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/global_menu_bar_x11.cc (right):

https://codereview.chromium.org/1103293004/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:765:
IDS_NEW_TAB_RECENTLY_CLOSED_WINDOW, tabs.size());
Nit: Strange indenting here (8 spaces?)

[gene, 2015-05-13 08:17:34]

lgtm for local_discovery

[jungshik at Google, 2015-05-13 20:37:43]

jshin@chromium.org changed reviewers:
+ dbeam@chromium.org evan stade
- estade@chromium.org

[jungshik at Google, 2015-05-13 20:37:44]

dbeam@ : can you approve the ntp4 changes? It's basically removing
recently_closed.js (that is NOT used any more). See estade's comment above. 
Thanks.

[Evan Stade, 2015-05-13 22:14:30]

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

[Evan Stade, 2015-05-13 22:14:32]

https://codereview.chromium.org/1103293004/diff/260001/chrome/browser/resourc...
File chrome/browser/resources/ntp4/new_tab.html (right):

https://codereview.chromium.org/1103293004/diff/260001/chrome/browser/resourc...
chrome/browser/resources/ntp4/new_tab.html:125: <button
id="recently-closed-menu-button"
I think you need to remove this stuff, and probably recently_closed_handler.cc?
There's a bit more stuff to remove here, I'll try to whip up a CL.

[Dan Beam, 2015-05-13 22:15:49]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org evan stade

[jungshik at Google, 2015-05-13 22:34:39]

On 2015/05/13 22:14:32, Evan Stade wrote:
>
https://codereview.chromium.org/1103293004/diff/260001/chrome/browser/resourc...
> File chrome/browser/resources/ntp4/new_tab.html (right):
> 
>
https://codereview.chromium.org/1103293004/diff/260001/chrome/browser/resourc...
> chrome/browser/resources/ntp4/new_tab.html:125: <button
> id="recently-closed-menu-button"
> I think you need to remove this stuff, and probably
recently_closed_handler.cc?
> There's a bit more stuff to remove here, I'll try to whip up a CL.

Thanks.  Then, I'll drop ntp4-related changes from this CL and land it.

[Evan Stade, 2015-05-13 22:40:27]

On 2015/05/13 22:34:39, Jungshik at google wrote:
> On 2015/05/13 22:14:32, Evan Stade wrote:
> >
>
https://codereview.chromium.org/1103293004/diff/260001/chrome/browser/resourc...
> > File chrome/browser/resources/ntp4/new_tab.html (right):
> > 
> >
>
https://codereview.chromium.org/1103293004/diff/260001/chrome/browser/resourc...
> > chrome/browser/resources/ntp4/new_tab.html:125: <button
> > id="recently-closed-menu-button"
> > I think you need to remove this stuff, and probably
> recently_closed_handler.cc?
> > There's a bit more stuff to remove here, I'll try to whip up a CL.
> 
> Thanks.  Then, I'll drop ntp4-related changes from this CL and land it.

sgtm

[jungshik at Google, 2015-05-13 22:50:57]

On 2015/05/13 22:40:27, Evan Stade wrote:
> On 2015/05/13 22:34:39, Jungshik at google wrote:
> > On 2015/05/13 22:14:32, Evan Stade wrote:
> > >
> >
>
https://codereview.chromium.org/1103293004/diff/260001/chrome/browser/resourc...
> > > File chrome/browser/resources/ntp4/new_tab.html (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1103293004/diff/260001/chrome/browser/resourc...
> > > chrome/browser/resources/ntp4/new_tab.html:125: <button
> > > id="recently-closed-menu-button"
> > > I think you need to remove this stuff, and probably
> > recently_closed_handler.cc?
> > > There's a bit more stuff to remove here, I'll try to whip up a CL.
> > 
> > Thanks.  Then, I'll drop ntp4-related changes from this CL and land it.
> 
> sgtm

I added 'TODO(estade)' about removing two messages that will become unnecessary
(generated_strings.grd) when recently_closed.js is dropped :-)

[jungshik at Google, 2015-05-13 22:51:08]

The CQ bit was checked by jshin@chromium.org

[jungshik at Google, 2015-05-13 22:51:09]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
shrike@chromium.org, finnur@chromium.org, pkasting@chromium.org,
gene@chromium.org
Link to the patchset: https://codereview.chromium.org/1103293004/#ps280001
(title: "recently_closed.js restored per estade")

[commit-bot: I haz the power, 2015-05-13 22:52:09]

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

[Dan Beam, 2015-05-13 23:31:41]

On 2015/05/13 22:50:57, Jungshik at google wrote:
> On 2015/05/13 22:40:27, Evan Stade wrote:
> > On 2015/05/13 22:34:39, Jungshik at google wrote:
> > > On 2015/05/13 22:14:32, Evan Stade wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1103293004/diff/260001/chrome/browser/resourc...
> > > > File chrome/browser/resources/ntp4/new_tab.html (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1103293004/diff/260001/chrome/browser/resourc...
> > > > chrome/browser/resources/ntp4/new_tab.html:125: <button
> > > > id="recently-closed-menu-button"
> > > > I think you need to remove this stuff, and probably
> > > recently_closed_handler.cc?
> > > > There's a bit more stuff to remove here, I'll try to whip up a CL.
> > > 
> > > Thanks.  Then, I'll drop ntp4-related changes from this CL and land it.
> > 
> > sgtm
> 
> I added 'TODO(estade)' about removing two messages that will become
unnecessary
> (generated_strings.grd) when recently_closed.js is dropped :-)

it's TODONE, btw: https://codereview.chromium.org/1130753007/

though this can be done after you CL, I assume

[commit-bot: I haz the power, 2015-05-14 00:03:42]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-05-14 00:04:51]

Patchset 15 (id:??) landed as
https://crrev.com/ec68f03e4f4be911752b005fb82b9e49acafea51
Cr-Commit-Position: refs/heads/master@{#329747}