[MD settings] fixes for OnStartup using the closure compiler

[MD settings] fixes for OnStartup using the closure compiler

This is a quick fix for errors in the closure
usage.  This fixes
http://build.chromium.org/p/chromium.fyi/builders/Closure%20Compilation%20Linux/builds/33761/steps/compile/logs/stdio

BUG=531750
Committed: https://crrev.com/d085d772e049818ab2744ccfef84d8ba5d70043f
Cr-Commit-Position: refs/heads/master@{#349750}

[dschuyler, 2015-09-18 01:37:11]

dschuyler@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Dan Beam, 2015-09-18 01:42:05]

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

[Dan Beam, 2015-09-18 01:42:05]

https://codereview.chromium.org/1353453003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js
(right):

https://codereview.chromium.org/1353453003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:52: },
you shouldn't need to make an extra scope just to please the compiler

https://codereview.chromium.org/1353453003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:63:
this.get('prefs.session.startup_urls'));
why can't you re-use the .get() call above?  e.g. store as a var?

[dschuyler, 2015-09-18 02:36:29]

https://codereview.chromium.org/1353453003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js
(right):

https://codereview.chromium.org/1353453003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:52: },
On 2015/09/18 01:42:05, Dan Beam wrote:
> you shouldn't need to make an extra scope just to please the compiler

I've moved the declaration to a var.
(I also found that I can declare a named function, but that just moved
the extra scope).

Done.

https://codereview.chromium.org/1353453003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:63:
this.get('prefs.session.startup_urls'));
On 2015/09/18 01:42:05, Dan Beam wrote:
> why can't you re-use the .get() call above?  e.g. store as a var?

Done.

[stevenjb, 2015-09-18 02:44:19]

lgtm

[Dan Beam, 2015-09-18 03:12:23]

https://codereview.chromium.org/1353453003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js
(right):

https://codereview.chromium.org/1353453003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:51:
updateStartupPages: localFunction,
this is probably some chrome-specific compiler pass nonsense

https://codereview.chromium.org/1353453003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:52: };
does this work?

var self = this;

cr.define('Settings', function() {
  return {
    updateStartupPages: function() {
      return self.updateStartupPages_.apply(self, arguments);
    },
  };
});

https://codereview.chromium.org/1353453003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:61: if
(this.savedUrlList == undefined && pref) {
nit: no curlies

https://codereview.chromium.org/1353453003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:61: if
(this.savedUrlList == undefined && pref) {
nit: !this.savedUrlList instead of == undefined

[dschuyler, 2015-09-18 17:31:13]

https://codereview.chromium.org/1353453003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js
(right):

https://codereview.chromium.org/1353453003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:51:
updateStartupPages: localFunction,
On 2015/09/18 03:12:23, Dan Beam wrote:
> this is probably some chrome-specific compiler pass nonsense

Acknowledged.

https://codereview.chromium.org/1353453003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:52: };
On 2015/09/18 03:12:23, Dan Beam wrote:
> does this work?
> 
> var self = this;
> 
> cr.define('Settings', function() {
>   return {
>     updateStartupPages: function() {
>       return self.updateStartupPages_.apply(self, arguments);
>     },
>   };
> });

Yes, the closure compiler is happy with that.

Done.

https://codereview.chromium.org/1353453003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:61: if
(this.savedUrlList == undefined && pref) {
On 2015/09/18 03:12:23, Dan Beam wrote:
> nit: !this.savedUrlList instead of == undefined

Done.

https://codereview.chromium.org/1353453003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:61: if
(this.savedUrlList == undefined && pref) {
On 2015/09/18 03:12:23, Dan Beam wrote:
> nit: no curlies

Done.

[Dan Beam, 2015-09-18 19:05:53]

lgtm

https://codereview.chromium.org/1353453003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js
(right):

https://codereview.chromium.org/1353453003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:63:
this.savedUrlList = pref.value.slice();
nit: try to use variables as close as you can to where they're declared.  in
this case, the optimal code (for never doing any needless work) is, imo:

if (!this.savedUrlList) {
  var pref = this.get('prefs.session.startup_urls');
  if (pref)
    this.savedUrlList = pref.value.slice();
}

or

if (this.savedUrlList)
  return;

var pref = this.get('prefs.session.startup_urls');
if (pref)
  this.savedUrlList = pref.value.slice();

[dschuyler, 2015-09-18 19:23:57]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2015-09-18 19:23:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1353453003/#ps80001
(title: "review nit")

[commit-bot: I haz the power, 2015-09-18 19:24:37]

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

[commit-bot: I haz the power, 2015-09-18 20:04:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-18 20:05:46]

Patchset 5 (id:??) landed as
https://crrev.com/d085d772e049818ab2744ccfef84d8ba5d70043f
Cr-Commit-Position: refs/heads/master@{#349750}