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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1365963004: Don't compile download notification code on !CrOS. (Closed)

Created:
5 years, 3 months ago by Evan Stade
Modified:
5 years, 2 months ago
Reviewers:
asanka, yoshiki, oshima, Dan Beam
CC:
benjhayden+dwatch_chromium.org, chromium-reviews, Randy Smith (Not in Mondays)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't compile download notification code on !CrOS. BUG=535617 TBR=oshima@chromium.org Committed: https://crrev.com/7fc53a3fbecd4fbb0c7d00a7db57960ed702fbc0 Cr-Commit-Position: refs/heads/master@{#351375}

Patch Set 1 #

Patch Set 2 : undo git cl formatting of about flags #

Patch Set 3 : fix comment #

Patch Set 4 : typo fix #

Total comments: 3

Patch Set 5 : images and strings #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+73 lines, -107 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 3 chunks +34 lines, -44 lines 0 comments Download
D chrome/app/theme/default_100_percent/legacy/download_error.png View 1 2 3 4 Binary file 0 comments Download
D chrome/app/theme/default_100_percent/legacy/download_icon.png View 1 2 3 4 Binary file 0 comments Download
D chrome/app/theme/default_100_percent/legacy/download_icon_incognito.png View 1 2 3 4 Binary file 0 comments Download
D chrome/app/theme/default_100_percent/legacy/download_warning_bad.png View 1 2 3 4 Binary file 0 comments Download
D chrome/app/theme/default_100_percent/legacy/download_warning_unwanted.png View 1 2 3 4 Binary file 0 comments Download
D chrome/app/theme/default_200_percent/legacy/download_error.png View 1 2 3 4 Binary file 0 comments Download
D chrome/app/theme/default_200_percent/legacy/download_icon.png View 1 2 3 4 Binary file 0 comments Download
D chrome/app/theme/default_200_percent/legacy/download_icon_incognito.png View 1 2 3 4 Binary file 0 comments Download
D chrome/app/theme/default_200_percent/legacy/download_warning_bad.png View 1 2 3 4 Binary file 0 comments Download
D chrome/app/theme/default_200_percent/legacy/download_warning_unwanted.png View 1 2 3 4 Binary file 0 comments Download
M chrome/app/theme/theme_resources.grd View 1 2 3 4 1 chunk +7 lines, -14 lines 1 comment Download
M chrome/browser/about_flags.cc View 1 2 3 4 4 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/download/download_browsertest.cc View 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/download/download_ui_controller.cc View 1 2 2 chunks +17 lines, -13 lines 0 comments Download
M chrome/browser/download/notification/download_item_notification.h View 3 chunks +0 lines, -8 lines 0 comments Download
M chrome/browser/download/notification/download_item_notification.cc View 1 2 3 4 4 chunks +3 lines, -12 lines 0 comments Download
M chrome/browser/download/notification/download_notification_manager.cc View 1 chunk +0 lines, -5 lines 0 comments Download
M chrome/browser/download/save_page_browsertest.cc View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 2 chunks +6 lines, -6 lines 0 comments Download

Messages

Total messages: 34 (11 generated)
Evan Stade
5 years, 3 months ago (2015-09-24 18:38:13 UTC) #2
yoshiki
https://codereview.chromium.org/1365963004/diff/60001/chrome/browser/download/download_ui_controller.cc File chrome/browser/download/download_ui_controller.cc (right): https://codereview.chromium.org/1365963004/diff/60001/chrome/browser/download/download_ui_controller.cc#newcode119 chrome/browser/download/download_ui_controller.cc:119: Profile::FromBrowserContext(manager->GetBrowserContext()))); Please set |DownloadShelfUIControllerDelegate| to the delegate if download ...
5 years, 2 months ago (2015-09-25 18:14:35 UTC) #4
Evan Stade
https://codereview.chromium.org/1365963004/diff/60001/chrome/browser/download/download_ui_controller.cc File chrome/browser/download/download_ui_controller.cc (right): https://codereview.chromium.org/1365963004/diff/60001/chrome/browser/download/download_ui_controller.cc#newcode119 chrome/browser/download/download_ui_controller.cc:119: Profile::FromBrowserContext(manager->GetBrowserContext()))); On 2015/09/25 18:14:35, yoshiki wrote: > Please set ...
5 years, 2 months ago (2015-09-25 19:06:54 UTC) #5
yoshiki
https://codereview.chromium.org/1365963004/diff/60001/chrome/browser/download/download_ui_controller.cc File chrome/browser/download/download_ui_controller.cc (right): https://codereview.chromium.org/1365963004/diff/60001/chrome/browser/download/download_ui_controller.cc#newcode119 chrome/browser/download/download_ui_controller.cc:119: Profile::FromBrowserContext(manager->GetBrowserContext()))); On 2015/09/25 19:06:53, Evan Stade wrote: > On ...
5 years, 2 months ago (2015-09-25 23:35:31 UTC) #6
asanka
This is going to orphan a bunch of strings on non-ChromeOS. Shall we conditionalize those ...
5 years, 2 months ago (2015-09-25 23:49:24 UTC) #8
Evan Stade
On 2015/09/25 23:49:24, asanka wrote: > This is going to orphan a bunch of strings ...
5 years, 2 months ago (2015-09-25 23:54:12 UTC) #9
Randy Smith (Not in Mondays)
I'm going to defer to Asanka on this CL.
5 years, 2 months ago (2015-09-28 16:45:55 UTC) #10
Evan Stade
culled/coralled some assets and strings.
5 years, 2 months ago (2015-09-28 17:18:16 UTC) #13
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1365963004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1365963004/80001
5 years, 2 months ago (2015-09-28 17:18:18 UTC) #14
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 2 months ago (2015-09-28 19:02:23 UTC) #16
asanka
Were the PNGs unused? The code looks good, but I'm still waiting on Hodie for ...
5 years, 2 months ago (2015-09-29 04:50:22 UTC) #17
asanka
On 2015/09/29 at 04:50:22, asanka wrote: > Were the PNGs unused? > > The code ...
5 years, 2 months ago (2015-09-29 14:41:08 UTC) #18
Evan Stade
On 2015/09/29 04:50:22, asanka wrote: > Were the PNGs unused? on cros, yes, but only ...
5 years, 2 months ago (2015-09-29 17:27:18 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1365963004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1365963004/80001
5 years, 2 months ago (2015-09-29 17:28:09 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/104789)
5 years, 2 months ago (2015-09-29 17:47:58 UTC) #23
Evan Stade
TBR oshima for removal of pngs
5 years, 2 months ago (2015-09-29 18:55:05 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1365963004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1365963004/80001
5 years, 2 months ago (2015-09-29 18:56:00 UTC) #27
oshima
lgtm
5 years, 2 months ago (2015-09-29 19:08:28 UTC) #28
commit-bot: I haz the power
Committed patchset #5 (id:80001)
5 years, 2 months ago (2015-09-29 19:44:59 UTC) #29
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/7fc53a3fbecd4fbb0c7d00a7db57960ed702fbc0 Cr-Commit-Position: refs/heads/master@{#351375}
5 years, 2 months ago (2015-09-29 19:46:23 UTC) #30
Dan Beam
https://codereview.chromium.org/1365963004/diff/80001/chrome/app/theme/theme_resources.grd File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/1365963004/diff/80001/chrome/app/theme/theme_resources.grd#newcode252 chrome/app/theme/theme_resources.grd:252: <structure type="chrome_scaled_image" name="IDR_DOWNLOAD_PROGRESS_FOREGROUND_32" file="common/download_progress_foreground32.png" /> ruh roh
5 years, 2 months ago (2015-10-06 16:13:04 UTC) #32
Evan Stade
On 2015/10/06 16:13:04, Dan Beam wrote: > https://codereview.chromium.org/1365963004/diff/80001/chrome/app/theme/theme_resources.grd > File chrome/app/theme/theme_resources.grd (right): > > https://codereview.chromium.org/1365963004/diff/80001/chrome/app/theme/theme_resources.grd#newcode252 ...
5 years, 2 months ago (2015-10-06 18:23:39 UTC) #33
Dan Beam
5 years, 2 months ago (2015-10-06 18:28:16 UTC) #34
Message was sent while issue was closed. 
On 2015/10/06 18:23:39, Evan Stade wrote:
> On 2015/10/06 16:13:04, Dan Beam wrote:
> >
>
https://codereview.chromium.org/1365963004/diff/80001/chrome/app/theme/theme_...
> > File chrome/app/theme/theme_resources.grd (right):
> > 
> >
>
https://codereview.chromium.org/1365963004/diff/80001/chrome/app/theme/theme_...
> > chrome/app/theme/theme_resources.grd:252: <structure
> type="chrome_scaled_image"
> > name="IDR_DOWNLOAD_PROGRESS_FOREGROUND_32"
> > file="common/download_progress_foreground32.png" />
> > ruh roh
> 
> whoops. do you have a fix ready or shall I prepare one?

https://codereview.chromium.org/1387203003

Powered by Google App Engine
This is Rietveld 408576698