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

Issue 892953002: Show icons for custom menuitems in contextmenu.

Created:
5 years, 10 months ago by pals
Modified:
5 years, 10 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Show icons for custom menuitems in contextmenu. BUG=87553

Patch Set 1 : #

Patch Set 2 : #

Total comments: 7

Patch Set 3 : Fixed #

Total comments: 6

Patch Set 4 : Fixed comments #

Total comments: 6

Patch Set 5 : Scaled to 16x16 #

Total comments: 8

Patch Set 6 : Fixed review comments #

Total comments: 2

Patch Set 7 : #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+63 lines, -5 lines) Patch
M chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/renderer_context_menu/spelling_menu_observer_browsertest.cc View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M components/renderer_context_menu/render_view_context_menu_base.h View 1 2 3 4 5 2 chunks +3 lines, -0 lines 0 comments Download
M components/renderer_context_menu/render_view_context_menu_base.cc View 1 2 3 4 5 6 6 chunks +44 lines, -3 lines 1 comment Download
M components/renderer_context_menu/render_view_context_menu_proxy.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M components/renderer_context_menu/views/toolkit_delegate_views.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M components/renderer_context_menu/views/toolkit_delegate_views.cc View 1 2 3 4 5 2 chunks +7 lines, -0 lines 0 comments Download
M content/common/view_messages.h View 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/menu_item.h View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M content/renderer/menu_item_builder.cc View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 33 (14 generated)
pals
Please review.
5 years, 10 months ago (2015-02-03 10:38:44 UTC) #3
lazyboy
https://chromiumcodereview.appspot.com/892953002/diff/40001/components/renderer_context_menu/render_view_context_menu_base.cc File components/renderer_context_menu/render_view_context_menu_base.cc (right): https://chromiumcodereview.appspot.com/892953002/diff/40001/components/renderer_context_menu/render_view_context_menu_base.cc#newcode84 components/renderer_context_menu/render_view_context_menu_base.cc:84: gfx::Image* icon = new gfx::Image(gfx::Image::CreateFrom1xBitmap(bitmaps[0])); |icon| is leaked after ...
5 years, 10 months ago (2015-02-05 01:09:55 UTC) #4
pals
Fixed the comments. Please take a look. https://codereview.chromium.org/892953002/diff/40001/components/renderer_context_menu/render_view_context_menu_base.cc File components/renderer_context_menu/render_view_context_menu_base.cc (right): https://codereview.chromium.org/892953002/diff/40001/components/renderer_context_menu/render_view_context_menu_base.cc#newcode84 components/renderer_context_menu/render_view_context_menu_base.cc:84: gfx::Image* icon ...
5 years, 10 months ago (2015-02-05 09:01:03 UTC) #6
lazyboy
Sorry for the delay, I completely forgot about the CL. https://chromiumcodereview.appspot.com/892953002/diff/80001/components/renderer_context_menu/render_view_context_menu_base.cc File components/renderer_context_menu/render_view_context_menu_base.cc (right): https://chromiumcodereview.appspot.com/892953002/diff/80001/components/renderer_context_menu/render_view_context_menu_base.cc#newcode105 ...
5 years, 10 months ago (2015-02-09 23:45:23 UTC) #7
pals
Fixed the issues. PTAL. https://codereview.chromium.org/892953002/diff/80001/components/renderer_context_menu/render_view_context_menu_base.cc File components/renderer_context_menu/render_view_context_menu_base.cc (right): https://codereview.chromium.org/892953002/diff/80001/components/renderer_context_menu/render_view_context_menu_base.cc#newcode105 components/renderer_context_menu/render_view_context_menu_base.cc:105: icon_map_[download_id] = command_id; On 2015/02/09 ...
5 years, 10 months ago (2015-02-10 12:19:41 UTC) #10
lazyboy
I'm curious to try this out when I get a chance. How does icon render ...
5 years, 10 months ago (2015-02-13 08:10:40 UTC) #11
pals
Ya. Scaled the icon to 16x16 now. https://codereview.chromium.org/892953002/diff/140001/components/renderer_context_menu/render_view_context_menu_base.cc File components/renderer_context_menu/render_view_context_menu_base.cc (right): https://codereview.chromium.org/892953002/diff/140001/components/renderer_context_menu/render_view_context_menu_base.cc#newcode101 components/renderer_context_menu/render_view_context_menu_base.cc:101: false, // ...
5 years, 10 months ago (2015-02-13 09:30:40 UTC) #12
lazyboy
lgtm
5 years, 10 months ago (2015-02-17 18:14:19 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/892953002/160001
5 years, 10 months ago (2015-02-18 05:20:50 UTC) #15
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/43439)
5 years, 10 months ago (2015-02-18 05:25:13 UTC) #17
lazyboy
FYI, you still need OWNERS review for: components/* And security/IPC review for content/common/view_messages.h
5 years, 10 months ago (2015-02-18 05:27:22 UTC) #18
pals
+jochen for components/* +dcheng for content/common/view_messages.h Please review.
5 years, 10 months ago (2015-02-18 05:36:59 UTC) #21
dcheng
https://codereview.chromium.org/892953002/diff/160001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc (right): https://codereview.chromium.org/892953002/diff/160001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc#newcode72 chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc:72: void SetIcon(int command_id, const gfx::Image& icon) override {}; Nit: ...
5 years, 10 months ago (2015-02-18 17:03:27 UTC) #22
jochen (gone - plz use gerrit)
can we have a test for this please? https://codereview.chromium.org/892953002/diff/180001/components/renderer_context_menu/render_view_context_menu_base.cc File components/renderer_context_menu/render_view_context_menu_base.cc (right): https://codereview.chromium.org/892953002/diff/180001/components/renderer_context_menu/render_view_context_menu_base.cc#newcode107 components/renderer_context_menu/render_view_context_menu_base.cc:107: web_contents->DownloadImage( ...
5 years, 10 months ago (2015-02-23 09:56:11 UTC) #24
pals
Fixed the review comments. Please take another look. https://codereview.chromium.org/892953002/diff/160001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc (right): https://codereview.chromium.org/892953002/diff/160001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc#newcode72 chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc:72: void ...
5 years, 10 months ago (2015-02-23 10:23:15 UTC) #28
dcheng
https://codereview.chromium.org/892953002/diff/240001/components/renderer_context_menu/render_view_context_menu_base.cc File components/renderer_context_menu/render_view_context_menu_base.cc (right): https://codereview.chromium.org/892953002/diff/240001/components/renderer_context_menu/render_view_context_menu_base.cc#newcode108 components/renderer_context_menu/render_view_context_menu_base.cc:108: GURL(items[i].icon), No GURL() here, since this is already a ...
5 years, 10 months ago (2015-02-23 16:33:04 UTC) #29
pals
I am finding it difficult to write a test as the icon will be updated ...
5 years, 10 months ago (2015-02-24 05:25:59 UTC) #31
jochen (gone - plz use gerrit)
is it possible to write a test for this CL? https://codereview.chromium.org/892953002/diff/280001/components/renderer_context_menu/render_view_context_menu_base.cc File components/renderer_context_menu/render_view_context_menu_base.cc (right): https://codereview.chromium.org/892953002/diff/280001/components/renderer_context_menu/render_view_context_menu_base.cc#newcode106 ...
5 years, 10 months ago (2015-02-24 10:26:57 UTC) #32
jochen (gone - plz use gerrit)
5 years, 10 months ago (2015-02-24 10:34:03 UTC) #33
just saw your mail that it's difficult to test.

however, without a test, this will just break.

Powered by Google App Engine
This is Rietveld 408576698