Reflecting the changes on bookmarks in History page....

Reflecting the changes on bookmarks in History page.

BUG=8399
TEST=Open a page, open the history page, bookmark the page, see if the history reflects the changes.

[tfarina (gmail-do not use), 2009-07-23 19:09:41]

Glen,

Can you review this to me?

Thanks!

[tfarina (gmail-do not use), 2009-07-23 22:25:56]

On 2009/07/23 19:09:41, tfarina wrote:
> Glen,
> 
> Can you review this to me?
> 
> Thanks!
Hi Mohamed,

Can you review this to me?

Thanks!

[Mohamed Mansour, 2009-07-24 00:36:21]

Hi Thiago, looks great, but have to get the LGTM from Glen :) He would know if
this way is accepted, looks good on my part.

[Mohamed Mansour, 2009-07-24 00:36:46]

Glen can you take a look at this?

[Glen Murphy, 2009-07-24 00:42:04]

I'm not quite sure I understand this CL - it looks like you add an observer to
the bookmarks model, then remove it as soon as the model does anything.

http://codereview.chromium.org/159068/diff/15/1007
File chrome/browser/dom_ui/history_ui.cc (right):

http://codereview.chromium.org/159068/diff/15/1007#newcode358
Line 358: type == NotificationType::URLS_STARRED) {
Why are you calling historyDeleted if a URL is starred?

http://codereview.chromium.org/159068/diff/15/1007#newcode383
Line 383: model_->RemoveObserver(this);
Why all the removes? I don't think you want to remove your observer whenever
anything changes. Also, shouldn't these method bodies just be stubbed out in the
header?

http://codereview.chromium.org/159068/diff/15/1007#newcode420
Line 420: const BookmarkNode* node) {
indentation

[tfarina (gmail-do not use), 2009-07-24 15:18:33]

http://codereview.chromium.org/159068/diff/15/1007
File chrome/browser/dom_ui/history_ui.cc (right):

http://codereview.chromium.org/159068/diff/15/1007#newcode358
Line 358: type == NotificationType::URLS_STARRED) {
On 2009/07/24 00:42:04, Glen Murphy wrote:
> Why are you calling historyDeleted if a URL is starred?
This type is used by BookmarkModel when bookmarks are moved or renamed and added
to notify the observers.

http://codereview.chromium.org/159068/diff/15/1007#newcode383
Line 383: model_->RemoveObserver(this);
On 2009/07/24 00:42:04, Glen Murphy wrote:
> Why all the removes? I don't think you want to remove your observer whenever
> anything changes. Also, shouldn't these method bodies just be stubbed out in
the
> header?

Done.

http://codereview.chromium.org/159068/diff/15/1007#newcode420
Line 420: const BookmarkNode* node) {
On 2009/07/24 00:42:04, Glen Murphy wrote:
> indentation

Done.

[brettw, 2009-07-24 16:18:54]

http://codereview.chromium.org/159068/diff/20/2004
File chrome/browser/dom_ui/history_ui.h (right):

http://codereview.chromium.org/159068/diff/20/2004#newcode65
Line 65: // BookmarkModelObserver implementation.
As far as I can tell all this BookmarkModelObserver stuff is doing nothing. This
is independent of the "NotificationObserver" and the registrar.

[tfarina (gmail-do not use), 2009-07-24 16:56:04]

http://codereview.chromium.org/159068/diff/20/2004
File chrome/browser/dom_ui/history_ui.h (right):

http://codereview.chromium.org/159068/diff/20/2004#newcode65
Line 65: // BookmarkModelObserver implementation.
On 2009/07/24 16:18:55, brettw wrote:
> As far as I can tell all this BookmarkModelObserver stuff is doing nothing.
This
> is independent of the "NotificationObserver" and the registrar.
You are right. This is not needed. Removed.

[tfarina (gmail-do not use), 2009-07-27 21:06:59]

On 2009/07/24 16:56:04, tfarina wrote:
> http://codereview.chromium.org/159068/diff/20/2004
> File chrome/browser/dom_ui/history_ui.h (right):
> 
> http://codereview.chromium.org/159068/diff/20/2004#newcode65
> Line 65: // BookmarkModelObserver implementation.
> On 2009/07/24 16:18:55, brettw wrote:
> > As far as I can tell all this BookmarkModelObserver stuff is doing nothing.
> This
> > is independent of the "NotificationObserver" and the registrar.
> You are right. This is not needed. Removed.
ping

[Glen Murphy, 2009-07-30 17:18:28]

http://codereview.chromium.org/159068/diff/27/2007
File chrome/browser/dom_ui/history_ui.cc (right):

http://codereview.chromium.org/159068/diff/27/2007#newcode355
Line 355: dom_ui_->CallJavascriptFunction(L"reloadList");
I don't think you want to make it reload the page - this will cause the view to
reset to default, which you don't want if you're on anything other than the
first page. The reason we do it for deletions is that large chunks tend to get
deleted at a time, invalidating the page you're looking at.

I think you need to figure out what URLs were starred, send those URLs to the
page, then get it to update the individual items accordingly. This is
significantly harder, and is why we didn't do it in the first place.