History: Add range navigation control for grouped visits

History: Add range navigation control for grouped visits

The current patch enables the user to select the time frame
for which the the grouped results are viewed by
adding a set of buttons. These buttons allow the user to
go back in time, reset to today or go forward until the current interval.

BUG=170690
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180715

[Sergiu, 2013-01-17 18:56:38]

Aaaand we're done, please take a look at this as well.

Thanks,
Sergiu

[Bernhard Bauer, 2013-01-18 16:08:27]

https://codereview.chromium.org/11886104/diff/3001/chrome/browser/resources/h...
File chrome/browser/resources/history/history.html (right):

https://codereview.chromium.org/11886104/diff/3001/chrome/browser/resources/h...
chrome/browser/resources/history/history.html:53: <button id="range-previous"
disabled>
This should fit on one line.

https://codereview.chromium.org/11886104/diff/3001/chrome/browser/resources/h...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/11886104/diff/3001/chrome/browser/resources/h...
chrome/browser/resources/history/history.js:486:
HistoryModel.prototype.getOffset = function() {
Use get/set accessors?

[Sergiu, 2013-01-21 14:05:24]

https://codereview.chromium.org/11886104/diff/3001/chrome/browser/resources/h...
File chrome/browser/resources/history/history.html (right):

https://codereview.chromium.org/11886104/diff/3001/chrome/browser/resources/h...
chrome/browser/resources/history/history.html:53: <button id="range-previous"
disabled>
On 2013/01/18 16:08:27, Bernhard Bauer wrote:
> This should fit on one line.

Done.

https://codereview.chromium.org/11886104/diff/3001/chrome/browser/resources/h...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/11886104/diff/3001/chrome/browser/resources/h...
chrome/browser/resources/history/history.js:486:
HistoryModel.prototype.getOffset = function() {
On 2013/01/18 16:08:27, Bernhard Bauer wrote:
> Use get/set accessors?

Done.

[Bernhard Bauer, 2013-01-21 15:24:45]

lgtm

[Sergiu, 2013-01-22 12:21:49]

Hey, this is the last patch of this series, please take a look.

Thanks.

[Sergiu, 2013-01-28 09:59:44]

On 2013/01/22 12:21:49, Sergiu wrote:
> Hey, this is the last patch of this series, please take a look.
> 
> Thanks.

Ping.

[James Hawkins, 2013-01-28 17:48:15]

https://codereview.chromium.org/11886104/diff/21004/chrome/browser/resources/...
File chrome/browser/resources/history/history.html (right):

https://codereview.chromium.org/11886104/diff/21004/chrome/browser/resources/...
chrome/browser/resources/history/history.html:53: <button id="range-previous"
disabled>◀</button>
I'm not sure we should be embedding 'resources' like this in the HTML.  We need
to get a proper asset.

https://codereview.chromium.org/11886104/diff/21004/chrome/browser/resources/...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/11886104/diff/21004/chrome/browser/resources/...
chrome/browser/resources/history/history.js:1459: buttonsState[2] = true;
Consider creating local const vars, e.g.:

PREVIOUS = 0;
TODAY = 1;
NEXT = 2;

Although taking a look back, I'm not sure the use of an array is really called
for here; it seems to make the code less readable for little gain.

https://codereview.chromium.org/11886104/diff/21004/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/11886104/diff/21004/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/history_ui.cc:564: options->end_time = base::Time();
This block of code (all the green) is somewhat hairy wrt calculations and
whatnot.  I suggest refactoring to pull out into one more more well-named
functions.

[Sergiu, 2013-01-30 16:09:39]

I've addressed some of James' comments. I'm still not sure about the < and >
buttons (see below).

I've also changed the code such that the range buttons work for the normal
pagination as well when the view is set to "recent".

In the future I plan to put the bar containing the "today", "<", ">", etc, bar
on the top so that it stays fixed when you scroll.

https://codereview.chromium.org/11886104/diff/21004/chrome/browser/resources/...
File chrome/browser/resources/history/history.html (right):

https://codereview.chromium.org/11886104/diff/21004/chrome/browser/resources/...
chrome/browser/resources/history/history.html:53: <button id="range-previous"
disabled>◀</button>
On 2013/01/28 17:48:15, James Hawkins wrote:
> I'm not sure we should be embedding 'resources' like this in the HTML.  We
need
> to get a proper asset.

Putting an image here is pretty tricky as you need to style the whole button
(can't have two bacgkground-image). Since it's pretty important to have this in,
should I change the unicode characters to "next" and "previous" and fix this
later on?

https://codereview.chromium.org/11886104/diff/21004/chrome/browser/resources/...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/11886104/diff/21004/chrome/browser/resources/...
chrome/browser/resources/history/history.js:1459: buttonsState[2] = true;
On 2013/01/28 17:48:15, James Hawkins wrote:
> Consider creating local const vars, e.g.:
> 
> PREVIOUS = 0;
> TODAY = 1;
> NEXT = 2;
> 
> Although taking a look back, I'm not sure the use of an array is really called
> for here; it seems to make the code less readable for little gain.

Changed them to local variables, initially I thought of returning the array but
I never did that in the end, hopefully it's clearer now.

https://codereview.chromium.org/11886104/diff/21004/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/11886104/diff/21004/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/history_ui.cc:564: options->end_time = base::Time();
On 2013/01/28 17:48:15, James Hawkins wrote:
> This block of code (all the green) is somewhat hairy wrt calculations and
> whatnot.  I suggest refactoring to pull out into one more more well-named
> functions.

I've refactored this making it simple, hopefully it is clearer now.

[James Hawkins, 2013-01-30 16:30:09]

LGTM with question.

https://codereview.chromium.org/11886104/diff/36001/chrome/browser/resources/...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/11886104/diff/36001/chrome/browser/resources/...
chrome/browser/resources/history/history.js:491:
Object.defineProperty(HistoryModel.prototype, 'offset', {
What is the purpose of using defineProperty?

[Sergiu, 2013-01-30 17:07:13]

https://codereview.chromium.org/11886104/diff/36001/chrome/browser/resources/...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/11886104/diff/36001/chrome/browser/resources/...
chrome/browser/resources/history/history.js:491:
Object.defineProperty(HistoryModel.prototype, 'offset', {
On 2013/01/30 16:30:09, James Hawkins wrote:
> What is the purpose of using defineProperty?

Well, I didn't want to access private members from outside  so I used getters
and setters. Then bauerb@ suggested using Javascript-specific getters and
setters instead of getOffset() and setOffset(offset). There are two ways to do
that, either when defining the prototype object or using defineProperty. In our
case only defineProperty is usable.

[Patrick Dubroy, 2013-01-30 23:25:33]

https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
chrome/browser/resources/history/history.js:768: window.scrollTo(0, 0);
I'm having deja vu...I swear I wrote this comment before. But, what's the reason
to scroll the page here?

https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
chrome/browser/resources/history/history.js:880:
HistoryView.prototype.setRangeButtonsState_ = function() {
I think "updateRangeButtons_" would be a slightly better name for this.

https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
chrome/browser/resources/history/history.js:882: var buttonsState = [false,
false, false];
What do you need this array for? Why can't you just set the disable property
directly on all the elements?

[Sergiu, 2013-01-30 23:34:51]

On 2013/01/30 23:25:33, dubroy wrote:
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> File chrome/browser/resources/history/history.js (right):
> 
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> chrome/browser/resources/history/history.js:768: window.scrollTo(0, 0);
> I'm having deja vu...I swear I wrote this comment before. But, what's the
reason
> to scroll the page here?
> 
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> chrome/browser/resources/history/history.js:880:
> HistoryView.prototype.setRangeButtonsState_ = function() {
> I think "updateRangeButtons_" would be a slightly better name for this.
> 
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> chrome/browser/resources/history/history.js:882: var buttonsState = [false,
> false, false];
> What do you need this array for? Why can't you just set the disable property
> directly on all the elements?

I'll rename the function and remove the page scroll but I think you took a look
at patch set 8 instead of 9 (the latest one), at least for the buttonsState
array stuff.

[Patrick Dubroy, 2013-01-30 23:42:08]

On 2013/01/30 23:34:51, Sergiu wrote:
> On 2013/01/30 23:25:33, dubroy wrote:
> >
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> > File chrome/browser/resources/history/history.js (right):
> > 
> >
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> > chrome/browser/resources/history/history.js:768: window.scrollTo(0, 0);
> > I'm having deja vu...I swear I wrote this comment before. But, what's the
> reason
> > to scroll the page here?
> > 
> >
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> > chrome/browser/resources/history/history.js:880:
> > HistoryView.prototype.setRangeButtonsState_ = function() {
> > I think "updateRangeButtons_" would be a slightly better name for this.
> > 
> >
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> > chrome/browser/resources/history/history.js:882: var buttonsState = [false,
> > false, false];
> > What do you need this array for? Why can't you just set the disable property
> > directly on all the elements?
> 
> I'll rename the function and remove the page scroll but I think you took a
look
> at patch set 8 instead of 9 (the latest one), at least for the buttonsState
> array stuff.

Whoops, not sure how that happened.

Ok, LGTM if you rename the function and remove the page scroll.

[Sergiu, 2013-02-04 19:58:16]

On 2013/01/30 23:42:08, dubroy wrote:
> On 2013/01/30 23:34:51, Sergiu wrote:
> > On 2013/01/30 23:25:33, dubroy wrote:
> > >
> >
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> > > File chrome/browser/resources/history/history.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> > > chrome/browser/resources/history/history.js:768: window.scrollTo(0, 0);
> > > I'm having deja vu...I swear I wrote this comment before. But, what's the
> > reason
> > > to scroll the page here?
> > > 
> > >
> >
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> > > chrome/browser/resources/history/history.js:880:
> > > HistoryView.prototype.setRangeButtonsState_ = function() {
> > > I think "updateRangeButtons_" would be a slightly better name for this.
> > > 
> > >
> >
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> > > chrome/browser/resources/history/history.js:882: var buttonsState =
[false,
> > > false, false];
> > > What do you need this array for? Why can't you just set the disable
property
> > > directly on all the elements?
> > 
> > I'll rename the function and remove the page scroll but I think you took a
> look
> > at patch set 8 instead of 9 (the latest one), at least for the buttonsState
> > array stuff.
> 
> Whoops, not sure how that happened.
> 
> Ok, LGTM if you rename the function and remove the page scroll.

Ok, final tweaks have been made, Patrick, it'd be great if you would take a last
quick look, the plan is to commit tomorrow early morning.

[Patrick Dubroy, 2013-02-04 23:52:02]

lgtm with nits.

On 2013/02/04 19:58:16, Sergiu wrote:
> On 2013/01/30 23:42:08, dubroy wrote:
> > On 2013/01/30 23:34:51, Sergiu wrote:
> > > On 2013/01/30 23:25:33, dubroy wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> > > > File chrome/browser/resources/history/history.js (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> > > > chrome/browser/resources/history/history.js:768: window.scrollTo(0, 0);
> > > > I'm having deja vu...I swear I wrote this comment before. But, what's
the
> > > reason
> > > > to scroll the page here?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> > > > chrome/browser/resources/history/history.js:880:
> > > > HistoryView.prototype.setRangeButtonsState_ = function() {
> > > > I think "updateRangeButtons_" would be a slightly better name for this.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/...
> > > > chrome/browser/resources/history/history.js:882: var buttonsState =
> [false,
> > > > false, false];
> > > > What do you need this array for? Why can't you just set the disable
> property
> > > > directly on all the elements?
> > > 
> > > I'll rename the function and remove the page scroll but I think you took a
> > look
> > > at patch set 8 instead of 9 (the latest one), at least for the
buttonsState
> > > array stuff.
> > 
> > Whoops, not sure how that happened.
> > 
> > Ok, LGTM if you rename the function and remove the page scroll.
> 
> Ok, final tweaks have been made, Patrick, it'd be great if you would take a
last
> quick look, the plan is to commit tomorrow early morning.

[Patrick Dubroy, 2013-02-04 23:52:20]

lgtm with nits.

https://codereview.chromium.org/11886104/diff/51001/chrome/browser/resources/...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/11886104/diff/51001/chrome/browser/resources/...
chrome/browser/resources/history/history.js:523: // Skip |offset_| *
weeks/months from the begining.
Nit: Add a blank line before this comment.

https://codereview.chromium.org/11886104/diff/51001/chrome/browser/resources/...
chrome/browser/resources/history/history.js:666: if (self.getRangeInDays() ==
HistoryModel.Range.ALLTIME)
Nit: Can you make this ALL_TIME and not ALLTIME? The convention is to separate
words with "_".

https://codereview.chromium.org/11886104/diff/51001/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/11886104/diff/51001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/history_ui.cc:279: // - the offset from which the search
should start in multiple of the range.
the offset from which to start the search (in multiples of the range).

[Sergiu, 2013-02-05 09:45:18]

Thanks! Flipping the commit bit.

https://codereview.chromium.org/11886104/diff/51001/chrome/browser/resources/...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/11886104/diff/51001/chrome/browser/resources/...
chrome/browser/resources/history/history.js:523: // Skip |offset_| *
weeks/months from the begining.
On 2013/02/04 23:52:20, dubroy wrote:
> Nit: Add a blank line before this comment.

Done.

https://codereview.chromium.org/11886104/diff/51001/chrome/browser/resources/...
chrome/browser/resources/history/history.js:666: if (self.getRangeInDays() ==
HistoryModel.Range.ALLTIME)
On 2013/02/04 23:52:20, dubroy wrote:
> Nit: Can you make this ALL_TIME and not ALLTIME? The convention is to separate
> words with "_".

Done.

https://codereview.chromium.org/11886104/diff/51001/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/11886104/diff/51001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/history_ui.cc:279: // - the offset from which the search
should start in multiple of the range.
On 2013/02/04 23:52:20, dubroy wrote:
> the offset from which to start the search (in multiples of the range).

Done.

[commit-bot: I haz the power, 2013-02-05 09:46:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergiu@chromium.org/11886104/64001

[commit-bot: I haz the power, 2013-02-05 14:25:57]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-02-05 15:07:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergiu@chromium.org/11886104/64001

[commit-bot: I haz the power, 2013-02-05 17:03:08]

Change committed as 180715