Implement History as HTML and add/change a bunch of stuff to make it easier t...

Implement History as HTML and add/change a bunch of stuff to make it easier to add HTML UIs in future.

This CL does the following:
* Adds the DOMUIContents TabContentsType. 
* Adds a DOMUI base class for HTML UIs.
* Adds HistoryUI and its HTML/JS.
* Changes 'chrome-resource://' to 'chrome://' globally (this is the bulk of the edited files) so we don't the situation where the display URL differs from the actual URL. This may have security implications that I'm not aware of, and someone should double-verify that there are no unpleasant side-effects of this change.

Future work:
* Remove the New Tab and Debugger TabContentTypes and make those classes use DOMUI instead.
* Add history deletion (better than day-range deletion).
* Once we're happy with the HTML UI, remove HistoryView/Model/TabUI and change all the history access points to point to chrome://history/
* Merge DOMUIContents with WebContents.

[Glen Murphy, 2008-11-25 23:10:21]

No rush on this review, sorry for the gigantifulness.

Tim (sorry, Evan is on vacation), can you review everything under browser/dom_ui

Ojan (sorry, Brett is on vacation), can you review everything else (other than
the giant HTML, it should just be minor changes to get the new TabContentsType
working, feel free to redirect if I'm lying).

[tim (not reviewing), 2008-12-01 20:45:04]

first pass of comments, I'm still reviewing/thinking about the dom_ui stuff more
closely. nothing major yet besides an ownership question.

http://codereview.chromium.org/12418/diff/337/102
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/12418/diff/337/102#newcode27
Line 27: // This URL scheme is never needed by code external to this module.
this comment really does not hold. it's used all over the place, several in this
CL. we should move this to somewhere common (though I'm not forcing you to do it
now since this CL is already big). Maybe yank this comment and add a todo?

http://codereview.chromium.org/12418/diff/337/110
File chrome/browser/dom_ui/dom_ui.cc (right):

http://codereview.chromium.org/12418/diff/337/110#newcode47
Line 47: const Value& arg) {
nit - indent

http://codereview.chromium.org/12418/diff/337/110#newcode72
Line 72: // DOMUI, protected:
----------------------------------------------------------
these separator comments aren't really consistent with the rest of the file

http://codereview.chromium.org/12418/diff/337/111
File chrome/browser/dom_ui/dom_ui.h (right):

http://codereview.chromium.org/12418/diff/337/111#newcode19
Line 19: DOMMessageHandler(DOMUI* dom_ui);
explicit

http://codereview.chromium.org/12418/diff/337/111#newcode28
Line 28: DOMUI* dom_ui_;
can this be * const?

http://codereview.chromium.org/12418/diff/337/111#newcode30
Line 30: DISALLOW_EVIL_CONSTRUCTORS(DOMMessageHandler);
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/12418/diff/337/111#newcode33
Line 33: class DOMUI {
this class could use a comment

http://codereview.chromium.org/12418/diff/337/111#newcode35
Line 35: DOMUI(DOMUIContents* contents);
explicit

http://codereview.chromium.org/12418/diff/337/111#newcode38
Line 38: virtual void Init() = 0;
this could use a comment. who owns the instances?

http://codereview.chromium.org/12418/diff/337/111#newcode78
Line 78: DISALLOW_EVIL_CONSTRUCTORS(DOMUI);
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/12418/diff/337/114
File chrome/browser/dom_ui/dom_ui_contents.cc (right):

http://codereview.chromium.org/12418/diff/337/114#newcode121
Line 121: if (!url->SchemeIs("chrome"))
we use this scheme several times in this file, I'd say add a constant (and maybe
make GetScheme return const std::string&) but it's defined as a constant in
chrome_url_data_manager.h with a comment saying it's not needed anywhere else :\
I guess that constant should be moved somewhere common; I'm not forcing you to
do that right now though

http://codereview.chromium.org/12418/diff/337/109
File chrome/browser/dom_ui/dom_ui_contents.h (right):

http://codereview.chromium.org/12418/diff/337/109#newcode41
Line 41: Profile* profile_;
can this be * const?

http://codereview.chromium.org/12418/diff/337/109#newcode48
Line 48: DISALLOW_EVIL_CONSTRUCTORS(FavIconSource);
DISALLOW_COPY_AND_ASSIGN (other class decls in this file also)

http://codereview.chromium.org/12418/diff/337/109#newcode72
Line 72: Profile* profile_;
* const?

http://codereview.chromium.org/12418/diff/337/109#newcode85
Line 85: bool DOMUIContentsHandleURL(GURL* url, TabContentsType* result_type);
might be better named DOMUIContentsCanHandleURL

http://codereview.chromium.org/12418/diff/337/109#newcode101
Line 101: // virtual WebPreferences GetWebkitPrefs();
looks like you don't need this? I remember  RenderViewHostDelegate used to throw
an assert if it wasn't overridden, maybe you found another way around it?

http://codereview.chromium.org/12418/diff/337/109#newcode117
Line 117: // The DOMUI we're currently showing.
who owns current_ui_?

http://codereview.chromium.org/12418/diff/337/112
File chrome/browser/dom_ui/history_ui.cc (right):

http://codereview.chromium.org/12418/diff/337/112#newcode93
Line 93: search_text_(L""),
you don't need this L"" init.

http://codereview.chromium.org/12418/diff/337/112#newcode116
Line 116: void BrowsingHistoryHandler::HandleGetHistory(const Value* value) {
this method is nearly 100 lines.. maybe you could split it once you've extracted
the string & month?

http://codereview.chromium.org/12418/diff/337/112#newcode133
Line 133: search_text_ = L"";
std::wstring()

http://codereview.chromium.org/12418/diff/337/112#newcode170
Line 170: exploded.day_of_month = 1;
these last 3 lines are identical to the first 3 lines in the  first if( block.
You could do them first and then check search_month_ to perform the adjustments
below

http://codereview.chromium.org/12418/diff/337/112#newcode215
Line 215: history::URLResult page = (*results)[i];
this could be const &

http://codereview.chromium.org/12418/diff/337/112#newcode254
Line 254: StringValue search_value(search_text_);
you could do this inside the function call

http://codereview.chromium.org/12418/diff/337/113
File chrome/browser/dom_ui/history_ui.h (right):

http://codereview.chromium.org/12418/diff/337/113#newcode25
Line 25: DISALLOW_EVIL_CONSTRUCTORS(HistoryUIHTMLSource);
DISALLOW_COPY_AND_ASSIGN (other places in this file too)

http://codereview.chromium.org/12418/diff/337/113#newcode66
Line 66: static GURL GetBaseURL();
what's a BaseURL?

http://codereview.chromium.org/12418/diff/337/100
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/12418/diff/337/100#newcode13
Line 13: #include "chrome/browser/dom_ui/dom_ui_contents.h"
is this really needed?

http://codereview.chromium.org/12418/diff/337/100#newcode694
Line 694: *url = GURL("chrome://new-tab/");
you could assemble the string using DOMUIContents::GetScheme(), if you keep the
#include

http://codereview.chromium.org/12418/diff/337/91
File chrome/browser/tab_contents_type.h (right):

http://codereview.chromium.org/12418/diff/337/91#newcode27
Line 27: TAB_CONTENTS_DOM_UI
this should definitely be before NUM_TYPES

[Glen Murphy, 2008-12-03 01:29:19]

These should be addressed, thanks a bunch for dealing with it.

In the course of using this, and based on comments from others, I think the
history system should be paginated by day (or days), so all the 'exploded' stuff
should get removed once I work out the new UI.

I took the lazy way out and left the "chrome" scheme defined as static consts in
the two files - they're TODOed for a quick followup CL.

> Line 117: // The DOMUI we're currently showing.
> who owns current_ui_?

current_ui_ / any DOMUI is owned by DOMUIContents.

> Line 101: // virtual WebPreferences GetWebkitPrefs();
> looks like you don't need this? I remember  RenderViewHostDelegate used to
throw
> an assert if it wasn't overridden, maybe you found another way around it?

I just forgot it - I've added this back in to force-enable images and JS for
DOMUIContentseses.



On 2008/12/01 20:45:04, timsteele wrote:
> first pass of comments, I'm still reviewing/thinking about the dom_ui stuff
more
> closely. nothing major yet besides an ownership question.
> 
> http://codereview.chromium.org/12418/diff/337/102
> File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):
> 
> http://codereview.chromium.org/12418/diff/337/102#newcode27
> Line 27: // This URL scheme is never needed by code external to this module.
> this comment really does not hold. it's used all over the place, several in
this
> CL. we should move this to somewhere common (though I'm not forcing you to do
it
> now since this CL is already big). Maybe yank this comment and add a todo?
> 
> http://codereview.chromium.org/12418/diff/337/110
> File chrome/browser/dom_ui/dom_ui.cc (right):
> 
> http://codereview.chromium.org/12418/diff/337/110#newcode47
> Line 47: const Value& arg) {
> nit - indent
> 
> http://codereview.chromium.org/12418/diff/337/110#newcode72
> Line 72: // DOMUI, protected:
> ----------------------------------------------------------
> these separator comments aren't really consistent with the rest of the file
> 
> http://codereview.chromium.org/12418/diff/337/111
> File chrome/browser/dom_ui/dom_ui.h (right):
> 
> http://codereview.chromium.org/12418/diff/337/111#newcode19
> Line 19: DOMMessageHandler(DOMUI* dom_ui);
> explicit
> 
> http://codereview.chromium.org/12418/diff/337/111#newcode28
> Line 28: DOMUI* dom_ui_;
> can this be * const?
> 
> http://codereview.chromium.org/12418/diff/337/111#newcode30
> Line 30: DISALLOW_EVIL_CONSTRUCTORS(DOMMessageHandler);
> DISALLOW_COPY_AND_ASSIGN
> 
> http://codereview.chromium.org/12418/diff/337/111#newcode33
> Line 33: class DOMUI {
> this class could use a comment
> 
> http://codereview.chromium.org/12418/diff/337/111#newcode35
> Line 35: DOMUI(DOMUIContents* contents);
> explicit
> 
> http://codereview.chromium.org/12418/diff/337/111#newcode38
> Line 38: virtual void Init() = 0;
> this could use a comment. who owns the instances?
> 
> http://codereview.chromium.org/12418/diff/337/111#newcode78
> Line 78: DISALLOW_EVIL_CONSTRUCTORS(DOMUI);
> DISALLOW_COPY_AND_ASSIGN
> 
> http://codereview.chromium.org/12418/diff/337/114
> File chrome/browser/dom_ui/dom_ui_contents.cc (right):
> 
> http://codereview.chromium.org/12418/diff/337/114#newcode121
> Line 121: if (!url->SchemeIs("chrome"))
> we use this scheme several times in this file, I'd say add a constant (and
maybe
> make GetScheme return const std::string&) but it's defined as a constant in
> chrome_url_data_manager.h with a comment saying it's not needed anywhere else
:\
> I guess that constant should be moved somewhere common; I'm not forcing you to
> do that right now though
> 
> http://codereview.chromium.org/12418/diff/337/109
> File chrome/browser/dom_ui/dom_ui_contents.h (right):
> 
> http://codereview.chromium.org/12418/diff/337/109#newcode41
> Line 41: Profile* profile_;
> can this be * const?
> 
> http://codereview.chromium.org/12418/diff/337/109#newcode48
> Line 48: DISALLOW_EVIL_CONSTRUCTORS(FavIconSource);
> DISALLOW_COPY_AND_ASSIGN (other class decls in this file also)
> 
> http://codereview.chromium.org/12418/diff/337/109#newcode72
> Line 72: Profile* profile_;
> * const?
> 
> http://codereview.chromium.org/12418/diff/337/109#newcode85
> Line 85: bool DOMUIContentsHandleURL(GURL* url, TabContentsType* result_type);
> might be better named DOMUIContentsCanHandleURL
> 
> http://codereview.chromium.org/12418/diff/337/109#newcode101
> Line 101: // virtual WebPreferences GetWebkitPrefs();
> looks like you don't need this? I remember  RenderViewHostDelegate used to
throw
> an assert if it wasn't overridden, maybe you found another way around it?
> 
> http://codereview.chromium.org/12418/diff/337/109#newcode117
> Line 117: // The DOMUI we're currently showing.
> who owns current_ui_?
> 
> http://codereview.chromium.org/12418/diff/337/112
> File chrome/browser/dom_ui/history_ui.cc (right):
> 
> http://codereview.chromium.org/12418/diff/337/112#newcode93
> Line 93: search_text_(L""),
> you don't need this L"" init.
> 
> http://codereview.chromium.org/12418/diff/337/112#newcode116
> Line 116: void BrowsingHistoryHandler::HandleGetHistory(const Value* value) {
> this method is nearly 100 lines.. maybe you could split it once you've
extracted
> the string & month?
> 
> http://codereview.chromium.org/12418/diff/337/112#newcode133
> Line 133: search_text_ = L"";
> std::wstring()
> 
> http://codereview.chromium.org/12418/diff/337/112#newcode170
> Line 170: exploded.day_of_month = 1;
> these last 3 lines are identical to the first 3 lines in the  first if( block.
> You could do them first and then check search_month_ to perform the
adjustments
> below
> 
> http://codereview.chromium.org/12418/diff/337/112#newcode215
> Line 215: history::URLResult page = (*results)[i];
> this could be const &
> 
> http://codereview.chromium.org/12418/diff/337/112#newcode254
> Line 254: StringValue search_value(search_text_);
> you could do this inside the function call
> 
> http://codereview.chromium.org/12418/diff/337/113
> File chrome/browser/dom_ui/history_ui.h (right):
> 
> http://codereview.chromium.org/12418/diff/337/113#newcode25
> Line 25: DISALLOW_EVIL_CONSTRUCTORS(HistoryUIHTMLSource);
> DISALLOW_COPY_AND_ASSIGN (other places in this file too)
> 
> http://codereview.chromium.org/12418/diff/337/113#newcode66
> Line 66: static GURL GetBaseURL();
> what's a BaseURL?
> 
> http://codereview.chromium.org/12418/diff/337/100
> File chrome/browser/dom_ui/new_tab_ui.cc (right):
> 
> http://codereview.chromium.org/12418/diff/337/100#newcode13
> Line 13: #include "chrome/browser/dom_ui/dom_ui_contents.h"
> is this really needed?
> 
> http://codereview.chromium.org/12418/diff/337/100#newcode694
> Line 694: *url = GURL("chrome://new-tab/");
> you could assemble the string using DOMUIContents::GetScheme(), if you keep
the
> #include
> 
> http://codereview.chromium.org/12418/diff/337/91
> File chrome/browser/tab_contents_type.h (right):
> 
> http://codereview.chromium.org/12418/diff/337/91#newcode27
> Line 27: TAB_CONTENTS_DOM_UI
> this should definitely be before NUM_TYPES

[tim (not reviewing), 2008-12-03 01:40:44]

http://codereview.chromium.org/12418/diff/674/707
File chrome/browser/dom_ui/dom_ui_contents.cc (right):

http://codereview.chromium.org/12418/diff/674/707#newcode170
Line 170: current_ui_ = NULL;
I don't see any call to delete any instance of current_ui_, did you miss that or
am I not looking in the right place?

[Glen Murphy, 2008-12-03 01:55:14]

Haha, when in doubt, assume I'm dimmer than two Das Keyboards.

Deletes and DOMUIContents destructor added.


> http://codereview.chromium.org/12418/diff/674/707
> File chrome/browser/dom_ui/dom_ui_contents.cc (right):
> 
> http://codereview.chromium.org/12418/diff/674/707#newcode170
> Line 170: current_ui_ = NULL;
> I don't see any call to delete any instance of current_ui_, did you miss that
or
> am I not looking in the right place?

[ojan, 2008-12-03 02:30:27]

Non-JS code looks good. One high-level comment about the JS before I do a
detailed line-by-line review (although it all mostly looks good).

http://codereview.chromium.org/12418/diff/674/701
File chrome/browser/resources/history.html (right):

http://codereview.chromium.org/12418/diff/674/701#newcode568
Line 568: historyView.init();
Can you make all of these proper classes and replace the init(_) methods with
the constructors? I think you can mostly avoid the use of globals as well by
passing the instances of each object to the constructors as appropriate. That
might take a bit of untangling work, but I think it'll make the code easier to
reason about in the end.

[tim (not reviewing), 2008-12-03 19:06:29]

just a couple more things, lookin' good

http://codereview.chromium.org/12418/diff/513/543
File chrome/browser/dom_ui/dom_ui.h (right):

http://codereview.chromium.org/12418/diff/513/543#newcode9
Line 9: 
nit - extra space

http://codereview.chromium.org/12418/diff/513/543#newcode62
Line 62: DISALLOW_EVIL_CONSTRUCTORS(DOMUI);
COPY_AND_ASSIGN

http://codereview.chromium.org/12418/diff/513/543#newcode68
Line 68: class DOMMessageHandler {
nit - indent in this class is off (public/protected decls)

http://codereview.chromium.org/12418/diff/513/543#newcode77
Line 77: const GURL& gurl);
nit-indent

http://codereview.chromium.org/12418/diff/513/546
File chrome/browser/dom_ui/dom_ui_contents.cc (right):

http://codereview.chromium.org/12418/diff/513/546#newcode175
Line 175: delete current_ui_;
cool, but I think you still want to NULL-out current_ui_, since we don't
necessarily reassign it below

http://codereview.chromium.org/12418/diff/513/546#newcode181
Line 181: if (new_ui) {
So if (!new_ui), pending_entry is a dom_ui handle-able URL, but we don't have a
DOMUI for it. do we want to return false and discard the nav in this case? It's
not clear to me how we proceed with a null current_ui_ here but we
DCHECK(current_ui_) in other places (ProcessDOMUIMessage)

http://codereview.chromium.org/12418/diff/513/541
File chrome/browser/dom_ui/dom_ui_contents.h (right):

http://codereview.chromium.org/12418/diff/513/541#newcode83
Line 83: // to the appropriate values for displaying the new tab page and
returns true.
comment references new tab page only?

http://codereview.chromium.org/12418/diff/513/544
File chrome/browser/dom_ui/history_ui.cc (right):

http://codereview.chromium.org/12418/diff/513/544#newcode147
Line 147: SetURLAndTitle(page_value,
can these go on the same line?

http://codereview.chromium.org/12418/diff/513/545
File chrome/browser/dom_ui/history_ui.h (right):

http://codereview.chromium.org/12418/diff/513/545#newcode25
Line 25: DISALLOW_EVIL_CONSTRUCTORS(HistoryUIHTMLSource);
should be in private: section, and DISALLOW_COPY_AND_ASSIGN

[Glen Murphy, 2008-12-03 21:58:26]

Tim, the below should be done. I made NavigateToPendingEntry return false on a
NULL current_ui_, which seems to be the right thing to do. In future, this
should instead return the equivalent of a Chrome 404.

Ojan, HistoryModel and HistoryView are now classes, though (as discussed in
person) global dirtiness remains and is TODOed.


On 2008/12/03 19:06:29, timsteele wrote:
> just a couple more things, lookin' good
> 
> http://codereview.chromium.org/12418/diff/513/543
> File chrome/browser/dom_ui/dom_ui.h (right):
> 
> http://codereview.chromium.org/12418/diff/513/543#newcode9
> Line 9: 
> nit - extra space
> 
> http://codereview.chromium.org/12418/diff/513/543#newcode62
> Line 62: DISALLOW_EVIL_CONSTRUCTORS(DOMUI);
> COPY_AND_ASSIGN
> 
> http://codereview.chromium.org/12418/diff/513/543#newcode68
> Line 68: class DOMMessageHandler {
> nit - indent in this class is off (public/protected decls)
> 
> http://codereview.chromium.org/12418/diff/513/543#newcode77
> Line 77: const GURL& gurl);
> nit-indent
> 
> http://codereview.chromium.org/12418/diff/513/546
> File chrome/browser/dom_ui/dom_ui_contents.cc (right):
> 
> http://codereview.chromium.org/12418/diff/513/546#newcode175
> Line 175: delete current_ui_;
> cool, but I think you still want to NULL-out current_ui_, since we don't
> necessarily reassign it below
> 
> http://codereview.chromium.org/12418/diff/513/546#newcode181
> Line 181: if (new_ui) {
> So if (!new_ui), pending_entry is a dom_ui handle-able URL, but we don't have
a
> DOMUI for it. do we want to return false and discard the nav in this case?
It's
> not clear to me how we proceed with a null current_ui_ here but we
> DCHECK(current_ui_) in other places (ProcessDOMUIMessage)
> 
> http://codereview.chromium.org/12418/diff/513/541
> File chrome/browser/dom_ui/dom_ui_contents.h (right):
> 
> http://codereview.chromium.org/12418/diff/513/541#newcode83
> Line 83: // to the appropriate values for displaying the new tab page and
> returns true.
> comment references new tab page only?
> 
> http://codereview.chromium.org/12418/diff/513/544
> File chrome/browser/dom_ui/history_ui.cc (right):
> 
> http://codereview.chromium.org/12418/diff/513/544#newcode147
> Line 147: SetURLAndTitle(page_value,
> can these go on the same line?
> 
> http://codereview.chromium.org/12418/diff/513/545
> File chrome/browser/dom_ui/history_ui.h (right):
> 
> http://codereview.chromium.org/12418/diff/513/545#newcode25
> Line 25: DISALLOW_EVIL_CONSTRUCTORS(HistoryUIHTMLSource);
> should be in private: section, and DISALLOW_COPY_AND_ASSIGN

[tim (not reviewing), 2008-12-03 22:21:12]

dom_ui/.* LGTM cap'n

http://codereview.chromium.org/12418/diff/749/581
File chrome/browser/dom_ui/dom_ui.h (right):

http://codereview.chromium.org/12418/diff/749/581#newcode21
Line 21: virtual void Init() = 0;
maybe add a comment here to point out this is where subclasses should
|AddMessageHandler| for each handler. DOMUIHost had an abstract
AttachMessageHandlers() that was called by super-Init, but what you have (+ a
comment) works just as well since you have a bunch of other initialization to
do.

[ojan, 2008-12-04 02:06:57]

Lots of nits. A few possible bugs...

http://codereview.chromium.org/12418/diff/749/578
File chrome/browser/resources/history.html (right):

http://codereview.chromium.org/12418/diff/749/578#newcode20
Line 20: // localStrings (static):
This isn't really static as it stores state (anything that references "this"
isn't static). I'd prefer if this were also just an instance of a LocalStrings
class.

http://codereview.chromium.org/12418/diff/749/578#newcode27
Line 27: strings : {},
private?

http://codereview.chromium.org/12418/diff/749/578#newcode30
Line 30: for (var i = 0; i < children.length; i++) {
http://www.corp.google.com/eng/doc/javascriptguide.html#nodelists

for (var i = 0, child; child = child[i]; i++) {
  var id = child.id;
  if (id) {
    this.strings[id] = child.innerHTML;
  }
}

http://codereview.chromium.org/12418/diff/749/578#newcode49
Line 49: * @param {Boolean} continued Whether this page is on the same day as
the
Nit: {boolean} primitive types should be lowercase.

http://codereview.chromium.org/12418/diff/749/578#newcode65
Line 65: this.snippet = ('snippet' in result) ? result.snippet : "";
Since these should never have legit values of 0, false, null, etc, you can just
use ||.
this.snippet = result.snippet || "";

http://codereview.chromium.org/12418/diff/749/578#newcode68
Line 68: this.continued = continued;
Aren't pretty much all of these properties private?

http://codereview.chromium.org/12418/diff/749/578#newcode127
Line 127: '</div>',
96-105 looks suspiciously like 119-127. maybe make a helper function for that
chunk?

http://codereview.chromium.org/12418/diff/749/578#newcode141
Line 141: Page.getHighlightedText_ = function(str, highlight) {
opt_highlight?

http://codereview.chromium.org/12418/diff/749/578#newcode142
Line 142: if (!highlight) { return str; }
Either ditch the brackets or make it multiline.

http://codereview.chromium.org/12418/diff/749/578#newcode154
Line 154: return str.replace(/([\\\.\+\*\?\[\^\]\$\(\)\{\}\=\!\<\>\|\:])/g,
"\\$1");
This is crazyness.

http://codereview.chromium.org/12418/diff/749/578#newcode170
Line 170: this.inflight_ = false; // Whether a query is inflight.
this.inFlight_

http://codereview.chromium.org/12418/diff/749/578#newcode205
Line 205: this.requestedPage_ = opt_page ? opt_page : 0;
How about adding a clearModel_ method that does 200-205 and is used here and in
the constructor? The code duplicated messes with my OCD.

http://codereview.chromium.org/12418/diff/749/578#newcode245
Line 245: lastDay = this.pages_[oldLength - 1].dateRelativeDay;
var oldPage = this.pages_[oldLength - 1];
lastURL = oldPage.url;
lastDay = oldPage.dateRelativeDay;

http://codereview.chromium.org/12418/diff/749/578#newcode248
Line 248: for (var i = 0; i < results.length; i++) {
For readability:
var thisResult = results[i];
var thisURL = thisResult.url;
if (...) {
  thisDay = thisResult.day;
  ...
}
}

http://codereview.chromium.org/12418/diff/749/578#newcode283
Line 283: * @param {Number} end The start index
s/start/end

http://codereview.chromium.org/12418/diff/749/578#newcode287
Line 287: if (start >= this.getSize())
What's the case where this would be true?

http://codereview.chromium.org/12418/diff/749/578#newcode327
Line 327: this.searchMonth_ = opt_month ? opt_month : 0;
opt_month || 0 seems fine to me here.

http://codereview.chromium.org/12418/diff/749/578#newcode348
Line 348: return ((page + 1) * RESULTS_PER_PAGE < this.getSize());
return this.haveDataForPage_(page + 1);

?

http://codereview.chromium.org/12418/diff/749/578#newcode365
Line 365: this.page_ = 0;
this.pageIndex_? At first I thought this might be pointing to an instance of
Page.

http://codereview.chromium.org/12418/diff/749/578#newcode378
Line 378: this.page_ = parseInt(opt_page ? opt_page : 0);
opt_page || 0

also, use the parseInt(..., 10) version. Not required, but avoids potential
confusion in the future.

http://codereview.chromium.org/12418/diff/749/578#newcode440
Line 440: } else if (lastTime - thisTime > 5 * 60 * 1000) {
Readability nit: Make a global variable FIVE_MINUTES? Or,
HistoryView.FIVE_MINUTES?

http://codereview.chromium.org/12418/diff/749/578#newcode458
Line 458: HistoryView.prototype.displaySummaryBar_ = function() {
Prefer avoiding the call to this.model_.getSearchText() twice. Also, this needs
to work if searchText is false, undefined, 0, etc. right?

var searchText = this.model_.getSearchText();
this.summaryDiv_.innerHTML = typeof searchText == 'undefined' ?
    localStrings.formatString('searchresultsfor', searchText) :
    localStrings.getString('history');

http://codereview.chromium.org/12418/diff/749/578#newcode473
Line 473: navOutput.push(this.createPageNavHTML_(0,
localStrings.getString('newest')));
80 chars.

http://codereview.chromium.org/12418/diff/749/578#newcode487
Line 487: return ['<a href="#" ',
You don't need the # right? Just href="" should do since you are cancelling the
onclick. Keeps the status bar from showing a hash.

That said, right now we'll show the URL of the current page. Can you put a TODO
to make the status bar show something reasonable?

http://codereview.chromium.org/12418/diff/749/578#newcode490
Line 490: '>', name, '</a>'].join("");
Can you return the non-joined version and just push it into the array in
displayNavBar_? Alternately, the benefit of using array join is of questionable
value for V8. It's of even more questionable value for small strings. 

I'd say that for now, either return the array or don't use an array here and
just use string concat. At some point, we should do a string test in chrome to
see if it matters. My guess is that it doesn't.

http://codereview.chromium.org/12418/diff/749/578#newcode500
Line 500: var state = function() {
Don't hate me, but...class with a global instance here as well?

http://codereview.chromium.org/12418/diff/749/578#newcode511
Line 511: if (this.checker_) {
When would this ever be set?

http://codereview.chromium.org/12418/diff/749/578#newcode516
Line 516: //     public model and view.
Can you just use the model and view variables directly since they are in scope
and would be available via a closure?

http://codereview.chromium.org/12418/diff/749/578#newcode544
Line 544: if (pair.length >= 1) {
Is this if-statement needed? When would this ever be false?

http://codereview.chromium.org/12418/diff/749/578#newcode572
Line 572: var hash = newHash.join("&");
Ditch the array. Use string concat.

http://codereview.chromium.org/12418/diff/749/578#newcode634
Line 634: .main #results-summary {
drop the ".main". it doesn't help disambiguate, right?

http://codereview.chromium.org/12418/diff/749/578#newcode642
Line 642: .main #results-display {
drop the ".main". it doesn't help disambiguate, right?

http://codereview.chromium.org/12418/diff/749/578#newcode690
Line 690: .main #results-pagination {
drop the ".main". it doesn't help disambiguate, right?

http://codereview.chromium.org/12418/diff/749/578#newcode706
Line 706: <a href="#" onclick="historyView.setSearch(''); return false;">
You should probably add a setSearch method as you have setPage above and both of
them should null check historyView since there's a window in which the user
could click before that variable has been initialized.

Even better, register the onclick handler and onsubmit handler below in the load
method.

Ditto above re: getting rid of the #.

[Aaron Boodman, 2008-12-04 04:16:33]

http://codereview.chromium.org/12418/diff/749/578
File chrome/browser/resources/history.html (right):

http://codereview.chromium.org/12418/diff/749/578#newcode154
Line 154: return str.replace(/([\\\.\+\*\?\[\^\]\$\(\)\{\}\=\!\<\>\|\:])/g,
"\\$1");
On 2008/12/04 02:06:57, ojan wrote:
> This is crazyness.

FYI, a less backslash-intensive way to do this would be to construct the regex
from a string:

Page.regexSpecialChars_ = new RegExp("([\\.+*?^...])", "g");
Page.pregQuote_ = function() {
  return str.replace(this.regexSpecialChars_, "\\$0");
}

[Glen Murphy, 2008-12-04 17:51:35]

Chrome ate my original reply. Changes are in, and here's what I remember from my
response:

> http://codereview.chromium.org/12418/diff/749/578#newcode68
> Line 68: this.continued = continued;
> Aren't pretty much all of these properties private?

No, they're used by the result displayer to figure out gaps and date layouts.

> http://codereview.chromium.org/12418/diff/749/578#newcode205
> Line 205: this.requestedPage_ = opt_page ? opt_page : 0;
> How about adding a clearModel_ method that does 200-205 and is used here and
in
> the constructor? The code duplicated messes with my OCD.

Done, but I don't like putting member variable declarations so far away from the
constructor.

> http://codereview.chromium.org/12418/diff/749/578#newcode287
> Line 287: if (start >= this.getSize())
> What's the case where this would be true?

View can request any range it wants.

> http://codereview.chromium.org/12418/diff/749/578#newcode348
> Line 348: return ((page + 1) * RESULTS_PER_PAGE < this.getSize());
> return this.haveDataForPage_(page + 1);

There was a bug here - it's possible to fill a page without having data for the
next page. Changed a <=

> http://codereview.chromium.org/12418/diff/749/578#newcode440
> Line 440: } else if (lastTime - thisTime > 5 * 60 * 1000) {
> Readability nit: Make a global variable FIVE_MINUTES? Or,
> HistoryView.FIVE_MINUTES?

Changed to a global BROWSING_GAP_TIME.

> http://codereview.chromium.org/12418/diff/749/578#newcode487
> Line 487: return ['<a href="#" ',
> You don't need the # right? Just href="" should do since you are cancelling
the
> onclick. Keeps the status bar from showing a hash.
> 
> That said, right now we'll show the URL of the current page. Can you put a
TODO
> to make the status bar show something reasonable?

Changed to use a real URL, though it still SetPage+returns false onclick so that
we don't have to way for the state poller to catch the change.

> http://codereview.chromium.org/12418/diff/749/578#newcode511
> Line 511: if (this.checker_) {
> When would this ever be set?

If state is initialized multiple times (which should be a DCHECK or equivalent,
but I'm letting it continue silently for now).


> http://codereview.chromium.org/12418/diff/749/578#newcode544
> Line 544: if (pair.length >= 1) {
> Is this if-statement needed? When would this ever be false?

Whups. Changed to > 1 (for the ?p&q=1 case)

> http://codereview.chromium.org/12418/diff/749/578#newcode634
> Line 634: .main #results-summary {
> drop the ".main". it doesn't help disambiguate, right?
> 
> http://codereview.chromium.org/12418/diff/749/578#newcode642
> Line 642: .main #results-display {
> drop the ".main". it doesn't help disambiguate, right?
> 
> http://codereview.chromium.org/12418/diff/749/578#newcode690
> Line 690: .main #results-pagination {
> drop the ".main". it doesn't help disambiguate, right?

Changed, but aren't we now polluting the global CSS namespace? I die :p

> Even better, register the onclick handler and onsubmit handler below in the
load
> method.

For later, when HistoryView is more.