New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 761034 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Task



Sign in to add a comment

Refactor MainPreferences to create Preferences once per activity

Project Member Reported by iuliah@google.com, Aug 31 2017

Issue description

Currently MainPreferences removes and adds again all preferences each time the activity is resumed, leading to objects being destroyed and re-created unnecessarily. Refactoring it such that Preferences are created only once and then updated as needed is necessary for counting the number of impressions and for recording several metrics regarding the personalized signin promos.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d587166649f391e57476cbb152fc407336abaa0d

commit d587166649f391e57476cbb152fc407336abaa0d
Author: Iulia Harasim <iuliah@google.com>
Date: Wed Sep 06 15:33:47 2017

[Android] Change the way preferences are managed in MainPreferenes

This CL refactors the way preferences are being updated. Instead of
creating new preferences each time the activity is resumed, they are now
created once, when the Settings screen is opened by the user, and are
removed/added to the PreferenceScreen as needed. The XML file is removed
and preferences are now created in the Java code. This change improves
flexibility as now it is possible to instantiate one or several
preferences without having to reload all of them from the XML file.

Bug:  761034 
Change-Id: Iba2cb91ab5cdff5762ba8c88d0580f8c6c7d8729
Reviewed-on: https://chromium-review.googlesource.com/645887
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Commit-Queue: Iulia Harasim <iuliah@google.com>
Cr-Commit-Position: refs/heads/master@{#499968}
[modify] https://crrev.com/d587166649f391e57476cbb152fc407336abaa0d/chrome/android/java/res/xml/main_preferences.xml
[modify] https://crrev.com/d587166649f391e57476cbb152fc407336abaa0d/chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java

Comment 2 by iuliah@google.com, Sep 6 2017

Status: Fixed (was: Started)

Comment 3 by iuliah@google.com, Sep 7 2017

Labels: Merge-Request-62
Project Member

Comment 4 by sheriffbot@chromium.org, Sep 8 2017

Labels: -Merge-Request-62 Hotlist-Merge-Approved Merge-Approved-62
Your change meets the bar and is auto-approved for M62. Please go ahead and merge the CL to branch 3202 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 8 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/10c6f24c354593878b2c54f293da3d8807c7b28d

commit 10c6f24c354593878b2c54f293da3d8807c7b28d
Author: Boris Sazonov <bsazonov@chromium.org>
Date: Fri Sep 08 12:02:01 2017

[Android] Change the way preferences are managed in MainPreferenes

This CL refactors the way preferences are being updated. Instead of
creating new preferences each time the activity is resumed, they are now
created once, when the Settings screen is opened by the user, and are
removed/added to the PreferenceScreen as needed. The XML file is removed
and preferences are now created in the Java code. This change improves
flexibility as now it is possible to instantiate one or several
preferences without having to reload all of them from the XML file.

(cherry picked from commit d587166649f391e57476cbb152fc407336abaa0d)

TBR=iuliah@google.com

Bug:  761034 
Change-Id: Iba2cb91ab5cdff5762ba8c88d0580f8c6c7d8729
Reviewed-on: https://chromium-review.googlesource.com/645887
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Commit-Queue: Iulia Harasim <iuliah@google.com>
Cr-Original-Commit-Position: refs/heads/master@{#499968}
Reviewed-on: https://chromium-review.googlesource.com/657657
Cr-Commit-Position: refs/branch-heads/3202@{#88}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/10c6f24c354593878b2c54f293da3d8807c7b28d/chrome/android/java/res/xml/main_preferences.xml
[modify] https://crrev.com/10c6f24c354593878b2c54f293da3d8807c7b28d/chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java

Cc: twelling...@chromium.org
Out of curiosity, why did this need to be merged to M62?

Comment 7 by ew...@chromium.org, Sep 8 2017

Cc: ew...@chromium.org
We count the number of impressions for the "personalized" signin promo in settings and automatically revert back to the smaller, text-based promo after 5 impressions.

Conceptually, we want to count an "impression" whenever the user opens the settings screen and sees the promo. However, we were counting an impression in certain situations that caused some odd behavior (e.g. if you clicked on the promo to start the signin flow, then clicked back to go back to the settings screen, you might see the promo change out underneath you to the old version, since we were counting going back as an impression).

This fix prevents those weird interactions and makes sure we're not overcounting impressions.
That makes sense. Thank you for the explanation!

Comment 9 by ew...@chromium.org, Sep 8 2017

No problem!

Sign in to add a comment