Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2587453002:
Chromad: Use different enterprise messaging in tray bubble. (Closed)
|
Created:
4 years ago by Roman Sorokin (ftl) Modified:
4 years ago CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChromad: Use different enterprise messaging in tray bubble.
Message is "This device is enterprise managed"
BUG=665445
TEST=manual
TBR=grt@chromium.org
Committed: https://crrev.com/73e62576ba72a91bbd365870f1d720310b5e5647
Cr-Commit-Position: refs/heads/master@{#440608}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Notify when IsActiveDirectoryManaged changed or it stayed false and enterprise domain changed. #
Total comments: 6
Patch Set 3 : Change string constant name. Rewrite if statement. #
Total comments: 2
Patch Set 4 : Remove braces #
Messages
Total messages: 29 (16 generated)
The CQ bit was checked by rsorokin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Chromad: Use different enterprise messaging in tray bubble. Message is "This device is enterprise managed" BUG=665445 TEST=manual ========== to ========== Chromad: Use different enterprise messaging in tray bubble. Message is "This device is enterprise managed" BUG=665445 TEST=manual TBR=grt@chromium.org ==========
rsorokin@chromium.org changed reviewers: + oshima@chromium.org
Oshima, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2587453002/diff/1/chrome/browser/ui/ash/syste... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2587453002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:871: if (!old_is_active_directory_managed) if (is_active_directory_managed && !old_is_active_directory_managed) { so we don't notify when the active directory is disabled?
https://codereview.chromium.org/2587453002/diff/1/chrome/browser/ui/ash/syste... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2587453002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:871: if (!old_is_active_directory_managed) On 2016/12/16 20:57:56, oshima wrote: > if (is_active_directory_managed && !old_is_active_directory_managed) { > > so we don't notify when the active directory is disabled? Good point! Fixed. Notifying when IsActiveDirectoryManaged changed or it stayed false and enterprise domain changed
rsorokin@chromium.org changed reviewers: + stevenjb@chromium.org
Hi Steven,
Could you PTAL at chrome/browser/ui/ash/system_tray_delegate_chromeos.{h,cc}
since Oshima is OOO? Thanks!
https://codereview.chromium.org/2587453002/diff/20001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2587453002/diff/20001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:5562: <message name="IDS_DEVICE_OWNED_BY_AD_NOTICE" desc="Text for notifications showing that this device is managed by Active Directory server."> We should spell out ACTIVE_DIRECTORY here, or make this more generic (e.g. IDS_DEVICE_ENTERPRISE_MANAGED). https://codereview.chromium.org/2587453002/diff/20001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2587453002/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:260: } else if (!GetEnterpriseDomain().empty()) { No else
https://codereview.chromium.org/2587453002/diff/20001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2587453002/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:870: if (is_active_directory_managed_) { Isn't this if ((is_active_directory_managed_ && enterprise_domain_ != old_) || (is_active_directory_managed != old_is_active_directory_managed) { Notify() } that is, (domain name changed, or enabled/disabled) ?
The CQ bit was checked by rsorokin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Oshima, Steven, PTAL https://codereview.chromium.org/2587453002/diff/20001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2587453002/diff/20001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:5562: <message name="IDS_DEVICE_OWNED_BY_AD_NOTICE" desc="Text for notifications showing that this device is managed by Active Directory server."> On 2016/12/21 18:18:04, stevenjb wrote: > We should spell out ACTIVE_DIRECTORY here, or make this more generic (e.g. > IDS_DEVICE_ENTERPRISE_MANAGED). Done. https://codereview.chromium.org/2587453002/diff/20001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2587453002/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:260: } else if (!GetEnterpriseDomain().empty()) { On 2016/12/21 18:18:04, stevenjb wrote: > No else Done. https://codereview.chromium.org/2587453002/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:870: if (is_active_directory_managed_) { On 2016/12/21 22:11:33, oshima (OOO til Jan 5th) wrote: > Isn't this > > if ((is_active_directory_managed_ && enterprise_domain_ != old_) || > (is_active_directory_managed != old_is_active_directory_managed) { > Notify() > } > > that is, (domain name changed, or enabled/disabled) > ? True. But slightly different if ((!is_active_directory_managed_ && enterprise_domain_ != old_) || (is_active_directory_managed != old_is_active_directory_managed) { Notify() } Changed it. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm https://codereview.chromium.org/2587453002/diff/40001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2587453002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:260: } nit: {} not needed here
The CQ bit was checked by rsorokin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2587453002/#ps60001 (title: "Remove braces")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2587453002/diff/40001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2587453002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:260: } On 2016/12/22 19:09:29, stevenjb wrote: > nit: {} not needed here Done.
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482484318646610,
"parent_rev": "ef181a9e2e82f07227c810dd8cc4383082a8d7e9", "commit_rev":
"981bc86b583707eb1544f5f224d55ac0ed27fa8a"}
Message was sent while issue was closed.
Description was changed from ========== Chromad: Use different enterprise messaging in tray bubble. Message is "This device is enterprise managed" BUG=665445 TEST=manual TBR=grt@chromium.org ========== to ========== Chromad: Use different enterprise messaging in tray bubble. Message is "This device is enterprise managed" BUG=665445 TEST=manual TBR=grt@chromium.org Review-Url: https://codereview.chromium.org/2587453002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Chromad: Use different enterprise messaging in tray bubble. Message is "This device is enterprise managed" BUG=665445 TEST=manual TBR=grt@chromium.org Review-Url: https://codereview.chromium.org/2587453002 ========== to ========== Chromad: Use different enterprise messaging in tray bubble. Message is "This device is enterprise managed" BUG=665445 TEST=manual TBR=grt@chromium.org Committed: https://crrev.com/73e62576ba72a91bbd365870f1d720310b5e5647 Cr-Commit-Position: refs/heads/master@{#440608} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/73e62576ba72a91bbd365870f1d720310b5e5647 Cr-Commit-Position: refs/heads/master@{#440608} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
