Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
439313004:
history: remove unused variable. (Closed)
|
Created:
6 years, 4 months ago by Dan Beam Modified:
6 years, 3 months ago CC:
chromium-reviews, Patrick Dubroy, pam+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Descriptionhistory: remove unused variable.
R=estade@chromium.org
BUG=none
Committed: https://crrev.com/823300b5d549a31dcfe99677a4bb54cc816c6ea5
Cr-Commit-Position: refs/heads/master@{#294322}
Patch Set 1 #
Total comments: 2
Patch Set 2 : estade@ review #
Total comments: 6
Patch Set 3 : sergiu@ review #Messages
Total messages: 15 (2 generated)
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbeam@chromium.org/439313004/1
https://codereview.chromium.org/439313004/diff/1/chrome/browser/resources/his... File chrome/browser/resources/history/history.js (left): https://codereview.chromium.org/439313004/diff/1/chrome/browser/resources/his... chrome/browser/resources/history/history.js:1801: grouped: false, can we get rid of this as well then?
https://codereview.chromium.org/439313004/diff/1/chrome/browser/resources/his... File chrome/browser/resources/history/history.js (left): https://codereview.chromium.org/439313004/diff/1/chrome/browser/resources/his... chrome/browser/resources/history/history.js:1801: grouped: false, On 2014/08/04 21:16:23, Evan Stade wrote: > can we get rid of this as well then? Done.
https://codereview.chromium.org/439313004/diff/20001/chrome/browser/resources... File chrome/browser/resources/history/history.js (left): https://codereview.chromium.org/439313004/diff/20001/chrome/browser/resources... chrome/browser/resources/history/history.js:861: HistoryModel.prototype.setGroupByDomain = function(groupByDomain) { it seems like this isn't used... https://codereview.chromium.org/439313004/diff/20001/chrome/browser/resources... chrome/browser/resources/history/history.js:1882: var grouped = (hashData.grouped == 'true') || historyModel.getGroupByDomain(); I guess that it would make more sense to either use this, i.e. historyView.setGroupedByDomain(grouped) or remove the grouping by domain completely. +sergiu who seems to own this feature
estade@chromium.org changed reviewers: + sergiu@chromium.org
actually +sergiu
lgtm https://codereview.chromium.org/439313004/diff/20001/chrome/browser/resources... File chrome/browser/resources/history/history.js (left): https://codereview.chromium.org/439313004/diff/20001/chrome/browser/resources... chrome/browser/resources/history/history.js:861: HistoryModel.prototype.setGroupByDomain = function(groupByDomain) { On 2014/08/04 21:50:05, Evan Stade wrote: > it seems like this isn't used... Yeah, this should be removed I guess. https://codereview.chromium.org/439313004/diff/20001/chrome/browser/resources... chrome/browser/resources/history/history.js:1882: var grouped = (hashData.grouped == 'true') || historyModel.getGroupByDomain(); On 2014/08/04 21:50:05, Evan Stade wrote: > I guess that it would make more sense to either use this, i.e. > > historyView.setGroupedByDomain(grouped) > > or remove the grouping by domain completely. > > +sergiu who seems to own this feature Removing this should be fine. We actually infer the "grouped" state from the range (you can see groupedByDomain being set in setPageState here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...).
https://codereview.chromium.org/439313004/diff/20001/chrome/browser/resources... File chrome/browser/resources/history/history.js (left): https://codereview.chromium.org/439313004/diff/20001/chrome/browser/resources... chrome/browser/resources/history/history.js:861: HistoryModel.prototype.setGroupByDomain = function(groupByDomain) { On 2014/09/09 08:59:24, Sergiu wrote: > On 2014/08/04 21:50:05, Evan Stade wrote: > > it seems like this isn't used... > > Yeah, this should be removed I guess. Done. https://codereview.chromium.org/439313004/diff/20001/chrome/browser/resources... chrome/browser/resources/history/history.js:1882: var grouped = (hashData.grouped == 'true') || historyModel.getGroupByDomain(); On 2014/09/09 08:59:24, Sergiu wrote: > On 2014/08/04 21:50:05, Evan Stade wrote: > > I guess that it would make more sense to either use this, i.e. > > > > historyView.setGroupedByDomain(grouped) > > > > or remove the grouping by domain completely. > > > > +sergiu who seems to own this feature > > Removing this should be fine. We actually infer the "grouped" state from the > range (you can see groupedByDomain being set in setPageState here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...). Removed the local var, left this.groupByDomain_ as it's still used.
lgtm
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/439313004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as f48bf1c5f3c0fe55bebdb4e5a7d99f89eeb47b0b
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/823300b5d549a31dcfe99677a4bb54cc816c6ea5 Cr-Commit-Position: refs/heads/master@{#294322} |
