Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1368653002:
Setting CommandLabel 'IDS_DOWNLOAD_NOTIFICATION_LABEL_OPEN" for finished Download Notification. (Closed)
DescriptionSetting CommandLabel 'IDS_DOWNLOAD_NOTIFICATION_LABEL_OPEN' when Download is completed and ready to open to distingush it from 'IDS_DOWNLOAD_NOTIFICATION_LABEL_OPEN_WHEN_COMPLETE' that indicated Download is in progress and not yet complete.
And will open when download is Finished.
BUG=535520
Committed: https://crrev.com/bca6c40e3d7482c9641a681bd05aa9c23b613277
Cr-Commit-Position: refs/heads/master@{#351719}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Changes as per review comments. #Patch Set 3 : Changes as per review comments #
Total comments: 2
Patch Set 4 : Changes as per review comments. #
Total comments: 2
Patch Set 5 : Updating indentation. #Patch Set 6 : Rebased. #
Messages
Total messages: 26 (7 generated)
deepak.m1@samsung.com changed reviewers: + asanka@chromium.org
PTAL
deepak.m1@samsung.com changed reviewers: + ajith.v@chromium.org
asanka@chromium.org changed reviewers: + yoshiki@chromium.org
https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... File chrome/browser/download/notification/download_item_notification.cc (right): https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... chrome/browser/download/notification/download_item_notification.cc:685: id = IDS_DOWNLOAD_STATUS_OPEN; yoshiki: Aren't these labels for the command buttons? The text "Opening when complete" isn't grammatically correct for the open button. The IDS_DOWNLOAD_STATUS_* strings are for indicating the status of the download. If the user elects to open the download when it completes, then the status says "Opening when complete" or "Opening when done". But that text isn't suited for use as a command label. A more appropriate string would be something closer to the IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and IDS_DOWNLOAD_MENU_OPEN.
https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... File chrome/browser/download/notification/download_item_notification.cc (right): https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... chrome/browser/download/notification/download_item_notification.cc:685: id = IDS_DOWNLOAD_STATUS_OPEN; On 2015/09/25 03:18:47, asanka wrote: > yoshiki: Aren't these labels for the command buttons? > > The text "Opening when complete" isn't grammatically correct for the open > button. The IDS_DOWNLOAD_STATUS_* strings are for indicating the status of the > download. If the user elects to open the download when it completes, then the > status says "Opening when complete" or "Opening when done". But that text isn't > suited for use as a command label. > > A more appropriate string would be something closer to the > IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and IDS_DOWNLOAD_MENU_OPEN. In DownloadShelfContextMenu::GetLabelForCommandId() it is using IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and IDS_DOWNLOAD_MENU_OPEN But here all the labels have IDS_DOWNLOAD_MENU_* strings that is menu strings. It appears that DownloadItemNotification::GetCommandLabel() is for non menu items. So I have introduced IDS_DOWNLOAD_STATUS_OPEN, on similar lines. This is as per my understanding. Please correct me if I misunderstood. I would like to hear more on this from @yoshiki. Thanks
https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... File chrome/browser/download/notification/download_item_notification.cc (right): https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... chrome/browser/download/notification/download_item_notification.cc:685: id = IDS_DOWNLOAD_STATUS_OPEN; On 2015/09/25 at 16:09:33, Deepak wrote: > On 2015/09/25 03:18:47, asanka wrote: > > yoshiki: Aren't these labels for the command buttons? > > > > The text "Opening when complete" isn't grammatically correct for the open > > button. The IDS_DOWNLOAD_STATUS_* strings are for indicating the status of the > > download. If the user elects to open the download when it completes, then the > > status says "Opening when complete" or "Opening when done". But that text isn't > > suited for use as a command label. > > > > A more appropriate string would be something closer to the > > IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and IDS_DOWNLOAD_MENU_OPEN. > > In DownloadShelfContextMenu::GetLabelForCommandId() > it is using IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and IDS_DOWNLOAD_MENU_OPEN > But here all the labels have IDS_DOWNLOAD_MENU_* strings that is menu strings. > It appears that DownloadItemNotification::GetCommandLabel() is for non menu items. So I have introduced IDS_DOWNLOAD_STATUS_OPEN, on similar lines. > This is as per my understanding. Please correct me if I misunderstood. > I would like to hear more on this from @yoshiki. > Thanks _DOWNLOAD_STATUS_ is for download status strings. They shouldn't be used to describe actions.
I understood your point. PTAL https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... File chrome/browser/download/notification/download_item_notification.cc (right): https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... chrome/browser/download/notification/download_item_notification.cc:685: id = IDS_DOWNLOAD_STATUS_OPEN; On 2015/09/25 17:09:35, asanka wrote: > On 2015/09/25 at 16:09:33, Deepak wrote: > > On 2015/09/25 03:18:47, asanka wrote: > > > yoshiki: Aren't these labels for the command buttons? > > > > > > The text "Opening when complete" isn't grammatically correct for the open > > > button. The IDS_DOWNLOAD_STATUS_* strings are for indicating the status of > the > > > download. If the user elects to open the download when it completes, then > the > > > status says "Opening when complete" or "Opening when done". But that text > isn't > > > suited for use as a command label. > > > > > > A more appropriate string would be something closer to the > > > IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and IDS_DOWNLOAD_MENU_OPEN. > > > > In DownloadShelfContextMenu::GetLabelForCommandId() > > it is using IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and IDS_DOWNLOAD_MENU_OPEN > > But here all the labels have IDS_DOWNLOAD_MENU_* strings that is menu strings. > > It appears that DownloadItemNotification::GetCommandLabel() is for non menu > items. So I have introduced IDS_DOWNLOAD_STATUS_OPEN, on similar lines. > > This is as per my understanding. Please correct me if I misunderstood. > > I would like to hear more on this from @yoshiki. > > Thanks > > _DOWNLOAD_STATUS_ is for download status strings. They shouldn't be used to > describe actions. I got your point. Changes done. Thanks
On 2015/09/27 06:19:57, Deepak wrote: > I understood your point. > PTAL > > https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... > File chrome/browser/download/notification/download_item_notification.cc (right): > > https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... > chrome/browser/download/notification/download_item_notification.cc:685: id = > IDS_DOWNLOAD_STATUS_OPEN; > On 2015/09/25 17:09:35, asanka wrote: > > On 2015/09/25 at 16:09:33, Deepak wrote: > > > On 2015/09/25 03:18:47, asanka wrote: > > > > yoshiki: Aren't these labels for the command buttons? > > > > > > > > The text "Opening when complete" isn't grammatically correct for the open > > > > button. The IDS_DOWNLOAD_STATUS_* strings are for indicating the status of > > the > > > > download. If the user elects to open the download when it completes, then > > the > > > > status says "Opening when complete" or "Opening when done". But that text > > isn't > > > > suited for use as a command label. > > > > > > > > A more appropriate string would be something closer to the > > > > IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and IDS_DOWNLOAD_MENU_OPEN. > > > > > > In DownloadShelfContextMenu::GetLabelForCommandId() > > > it is using IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and IDS_DOWNLOAD_MENU_OPEN > > > But here all the labels have IDS_DOWNLOAD_MENU_* strings that is menu > strings. > > > It appears that DownloadItemNotification::GetCommandLabel() is for non menu > > items. So I have introduced IDS_DOWNLOAD_STATUS_OPEN, on similar lines. > > > This is as per my understanding. Please correct me if I misunderstood. > > > I would like to hear more on this from @yoshiki. > > > Thanks > > > > _DOWNLOAD_STATUS_ is for download status strings. They shouldn't be used to > > describe actions. > > I got your point. Changes done. > Thanks I agree that IDS_DOWNLOAD_STATUS_* are not preferred, since GetCommandLabel() should return the label for action buttons on download notifications. But IDS_DOWNLOAD_MENU_* are also not good, since these strings contains "&" for shortcuts on menus. They are useless for notification buttons. I think it's better to add new strings for the labels of download notification buttons.
On 2015/09/28 16:05:40, yoshiki wrote: > On 2015/09/27 06:19:57, Deepak wrote: > > I understood your point. > > PTAL > > > > > https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... > > File chrome/browser/download/notification/download_item_notification.cc > (right): > > > > > https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... > > chrome/browser/download/notification/download_item_notification.cc:685: id = > > IDS_DOWNLOAD_STATUS_OPEN; > > On 2015/09/25 17:09:35, asanka wrote: > > > On 2015/09/25 at 16:09:33, Deepak wrote: > > > > On 2015/09/25 03:18:47, asanka wrote: > > > > > yoshiki: Aren't these labels for the command buttons? > > > > > > > > > > The text "Opening when complete" isn't grammatically correct for the > open > > > > > button. The IDS_DOWNLOAD_STATUS_* strings are for indicating the status > of > > > the > > > > > download. If the user elects to open the download when it completes, > then > > > the > > > > > status says "Opening when complete" or "Opening when done". But that > text > > > isn't > > > > > suited for use as a command label. > > > > > > > > > > A more appropriate string would be something closer to the > > > > > IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and IDS_DOWNLOAD_MENU_OPEN. > > > > > > > > In DownloadShelfContextMenu::GetLabelForCommandId() > > > > it is using IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and > IDS_DOWNLOAD_MENU_OPEN > > > > But here all the labels have IDS_DOWNLOAD_MENU_* strings that is menu > > strings. > > > > It appears that DownloadItemNotification::GetCommandLabel() is for non > menu > > > items. So I have introduced IDS_DOWNLOAD_STATUS_OPEN, on similar lines. > > > > This is as per my understanding. Please correct me if I misunderstood. > > > > I would like to hear more on this from @yoshiki. > > > > Thanks > > > > > > _DOWNLOAD_STATUS_ is for download status strings. They shouldn't be used to > > > describe actions. > > > > I got your point. Changes done. > > Thanks > > I agree that IDS_DOWNLOAD_STATUS_* are not preferred, since GetCommandLabel() > should return the label for action buttons on download notifications. But > IDS_DOWNLOAD_MENU_* are also not good, since these strings contains "&" for > shortcuts on menus. They are useless for notification buttons. > > I think it's better to add new strings for the labels of download notification > buttons. @Asanka I agree with yoshiki, how about adding 2 new strings like <message name="IDS_DOWNLOAD_NOTIFICATION_LABEL_OPEN_WHEN_COMPLETE" desc="Download Notification label: Open when download is finished"> Open when done </message> <message name="IDS_DOWNLOAD_NOTIFICATION_LABEL_OPEN" desc="Download Notification label: Open download"> Open </message>
On 2015/09/29 at 04:09:21, deepak.m1 wrote: > On 2015/09/28 16:05:40, yoshiki wrote: > > On 2015/09/27 06:19:57, Deepak wrote: > > > I understood your point. > > > PTAL > > > > > > > > https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... > > > File chrome/browser/download/notification/download_item_notification.cc > > (right): > > > > > > > > https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... > > > chrome/browser/download/notification/download_item_notification.cc:685: id = > > > IDS_DOWNLOAD_STATUS_OPEN; > > > On 2015/09/25 17:09:35, asanka wrote: > > > > On 2015/09/25 at 16:09:33, Deepak wrote: > > > > > On 2015/09/25 03:18:47, asanka wrote: > > > > > > yoshiki: Aren't these labels for the command buttons? > > > > > > > > > > > > The text "Opening when complete" isn't grammatically correct for the > > open > > > > > > button. The IDS_DOWNLOAD_STATUS_* strings are for indicating the status > > of > > > > the > > > > > > download. If the user elects to open the download when it completes, > > then > > > > the > > > > > > status says "Opening when complete" or "Opening when done". But that > > text > > > > isn't > > > > > > suited for use as a command label. > > > > > > > > > > > > A more appropriate string would be something closer to the > > > > > > IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and IDS_DOWNLOAD_MENU_OPEN. > > > > > > > > > > In DownloadShelfContextMenu::GetLabelForCommandId() > > > > > it is using IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and > > IDS_DOWNLOAD_MENU_OPEN > > > > > But here all the labels have IDS_DOWNLOAD_MENU_* strings that is menu > > > strings. > > > > > It appears that DownloadItemNotification::GetCommandLabel() is for non > > menu > > > > items. So I have introduced IDS_DOWNLOAD_STATUS_OPEN, on similar lines. > > > > > This is as per my understanding. Please correct me if I misunderstood. > > > > > I would like to hear more on this from @yoshiki. > > > > > Thanks > > > > > > > > _DOWNLOAD_STATUS_ is for download status strings. They shouldn't be used to > > > > describe actions. > > > > > > I got your point. Changes done. > > > Thanks > > > > I agree that IDS_DOWNLOAD_STATUS_* are not preferred, since GetCommandLabel() > > should return the label for action buttons on download notifications. But > > IDS_DOWNLOAD_MENU_* are also not good, since these strings contains "&" for > > shortcuts on menus. They are useless for notification buttons. > > > > I think it's better to add new strings for the labels of download notification > > buttons. > > @Asanka > I agree with yoshiki, > how about adding 2 new strings like > <message name="IDS_DOWNLOAD_NOTIFICATION_LABEL_OPEN_WHEN_COMPLETE" > desc="Download Notification label: Open when download is finished"> > Open when done > </message> > <message name="IDS_DOWNLOAD_NOTIFICATION_LABEL_OPEN" > desc="Download Notification label: Open download"> > Open > </message> Sounds good.
On 2015/09/29 04:21:52, asanka wrote: > On 2015/09/29 at 04:09:21, deepak.m1 wrote: > > On 2015/09/28 16:05:40, yoshiki wrote: > > > On 2015/09/27 06:19:57, Deepak wrote: > > > > I understood your point. > > > > PTAL > > > > > > > > > > > > https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... > > > > File chrome/browser/download/notification/download_item_notification.cc > > > (right): > > > > > > > > > > > > https://codereview.chromium.org/1368653002/diff/1/chrome/browser/download/not... > > > > chrome/browser/download/notification/download_item_notification.cc:685: id > = > > > > IDS_DOWNLOAD_STATUS_OPEN; > > > > On 2015/09/25 17:09:35, asanka wrote: > > > > > On 2015/09/25 at 16:09:33, Deepak wrote: > > > > > > On 2015/09/25 03:18:47, asanka wrote: > > > > > > > yoshiki: Aren't these labels for the command buttons? > > > > > > > > > > > > > > The text "Opening when complete" isn't grammatically correct for the > > > open > > > > > > > button. The IDS_DOWNLOAD_STATUS_* strings are for indicating the > status > > > of > > > > > the > > > > > > > download. If the user elects to open the download when it completes, > > > then > > > > > the > > > > > > > status says "Opening when complete" or "Opening when done". But that > > > text > > > > > isn't > > > > > > > suited for use as a command label. > > > > > > > > > > > > > > A more appropriate string would be something closer to the > > > > > > > IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and IDS_DOWNLOAD_MENU_OPEN. > > > > > > > > > > > > In DownloadShelfContextMenu::GetLabelForCommandId() > > > > > > it is using IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE and > > > IDS_DOWNLOAD_MENU_OPEN > > > > > > But here all the labels have IDS_DOWNLOAD_MENU_* strings that is menu > > > > strings. > > > > > > It appears that DownloadItemNotification::GetCommandLabel() is for non > > > menu > > > > > items. So I have introduced IDS_DOWNLOAD_STATUS_OPEN, on similar lines. > > > > > > This is as per my understanding. Please correct me if I misunderstood. > > > > > > I would like to hear more on this from @yoshiki. > > > > > > Thanks > > > > > > > > > > _DOWNLOAD_STATUS_ is for download status strings. They shouldn't be used > to > > > > > describe actions. > > > > > > > > I got your point. Changes done. > > > > Thanks > > > > > > I agree that IDS_DOWNLOAD_STATUS_* are not preferred, since > GetCommandLabel() > > > should return the label for action buttons on download notifications. But > > > IDS_DOWNLOAD_MENU_* are also not good, since these strings contains "&" > for > > > shortcuts on menus. They are useless for notification buttons. > > > > > > I think it's better to add new strings for the labels of download > notification > > > buttons. > > > > @Asanka > > I agree with yoshiki, > > how about adding 2 new strings like > > <message name="IDS_DOWNLOAD_NOTIFICATION_LABEL_OPEN_WHEN_COMPLETE" > > desc="Download Notification label: Open when download is finished"> > > Open when done > > </message> > > <message name="IDS_DOWNLOAD_NOTIFICATION_LABEL_OPEN" > > desc="Download Notification label: Open download"> > > Open > > </message> > > Sounds good. @Asanka Thanks, Changes done. PTAL
You'd also need to coordinate landing this around https://codereview.chromium.org/1365963004. https://codereview.chromium.org/1368653002/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1368653002/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:1819: + desc="Download Notification label: Open when download is finished"> Note that the translators aren't necessarily familiar with the UI surfaces that these strings go on. So you'd want to be a bit more specific. I.e. something like: "Download notifications: Label for 'Open when download is complete' action button on notification"
@Asanka yes, I will coordinate this patch with https://codereview.chromium.org/1365963004. and will submit after above CL get merged. Other changes done. PTAL Thanks https://codereview.chromium.org/1368653002/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1368653002/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:1819: + desc="Download Notification label: Open when download is finished"> On 2015/09/29 15:20:30, asanka wrote: > Note that the translators aren't necessarily familiar with the UI surfaces that > these strings go on. So you'd want to be a bit more specific. I.e. something > like: > > "Download notifications: Label for 'Open when download is complete' action > button on notification" Done.
https://codereview.chromium.org/1368653002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1368653002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:1822: + <message name="IDS_DOWNLOAD_NOTIFICATION_LABEL_OPEN" Format these consistently with the surrounding code. The indentation is off.
@Asanka PTAL https://codereview.chromium.org/1368653002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1368653002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:1822: + <message name="IDS_DOWNLOAD_NOTIFICATION_LABEL_OPEN" On 2015/09/29 16:35:02, asanka wrote: > Format these consistently with the surrounding code. The indentation is off. Done.
lgtm
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1368653002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1368653002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by deepak.m1@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/1368653002/#ps100001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1368653002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1368653002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/bca6c40e3d7482c9641a681bd05aa9c23b613277 Cr-Commit-Position: refs/heads/master@{#351719} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
