Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2339723002:
Add Signin_Impression_FromSettings user action for Desktops (Closed)
|
Created:
4 years, 3 months ago by gogerald1 Modified:
4 years, 3 months ago Reviewers:
Dan Beam CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, tommycli Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Signin_Impression_FromSettings user action for Desktops
BUG=639860
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/4aa87e1c2f1adf77a34120b9ee4dc011208a99be
Cr-Commit-Position: refs/heads/master@{#419270}
Patch Set 1 : format #
Total comments: 3
Patch Set 2 : address comments #
Messages
Total messages: 25 (16 generated)
Description was changed from ========== format format add Signin_Impression_FromSettings BUG= ========== to ========== format format add Signin_Impression_FromSettings BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== format format add Signin_Impression_FromSettings BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add Signin_Impression_FromSettings user action for Desktops BUG=639860 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by gogerald@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...
gogerald@chromium.org changed reviewers: + dbeam@chromium.org
Hi, PTAL,
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/nits /cc tommycli@ https://codereview.chromium.org/2339723002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.js (right): https://codereview.chromium.org/2339723002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.js:187: if (this.syncStatus == null && !syncStatus.signedIn) { if (!this.syncStatus && syncStatus && !syncStatus.signedIn) chrome.metricsPrivate.recordUserAction('Signin_Impression_FromSettings');
The CQ bit was checked by gogerald@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2339723002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.js (right): https://codereview.chromium.org/2339723002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.js:187: if (this.syncStatus == null && !syncStatus.signedIn) { On 2016/09/16 18:03:03, Dan Beam wrote: > if (!this.syncStatus && syncStatus && !syncStatus.signedIn) > chrome.metricsPrivate.recordUserAction('Signin_Impression_FromSettings'); Done.
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2339723002/#ps40001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add Signin_Impression_FromSettings user action for Desktops BUG=639860 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add Signin_Impression_FromSettings user action for Desktops BUG=639860 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add Signin_Impression_FromSettings user action for Desktops BUG=639860 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add Signin_Impression_FromSettings user action for Desktops BUG=639860 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/4aa87e1c2f1adf77a34120b9ee4dc011208a99be Cr-Commit-Position: refs/heads/master@{#419270} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4aa87e1c2f1adf77a34120b9ee4dc011208a99be Cr-Commit-Position: refs/heads/master@{#419270}
Message was sent while issue was closed.
https://codereview.chromium.org/2339723002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.js (right): https://codereview.chromium.org/2339723002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.js:187: if (this.syncStatus == null && !syncStatus.signedIn) { On 2016/09/16 20:18:07, gogerald1 wrote: > On 2016/09/16 18:03:03, Dan Beam wrote: > > if (!this.syncStatus && syncStatus && !syncStatus.signedIn) > > chrome.metricsPrivate.recordUserAction('Signin_Impression_FromSettings'); > > Done. for future reference, I meant not to use curly braces (like the rest of the code), which is why I wrote it the way I did
Message was sent while issue was closed.
On 2016/09/16 21:46:36, Dan Beam wrote: > https://codereview.chromium.org/2339723002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/people_page/people_page.js (right): > > https://codereview.chromium.org/2339723002/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/people_page/people_page.js:187: if > (this.syncStatus == null && !syncStatus.signedIn) { > On 2016/09/16 20:18:07, gogerald1 wrote: > > On 2016/09/16 18:03:03, Dan Beam wrote: > > > if (!this.syncStatus && syncStatus && !syncStatus.signedIn) > > > chrome.metricsPrivate.recordUserAction('Signin_Impression_FromSettings'); > > > > Done. > > for future reference, I meant not to use curly braces (like the rest of the > code), which is why I wrote it the way I did Ah, may misunderstand you, but we are supposed to use braces here according to Google javascript style guide (4.1 in https://engdoc.corp.google.com/eng/doc/devguide/js/styleguide.shtml?cl=head#f...). Do we use different style in Chrome or am I missing something?
Message was sent while issue was closed.
On 2016/09/19 14:20:19, gogerald1 wrote: > On 2016/09/16 21:46:36, Dan Beam wrote: > > > https://codereview.chromium.org/2339723002/diff/20001/chrome/browser/resource... > > File chrome/browser/resources/settings/people_page/people_page.js (right): > > > > > https://codereview.chromium.org/2339723002/diff/20001/chrome/browser/resource... > > chrome/browser/resources/settings/people_page/people_page.js:187: if > > (this.syncStatus == null && !syncStatus.signedIn) { > > On 2016/09/16 20:18:07, gogerald1 wrote: > > > On 2016/09/16 18:03:03, Dan Beam wrote: > > > > if (!this.syncStatus && syncStatus && !syncStatus.signedIn) > > > > > chrome.metricsPrivate.recordUserAction('Signin_Impression_FromSettings'); > > > > > > Done. > > > > for future reference, I meant not to use curly braces (like the rest of the > > code), which is why I wrote it the way I did > > Ah, may misunderstand you, but we are supposed to use braces here according to > Google javascript style guide (4.1 in > https://engdoc.corp.google.com/eng/doc/devguide/js/styleguide.shtml?cl=head#f...). > Do we use different style in Chrome or am I missing something? yes, and we're also not writing ES6 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
