Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2408063007:
Re-sort preference update protocol between Chrome and ArcSupport. (Closed)
|
Created:
4 years, 2 months ago by hidehiko Modified:
4 years, 2 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, alemate+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, khmel Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRe-sort preference update protocol between Chrome and ArcSupport.
Currently, the preference connected checkbox is individually
implemented so that there are many "copy-and-paste then
customize" code. This CL re-sorts the structure so that
the protocol from the Chrome to the extension is a pair of
(enabled, managed). Also, it reduces the dupped code.
Along with the change, the UI update code in the
ArcSupportHost is moved to the extension, too.
This is the preparation to split the params in onAgree()
callback into indivisual checkbox click.
Once it's done, then the dependency between ArcSupportHost
and ArcAuthService will be reverted.
TEST=Ran on test device. Ran bots.
BUG=633258
, b/31079732
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/04ad939f0adbbacd67d8631f38e488221b26cc4b
Cr-Commit-Position: refs/heads/master@{#425883}
Patch Set 1 #Patch Set 2 : Fix compiler error mistake. #
Total comments: 19
Patch Set 3 : Address comments. #Patch Set 4 : Rebase #
Total comments: 1
Messages
Total messages: 32 (18 generated)
Description was changed from ========== Re-sort preference update protocol between Chrome and ArcSupport. Currently, the preference connected checkbox is individually implemented so that there are many "copy-and-paste then customize" code. This CL re-sorts the structure so that the protocol from the Chrome to the extension is a pair of (enabled, managed). Also, it reduces the dupped code. Along with the change, the UI update code in the ArcSupportHost is moved to the extension, too. This is the preparation to split the params in onAgree() callback into indivisual checkbox click. Once it's done, then the dependency between ArcSupportHost and ArcAuthService will be reverted. TEST=Ran on test device. Ran bots. BUG=633258, b/31079732 ========== to ========== Re-sort preference update protocol between Chrome and ArcSupport. Currently, the preference connected checkbox is individually implemented so that there are many "copy-and-paste then customize" code. This CL re-sorts the structure so that the protocol from the Chrome to the extension is a pair of (enabled, managed). Also, it reduces the dupped code. Along with the change, the UI update code in the ArcSupportHost is moved to the extension, too. This is the preparation to split the params in onAgree() callback into indivisual checkbox click. Once it's done, then the dependency between ArcSupportHost and ArcAuthService will be reverted. TEST=Ran on test device. Ran bots. BUG=633258, b/31079732 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hidehiko@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: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hidehiko@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.
hidehiko@chromium.org changed reviewers: + lhchavez@chromium.org, xiyuan@chromium.org
PTAL: Luis, Xiyuan. FYI: Yury (Note this is somehow related with your CL, but conceptually independent).
khmel@chromium.org changed reviewers: + khmel@chromium.org
Hi Hidehiko, I don't really understand what do you want to achieve with this CL? This CL statistics shows: Stats (+259 lines, -212 lines) That means we have more code then before? >> This is the preparation to split the params in onAgree() >> callback into indivisual checkbox click. >> Once it's done, then the dependency between ArcSupportHost >> and ArcAuthService will be reverted. ArcSupportHost is not used only for straightforward OptIn flow. It also used for showing an error and requesting authority code on demand. Please make sure that this functionality is not broken. I don't see how you can completely drop dependencies from ArcAuthService to ArcSupportHost. So I defer this to Xiyuan, who is much bigger expert in HTML. https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:123: // state and the native state can be in sync. What do you mean by this? Do you want to change pref on checkbox click? If so this may be confusing for user. Current behavior is update prefs/metrics mode only if user presses Agree button. In case of cancel, prefs and metrics mode are not updated.
On 2016/10/13 15:12:43, khmel wrote: > Hi Hidehiko, > > I don't really understand what do you want to achieve with this CL? > This CL statistics shows: > Stats (+259 lines, -212 lines) That means we have more code then before? > The main goals are; - Share the same code if what's needed is semantically same. - Let ArcSuppotHost focus on message routing. Comments / some boilarplate vars / TODOs easily increase the number of lines unfortunately. > >> This is the preparation to split the params in onAgree() > >> callback into indivisual checkbox click. > >> Once it's done, then the dependency between ArcSupportHost > >> and ArcAuthService will be reverted. > > ArcSupportHost is not used only for straightforward OptIn flow. It also used for > showing an error and requesting authority code on demand. Please make sure that > this functionality is not broken. Clar: error for ArcAuthService, do you mean? I'm taking care of them. Please let me know if there needs more care. > I don't see how you can completely drop > dependencies from ArcAuthService to ArcSupportHost. > > So I defer this to Xiyuan, who is much bigger expert in HTML. > > https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... > File chrome/browser/resources/chromeos/arc_support/background.js (right): > > https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... > chrome/browser/resources/chromeos/arc_support/background.js:123: // state and > the native state can be in sync. > What do you mean by this? Do you want to change pref on checkbox click? If so > this may be confusing for user. Current behavior is update prefs/metrics mode > only if user presses Agree button. In case of cancel, prefs and metrics mode are > not updated.
https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:123: // state and the native state can be in sync. On 2016/10/13 15:12:43, khmel wrote: > What do you mean by this? Do you want to change pref on checkbox click? If so > this may be confusing for user. Current behavior is update prefs/metrics mode > only if user presses Agree button. In case of cancel, prefs and metrics mode are > not updated. Oh, is it intentional behavior? It looks a (small) bug to me, because I thought we should keep the prev state, e.g., "open extension -> click checkbox -> close by x-button (not cancel) -> reopen the extension"? I'm ok if it is intentional. Then I'll update the following CLs.
LGTM with nits https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:123: // state and the native state can be in sync. On 2016/10/13 16:04:31, hidehiko wrote: > On 2016/10/13 15:12:43, khmel wrote: > > What do you mean by this? Do you want to change pref on checkbox click? If so > > this may be confusing for user. Current behavior is update prefs/metrics mode > > only if user presses Agree button. In case of cancel, prefs and metrics mode > are > > not updated. > > Oh, is it intentional behavior? It looks a (small) bug to me, because I thought > we should keep the prev state, e.g., "open extension -> click checkbox -> close > by x-button (not cancel) -> reopen the extension"? > > I'm ok if it is intentional. Then I'll update the following CLs. Not a lawyer but think this is intentional similar to OOBE Eula screen where the metrics etc is only enabled after user agrees to the terms. This TODO probably is not needed. https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:187: this.isOwned_ = isOwned; nit: isOwned -> isOwner to be more clear https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:217: console.error(settingsLink); nit: remove debugging log?
lgtm
https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:10: var UI_PAGES = ['none', not your fault, but can this be const instead of var? https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:80: var INNER_WIDTH = 960; can this and the height be const as well? https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:106: * @param {Element} div The container this checkbox corresponds to. nit: I'd rather call this |container| rather than |div|. https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:145: isEnabled() { return this.checkbox_.checked; } This was a bit confusing, since "enabled" means different things for preferences and checkboxes. How about isChecked() so you don't overload the term and is unambiguous? isPreferenceEnabled() is also acceptable, but more verbose. https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:189: // Build a text array. The index is "isManaged * 2 + isEnabled". See nit: Isn't it better to build a multi-dimensional array to avoid the index arithmetic? Also |labels_| might be a slightly better name. this.labels_ = [[textDisabled, textEnabled], [textManagedDisabled, textManagedEnabled]];
Thank you for review. PTAL. https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:10: var UI_PAGES = ['none', On 2016/10/17 02:26:01, Luis Héctor Chávez wrote: > not your fault, but can this be const instead of var? Unfortunately, the lint warns it. Instead @const is encouraged. However, it is not a part of the focus of this CL, can we work on it in a follow-up CL? https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:80: var INNER_WIDTH = 960; On 2016/10/17 02:26:00, Luis Héctor Chávez wrote: > can this and the height be const as well? Acknowledged. https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:106: * @param {Element} div The container this checkbox corresponds to. On 2016/10/17 02:26:01, Luis Héctor Chávez wrote: > nit: I'd rather call this |container| rather than |div|. Done. https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:123: // state and the native state can be in sync. On 2016/10/13 21:28:59, xiyuan wrote: > On 2016/10/13 16:04:31, hidehiko wrote: > > On 2016/10/13 15:12:43, khmel wrote: > > > What do you mean by this? Do you want to change pref on checkbox click? If > so > > > this may be confusing for user. Current behavior is update prefs/metrics > mode > > > only if user presses Agree button. In case of cancel, prefs and metrics mode > > are > > > not updated. > > > > Oh, is it intentional behavior? It looks a (small) bug to me, because I > thought > > we should keep the prev state, e.g., "open extension -> click checkbox -> > close > > by x-button (not cancel) -> reopen the extension"? > > > > I'm ok if it is intentional. Then I'll update the following CLs. > > Not a lawyer but think this is intentional similar to OOBE Eula screen where the > metrics etc is only enabled after user agrees to the terms. This TODO probably > is not needed. Thank you for clarification. Done. https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:145: isEnabled() { return this.checkbox_.checked; } On 2016/10/17 02:26:01, Luis Héctor Chávez wrote: > This was a bit confusing, since "enabled" means different things for preferences > and checkboxes. How about isChecked() so you don't overload the term and is > unambiguous? isPreferenceEnabled() is also acceptable, but more verbose. Renamed to isChecked and added comments. https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:187: this.isOwned_ = isOwned; On 2016/10/13 21:28:59, xiyuan wrote: > nit: isOwned -> isOwner to be more clear Done. https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:189: // Build a text array. The index is "isManaged * 2 + isEnabled". See On 2016/10/17 02:26:01, Luis Héctor Chávez wrote: > nit: Isn't it better to build a multi-dimensional array to avoid the index > arithmetic? Also |labels_| might be a slightly better name. > > this.labels_ = [[textDisabled, textEnabled], [textManagedDisabled, > textManagedEnabled]]; Done. https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:217: console.error(settingsLink); On 2016/10/13 21:28:59, xiyuan wrote: > nit: remove debugging log? Oops. Removed.
lgtm https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2408063007/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:10: var UI_PAGES = ['none', On 2016/10/17 09:27:56, hidehiko wrote: > On 2016/10/17 02:26:01, Luis Héctor Chávez wrote: > > not your fault, but can this be const instead of var? > > Unfortunately, the lint warns it. Instead @const is encouraged. > However, it is not a part of the focus of this CL, can we work on it in a > follow-up CL? sgtm
The CQ bit was checked by hidehiko@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...
Thank you for review, Luis! ... but ... https://codereview.chromium.org/2408063007/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2408063007/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:613: isMetricsEnabled: metricsCheckbox.isChecked(), Ugrrr... I mistakenly uploaded an older revision (containing a mistake...) Could you take another look just in case, Luis?
lgtm++
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, khmel@chromium.org Link to the patchset: https://codereview.chromium.org/2408063007/#ps60001 (title: "Rebase")
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Re-sort preference update protocol between Chrome and ArcSupport. Currently, the preference connected checkbox is individually implemented so that there are many "copy-and-paste then customize" code. This CL re-sorts the structure so that the protocol from the Chrome to the extension is a pair of (enabled, managed). Also, it reduces the dupped code. Along with the change, the UI update code in the ArcSupportHost is moved to the extension, too. This is the preparation to split the params in onAgree() callback into indivisual checkbox click. Once it's done, then the dependency between ArcSupportHost and ArcAuthService will be reverted. TEST=Ran on test device. Ran bots. BUG=633258, b/31079732 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Re-sort preference update protocol between Chrome and ArcSupport. Currently, the preference connected checkbox is individually implemented so that there are many "copy-and-paste then customize" code. This CL re-sorts the structure so that the protocol from the Chrome to the extension is a pair of (enabled, managed). Also, it reduces the dupped code. Along with the change, the UI update code in the ArcSupportHost is moved to the extension, too. This is the preparation to split the params in onAgree() callback into indivisual checkbox click. Once it's done, then the dependency between ArcSupportHost and ArcAuthService will be reverted. TEST=Ran on test device. Ran bots. BUG=633258, b/31079732 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/04ad939f0adbbacd67d8631f38e488221b26cc4b Cr-Commit-Position: refs/heads/master@{#425883} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/04ad939f0adbbacd67d8631f38e488221b26cc4b Cr-Commit-Position: refs/heads/master@{#425883} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
