Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2235323002:
MD Settings: Fix advanced sync page reload. (Closed)
|
Created:
4 years, 4 months ago by dpapad Modified:
4 years, 4 months ago Reviewers:
tommycli 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: Fix advanced sync page reload.
A blank page was shown on direct navigation after
https://codereview.chromium.org/2229443003. It turns out that wrapping a
settings-subpage's dom-if within another dom-if is problematic, because the
settings-animated-pages ensureSubpageInstance_() executes before the outer
dom-if has been evaluated to true, leaving the page blank.
Fixed by removing the nested dom-if and using the no-search attribute instead,
to instruct the searching algorithm when to ignore the "advanced sync settings"
page.
BUG=636816
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/a629fcba45a437827cfc2ac36fba9ad97b6bb3f1
Cr-Commit-Position: refs/heads/master@{#411459}
Patch Set 1 #
Messages
Total messages: 20 (11 generated)
Description was changed from ========== MD Settings: Fix advanced sync page reload. A blank page was shown on direct navigation. It turns out that wrapping a settings-subpage's dom-if within another dom-if is problematic, because the settings-animated-pages ensureSubpageInstance_() executes before the outer dom-if has been evaluated to true, leaving the page blank. Instead using the no-search attribute to instruct the searching algorithm when to ignore the "advanced sync settings" page. BUG=636816 ========== to ========== MD Settings: Fix advanced sync page reload. A blank page was shown on direct navigation. It turns out that wrapping a settings-subpage's dom-if within another dom-if is problematic, because the settings-animated-pages ensureSubpageInstance_() executes before the outer dom-if has been evaluated to true, leaving the page blank. Instead using the no-search attribute to instruct the searching algorithm when to ignore the "advanced sync settings" page. BUG=636816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Description was changed from ========== MD Settings: Fix advanced sync page reload. A blank page was shown on direct navigation. It turns out that wrapping a settings-subpage's dom-if within another dom-if is problematic, because the settings-animated-pages ensureSubpageInstance_() executes before the outer dom-if has been evaluated to true, leaving the page blank. Instead using the no-search attribute to instruct the searching algorithm when to ignore the "advanced sync settings" page. BUG=636816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Fix advanced sync page reload. A blank page was shown on direct navigation after https://codereview.chromium.org/2229443003. It turns out that wrapping a settings-subpage's dom-if within another dom-if is problematic, because the settings-animated-pages ensureSubpageInstance_() executes before the outer dom-if has been evaluated to true, leaving the page blank. Instead using the no-search attribute to instruct the searching algorithm when to ignore the "advanced sync settings" page. BUG=636816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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
Description was changed from ========== MD Settings: Fix advanced sync page reload. A blank page was shown on direct navigation after https://codereview.chromium.org/2229443003. It turns out that wrapping a settings-subpage's dom-if within another dom-if is problematic, because the settings-animated-pages ensureSubpageInstance_() executes before the outer dom-if has been evaluated to true, leaving the page blank. Instead using the no-search attribute to instruct the searching algorithm when to ignore the "advanced sync settings" page. BUG=636816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Fix advanced sync page reload. A blank page was shown on direct navigation after https://codereview.chromium.org/2229443003. It turns out that wrapping a settings-subpage's dom-if within another dom-if is problematic, because the settings-animated-pages ensureSubpageInstance_() executes before the outer dom-if has been evaluated to true, leaving the page blank. Fixed by removing the nested dom-if and using the no-search attribute instead, to instruct the searching algorithm when to ignore the "advanced sync settings" page. BUG=636816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
lgtm: I assume both no-search attributes are necessary to handle both the rendered and unrendered case?
On 2016/08/11 at 19:02:52, tommycli wrote: > lgtm: I assume both no-search attributes are necessary to handle both the rendered and unrendered case? Correct. The 1st one ensures that the searching algorithm will not force-render the subpage if it is not applicable, the 2nd one ensures that even if the page has been rendered already it will not be searched if not applicable (addressing the case where the user was logged initially and then logged out). Normally we carry-over the "no-search" attribute from the template to the stamped instance programmatically (see settings_animated_pages.js:147-148), but because it can change after stamping, the data-binding needs to be duplicated.
On 2016/08/11 19:12:11, dpapad wrote: > On 2016/08/11 at 19:02:52, tommycli wrote: > > lgtm: I assume both no-search attributes are necessary to handle both the > rendered and unrendered case? > > Correct. The 1st one ensures that the searching algorithm will not force-render > the subpage if it is not applicable, the 2nd one ensures that even if the page > has been rendered already it will not be searched if not applicable (addressing > the case where the user was logged initially and then logged out). > > Normally we carry-over the "no-search" attribute from the template to the > stamped instance programmatically (see settings_animated_pages.js:147-148), but > because it can change after stamping, the data-binding needs to be duplicated. cool cool. thanks for clarifying.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Fix advanced sync page reload. A blank page was shown on direct navigation after https://codereview.chromium.org/2229443003. It turns out that wrapping a settings-subpage's dom-if within another dom-if is problematic, because the settings-animated-pages ensureSubpageInstance_() executes before the outer dom-if has been evaluated to true, leaving the page blank. Fixed by removing the nested dom-if and using the no-search attribute instead, to instruct the searching algorithm when to ignore the "advanced sync settings" page. BUG=636816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Fix advanced sync page reload. A blank page was shown on direct navigation after https://codereview.chromium.org/2229443003. It turns out that wrapping a settings-subpage's dom-if within another dom-if is problematic, because the settings-animated-pages ensureSubpageInstance_() executes before the outer dom-if has been evaluated to true, leaving the page blank. Fixed by removing the nested dom-if and using the no-search attribute instead, to instruct the searching algorithm when to ignore the "advanced sync settings" page. BUG=636816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/a629fcba45a437827cfc2ac36fba9ad97b6bb3f1 Cr-Commit-Position: refs/heads/master@{#411459} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/a629fcba45a437827cfc2ac36fba9ad97b6bb3f1 Cr-Commit-Position: refs/heads/master@{#411459} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
