Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2754563002: MD Settings: Lazy load the contents of the "advanced" settings. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by dpapad
Modified:
2 months ago
Reviewers:
tommycli, Dan Beam
CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, dschuyler, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, michaelpg, stevenjb+watch-md-settings_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

MD Settings: Lazy load the contents of the "advanced" settings. BUG=597347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2754563002 Cr-Commit-Position: refs/heads/master@{#458527} Committed: https://chromium.googlesource.com/chromium/src/+/f9e8f9e6d60d8cd26c4239a377419cbb37fe8724

Patch Set 1 #

Patch Set 2 : Simplify #

Patch Set 3 : Attempt to fix some tests. #

Patch Set 4 : Fix remaining tests. #

Patch Set 5 : Simplify, remove unnecessary file. #

Total comments: 5

Patch Set 6 : Fix direct navigation to lazy loaded sections and overscrolling. #

Total comments: 20

Patch Set 7 : Address comments. #

Patch Set 8 : Fix tests. #

Total comments: 19

Patch Set 9 : Address comments, fix issue with non-sectioned routes. #

Total comments: 1

Patch Set 10 : Fix typo #

Total comments: 4

Patch Set 11 : Address comment. #

Total comments: 4

Patch Set 12 : Address comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+269 lines, -224 lines) Patch
M chrome/browser/resources/settings/basic_page/basic_page.html View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/resources/settings/basic_page/basic_page.js View 1 1 chunk +4 lines, -2 lines 0 comments Download
A + chrome/browser/resources/settings/controls/settings_idle_load.html View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
A chrome/browser/resources/settings/controls/settings_idle_load.js View 1 2 3 4 5 6 7 8 1 chunk +88 lines, -0 lines 0 comments Download
D chrome/browser/resources/settings/controls/settings_idle_render.html View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/resources/settings/controls/settings_idle_render.js View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -76 lines 0 comments Download
M chrome/browser/resources/settings/route.js View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +14 lines, -1 line 0 comments Download
M chrome/browser/resources/settings/settings_main/settings_main.js View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/resources/settings/settings_page/main_page_behavior.js View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/resources/settings/settings_resources.grd View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -4 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M chrome/test/data/webui/settings/advanced_page_browsertest.js View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/test/data/webui/settings/basic_page_browsertest.js View 1 2 3 4 5 6 5 chunks +4 lines, -6 lines 0 comments Download
M chrome/test/data/webui/settings/cr_settings_browsertest.js View 1 2 3 4 5 6 2 chunks +5 lines, -1 line 0 comments Download
M chrome/test/data/webui/settings/easy_unlock_browsertest_chromeos.js View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
A chrome/test/data/webui/settings/ensure_lazy_loaded.js View 1 2 3 4 5 6 1 chunk +22 lines, -0 lines 0 comments Download
M chrome/test/data/webui/settings/help_page_browsertest.js View 1 2 3 4 5 6 7 8 2 chunks +9 lines, -2 lines 0 comments Download
M chrome/test/data/webui/settings/languages_page_browsertest.js View 1 2 3 4 5 6 2 chunks +7 lines, -3 lines 0 comments Download
M chrome/test/data/webui/settings/on_startup_browsertest.js View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/test/data/webui/settings/passwords_and_forms_browsertest.js View 1 2 3 4 5 6 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/test/data/webui/settings/route_tests.js View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +15 lines, -0 lines 0 comments Download
M chrome/test/data/webui/settings/settings_autofill_section_browsertest.js View 1 2 5 chunks +29 lines, -13 lines 0 comments Download
A + chrome/test/data/webui/settings/settings_idle_load_browsertest.js View 1 2 3 4 5 6 7 8 4 chunks +14 lines, -12 lines 0 comments Download
M chrome/test/data/webui/settings/settings_idle_render_browsertest.js View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -67 lines 0 comments Download
M chrome/test/data/webui/settings/settings_page_browsertest.js View 1 2 3 4 5 6 2 chunks +17 lines, -6 lines 0 comments Download
M chrome/test/data/webui/settings/settings_passwords_section_browsertest.js View 1 2 1 chunk +5 lines, -2 lines 0 comments Download
M chrome/test/data/webui/settings/settings_subpage_browsertest.js View 1 2 3 4 5 6 7 5 chunks +7 lines, -16 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 54 (32 generated)
dpapad
https://codereview.chromium.org/2754563002/diff/100001/chrome/browser/resources/settings/controls/settings_idle_render.js File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2754563002/diff/100001/chrome/browser/resources/settings/controls/settings_idle_render.js#newcode15 chrome/browser/resources/settings/controls/settings_idle_render.js:15: is: 'settings-idle-load', I'll rename this file to settings_idle_load.js. Just ...
2 months, 1 week ago (2017-03-16 20:47:16 UTC) #14
Dan Beam
https://codereview.chromium.org/2754563002/diff/100001/chrome/test/data/webui/settings/settings_page_browsertest.js File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/2754563002/diff/100001/chrome/test/data/webui/settings/settings_page_browsertest.js#newcode64 chrome/test/data/webui/settings/settings_page_browsertest.js:64: * @return {!PolymerElement} The PolymerElement for the page. !Promise<!PolymerElement> ...
2 months, 1 week ago (2017-03-16 20:52:22 UTC) #15
dpapad
+dschuyler: Can you take a look at the changes in settings_main.js? In order to make ...
2 months, 1 week ago (2017-03-17 00:08:09 UTC) #16
Dan Beam
https://codereview.chromium.org/2754563002/diff/120001/chrome/browser/resources/settings/settings_main/settings_main.js File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2754563002/diff/120001/chrome/browser/resources/settings/settings_main/settings_main.js#newcode88 chrome/browser/resources/settings/settings_main/settings_main.js:88: this.listen(this, 'lazy-loaded', 'onLazyLoaded_'); use listeners: or unlisten in detached()? ...
2 months, 1 week ago (2017-03-17 00:30:06 UTC) #17
Dan Beam
https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui/settings/ensure_lazy_loaded.js File chrome/test/data/webui/settings/ensure_lazy_loaded.js (right): https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui/settings/ensure_lazy_loaded.js#newcode6 chrome/test/data/webui/settings/ensure_lazy_loaded.js:6: nit: no \n https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui/settings/ensure_lazy_loaded.js#newcode9 chrome/test/data/webui/settings/ensure_lazy_loaded.js:9: if (window.location.href == 'chrome://md-settings/') ...
2 months, 1 week ago (2017-03-17 00:31:44 UTC) #18
dpapad
Waiting for green tryjobs. https://codereview.chromium.org/2754563002/diff/120001/chrome/browser/resources/settings/settings_main/settings_main.js File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2754563002/diff/120001/chrome/browser/resources/settings/settings_main/settings_main.js#newcode88 chrome/browser/resources/settings/settings_main/settings_main.js:88: this.listen(this, 'lazy-loaded', 'onLazyLoaded_'); On 2017/03/17 ...
2 months, 1 week ago (2017-03-17 20:53:55 UTC) #20
Dan Beam
https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resources/settings/controls/settings_idle_render.js File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resources/settings/controls/settings_idle_render.js#newcode56 chrome/browser/resources/settings/controls/settings_idle_render.js:56: function() { i think this code could use some ...
2 months, 1 week ago (2017-03-18 00:15:51 UTC) #23
Dan Beam
https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resources/settings/controls/settings_idle_render.js File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resources/settings/controls/settings_idle_render.js#newcode15 chrome/browser/resources/settings/controls/settings_idle_render.js:15: is: 'settings-idle-load', can you rename this file now?
2 months, 1 week ago (2017-03-18 00:36:40 UTC) #24
Dan Beam
lgtm https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resources/settings/settings_main/settings_main.js File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resources/settings/settings_main/settings_main.js#newcode93 chrome/browser/resources/settings/settings_main/settings_main.js:93: Polymer.dom.flush(); On 2017/03/18 00:15:51, Dan Beam wrote: > ...
2 months, 1 week ago (2017-03-18 00:41:07 UTC) #25
dpapad
@dbeam: See again change in main_page_behavior.js and new comment. https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resources/settings/controls/settings_idle_render.js File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resources/settings/controls/settings_idle_render.js#newcode59 chrome/browser/resources/settings/controls/settings_idle_render.js:59: ...
2 months ago (2017-03-20 20:34:55 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2754563002/220001
2 months ago (2017-03-20 23:44:00 UTC) #33
Dan Beam
https://codereview.chromium.org/2754563002/diff/220001/chrome/browser/resources/settings/settings_page/main_page_behavior.js File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2754563002/diff/220001/chrome/browser/resources/settings/settings_page/main_page_behavior.js#newcode124 chrome/browser/resources/settings/settings_page/main_page_behavior.js:124: currentRoute != settings.Route.RESET_DIALOG) { can you do this without ...
2 months ago (2017-03-21 00:29:26 UTC) #34
dpapad
@Tommy: Can you take a look at route.js and route_tests.js? https://codereview.chromium.org/2754563002/diff/220001/chrome/browser/resources/settings/settings_page/main_page_behavior.js File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2754563002/diff/220001/chrome/browser/resources/settings/settings_page/main_page_behavior.js#newcode124 ...
2 months ago (2017-03-21 00:54:51 UTC) #37
Dan Beam
https://codereview.chromium.org/2754563002/diff/220001/chrome/browser/resources/settings/settings_page/main_page_behavior.js File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2754563002/diff/220001/chrome/browser/resources/settings/settings_page/main_page_behavior.js#newcode124 chrome/browser/resources/settings/settings_page/main_page_behavior.js:124: currentRoute != settings.Route.RESET_DIALOG) { On 2017/03/21 00:54:51, dpapad wrote: ...
2 months ago (2017-03-21 01:54:16 UTC) #38
dpapad
https://codereview.chromium.org/2754563002/diff/220001/chrome/browser/resources/settings/settings_page/main_page_behavior.js File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2754563002/diff/220001/chrome/browser/resources/settings/settings_page/main_page_behavior.js#newcode124 chrome/browser/resources/settings/settings_page/main_page_behavior.js:124: currentRoute != settings.Route.RESET_DIALOG) { On 2017/03/21 at 01:54:16, Dan ...
2 months ago (2017-03-21 05:36:36 UTC) #39
Dan Beam
lgtm
2 months ago (2017-03-21 05:41:52 UTC) #40
Dan Beam
https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resources/settings/route.js File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resources/settings/route.js#newcode409 chrome/browser/resources/settings/route.js:409: * @param {!Route} @param {!Route} route
2 months ago (2017-03-21 08:16:12 UTC) #45
tommycli
https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resources/settings/route.js File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resources/settings/route.js#newcode109 chrome/browser/resources/settings/route.js:109: ]); the isNavigableDialog thing is fine. I may suggest ...
2 months ago (2017-03-21 16:44:57 UTC) #46
dpapad
https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resources/settings/route.js File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resources/settings/route.js#newcode109 chrome/browser/resources/settings/route.js:109: ]); On 2017/03/21 at 16:44:57, tommycli wrote: > the ...
2 months ago (2017-03-21 17:54:15 UTC) #47
tommycli
On 2017/03/21 17:54:15, dpapad wrote: > https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resources/settings/route.js > File chrome/browser/resources/settings/route.js (right): > > https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resources/settings/route.js#newcode109 > ...
2 months ago (2017-03-21 17:56:29 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2754563002/260001
2 months ago (2017-03-21 18:23:12 UTC) #51
commit-bot: I haz the power
2 months ago (2017-03-21 20:12:18 UTC) #54
Message was sent while issue was closed.
Committed patchset #12 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/f9e8f9e6d60d8cd26c4239a37741...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06