In pdf context menu 'Print..' is coming 2 times.

In the OOP PDF context menu 'Print..' is being shown 2 times

When we launch pdf with --out-of-process-pdf --enable-mime-handler-view
and then select a word and then right click outside selection
then context menu has 'Print..' 2 times.
IDC_PRINT is getting added once as part of AppendPluginItem()
and again as part of AppendPrintItem().

BUG=420954
Test=
1.launch chrome browser with --out-of-process-pdf --enable-mime-handler-view options.
2.Open a PDF and select some word.
3.Then right click outside selection.
Then we will see 2 |Print..| context menu items.

Committed: https://crrev.com/b3115e6252af69ca0aca60ed8b1334d46561b2b7
Cr-Commit-Position: refs/heads/master@{#298648}

[Deepak, 2014-10-07 08:04:59]

deepak.m1@samsung.com changed reviewers:
+ lazyboy@chromium.org, raymes@chromium.org

[Deepak, 2014-10-07 08:05:00]

PTAL

[lazyboy, 2014-10-07 15:32:24]

I'd change the CL description a bit, this is not specific to out-of-process-pdf.
It happens regardless, right? So don't mention out-of-process-pdf and
enable-mime-handler-view in the CL description.

Also some description should move under
TEST=
section, where you describe how to test the change, this is to help someone
who's trying to verify the change without knowing the code.

https://chromiumcodereview.appspot.com/628403003/diff/20001/chrome/browser/re...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://chromiumcodereview.appspot.com/628403003/diff/20001/chrome/browser/re...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:831:
menu_model_.GetIndexOfCommandId(IDC_PRINT) == -1) {
It's unfortunate that we have 3 places that decides whether to add print items
:(
Hopefully we can clean this up later.
Can you add a note here saying AppendPluginItems() could already add IDC_PRINT
so we are skipping here.

[Deepak, 2014-10-07 15:57:01]

On 2014/10/07 15:32:24, lazyboy wrote:
> I'd change the CL description a bit, this is not specific to
out-of-process-pdf.
> It happens regardless, right? So don't mention out-of-process-pdf and
> enable-mime-handler-view in the CL description.
> 
> Also some description should move under
> TEST=
> section, where you describe how to test the change, this is to help someone
> who's trying to verify the change without knowing the code.
> 
>
https://chromiumcodereview.appspot.com/628403003/diff/20001/chrome/browser/re...
> File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):
> 
>
https://chromiumcodereview.appspot.com/628403003/diff/20001/chrome/browser/re...
> chrome/browser/renderer_context_menu/render_view_context_menu.cc:831:
> menu_model_.GetIndexOfCommandId(IDC_PRINT) == -1) {
> It's unfortunate that we have 3 places that decides whether to add print items
> :(
> Hopefully we can clean this up later.
> Can you add a note here saying AppendPluginItems() could already add IDC_PRINT
> so we are skipping here.

yes, I agree, I will do cleanup in separate issue, For this I have added
comments as suggested.
PTAL

[Deepak, 2014-10-07 15:57:11]

PTAL

https://codereview.chromium.org/628403003/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/628403003/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:831:
menu_model_.GetIndexOfCommandId(IDC_PRINT) == -1) {
On 2014/10/07 15:32:24, lazyboy wrote:
> It's unfortunate that we have 3 places that decides whether to add print items
> :(
> Hopefully we can clean this up later.
> Can you add a note here saying AppendPluginItems() could already add IDC_PRINT
> so we are skipping here.

Done.

[lazyboy, 2014-10-07 16:23:38]

lgtm

Please also change the description of the CL
Add the following section after BUG=

Test=Open a PDF and select .... (add how to make sure this change works as
intended after this CL lands).

https://codereview.chromium.org/628403003/diff/40001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/628403003/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:829: // here
if.|IDC_PRINT| is already present in context menu.
nit: space before |IDC_PRINT|

[Deepak, 2014-10-07 16:33:20]

All changes done as suggested.
Thanks

https://codereview.chromium.org/628403003/diff/40001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/628403003/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:829: // here
if.|IDC_PRINT| is already present in context menu.
On 2014/10/07 16:23:38, lazyboy wrote:
> nit: space before |IDC_PRINT|

Done.

[Deepak, 2014-10-07 16:33:25]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2014-10-07 16:34:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/628403003/60001

[raymes, 2014-10-07 17:09:06]

The CQ bit was unchecked by raymes@chromium.org

[raymes, 2014-10-07 17:09:14]

https://codereview.chromium.org/628403003/diff/60001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/628403003/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:829: // here if
|IDC_PRINT| is already present in context menu.
nit: 
// AppendPluginItems() could have already added |IDC_PRINT| so we are skipping
// it here if |IDC_PRINT| is already present in the context menu.

[Deepak, 2014-10-08 02:43:44]

On 2014/10/07 17:09:14, raymes wrote:
>
https://codereview.chromium.org/628403003/diff/60001/chrome/browser/renderer_...
> File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):
> 
>
https://codereview.chromium.org/628403003/diff/60001/chrome/browser/renderer_...
> chrome/browser/renderer_context_menu/render_view_context_menu.cc:829: // here
if
> |IDC_PRINT| is already present in context menu.
> nit: 
> // AppendPluginItems() could have already added |IDC_PRINT| so we are skipping
> // it here if |IDC_PRINT| is already present in the context menu.

nit addressed.
Thanks

[Deepak, 2014-10-08 02:43:49]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2014-10-08 02:44:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/628403003/80001

[commit-bot: I haz the power, 2014-10-08 03:33:35]

Committed patchset #5 (id:80001) as 711b255d37814790d28120be84b497b6a51109bb

[commit-bot: I haz the power, 2014-10-08 03:34:19]

Patchset 5 (id:??) landed as
https://crrev.com/b3115e6252af69ca0aca60ed8b1334d46561b2b7
Cr-Commit-Position: refs/heads/master@{#298648}