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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 10038033: Nukes wrench_menu_model_chromeos and folds functionality int (Closed)

Created:
8 years, 8 months ago by sky
Modified:
8 years, 8 months ago
CC:
chromium-reviews
Visibility:
Public.

Description

Nukes wrench_menu_model_chromeos and folds functionality int wrench_menu_model. Chromeos no longer diverges that much from current. BUG=123233 TEST=make sure menu layout is right. R=saintlou@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=132275

Patch Set 1 #

Patch Set 2 : Fix non-chromeos build #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+33 lines, -95 lines) Patch
M chrome/browser/defaults.h View 1 chunk +5 lines, -1 line 0 comments Download
M chrome/browser/defaults.cc View 2 chunks +9 lines, -1 line 0 comments Download
M chrome/browser/ui/toolbar/wrench_menu_model.cc View 1 4 chunks +19 lines, -6 lines 5 comments Download
D chrome/browser/ui/toolbar/wrench_menu_model_chromeos.cc View 1 chunk +0 lines, -86 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 12 (0 generated)
sky
8 years, 8 months ago (2012-04-12 22:21:08 UTC) #1
Emmanuel Saint-loubert-Bié
lgtm Awesome
8 years, 8 months ago (2012-04-12 23:42:31 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sky@chromium.org/10038033/1
8 years, 8 months ago (2012-04-13 15:41:28 UTC) #3
commit-bot: I haz the power
Try job failure for 10038033-1 (retry) on linux_rel for step "compile" (clobber build). It's a ...
8 years, 8 months ago (2012-04-13 16:15:55 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sky@chromium.org/10038033/15001
8 years, 8 months ago (2012-04-13 16:45:09 UTC) #5
commit-bot: I haz the power
Try job failure for 10038033-15001 on mac_rel for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=17882 Step "update" is always ...
8 years, 8 months ago (2012-04-13 16:47:17 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sky@chromium.org/10038033/15001
8 years, 8 months ago (2012-04-13 19:48:44 UTC) #7
tfarina
http://codereview.chromium.org/10038033/diff/15001/chrome/browser/ui/toolbar/wrench_menu_model.cc File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/10038033/diff/15001/chrome/browser/ui/toolbar/wrench_menu_model.cc#newcode518 chrome/browser/ui/toolbar/wrench_menu_model.cc:518: *ui::ResourceBundle::GetSharedInstance(). nit: hum? could you revert this change? http://codereview.chromium.org/10038033/diff/15001/chrome/browser/ui/toolbar/wrench_menu_model.cc#newcode524 ...
8 years, 8 months ago (2012-04-13 20:13:59 UTC) #8
commit-bot: I haz the power
List of reviewers changed. tfarina@chromium.org did a drive-by without LGTM'ing!
8 years, 8 months ago (2012-04-13 21:16:16 UTC) #9
sky
http://codereview.chromium.org/10038033/diff/15001/chrome/browser/ui/toolbar/wrench_menu_model.cc File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/10038033/diff/15001/chrome/browser/ui/toolbar/wrench_menu_model.cc#newcode518 chrome/browser/ui/toolbar/wrench_menu_model.cc:518: *ui::ResourceBundle::GetSharedInstance(). On 2012/04/13 20:13:59, tfarina wrote: > nit: hum? ...
8 years, 8 months ago (2012-04-13 21:45:09 UTC) #10
tfarina
http://codereview.chromium.org/10038033/diff/15001/chrome/browser/ui/toolbar/wrench_menu_model.cc File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/10038033/diff/15001/chrome/browser/ui/toolbar/wrench_menu_model.cc#newcode518 chrome/browser/ui/toolbar/wrench_menu_model.cc:518: *ui::ResourceBundle::GetSharedInstance(). On 2012/04/13 21:45:09, sky wrote: > On 2012/04/13 ...
8 years, 8 months ago (2012-04-13 22:39:53 UTC) #11
sky
8 years, 8 months ago (2012-04-13 22:54:05 UTC) #12 
http://codereview.chromium.org/10038033/diff/15001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

http://codereview.chromium.org/10038033/diff/15001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/wrench_menu_model.cc:518:
*ui::ResourceBundle::GetSharedInstance().
On 2012/04/13 22:39:53, tfarina wrote:
> On 2012/04/13 21:45:09, sky wrote:
> > On 2012/04/13 20:13:59, tfarina wrote:
> > > nit: hum? could you revert this change?
> > 
> > No, otherwise 524 is going to be problematic.
> OK, I see. So we have two options here:
> 
> Move rb to a upper scope, which I don't see much problem, and avoids calling
> GetSharedInstance twice. Or stay with what you have, but fix the indentation
to
> 4 spaces and fix the indentation of GetBitmapNamed?

You can't move rb to upper scope since it isn't always used. I think the
indentation as I have it is fine, but if you want to change go for it.
I've already landed this.

Powered by Google App Engine
This is Rietveld 408576698