Chromium Code Reviews
Help | Chromium Project | Sign in
(210)

Issue 11886104: History: Add range navigation control for grouped visits (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 3 months ago by Sergiu
Modified:
1 year, 2 months ago
CC:
chromium-reviews_chromium.org, arv
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Rebase #

Total comments: 4

Patch Set 3 : Rebase #

Patch Set 4 : Getters setters and minor edits. #

Patch Set 5 : Rebase #

Patch Set 6 : Rebase #

Patch Set 7 : Fix minor bug #

Total comments: 6

Patch Set 8 : Rebase with the other commit and fix some things. #

Total comments: 3

Patch Set 9 : Replies to comments. #

Total comments: 2

Patch Set 10 : Rebase #

Patch Set 11 : Fix the tests and make the last fixes #

Patch Set 12 : Fix so that you can only have an offset request at a time. #

Total comments: 6

Patch Set 13 : Last edits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+202 lines, -48 lines) Lint Patch
M chrome/app/generated_resources.grd View 2 chunks +4 lines, -1 line 0 comments ? errors Download
M chrome/browser/resources/history/history.html View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments ? errors Download
M chrome/browser/resources/history/history.js View 1 2 3 4 5 6 7 8 9 10 11 12 22 chunks +129 lines, -18 lines 0 comments ? errors Download
M chrome/browser/ui/webui/history_ui.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +5 lines, -5 lines 0 comments ? errors Download
M chrome/browser/ui/webui/history_ui.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +56 lines, -21 lines 0 comments ? errors Download
M chrome/test/data/webui/history_browsertest.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -3 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 21
Sergiu
Aaaand we're done, please take a look at this as well. Thanks, Sergiu
1 year, 3 months ago #1
Bernhard Bauer
https://codereview.chromium.org/11886104/diff/3001/chrome/browser/resources/history/history.html File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/11886104/diff/3001/chrome/browser/resources/history/history.html#newcode53 chrome/browser/resources/history/history.html:53: <button id="range-previous" disabled> This should fit on one line. ...
1 year, 3 months ago #2
Sergiu
https://codereview.chromium.org/11886104/diff/3001/chrome/browser/resources/history/history.html File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/11886104/diff/3001/chrome/browser/resources/history/history.html#newcode53 chrome/browser/resources/history/history.html:53: <button id="range-previous" disabled> On 2013/01/18 16:08:27, Bernhard Bauer wrote: ...
1 year, 2 months ago #3
Bernhard Bauer
lgtm
1 year, 2 months ago #4
Sergiu
Hey, this is the last patch of this series, please take a look. Thanks.
1 year, 2 months ago #5
Sergiu
On 2013/01/22 12:21:49, Sergiu wrote: > Hey, this is the last patch of this series, ...
1 year, 2 months ago #6
James Hawkins
https://codereview.chromium.org/11886104/diff/21004/chrome/browser/resources/history/history.html File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/11886104/diff/21004/chrome/browser/resources/history/history.html#newcode53 chrome/browser/resources/history/history.html:53: <button id="range-previous" disabled>◀</button> I'm not sure we should be ...
1 year, 2 months ago #7
Sergiu
I've addressed some of James' comments. I'm still not sure about the < and > ...
1 year, 2 months ago #8
James Hawkins
LGTM with question. https://codereview.chromium.org/11886104/diff/36001/chrome/browser/resources/history/history.js File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/11886104/diff/36001/chrome/browser/resources/history/history.js#newcode491 chrome/browser/resources/history/history.js:491: Object.defineProperty(HistoryModel.prototype, 'offset', { What is the ...
1 year, 2 months ago #9
Sergiu
https://codereview.chromium.org/11886104/diff/36001/chrome/browser/resources/history/history.js File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/11886104/diff/36001/chrome/browser/resources/history/history.js#newcode491 chrome/browser/resources/history/history.js:491: Object.defineProperty(HistoryModel.prototype, 'offset', { On 2013/01/30 16:30:09, James Hawkins wrote: ...
1 year, 2 months ago #10
Patrick Dubroy gone til Apr 22
https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/history/history.js File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/history/history.js#newcode768 chrome/browser/resources/history/history.js:768: window.scrollTo(0, 0); I'm having deja vu...I swear I wrote ...
1 year, 2 months ago #11
Sergiu
On 2013/01/30 23:25:33, dubroy wrote: > https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/history/history.js > File chrome/browser/resources/history/history.js (right): > > https://codereview.chromium.org/11886104/diff/31001/chrome/browser/resources/history/history.js#newcode768 > ...
1 year, 2 months ago #12
Patrick Dubroy gone til Apr 22
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/history/history.js ...
1 year, 2 months ago #13
Sergiu
On 2013/01/30 23:42:08, dubroy wrote: > On 2013/01/30 23:34:51, Sergiu wrote: > > On 2013/01/30 ...
1 year, 2 months ago #14
Patrick Dubroy gone til Apr 22
lgtm with nits. On 2013/02/04 19:58:16, Sergiu wrote: > On 2013/01/30 23:42:08, dubroy wrote: > ...
1 year, 2 months ago #15
Patrick Dubroy gone til Apr 22
lgtm with nits. https://codereview.chromium.org/11886104/diff/51001/chrome/browser/resources/history/history.js File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/11886104/diff/51001/chrome/browser/resources/history/history.js#newcode523 chrome/browser/resources/history/history.js:523: // Skip |offset_| * weeks/months from ...
1 year, 2 months ago #16
Sergiu
Thanks! Flipping the commit bit. https://codereview.chromium.org/11886104/diff/51001/chrome/browser/resources/history/history.js File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/11886104/diff/51001/chrome/browser/resources/history/history.js#newcode523 chrome/browser/resources/history/history.js:523: // Skip |offset_| * ...
1 year, 2 months ago #17
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/11886104/64001
1 year, 2 months ago #18
I haz the power (commit-bot)
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&number=108218
1 year, 2 months ago #19
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/11886104/64001
1 year, 2 months ago #20
I haz the power (commit-bot)
1 year, 2 months ago #21
Message was sent while issue was closed.
Change committed as 180715
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6