Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2229443003:
MD Settings: Ensure that "advanced sync page" is force rendered, only if applicable. (Closed)
|
Created:
4 years, 4 months ago by dpapad Modified:
4 years, 4 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Ensure that "advanced sync page" is force rendered, only if applicable.
BUG=635408
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/bcce6054eba2ba13a79eccd05e2571a62739d8e0
Cr-Commit-Position: refs/heads/master@{#410818}
Patch Set 1 #Patch Set 2 : Nit. #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 19 (9 generated)
Description was changed from ========== MD Settings: Ensure that "advanced sync page" is force rendered, only if applicable. BUG=635017 ========== to ========== MD Settings: Ensure that "advanced sync page" is force rendered, only if applicable. BUG=635017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2229443003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/2229443003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.html:237: <template is="dom-if" if="[[isAdvancedSyncSettingsVisible_(syncStatus)]]"> See lines 129-132, where the element that triggers this subpage is also inside a similar dom-if. Without this, the searching algorithm erroneously expands the "advanced sync page" when it is not applicable (user is signed out), and an assertion is thrown while searching by https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/search....
Description was changed from ========== MD Settings: Ensure that "advanced sync page" is force rendered, only if applicable. BUG=635017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Ensure that "advanced sync page" is force rendered, only if applicable. BUG=635408 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
lgtm but you might want tommycli@ to verify this is correct for his needs
The CQ bit was checked by dpapad@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...
dpapad@chromium.org changed reviewers: + tommycli@chromium.org
+tommycli: I don
It looks good to me. I'm assuming you verified that navigating to the Advanced Sync Settings page still works?
On 2016/08/09 at 18:48:34, dpapad wrote: > +tommycli: I don @tommycli: I don't think this change affects any observable user behavior, while at the same time it is fixing a logical error that was happening from the searching algorithm (advanced sync subpage should not be force-rendered or searched when not applicable). Can you verify?
On 2016/08/09 at 18:50:06, tommycli wrote: > It looks good to me. I'm assuming you verified that navigating to the Advanced Sync Settings page still works? Yes, I manually tested navigating to the "advanced sync page" and back, when logged in.
On 2016/08/09 18:53:16, dpapad wrote: > On 2016/08/09 at 18:50:06, tommycli wrote: > > It looks good to me. I'm assuming you verified that navigating to the Advanced > Sync Settings page still works? > > Yes, I manually tested navigating to the "advanced sync page" and back, when > logged in. cool then lgtm
The CQ bit was unchecked by dpapad@chromium.org
The CQ bit was checked by dpapad@chromium.org
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 #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Ensure that "advanced sync page" is force rendered, only if applicable. BUG=635408 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Ensure that "advanced sync page" is force rendered, only if applicable. BUG=635408 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/bcce6054eba2ba13a79eccd05e2571a62739d8e0 Cr-Commit-Position: refs/heads/master@{#410818} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/bcce6054eba2ba13a79eccd05e2571a62739d8e0 Cr-Commit-Position: refs/heads/master@{#410818} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
