Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(6)

Issue 27106: Get save page working on posix. (Closed)

Created:
11 years, 10 months ago by Evan Stade
Modified:
9 years, 6 months ago
Reviewers:
tony, brettw
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Get save page working on posix. * rearrange some IFDEFs * trim whitespace on suggest name Also fix a relatively new bug in DomSerializer. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=10314

Patch Set 1 #

Total comments: 2

Patch Set 2 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -18 lines) Patch
M chrome/browser/browser.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/browser.cc View 3 chunks +6 lines, -6 lines 0 comments Download
M chrome/browser/download/save_package.cc View 1 7 chunks +4 lines, -8 lines 0 comments Download
M webkit/glue/dom_serializer.cc View 2 chunks +6 lines, -3 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
Evan Stade
note that trimming the whitespace on the suggested file name will not affect windows (except ...
11 years, 10 months ago (2009-02-24 22:58:45 UTC) #1
brettw
LGTM with one change http://codereview.chromium.org/27106/diff/1/4 File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/27106/diff/1/4#newcode936 Line 936: DCHECK(download_); I don't think ...
11 years, 10 months ago (2009-02-24 23:06:12 UTC) #2
tony
LGTM2. On 2009/02/24 23:06:12, brettw wrote: > LGTM with one change > > http://codereview.chromium.org/27106/diff/1/4 > ...
11 years, 10 months ago (2009-02-24 23:07:24 UTC) #3
Evan Stade
11 years, 10 months ago (2009-02-24 23:19:36 UTC) #4
http://codereview.chromium.org/27106/diff/1/4
File chrome/browser/download/save_package.cc (right):

http://codereview.chromium.org/27106/diff/1/4#newcode936
Line 936: DCHECK(download_);
On 2009/02/24 23:06:12, brettw wrote:
> I don't think its useful to DCHECK for NULL pointers in cases like this, since
> it will immediately crash on the next line with a really obvious NULL pointer
> exception even in release mode. So I would remove this addition.

I added it for consistency because there are tons of places in this file where
we DCHECK(download_) right before dereferencing it. Shall I remove the other
DCHECKS as well or just this one?

Powered by Google App Engine
This is Rietveld 408576698