Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11975053:
History: Add option to group visits by domain (Closed)
|
Created:
7 years, 11 months ago by Sergiu Modified:
7 years, 11 months ago CC:
chromium-reviews, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHistory: Add option to group visits by domain
This patch is part of a series of 3 patches that add grouping by domain name
to the Chrome history page.
The current patch adds checkbox to the history page, which, when enabled,
groups the results in a day by domain and orders them by the number of visits
to that domain.
The change is only active when the "--enable-grouped-history" command line
switch is passed when running Chrome.
BUG=170690
TBR=jhawkins@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178007
Patch Set 1 #
Total comments: 5
Patch Set 2 : Optimize twisty_closed PNGs. #Patch Set 3 : Move input inside label. #
Total comments: 34
Patch Set 4 : Fix collapse/toogle #
Total comments: 10
Patch Set 5 : Replying to comments. #Patch Set 6 : Rebase #
Total comments: 2
Patch Set 7 : Proper switch code. #
Total comments: 4
Patch Set 8 : Fix some strings and CSS #
Total comments: 10
Patch Set 9 : Minor fix #Patch Set 10 : Rebase #Patch Set 11 : Replace images with characters and minor fix. #
Total comments: 6
Messages
Total messages: 22 (0 generated)
Hey, I've managed to split the CL from here[1] into 3 different CLs, which should make things more manageable. I uploaded this separately since it's only a part of the original CL. This was sort of extracted from the original patch so please take a look that I didn't mess something on the way, although everything works. Thanks! [1] https://codereview.chromium.org/11586015/
https://codereview.chromium.org/11975053/diff/1/chrome/browser/resources/hist... File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/11975053/diff/1/chrome/browser/resources/hist... chrome/browser/resources/history/history.html:52: <label i18n-content="displayfiltersites" for="display-filter-sites"> It puts the input element inside of the label element, or it gets the hose again. https://codereview.chromium.org/11975053/diff/1/chrome/browser/resources/twis... File chrome/browser/resources/twisty_closed.png (right): https://codereview.chromium.org/11975053/diff/1/chrome/browser/resources/twis... chrome/browser/resources/twisty_closed.png:1: PNG I think this already landed?
https://codereview.chromium.org/11975053/diff/1/chrome/browser/resources/hist... File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/11975053/diff/1/chrome/browser/resources/hist... chrome/browser/resources/history/history.html:52: <label i18n-content="displayfiltersites" for="display-filter-sites"> On 2013/01/17 19:05:14, Bernhard Bauer wrote: > It puts the input element inside of the label element, or it gets the hose > again. Done. https://codereview.chromium.org/11975053/diff/1/chrome/browser/resources/twis... File chrome/browser/resources/twisty_closed.png (right): https://codereview.chromium.org/11975053/diff/1/chrome/browser/resources/twis... chrome/browser/resources/twisty_closed.png:1: PNG On 2013/01/17 19:05:14, Bernhard Bauer wrote: > I think this already landed? Well, as far as I remember they haven't, and git log tells me the last change was in August 2011.
LGTM, but I'm still confused about the PNGs. https://codereview.chromium.org/11975053/diff/1/chrome/browser/resources/twis... File chrome/browser/resources/twisty_closed.png (right): https://codereview.chromium.org/11975053/diff/1/chrome/browser/resources/twis... chrome/browser/resources/twisty_closed.png:1: PNG On 2013/01/18 09:21:46, Sergiu wrote: > On 2013/01/17 19:05:14, Bernhard Bauer wrote: > > I think this already landed? > > Well, as far as I remember they haven't, and git log tells me the last change > was in August 2011. What's the change that you're making here?
This is getting close! BTW, I think you should change the description to "History: Add option to group visits by domain". You don't need to make any reference to the other patch sets; this functionality can pretty much stand alone. Can you also upload a screenshot to the bug showing what the page looks like with your change in there? Also include the changes you are making to the twisty PNGs. https://codereview.chromium.org/11975053/diff/10002/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11975053/diff/10002/chrome/app/generated_reso... chrome/app/generated_resources.grd:465: + <message name="IDS_HISTORY_DISPLAY_FILTER_SITES" desc="The text on the radio box that sets the mode to sites."> nit: it's a radio _button_, not a radio _box_. See http://www.knowahead.in/wp-content/uploads/2012/05/car-radio-buttons.jpg :-) https://codereview.chromium.org/11975053/diff/10002/chrome/app/generated_reso... chrome/app/generated_resources.grd:466: + Group domains I think "Group by domain" is a better description. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.css:28: #filter-controls { Can you put #results-display here too? It's currently hanging out too far. To get everything to line up, you'll need to remove the margins on the menu-buttons and the "Search history" button too. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.css:41: float: right; I think we should eventually put all of the controls at the top of the page into a flexbox. But, for the time being, I'm ok with adding this as a float. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.css:82: .site-domain-wrapper { Why not .site-name-wrapper? Is there a distinction between domain and site? https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.css:83: /* So that everything stacks horizontally. */ Get rid of this comment. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.css:103: line-height: 1.75em; Is this necessary, since it's already on the .site-domain-wrapper? https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:78: * </ul> Are we really supposed to embed HTML in jsdoc? Ew. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:491: if (!this.groupByDomain_ && !doneLoading && !this.inFlight_) Why don't we want to try to fill the page when groupByDomain is on? https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:504: if (!this.getGroupByDomain()) { I'm also confused about why we don't run this code when groupByDomain is on. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:518: RESULTS_PER_PAGE]); No need to wrap this line. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:621: // the search term if there was one. Shouldn't it be the other way around? When you search, it unchecks and disables group by domain? https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:736: var innerResultList = this.querySelector('.site-results'); I'd just fetch the parent rather than binding this function to it. And pull this function out to the top level. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:745: innerArrow.className = 'site-domain-arrow expand'; I'd remove collapse, and add expand. It's more verbose, but then you're not making assumptions about what other classes might be used on this element. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:877: if (results.length == 0) { I'd combine these two conditions, and remove a level of nesting. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:887: for (var i = 0, visit; visit = results[i]; i++) { I this loop is a bit confusing. I think it might be better if you re-wrote it as a 'while' loop (or possibly do/while) with a nested 'for' loop. https://codereview.chromium.org/11975053/diff/14002/chrome/browser/ui/webui/h... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/11975053/diff/14002/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:124: IDS_HISTORY_NO_RESULTS_INTERVAL); Looks unrelated to group by domain. https://codereview.chromium.org/11975053/diff/14002/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:141: IDS_HISTORY_DISPLAY_FILTER_SITES); Looks like it's also unrelated. https://codereview.chromium.org/11975053/diff/14002/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:147: DictionaryValue localized_strings; Why can't you just add the string by itself? Why do you need to create the dictionaryvalue? https://codereview.chromium.org/11975053/diff/14002/chrome/common/chrome_swit... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/11975053/diff/14002/chrome/common/chrome_swit... chrome/common/chrome_switches.h:206: extern const char kHistoryEnableGrouped[]; Why not just EnableGroupByDomain for now?
On 2013/01/18 11:29:25, Bernhard Bauer wrote: > LGTM, but I'm still confused about the PNGs. > > https://codereview.chromium.org/11975053/diff/1/chrome/browser/resources/twis... > File chrome/browser/resources/twisty_closed.png (right): > > https://codereview.chromium.org/11975053/diff/1/chrome/browser/resources/twis... > chrome/browser/resources/twisty_closed.png:1: PNG > On 2013/01/18 09:21:46, Sergiu wrote: > > On 2013/01/17 19:05:14, Bernhard Bauer wrote: > > > I think this already landed? > > > > Well, as far as I remember they haven't, and git log tells me the last change > > was in August 2011. > > What's the change that you're making here? Sorry, I wasn't very clear about that. The difference between the old triangle and the new one is minor, the new triangle uses a slightly different color and shape and looks a bit better I think. The only other place where it is used now is the Safebrowsing interstitial.
On 2013/01/18 12:21:35, Sergiu wrote: > On 2013/01/18 11:29:25, Bernhard Bauer wrote: > > LGTM, but I'm still confused about the PNGs. > > > > > https://codereview.chromium.org/11975053/diff/1/chrome/browser/resources/twis... > > File chrome/browser/resources/twisty_closed.png (right): > > > > > https://codereview.chromium.org/11975053/diff/1/chrome/browser/resources/twis... > > chrome/browser/resources/twisty_closed.png:1: PNG > > On 2013/01/18 09:21:46, Sergiu wrote: > > > On 2013/01/17 19:05:14, Bernhard Bauer wrote: > > > > I think this already landed? > > > > > > Well, as far as I remember they haven't, and git log tells me the last > change > > > was in August 2011. > > > > What's the change that you're making here? > > Sorry, I wasn't very clear about that. The difference between the old triangle > and the new one is minor, the new triangle uses a slightly different color and > shape and looks a bit better I think. The only other place where it is used now > is the Safebrowsing interstitial. Okay. It didn't show up as changed here, but that's probably just Rietveld being weird. I assume UI people are fine with changing the assets?
Please take another look. Thanks, Sergiu https://codereview.chromium.org/11975053/diff/10002/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11975053/diff/10002/chrome/app/generated_reso... chrome/app/generated_resources.grd:465: + <message name="IDS_HISTORY_DISPLAY_FILTER_SITES" desc="The text on the radio box that sets the mode to sites."> On 2013/01/18 11:43:26, dubroy wrote: > nit: it's a radio _button_, not a radio _box_. See > http://www.knowahead.in/wp-content/uploads/2012/05/car-radio-buttons.jpg :-) Combo box + radio button = radio box, easy :) Fixed. https://codereview.chromium.org/11975053/diff/10002/chrome/app/generated_reso... chrome/app/generated_resources.grd:466: + Group domains On 2013/01/18 11:43:26, dubroy wrote: > I think "Group by domain" is a better description. Yeah, I sort of cut it from "Group domains in the past [combo box]" to "Group domains". I've changed it for this CL to "Group by domain." https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.css:28: #filter-controls { On 2013/01/18 11:43:26, dubroy wrote: > Can you put #results-display here too? It's currently hanging out too far. > > To get everything to line up, you'll need to remove the margins on the > menu-buttons and the "Search history" button too. Done. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.css:41: float: right; On 2013/01/18 11:43:26, dubroy wrote: > I think we should eventually put all of the controls at the top of the page into > a flexbox. But, for the time being, I'm ok with adding this as a float. The UI is definitely going to have some work done on it, in managed mode we'll also have some extra stuff that appears in the history page so we'll have to see how that degrades well for non-managed users while not having two completely different views. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.css:82: .site-domain-wrapper { On 2013/01/18 11:43:26, dubroy wrote: > Why not .site-name-wrapper? Is there a distinction between domain and site? I think you meant between domain and name. Out of these site-name is a pretty bad description, I've changed it to site-entry to make it clearer. The .site-domain the domain of the .site-entry . https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.css:83: /* So that everything stacks horizontally. */ On 2013/01/18 11:43:26, dubroy wrote: > Get rid of this comment. Done. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.css:103: line-height: 1.75em; On 2013/01/18 11:43:26, dubroy wrote: > Is this necessary, since it's already on the .site-domain-wrapper? No, I see now that it is enough if it applies both to .site-domain-wrapper and .entry-box. Removed https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:78: * </ul> On 2013/01/18 11:43:26, dubroy wrote: > Are we really supposed to embed HTML in jsdoc? Ew. Not that thrilled about it myself, JS style doc says: > Like JavaDoc, JSDoc supports many HTML tags, like <code>, > <pre>, <tt>, <strong>, <ul>, <ol>, <li>, <a>, and others. > > This means that plaintext formatting is not respected. So, > don't rely on whitespace to format JSDoc: https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:491: if (!this.groupByDomain_ && !doneLoading && !this.inFlight_) On 2013/01/18 11:43:26, dubroy wrote: > Why don't we want to try to fill the page when groupByDomain is on? Done. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:504: if (!this.getGroupByDomain()) { On 2013/01/18 11:43:26, dubroy wrote: > I'm also confused about why we don't run this code when groupByDomain is on. Done. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:518: RESULTS_PER_PAGE]); On 2013/01/18 11:43:26, dubroy wrote: > No need to wrap this line. Done. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:621: // the search term if there was one. On 2013/01/18 11:43:26, dubroy wrote: > Shouldn't it be the other way around? When you search, it unchecks and disables > group by domain? setGroupByDomain clears the search if there was one, setSearchText calls this.clearModel_ which sets grouped by domain to false. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:736: var innerResultList = this.querySelector('.site-results'); On 2013/01/18 11:43:26, dubroy wrote: > I'd just fetch the parent rather than binding this function to it. And pull this > function out to the top level. Done. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:745: innerArrow.className = 'site-domain-arrow expand'; On 2013/01/18 11:43:26, dubroy wrote: > I'd remove collapse, and add expand. It's more verbose, but then you're not > making assumptions about what other classes might be used on this element. Good idea, done. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:877: if (results.length == 0) { On 2013/01/18 11:43:26, dubroy wrote: > I'd combine these two conditions, and remove a level of nesting. Done. https://codereview.chromium.org/11975053/diff/10002/chrome/browser/resources/... chrome/browser/resources/history/history.js:887: for (var i = 0, visit; visit = results[i]; i++) { On 2013/01/18 11:43:26, dubroy wrote: > I this loop is a bit confusing. I think it might be better if you re-wrote it as > a 'while' loop (or possibly do/while) with a nested 'for' loop. Done. https://codereview.chromium.org/11975053/diff/14002/chrome/browser/ui/webui/h... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/11975053/diff/14002/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:124: IDS_HISTORY_NO_RESULTS_INTERVAL); On 2013/01/18 11:43:27, dubroy wrote: > Looks unrelated to group by domain. It is, this is the "No results were found in this interval" message. if there are no results for a certain interval. This works for normal results as well, but I'm not sure if it would need its own CL or not. https://codereview.chromium.org/11975053/diff/14002/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:141: IDS_HISTORY_DISPLAY_FILTER_SITES); On 2013/01/18 11:43:27, dubroy wrote: > Looks like it's also unrelated. It is related, renamed to be more descriptive. https://codereview.chromium.org/11975053/diff/14002/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:147: DictionaryValue localized_strings; On 2013/01/18 11:43:27, dubroy wrote: > Why can't you just add the string by itself? Why do you need to create the > dictionaryvalue? Well, the thing is that |source| only had AddString but not AddBoolean, and I'm not exactly sure how the dictionary way works (it merges the dictionaries while keeping Booleans as they are). Anyway, I used strings in the end. https://codereview.chromium.org/11975053/diff/14002/chrome/common/chrome_swit... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/11975053/diff/14002/chrome/common/chrome_swit... chrome/common/chrome_switches.h:206: extern const char kHistoryEnableGrouped[]; On 2013/01/18 11:43:27, dubroy wrote: > Why not just EnableGroupByDomain for now? Done.
https://codereview.chromium.org/11975053/diff/14002/chrome/browser/ui/webui/h... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/11975053/diff/14002/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:147: DictionaryValue localized_strings; On 2013/01/21 08:56:16, Sergiu wrote: > On 2013/01/18 11:43:27, dubroy wrote: > > Why can't you just add the string by itself? Why do you need to create the > > dictionaryvalue? > > Well, the thing is that |source| only had AddString but not AddBoolean, and I'm > not exactly sure how the dictionary way works (it merges the dictionaries while > keeping Booleans as they are). The problem is that AddLocalizedString takes a resource ID and looks up the corresponding string itself, but we want to pass in a boolean. The dictionary merge does nothing special for booleans or strings, it just merges the dictionary containing the boolean into the the one containing strings. > Anyway, I used strings in the end. And why exactly did you do that? The idiomatic way in WebUI is to use booleans. It also seems more natural to me to encode a two-state value with a boolean instead of a string.
https://codereview.chromium.org/11975053/diff/14002/chrome/browser/ui/webui/h... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/11975053/diff/14002/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:147: DictionaryValue localized_strings; On 2013/01/21 09:10:03, Bernhard Bauer wrote: > On 2013/01/21 08:56:16, Sergiu wrote: > > On 2013/01/18 11:43:27, dubroy wrote: > > > Why can't you just add the string by itself? Why do you need to create the > > > dictionaryvalue? > > > > Well, the thing is that |source| only had AddString but not AddBoolean, and > I'm > > not exactly sure how the dictionary way works (it merges the dictionaries > while > > keeping Booleans as they are). > > The problem is that AddLocalizedString takes a resource ID and looks up the > corresponding string itself, but we want to pass in a boolean. The dictionary > merge does nothing special for booleans or strings, it just merges the > dictionary containing the boolean into the the one containing strings. > > > Anyway, I used strings in the end. > > And why exactly did you do that? The idiomatic way in WebUI is to use booleans. > It also seems more natural to me to encode a two-state value with a boolean > instead of a string. Well this is odd.. Initially when I looked at that I didn't see ChromeWebUIDataSource's AddBoolean method, although I looked twice :( Fixed it now, sorry about that.
https://codereview.chromium.org/11975053/diff/10002/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11975053/diff/10002/chrome/app/generated_reso... chrome/app/generated_resources.grd:465: + <message name="IDS_HISTORY_DISPLAY_FILTER_SITES" desc="The text on the radio box that sets the mode to sites."> On 2013/01/21 08:56:15, Sergiu wrote: > On 2013/01/18 11:43:26, dubroy wrote: > > nit: it's a radio _button_, not a radio _box_. See > > http://www.knowahead.in/wp-content/uploads/2012/05/car-radio-buttons.jpg :-) > > Combo box + radio button = radio box, easy :) > Fixed. My bad, this is actually not a radio anything. It's a checkbox. Can you change the description to "Label for the checkbox that toggles the mode to group history visits by domain" https://codereview.chromium.org/11975053/diff/21003/chrome/browser/ui/webui/h... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/11975053/diff/21003/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:145: IDS_GROUP_BY_DOMAIN_CONTROLS); Maybe IDS_HISTORY_GROUP_BY_DOMAIN_LABEL? https://codereview.chromium.org/11975053/diff/21005/chrome/browser/resources/... File chrome/browser/resources/history/history.css (left): https://codereview.chromium.org/11975053/diff/21005/chrome/browser/resources/... chrome/browser/resources/history/history.css:33: max-width: 740px; I don't see the CSS that will ensure that everything will line up on the right hand side. Can you please add that? I think you need to add "margin: 0" for the .menu-button items and for the "Search history" button. That should do the trick. https://codereview.chromium.org/11975053/diff/21005/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/11975053/diff/21005/chrome/browser/resources/... chrome/browser/resources/history/history.js:855: noResults.textContent = loadTimeData.getString('noresultsinterval'); This message doesn't make sense for the normal history page that has no concept of an "interval".
https://codereview.chromium.org/11975053/diff/10002/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11975053/diff/10002/chrome/app/generated_reso... chrome/app/generated_resources.grd:465: + <message name="IDS_HISTORY_DISPLAY_FILTER_SITES" desc="The text on the radio box that sets the mode to sites."> On 2013/01/21 09:42:40, dubroy wrote: > On 2013/01/21 08:56:15, Sergiu wrote: > > On 2013/01/18 11:43:26, dubroy wrote: > > > nit: it's a radio _button_, not a radio _box_. See > > > http://www.knowahead.in/wp-content/uploads/2012/05/car-radio-buttons.jpg :-) > > > > Combo box + radio button = radio box, easy :) > > Fixed. > > My bad, this is actually not a radio anything. It's a checkbox. Can you change > the description to "Label for the checkbox that toggles the mode to group > history visits by domain" Done. https://codereview.chromium.org/11975053/diff/21003/chrome/browser/ui/webui/h... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/11975053/diff/21003/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:145: IDS_GROUP_BY_DOMAIN_CONTROLS); On 2013/01/21 09:42:40, dubroy wrote: > Maybe IDS_HISTORY_GROUP_BY_DOMAIN_LABEL? Done. https://codereview.chromium.org/11975053/diff/21005/chrome/browser/resources/... File chrome/browser/resources/history/history.css (left): https://codereview.chromium.org/11975053/diff/21005/chrome/browser/resources/... chrome/browser/resources/history/history.css:33: max-width: 740px; On 2013/01/21 09:42:40, dubroy wrote: > I don't see the CSS that will ensure that everything will line up on the right > hand side. Can you please add that? I think you need to add "margin: 0" for the > .menu-button items and for the "Search history" button. That should do the > trick. Done. https://codereview.chromium.org/11975053/diff/21005/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/11975053/diff/21005/chrome/browser/resources/... chrome/browser/resources/history/history.js:855: noResults.textContent = loadTimeData.getString('noresultsinterval'); On 2013/01/21 09:42:40, dubroy wrote: > This message doesn't make sense for the normal history page that has no concept > of an "interval". Done, renamed all the strings related to "no results found" to make more sense.
LGTM, if you change the stuff mentioned below. Also, can you please update the CL description like I mentioned in one of my earlier comments? https://codereview.chromium.org/11975053/diff/28004/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11975053/diff/28004/chrome/app/generated_reso... chrome/app/generated_resources.grd:430: + No history entries found. Can you change this to "No visits found."? https://codereview.chromium.org/11975053/diff/28004/chrome/browser/resources/... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/11975053/diff/28004/chrome/browser/resources/... chrome/browser/resources/history/history.css:19: /* Remove the margin to align the button to the right side */ I don't think you need this comment. https://codereview.chromium.org/11975053/diff/28004/chrome/browser/ui/webui/h... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/11975053/diff/28004/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:124: IDS_HISTORY_NO_RESULTS); Don't need to wrap this line. https://codereview.chromium.org/11975053/diff/28004/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:145: IDS_GROUP_BY_DOMAIN_LABEL); Also doesn't need to be wrapped. https://codereview.chromium.org/11975053/diff/28004/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:147: source->AddBoolean("historyGroupEnabled", Can you name this "groupByDomain"?
Done, I'll click the commit box tomorrow morning. https://codereview.chromium.org/11975053/diff/28004/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11975053/diff/28004/chrome/app/generated_reso... chrome/app/generated_resources.grd:430: + No history entries found. On 2013/01/21 14:35:44, dubroy wrote: > Can you change this to "No visits found."? Done. https://codereview.chromium.org/11975053/diff/28004/chrome/browser/resources/... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/11975053/diff/28004/chrome/browser/resources/... chrome/browser/resources/history/history.css:19: /* Remove the margin to align the button to the right side */ On 2013/01/21 14:35:44, dubroy wrote: > I don't think you need this comment. Done. https://codereview.chromium.org/11975053/diff/28004/chrome/browser/ui/webui/h... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/11975053/diff/28004/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:124: IDS_HISTORY_NO_RESULTS); On 2013/01/21 14:35:44, dubroy wrote: > Don't need to wrap this line. Done. https://codereview.chromium.org/11975053/diff/28004/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:145: IDS_GROUP_BY_DOMAIN_LABEL); On 2013/01/21 14:35:44, dubroy wrote: > Also doesn't need to be wrapped. Done. https://codereview.chromium.org/11975053/diff/28004/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:147: source->AddBoolean("historyGroupEnabled", On 2013/01/21 14:35:44, dubroy wrote: > Can you name this "groupByDomain"? Done.
Hey James, Can I get an OWNERS review for the history_ui.cc change? Thanks, Sergiu
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/11975053/23005
Message was sent while issue was closed.
Change committed as 178007
Message was sent while issue was closed.
On 2013/01/22 14:59:05, I haz the power (commit-bot) wrote: > Change committed as 178007 Please don't commit non-trivial changes without OWNERS approval.
Message was sent while issue was closed.
https://codereview.chromium.org/11975053/diff/23005/chrome/browser/resources/... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/11975053/diff/23005/chrome/browser/resources/... chrome/browser/resources/history/history.css:252: color: #8F8F8F; nit: Be consistent: you use rgb() elsewhere and hex here. https://codereview.chromium.org/11975053/diff/23005/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/11975053/diff/23005/chrome/browser/resources/... chrome/browser/resources/history/history.js:75: * <ul> We don't really use the JSDocs to generate API pages, so please don't add HTML. The most important part of the JSDoc comments are for devs reading the code. https://codereview.chromium.org/11975053/diff/23005/chrome/browser/resources/... chrome/browser/resources/history/history.js:382: // If the results are not for the current search term there's nothing more nit: "term, there's" https://codereview.chromium.org/11975053/diff/23005/chrome/browser/resources/... chrome/browser/resources/history/history.js:701: * This function generates and adds the grouped visits DOM for a certain s/This function g/G/ https://codereview.chromium.org/11975053/diff/23005/chrome/browser/resources/... chrome/browser/resources/history/history.js:768: for (var i = 0, domain; domain = domains[i]; i++) { nit: No braces for single-line blocks. https://codereview.chromium.org/11975053/diff/23005/chrome/browser/resources/... chrome/browser/resources/history/history.js:972: if (grouped) { nit: No braces for single-line blocks.
Message was sent while issue was closed.
Message was sent while issue was closed.
On 2013/01/22 17:17:54, James Hawkins wrote: > On 2013/01/22 14:59:05, I haz the power (commit-bot) wrote: > > Change committed as 178007 > > Please don't commit non-trivial changes without OWNERS approval. Sorry James, that's my fault -- I told Sergiu to TBR you on it. Since I'm an owner of chrome/browser/resources/history, the only non-owner-reviewed change was the addition of the localized strings in history_ui.cc. I figured that was trivial, but we'll make sure to wait for your LGTM on changes like that in the future.
Message was sent while issue was closed.
On 2013/01/22 18:54:06, dubroy wrote: > On 2013/01/22 17:17:54, James Hawkins wrote: > > On 2013/01/22 14:59:05, I haz the power (commit-bot) wrote: > > > Change committed as 178007 > > > > Please don't commit non-trivial changes without OWNERS approval. > > Sorry James, that's my fault -- I told Sergiu to TBR you on it. Since I'm an > owner of chrome/browser/resources/history, the only non-owner-reviewed change > was the addition of the localized strings in history_ui.cc. I figured that was > trivial, but we'll make sure to wait for your LGTM on changes like that in the > future. Thanks, Patrick: appreciate it. I think we're all on the same page now. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
