Settings Rewrite: Fix subpage animations for direct URLs to subpages.

Settings Rewrite: Fix subpage animations for direct URLs to subpages.

Previously, if a user navigated to a subpage directly via URL, i.e.
chrome://settings/searchEngines, and then tried to close that subpage,
the animation would not work correctly.

This patch fixes that issue.

BUG=535013
Committed: https://crrev.com/3446a1d9927c166bf4bfba76343040d84411e4b0
Cr-Commit-Position: refs/heads/master@{#351189}

[tommycli, 2015-09-25 22:16:09]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[tommycli, 2015-09-25 22:16:23]

tommycli@chromium.org changed reviewers:
+ dbeam@chromium.org

[tommycli, 2015-09-25 22:16:23]

dbeam: ptal

[commit-bot: I haz the power, 2015-09-25 22:17:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372713002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372713002/20001

[Dan Beam, 2015-09-25 22:28:57]

https://codereview.chromium.org/1372713002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1372713002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:83:
this.expanded_ = expanded;
every time i see "if (local != this.local_) { ... }" I think "why aren't we just
using property change listeners?

so, could we do expandedChanged_ and then simply:

  this.expanded_ = this.currentRoute.section == this.section;

?

https://codereview.chromium.org/1372713002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:119:
runOnDocumentReady_: function(task) {
this should be a Promise, IMO, and also potentially in a different file:

cr.define('Settings', function() {
  return {
    documentReady: new Promise(function(resolve) {
      assert(document.readyState == 'loading');
      document.addEventListener('DOMContentLoaded', resolve);
    }),
  };
});

and then calling code can do this:

Settings.documentReady.then(function() {
  ...
});

and if it's already resolved, it'll fire immediately

[Dan Beam, 2015-09-25 22:29:32]

https://codereview.chromium.org/1372713002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1372713002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:119:
runOnDocumentReady_: function(task) {
On 2015/09/25 22:28:57, Dan Beam wrote:
> this should be a Promise, IMO, and also potentially in a different file:
> 
> cr.define('Settings', function() {
>   return {
>     documentReady: new Promise(function(resolve) {
>       assert(document.readyState == 'loading');
>       document.addEventListener('DOMContentLoaded', resolve);
>     }),
>   };
> });
> 
> and then calling code can do this:
> 
> Settings.documentReady.then(function() {
>   ...
> });
> 
> and if it's already resolved, it'll fire immediately

(same story as this meta pattern: if (something) go(); else
waitForSomething(go); should almost always be a Promise, IMO)

[tommycli, 2015-09-25 23:11:44]

On 2015/09/25 22:29:32, Dan Beam wrote:
>
https://codereview.chromium.org/1372713002/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_page/settings_section.js
> (right):
> 
>
https://codereview.chromium.org/1372713002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_page/settings_section.js:119:
> runOnDocumentReady_: function(task) {
> On 2015/09/25 22:28:57, Dan Beam wrote:
> > this should be a Promise, IMO, and also potentially in a different file:
> > 
> > cr.define('Settings', function() {
> >   return {
> >     documentReady: new Promise(function(resolve) {
> >       assert(document.readyState == 'loading');
> >       document.addEventListener('DOMContentLoaded', resolve);
> >     }),
> >   };
> > });
> > 
> > and then calling code can do this:
> > 
> > Settings.documentReady.then(function() {
> >   ...
> > });
> > 
> > and if it's already resolved, it'll fire immediately
> 
> (same story as this meta pattern: if (something) go(); else
> waitForSomething(go); should almost always be a Promise, IMO)

Sounds good, I will refactor the above, probably get the fixed version to you
Monday.

Before I do that: is everything in this ancestor patch ok?
https://codereview.chromium.org/1358313004#ps100001

[commit-bot: I haz the power, 2015-09-25 23:36:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-25 23:36:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dan Beam, 2015-09-28 17:56:30]

> Sounds good, I will refactor the above, probably get the fixed version to you
Monday.

let me know when you need a re-review after this ^

[chromium-reviews, 2015-09-28 18:01:48]

dbeam: No problem. I be taking my time to try to "get it right" today.

On Mon, Sep 28, 2015 at 10:56 AM, <dbeam@chromium.org> wrote:

> Sounds good, I will refactor the above, probably get the fixed version to
>> you
>>
> Monday.
>
> let me know when you need a re-review after this ^
>
> https://codereview.chromium.org/1372713002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tommycli, 2015-09-28 21:15:01]

dbeam: ready for a rereview. thanks

https://codereview.chromium.org/1372713002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1372713002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:83:
this.expanded_ = expanded;
On 2015/09/25 22:28:57, Dan Beam wrote:
> every time i see "if (local != this.local_) { ... }" I think "why aren't we
just
> using property change listeners?
> 
> so, could we do expandedChanged_ and then simply:
> 
>   this.expanded_ = this.currentRoute.section == this.section;
> 
> ?

Done. I just eliminated this piece of internal state and instead use both the
old and new route arguments.

https://codereview.chromium.org/1372713002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:119:
runOnDocumentReady_: function(task) {
On 2015/09/25 22:29:32, Dan Beam wrote:
> On 2015/09/25 22:28:57, Dan Beam wrote:
> > this should be a Promise, IMO, and also potentially in a different file:
> > 
> > cr.define('Settings', function() {
> >   return {
> >     documentReady: new Promise(function(resolve) {
> >       assert(document.readyState == 'loading');
> >       document.addEventListener('DOMContentLoaded', resolve);
> >     }),
> >   };
> > });
> > 
> > and then calling code can do this:
> > 
> > Settings.documentReady.then(function() {
> >   ...
> > });
> > 
> > and if it's already resolved, it'll fire immediately
> 
> (same story as this meta pattern: if (something) go(); else
> waitForSomething(go); should almost always be a Promise, IMO)

Acknowledged.

https://codereview.chromium.org/1372713002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:119:
runOnDocumentReady_: function(task) {
On 2015/09/25 22:28:57, Dan Beam wrote:
> this should be a Promise, IMO, and also potentially in a different file:
> 
> cr.define('Settings', function() {
>   return {
>     documentReady: new Promise(function(resolve) {
>       assert(document.readyState == 'loading');
>       document.addEventListener('DOMContentLoaded', resolve);
>     }),
>   };
> });
> 
> and then calling code can do this:
> 
> Settings.documentReady.then(function() {
>   ...
> });
> 
> and if it's already resolved, it'll fire immediately

The better solution was just to skip animations for initial load. Looks a lot
faster.

https://codereview.chromium.org/1372713002/diff/70001/chrome/browser/resource...
File
chrome/browser/resources/settings/settings_page/settings_animated_pages.html
(right):

https://codereview.chromium.org/1372713002/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.html:16:
<neon-animated-pages id="animatedPages" attr-for-selected="id">
Don't select a page until route is updated. Stops a flicker for direct to
subpage URLs.

[Dan Beam, 2015-09-28 21:18:59]

lgtm

https://codereview.chromium.org/1372713002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1372713002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:119:
runOnDocumentReady_: function(task) {
On 2015/09/28 21:15:01, tommycli wrote:
> On 2015/09/25 22:28:57, Dan Beam wrote:
> > this should be a Promise, IMO, and also potentially in a different file:
> > 
> > cr.define('Settings', function() {
> >   return {
> >     documentReady: new Promise(function(resolve) {
> >       assert(document.readyState == 'loading');
> >       document.addEventListener('DOMContentLoaded', resolve);
> >     }),
> >   };
> > });
> > 
> > and then calling code can do this:
> > 
> > Settings.documentReady.then(function() {
> >   ...
> > });
> > 
> > and if it's already resolved, it'll fire immediately
> 
> The better solution was just to skip animations for initial load. Looks a lot
> faster.

+1

https://codereview.chromium.org/1372713002/diff/90001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1372713002/diff/90001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:179: 
var card here (as log as possible)

[tommycli, 2015-09-28 21:35:52]

thx... I guess the 'being more deliberate' strategy did lead to a shorter cycle
process in this case.

https://codereview.chromium.org/1372713002/diff/90001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.js
(right):

https://codereview.chromium.org/1372713002/diff/90001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:179: 
On 2015/09/28 21:18:59, Dan Beam wrote:
> var card here (as log as possible)

Done.

[tommycli, 2015-09-28 21:35:56]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2015-09-28 21:35:57]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1372713002/#ps110001
(title: " ")

[commit-bot: I haz the power, 2015-09-28 21:37:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372713002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372713002/110001

[commit-bot: I haz the power, 2015-09-28 22:49:54]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-09-28 22:52:30]

Patchset 7 (id:??) landed as
https://crrev.com/3446a1d9927c166bf4bfba76343040d84411e4b0
Cr-Commit-Position: refs/heads/master@{#351189}