Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 28104: Enable history and downloads by default, port NewTabUI from DOMUIHost to DOMU... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 6 months ago by Glen Murphy
Modified:
6 years, 2 months ago
CC:
chromium-reviews_googlegroups.com, brettw
Visibility:
Public.

Description

Enable history and downloads by default, port NewTabUI from DOMUIHost to DOMUI. NewTabUI is only included on OS(WIN) because DOMUI doesn't appear to have been ported. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=10344

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Patch Set 7 : '' #

Patch Set 8 : '' #

Total comments: 15

Patch Set 9 : '' #

Patch Set 10 : '' #

Patch Set 11 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+298 lines, -254 lines) Patch
M chrome/browser/browser.cc View 1 2 3 4 5 6 7 8 9 10 7 chunks +12 lines, -8 lines 0 comments Download
M chrome/browser/browser_url_handler.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +0 lines, -4 lines 0 comments Download
M chrome/browser/debugger/debugger_contents.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +11 lines, -6 lines 0 comments Download
M chrome/browser/debugger/debugger_contents.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +17 lines, -7 lines 0 comments Download
M chrome/browser/dom_ui/dom_ui.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +15 lines, -2 lines 0 comments Download
M chrome/browser/dom_ui/dom_ui.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/dom_ui/dom_ui_contents.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +11 lines, -0 lines 0 comments Download
M chrome/browser/dom_ui/dom_ui_contents.cc View 1 2 3 4 5 6 7 8 9 10 6 chunks +53 lines, -22 lines 0 comments Download
M chrome/browser/dom_ui/dom_ui_host.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -14 lines 0 comments Download
M chrome/browser/dom_ui/history_ui.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/dom_ui/history_ui.cc View 1 2 3 4 5 6 7 8 9 10 8 chunks +17 lines, -8 lines 0 comments Download
M chrome/browser/dom_ui/new_tab_ui.h View 1 2 3 4 5 6 7 8 9 10 15 chunks +32 lines, -41 lines 0 comments Download
M chrome/browser/dom_ui/new_tab_ui.cc View 1 2 3 4 5 6 7 8 9 10 25 chunks +80 lines, -102 lines 0 comments Download
M chrome/browser/resources/history.html View 1 2 3 4 5 6 7 8 9 10 15 chunks +30 lines, -23 lines 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_blocking_page.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/tab_contents/tab_contents_factory.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +1 line, -5 lines 0 comments Download
M chrome/browser/tabs/tab_strip_model_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/views/download_shelf_view.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/views/options/general_page_view.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -1 line 0 comments Download
M chrome/common/temp_scaffolding_stubs.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +1 line, -3 lines 0 comments Download
M chrome/common/temp_scaffolding_stubs.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -4 lines 0 comments Download
Trybot results:
Commit queue not available (can’t edit this change).

Messages

Total messages: 4 (0 generated)
Glen Murphy
8 years, 6 months ago (2009-02-25 02:47:13 UTC) #1
Ben Goodger (Google)
http://codereview.chromium.org/28104/diff/112/118 File chrome/browser/browser.cc (right): http://codereview.chromium.org/28104/diff/112/118#newcode963 Line 963: NULL); nit: indent to ( http://codereview.chromium.org/28104/diff/112/118#newcode976 Line 976: ...
8 years, 6 months ago (2009-02-25 02:57:27 UTC) #2
Ben Goodger (Google)
This seems OK to me. One request I do have is that you put together ...
8 years, 6 months ago (2009-02-25 03:05:39 UTC) #3
pink (ping after 24hrs)
8 years, 6 months ago (2009-02-25 15:42:00 UTC) #4
I think this should probably work now on POSIX w/out the new_tab_ui ifdefs. If
you want to make those changes and give it a shot on the try server (or tell me
when and i'll apply the patch on mac), let me know.

Mac&Linux are still building dom_ui_host.cc, so you should remove those in a
follow-up CL. File a bug so you don't forget?

http://codereview.chromium.org/28104/diff/112/118
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/28104/diff/112/118#newcode961
Line 961: GURL downloads_url = HistoryUI::GetBaseURL();
why would the history url be put a variable named |downloads_url|? Cut and paste
error?

http://codereview.chromium.org/28104/diff/112/131
File chrome/browser/dom_ui/dom_ui_contents.cc (right):

http://codereview.chromium.org/28104/diff/112/131#newcode15
Line 15: #if defined(OS_WIN)
probably not necessary anymore since new_tab_ui compiles on the trunk.

http://codereview.chromium.org/28104/diff/112/131#newcode244
Line 244: #if defined(OS_WIN)
this would probably work, but if not for some reason, make sure you add a
TODO(PORT) comment and a NOTIMPLEMENTED(); so we can find it later.

http://codereview.chromium.org/28104/diff/112/119
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/28104/diff/112/119#newcode27
Line 27: #include "chrome/browser/dom_ui/new_tab_ui.h"
this would probably work on posix.

http://codereview.chromium.org/28104/diff/112/117
File chrome/browser/tab_contents/tab_contents_factory.cc (right):

http://codereview.chromium.org/28104/diff/112/117#newcode19
Line 19: #include "chrome/browser/dom_ui/new_tab_ui.h"
this will probably work on posix.

http://codereview.chromium.org/28104/diff/112/117#newcode68
Line 68: case TAB_CONTENTS_NEW_TAB_UI:
not sure if we're hooked up for this to work, but it might. maybe do this in
stages where if we can get everything landed, we can go in with a round-two and
pull this out of the define?

http://codereview.chromium.org/28104/diff/112/132
File chrome/common/temp_scaffolding_stubs.h (right):

http://codereview.chromium.org/28104/diff/112/132#newcode689
Line 689: class NewTabUI {
might not be necessary.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b