Mahmoud Rashad (Gerrit)

unread,

4:57 AM (4 hours ago) 4:57 AM

to Chrome Signin Desktop Reviews, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, droger+w...@chromium.org, tbarzi...@chromium.org, rrsilva+wat...@google.com, crost...@chromium.org, srahim...@chromium.org

Attention needed from Chrome Signin Desktop Reviews

Mahmoud Rashad added 1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Mahmoud Rashad . resolved

Adding chrome-signin-desktop-reviews for people_page

Open in Gerrit

Related details

Attention is currently required from:

Chrome Signin Desktop Reviews

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

gwsq (Gerrit)

unread,

4:59 AM (4 hours ago) 4:59 AM

to Chrome Signin Desktop Reviews, Mihai Sardarescu, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, droger+w...@chromium.org, tbarzi...@chromium.org, rrsilva+wat...@google.com, crost...@chromium.org, srahim...@chromium.org

Attention needed from Mihai Sardarescu

Message from gwsq

Reviewer source(s):
msa...@chromium.org is from context(googleclient/chrome/chromium_gwsq/components/signin/config.gwsq)

Open in Gerrit

Related details

Attention is currently required from:

Mihai Sardarescu

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Mihai Sardarescu (Gerrit)

unread,

7:19 AM (1 hour ago) 7:19 AM

to Mahmoud Rashad, Amelie Schneider, Chrome Signin Desktop Reviews, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, droger+w...@chromium.org, tbarzi...@chromium.org, rrsilva+wat...@google.com, crost...@chromium.org, srahim...@chromium.org

Attention needed from Amelie Schneider and Mahmoud Rashad

Mihai Sardarescu added 2 comments

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Mihai Sardarescu . resolved

I do not think I know this code enough to provide a good review. Re-routing this review to Amelie.

File chrome/app/settings_strings.grdp

Line 3872, Patchset 7 (Latest):

    <message name="IDS_SETTINGS_GOOGLE_SERVICES_PAGE_TITLE" desc="Name of the settings page for managing Google services.">

Mihai Sardarescu . unresolved

Why is this change needed?

Open in Gerrit

Related details

Attention is currently required from:

Amelie Schneider
Mahmoud Rashad

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Mahmoud Rashad (Gerrit)

unread,

7:44 AM (1 hour ago) 7:44 AM

to Amelie Schneider, Mihai Sardarescu, Chrome Signin Desktop Reviews, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, droger+w...@chromium.org, tbarzi...@chromium.org, rrsilva+wat...@google.com, crost...@chromium.org, srahim...@chromium.org

Attention needed from Amelie Schneider and Mihai Sardarescu

Mahmoud Rashad added 1 comment

File chrome/app/settings_strings.grdp

Line 3872, Patchset 7 (Latest):

    <message name="IDS_SETTINGS_GOOGLE_SERVICES_PAGE_TITLE" desc="Name of the settings page for managing Google services.">

Mihai Sardarescu . unresolved

Why is this change needed?

Mahmoud Rashad

This string needs to be exposed to ChromeOS to be used by google-services in people_page.html.

Open in Gerrit

Related details

Attention is currently required from:

Amelie Schneider
Mihai Sardarescu

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Amelie Schneider (Gerrit)

unread,

7:59 AM (1 hour ago) 7:59 AM

to Mahmoud Rashad, Mihai Sardarescu, Chrome Signin Desktop Reviews, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, droger+w...@chromium.org, tbarzi...@chromium.org, rrsilva+wat...@google.com, crost...@chromium.org, srahim...@chromium.org

Attention needed from Mahmoud Rashad and Mihai Sardarescu

Amelie Schneider added 3 comments

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Amelie Schneider . resolved

Thanks Mahmoud!
What's the general idea for the settings page? Should it look exactly the same as on other Desktop platforms or are there different pages, as seen in the screenshots you linked? Do you have some mocks for reference?

File chrome/browser/resources/settings/people_page/people_page.html

Line 191, Patchset 7 (Latest):

        <cr-link-row id="account-subpage-row" on-click="onAccountClick_"
            hidden="[[!shouldLinkToAccountSettingsPage_(
              syncStatus.signedInState)]]">
          <div id="profile-icon"
              style="background-image: [[getIconImageSet_(
                  profileIconUrl_)]]">
          </div>
          <div class="cr-row-gap cr-padded-text flex no-min-width">
            <div id="account-name" class="text-elide">
              [[profileName_]]
            </div>
            <div id="account-subtitle" class="secondary">
              [[getAccountRowSubtitle_(syncStatus)]]
            </div>
          </div>
        </cr-link-row>

        <cr-link-row id="sync-setup"
            label="$i18n{syncAndNonPersonalizedServices}"
            sub-label="[[getSyncAndNonPersonalizedServicesSubtext_(syncStatus)]]"
            on-click="onSyncClick_"
            role-description="$i18n{subpageArrowRoleDescription}"
            hidden="[[shouldHideSyncSetupLinkRow_(syncStatus)]]">
        </cr-link-row>
        <cr-link-row id="google-services"
            label="$i18n{googleServicesPageTitle}"
            on-click="onGoogleServicesClick_"
            role-description="$i18n{subpageArrowRoleDescription}"
            hidden="[[!shouldHideSyncSetupLinkRow_(syncStatus)]]">
        </cr-link-row>

Amelie Schneider . unresolved

This is now mostly duplicated with the non-chromeos platform's code. I would advocate for combining the code blocks for chromeos and non-chromeos platforms, what do you think?

File chrome/browser/resources/settings/people_page/people_page.ts

Line 513, Patchset 7 (Latest): return '';

Amelie Schneider . unresolved

Can we make this an early return instead to avoid some of the nesting?
Same with `return this.syncStatus.signedInUsername;`

Something like

```
if (!this.syncStatus || this.syncStatus.signedInUsername) {
  return '';
}

if (!this.syncStatus.statusText) {
  return this.syncStatus.signedInUsername;
}

...

```

Open in Gerrit

Related details

Attention is currently required from:

Mahmoud Rashad
Mihai Sardarescu

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

[Settings] Update people page on ChromeOS [chromium/src : main]

Mahmoud Rashad (Gerrit)

Mahmoud Rashad added 1 comment

Related details

gwsq (Gerrit)

Message from gwsq

Related details

Mihai Sardarescu (Gerrit)

Mihai Sardarescu added 2 comments

Related details

Mahmoud Rashad (Gerrit)

Mahmoud Rashad added 1 comment

Related details

Amelie Schneider (Gerrit)

Amelie Schneider added 3 comments

Related details