Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1344613002:
[MD settings] on startup use current pages (Closed)
|
Created:
5 years, 3 months ago by dschuyler Modified:
5 years, 3 months ago Reviewers:
michaelpg CC:
chromium-reviews, khorimoto+watch-md-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, jhawkins+watch-md-settings_chromium.org, orenb+watch-md-settings_chromium.org, arv+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+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. |
Description[MD settings] on startup use current pages
This CL connects the use current pages button.
The saved list is tested against undefined to make sure that
cancel works properly. We don't want to test against a
non-empty list because an empty list may be a valid state to
restore to.
BUG=425627
Committed: https://crrev.com/088dc949a18ffde1490f48d08c9fade291f89078
Cr-Commit-Position: refs/heads/master@{#349551}
Patch Set 1 #
Total comments: 12
Patch Set 2 : review changes #Patch Set 3 : added @private #
Total comments: 1
Patch Set 4 : review changes #Messages
Total messages: 11 (3 generated)
dschuyler@chromium.org changed reviewers: + michaelpg@chromium.org
https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js (right): https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:47: var caller = this; use .bind instead of caching this https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:48: cr.define('Settings', function() { What is this for? https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:58: if (this.savedUrlList === undefined && use double equals (the property shouldn't be null, right?) https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:67: for (var i = 0; i < data.length; ++i) { no braces around single line https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:68: urlArray.push(data[i].url); or: var urlArray = data.map(function(datum) { return datum.url; });
https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js (right): https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:47: var caller = this; On 2015/09/14 19:13:49, michaelpg wrote: > use .bind instead of caching this Done. https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:48: cr.define('Settings', function() { On 2015/09/14 19:13:48, michaelpg wrote: > What is this for? This is routing the 'Settings.updateStartupPages' javascript call from c++ to the local this.updateStartupPages_. https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:58: if (this.savedUrlList === undefined && On 2015/09/14 19:13:48, michaelpg wrote: > use double equals (the property shouldn't be null, right?) Actually, I'm expecting it to be undefined. I don't expect it to be null. So I think that means === is the correct choice(?). i.e. undefined === undefined (but undefined === null is false). whereas with x == undefined then x may be undefined or null? Or are you saying that we should allow savedUrlList to be updated, if somehow (and unexpectedly) savedUrlList becomes null? I'm changing it to == assuming that you're looking for null check just-to-be-extra-safe. I.e. it's not strictly required. https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:67: for (var i = 0; i < data.length; ++i) { On 2015/09/14 19:13:48, michaelpg wrote: > no braces around single line Done. https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:68: urlArray.push(data[i].url); On 2015/09/14 19:13:49, michaelpg wrote: > or: > var urlArray = data.map(function(datum) { return datum.url; }); heh, was this an fyi and I can use either one - or a request to map instead of a for loop? I'm used to for-loops being remarkably faster than map(), but maybe that's changed since I last checked? Done.
On 2015/09/14 23:20:00, dschuyler wrote: > https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js > (right): > > https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:47: var > caller = this; > On 2015/09/14 19:13:49, michaelpg wrote: > > use .bind instead of caching this > > Done. > > https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:48: > cr.define('Settings', function() { > On 2015/09/14 19:13:48, michaelpg wrote: > > What is this for? > > This is routing the 'Settings.updateStartupPages' javascript call from c++ to > the local this.updateStartupPages_. > > https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:58: if > (this.savedUrlList === undefined && > On 2015/09/14 19:13:48, michaelpg wrote: > > use double equals (the property shouldn't be null, right?) > > Actually, I'm expecting it to be undefined. I don't expect it to be null. So I > think that means === is the correct choice(?). > > i.e. undefined === undefined (but undefined === null is false). > whereas with > x == undefined then x may be undefined or null? > > Or are you saying that we should allow savedUrlList to be updated, if somehow > (and unexpectedly) savedUrlList becomes null? > > I'm changing it to == assuming that you're looking for null check > just-to-be-extra-safe. I.e. it's not strictly required. > > https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:67: for > (var i = 0; i < data.length; ++i) { > On 2015/09/14 19:13:48, michaelpg wrote: > > no braces around single line > > Done. > > https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:68: > urlArray.push(data[i].url); > On 2015/09/14 19:13:49, michaelpg wrote: > > or: > > var urlArray = data.map(function(datum) { return datum.url; }); > > heh, was this an fyi and I can use either one - or a request to map instead of a > for loop? > > I'm used to for-loops being remarkably faster than map(), but maybe that's > changed since I last checked? > > Done. Does this look good now?
lgtm https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js (right): https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:58: if (this.savedUrlList === undefined && On 2015/09/14 23:20:00, dschuyler wrote: > On 2015/09/14 19:13:48, michaelpg wrote: > > use double equals (the property shouldn't be null, right?) > > Actually, I'm expecting it to be undefined. I don't expect it to be null. So I > think that means === is the correct choice(?). We rarely use strict equality (===) which is the main reason I mentioned it. If: * there's no way for "this.savedUrlList" to be null, and * if hypothetically "this.savedUrlList" WERE null, we'd want to do this action anyway then there's no reason to use === and then less specific == is fine. > > i.e. undefined === undefined (but undefined === null is false). > whereas with > x == undefined then x may be undefined or null? > > Or are you saying that we should allow savedUrlList to be updated, if somehow > (and unexpectedly) savedUrlList becomes null? basically > > I'm changing it to == assuming that you're looking for null check > just-to-be-extra-safe. I.e. it's not strictly required. Just convention, not safety. It's only safer in the sense that if we decide to initialize savedUrlList to null for some random reason instead of undefined, we won't break this. https://codereview.chromium.org/1344613002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:68: urlArray.push(data[i].url); On 2015/09/14 23:20:00, dschuyler wrote: > On 2015/09/14 19:13:49, michaelpg wrote: > > or: > > var urlArray = data.map(function(datum) { return datum.url; }); > > heh, was this an fyi and I can use either one - or a request to map instead of a > for loop? > > I'm used to for-loops being remarkably faster than map(), but maybe that's > changed since I last checked? > > Done. "or" meant "optional" :-) I personally prefer this style when you're essentially doing something as simple as x = [lambda(x) -> x.url]. But feel free to change it back. I dunno if map is necessarily slower than a loop. I'm seeing conflicting results here: http://jsperf.com/aoeuaoeuaoeuaoeuueoueo Sometimes the loops is faster, sometimes the map is faster ¯\_(ツ)_/¯ https://codereview.chromium.org/1344613002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js (right): https://codereview.chromium.org/1344613002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:50: updateStartupPages: function(data) { why not just: updateStartupPages: updateFunction,
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1344613002/#ps60001 (title: "review changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1344613002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1344613002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/088dc949a18ffde1490f48d08c9fade291f89078 Cr-Commit-Position: refs/heads/master@{#349551} |
