|Print..| menu should be consistent in contextmenu with and without selection.

|Print..| menu should be consistent in contextmenu with and without selection.

|Print..| menu item is coming before the |Roatation Clockwise| and
|Rotation counterclockwise| when we right click and no word in selected.
But comes after the Rotation items when some word is selected.
Before we are adding the Rotation menu items we should check and add
|Print..|Changes done for adding |Print..|earlier than Rotation menu items.

BUG=420991
Committed: https://crrev.com/57cd2b4e34ec3e480f4c5d19d8719e197c081684
Cr-Commit-Position: refs/heads/master@{#299665}

[Deepak, 2014-10-07 09:18:44]

deepak.m1@samsung.com changed reviewers:
+ avi@chromium.org

[Deepak, 2014-10-07 09:18:45]

PTAL

[Avi (use Gerrit), 2014-10-07 14:28:44]

On 2014/10/07 09:18:45, Deepak wrote:
> PTAL

Commenting on the bug.

[Deepak, 2014-10-07 14:47:13]

On 2014/10/07 14:28:44, Avi wrote:
> On 2014/10/07 09:18:45, Deepak wrote:
> > PTAL
> 
> Commenting on the bug.

Thanks @avi for review.
If I understand correctly then it should be:

If we have a selection we should see:

html: copy search print | inspect ( | native services on the mac)
pdf: copy search print  | rotatecw rotateccw  | inspect ( | native services on
the mac)

right!.
Please let me know your opinion so that I will go ahead.

[Deepak, 2014-10-08 07:22:32]

On 2014/10/07 14:47:13, Deepak wrote:
> On 2014/10/07 14:28:44, Avi wrote:
> > On 2014/10/07 09:18:45, Deepak wrote:
> > > PTAL
> > 
> > Commenting on the bug.
> 
> Thanks @avi for review.
> If I understand correctly then it should be:
> 
> If we have a selection we should see:
> 
> html: copy search print | inspect ( | native services on the mac)
> pdf: copy search print  | rotatecw rotateccw  | inspect ( | native services on
> the mac)
> 
> right!.
> Please let me know your opinion so that I will go ahead.

@avi 
I have done changes as you suggested. After my changes it is like.

1) Now when I launch the browser then 

If we does not have selection:
html: back forward reload | saveas print translate viewsource viewinfo | inspect
pdf: back forward reload | saveas print translate viewsource viewinfo | rotatecw
rotateccw | inspect

If we have selection:
html: copy search print | inspect ( | native services on the mac)
pdf:  copy search print | rotatecw rotateccw | inspect ( | native services on
the mac)

2) When I launch the browser with --out-of-process-pdf
--enable-mime-handler-view

If we does not have selection:
html: back forward reload | saveas print translate viewsource viewinfo | inspect
pdf:  saveas print| rotatecw rotateccw | inspect

If we have selection:
html: copy search print | inspect ( | native services on the mac)
pdf:  saveas copy search print | rotatecw rotateccw | inspect ( | native
services on the mac)

Please let me know your opinion for this behavior.
Thanks.

[Avi (use Gerrit), 2014-10-08 12:37:38]

On 2014/10/08 07:22:32, Deepak wrote:
> On 2014/10/07 14:47:13, Deepak wrote:
> > On 2014/10/07 14:28:44, Avi wrote:
> > > On 2014/10/07 09:18:45, Deepak wrote:
> > > > PTAL
> > > 
> > > Commenting on the bug.
> > 
> > Thanks @avi for review.
> > If I understand correctly then it should be:
> > 
> > If we have a selection we should see:
> > 
> > html: copy search print | inspect ( | native services on the mac)
> > pdf: copy search print  | rotatecw rotateccw  | inspect ( | native services
on
> > the mac)
> > 
> > right!.
> > Please let me know your opinion so that I will go ahead.
> 
> @avi 
> I have done changes as you suggested. After my changes it is like.
> 
> 1) Now when I launch the browser then 
> 
> If we does not have selection:
> html: back forward reload | saveas print translate viewsource viewinfo |
inspect
> pdf: back forward reload | saveas print translate viewsource viewinfo |
rotatecw
> rotateccw | inspect
> 
> If we have selection:
> html: copy search print | inspect ( | native services on the mac)
> pdf:  copy search print | rotatecw rotateccw | inspect ( | native services on
> the mac)
> 
> 2) When I launch the browser with --out-of-process-pdf
> --enable-mime-handler-view
> 
> If we does not have selection:
> html: back forward reload | saveas print translate viewsource viewinfo |
inspect
> pdf:  saveas print| rotatecw rotateccw | inspect
> 
> If we have selection:
> html: copy search print | inspect ( | native services on the mac)
> pdf:  saveas copy search print | rotatecw rotateccw | inspect ( | native
> services on the mac)
> 
> Please let me know your opinion for this behavior.
> Thanks.

I haven't yet looked at the code yet, but that sounds a lot better. I'm not
happy that the order differs so much between in-process and out-of-process PDF,
but that's not your fault :)

[Avi (use Gerrit), 2014-10-08 12:42:48]

avi@chromium.org changed reviewers:
+ raymes@chromium.org

[Avi (use Gerrit), 2014-10-08 12:42:49]

Adding raymes for his thoughts as he is pdf plugin master.

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

https://codereview.chromium.org/632243002/diff/30003/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:769: // context
menu items.
:\ When you return bool from a function it means success/no or maybe the
accessor of a feature. When I see bool returned here, it says to me "did this
function succeed at appending the plugin items?" which is not what it means. If
I were going this route I'd return a bool as a named parameter.

https://codereview.chromium.org/632243002/diff/30003/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:786: return
true;
Is there a reason why we need the boolean at all? Can we move this if() inside
of AppendRotationItems?

[Deepak, 2014-10-08 13:38:26]

Changes done as suggested.Thanks
PTAL

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

https://codereview.chromium.org/632243002/diff/30003/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:769: // context
menu items.
On 2014/10/08 12:42:48, Avi wrote:
> :\ When you return bool from a function it means success/no or maybe the
> accessor of a feature. When I see bool returned here, it says to me "did this
> function succeed at appending the plugin items?" which is not what it means.
If
> I were going this route I'd return a bool as a named parameter.

Done.

https://codereview.chromium.org/632243002/diff/30003/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:786: return
true;
On 2014/10/08 12:42:48, Avi wrote:
> Is there a reason why we need the boolean at all? Can we move this if() inside
> of AppendRotationItems?

Done.

[Avi (use Gerrit), 2014-10-08 16:16:47]

So I'm pretty happy with the code as code. I still want raymes's thoughts as
this touches his area.

[Deepak, 2014-10-09 02:50:48]

On 2014/10/08 16:16:47, Avi wrote:
> So I'm pretty happy with the code as code. I still want raymes's thoughts as
> this touches his area.

Thanks Avi.
@raymes, Please let us know your opinion for this change.

[raymes, 2014-10-09 20:29:09]

Thanks for taking a look at this.

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

https://codereview.chromium.org/632243002/diff/70001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:780: if
(!content_type_->SupportsGroup(ContextMenuContentType::ITEM_GROUP_PRINT))
I don't fully understand this bit. Why do we add an extra condition in here?

[Deepak, 2014-10-10 04:53:37]

On 2014/10/09 20:29:09, raymes wrote:
> Thanks for taking a look at this.
> 
>
https://codereview.chromium.org/632243002/diff/70001/chrome/browser/renderer_...
> File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):
> 
>
https://codereview.chromium.org/632243002/diff/70001/chrome/browser/renderer_...
> chrome/browser/renderer_context_menu/render_view_context_menu.cc:780: if
> (!content_type_->SupportsGroup(ContextMenuContentType::ITEM_GROUP_PRINT))
> I don't fully understand this bit. Why do we add an extra condition in here?

When we launch the browser with "Out Of Process" flags and we does not have
selection , then we got context menu items as:
html: back forward reload | saveas print translate viewsource viewinfo |inspect
pdf:  saveas print| rotatecw rotateccw | inspect

as content_type_->SupportsGroup() does not have
ContextMenuContentType::ITEM_GROUP_PRINT. So |Print| will not added from
AppendPrintItem().
Here in the AppendPluginItems() we are adding |Print|, as |Print| is their in
normal html page when their is no selection.

If I am not adding |Print| here then we will not have the |Print| in the context
menu while their is no selection in pdf and browser launched with OOP flags but
html page will have.
So to make similarity, we should have this check.

[raymes, 2014-10-10 19:52:26]

Prior to this CL we used to always add "Print" to the context menu (there was no
extra condition). Why do we need to add this condition now?

Sorry I'm a bit slow on this because I don't know this code at all :(

Also note that the order isn't differing because it is out-of-process per-se but
I think it differs as a result of the fact that it is an embedded plugin. Could
you verify that? I think we should avoid distinguishing between in-process vs
out of process in the code/comments where possible. I think in this case it's
better to talk about the embedded vs un-embedded case.

[Deepak, 2014-10-11 07:00:02]

On 2014/10/10 19:52:26, raymes wrote:
> Prior to this CL we used to always add "Print" to the context menu (there was
no
> extra condition). Why do we need to add this condition now?
> 
> Sorry I'm a bit slow on this because I don't know this code at all :(
> 
> Also note that the order isn't differing because it is out-of-process per-se
but
> I think it differs as a result of the fact that it is an embedded plugin.
Could
> you verify that? I think we should avoid distinguishing between in-process vs
> out of process in the code/comments where possible. I think in this case it's
> better to talk about the embedded vs un-embedded case.

ok, @raymes, I am explaining in more detail now.Launching browser with OOP flags
that is un-embedded case,

I agree that earlier we are adding |Print| in the AppendPluginItems() but that
is not good, this is due to 2 reasons.
1) When we add |Print.| in AppendPluginItems() then later we are also calling
AppendPrintItem(), and that is adding |Print.| 2 times.
   I have already put a check for avoiding this in AppendPrintItem().
2) we want menu item sequence when their is selection in pdf case almost same as
html case.
html : copy search print|inspect
pdf  : SaveAs copy search print |rotatecw rotateccw|inspect

So earlier when we are adding |Print.| in AppendPluginItems() along with |Save
As.| then it will become.
html : copy search print|inspect
pdf  : SaveAs print copy search |rotatecw rotateccw|inspect

that is wrong, as |Print| should be after |copy search |. So I am not adding
|Print.| in AppendPluginItems(), when it will get added as a part of
AppendPrintItem(), that get called after AppendPluginItems() as
|content_type_->SupportsGroup(ContextMenuContentType::ITEM_GROUP_PRINT)| is
true.


And When we does not have any selection, Then 

html:back forward reload|saveas print translate viewpagesource
viewpageinfo|inspect
pdf :saveas print|rotatecw rotateccw|inspect

here we should add |Print| in AppendPluginItems() as we have no selection and
|copy search print| will not be getting added later.
And as |content_type_->SupportsGroup(ContextMenuContentType::ITEM_GROUP_PRINT)|
is false, that confirm that no print will be added later.

Above is the whole explanation & reason for my changes.
I am changing the comments for OOP to un-embedded one.
PTAL.

[raymes, 2014-10-13 16:53:29]

Thanks for explanation :) Please see comments inline.

On 2014/10/11 07:00:02, Deepak wrote:
> On 2014/10/10 19:52:26, raymes wrote:
> > Prior to this CL we used to always add "Print" to the context menu (there
was
> no
> > extra condition). Why do we need to add this condition now?
> > 
> > Sorry I'm a bit slow on this because I don't know this code at all :(
> > 
> > Also note that the order isn't differing because it is out-of-process per-se
> but
> > I think it differs as a result of the fact that it is an embedded plugin.
> Could
> > you verify that? I think we should avoid distinguishing between in-process
vs
> > out of process in the code/comments where possible. I think in this case
it's
> > better to talk about the embedded vs un-embedded case.
> 
> ok, @raymes, I am explaining in more detail now.Launching browser with OOP
flags
> that is un-embedded case,

With the out of process plugin, even though the PDF viewer is opened at the top
level, the pepper plugin is still embedded. I'm guessing that this is what is
causing the difference in ordering, not the fact that it is running
out-of-process. The context menu shouldn't be able to tell the difference
between an in-process and out-of-process plugin. (Take a look at how we always
create an object tag in:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
) 

My guess is that you can reproduce this bug with an embedded plugin running
in-process.

> 
> I agree that earlier we are adding |Print| in the AppendPluginItems() but that
> is not good, this is due to 2 reasons.
> 1) When we add |Print.| in AppendPluginItems() then later we are also calling
> AppendPrintItem(), and that is adding |Print.| 2 times.
>    I have already put a check for avoiding this in AppendPrintItem().

Does that mean that we can remove that check now and instead rely on this check?

> 2) we want menu item sequence when their is selection in pdf case almost same
as
> html case.
> html : copy search print|inspect
> pdf  : SaveAs copy search print |rotatecw rotateccw|inspect
> 
> So earlier when we are adding |Print.| in AppendPluginItems() along with |Save
> As.| then it will become.
> html : copy search print|inspect
> pdf  : SaveAs print copy search |rotatecw rotateccw|inspect
> 
> that is wrong, as |Print| should be after |copy search |. So I am not adding
> |Print.| in AppendPluginItems(), when it will get added as a part of
> AppendPrintItem(), that get called after AppendPluginItems() as
> |content_type_->SupportsGroup(ContextMenuContentType::ITEM_GROUP_PRINT)| is
> true.
> 
> 
> And When we does not have any selection, Then 
> 
> html:back forward reload|saveas print translate viewpagesource
> viewpageinfo|inspect
> pdf :saveas print|rotatecw rotateccw|inspect
> 
> here we should add |Print| in AppendPluginItems() as we have no selection and
> |copy search print| will not be getting added later.
> And as
|content_type_->SupportsGroup(ContextMenuContentType::ITEM_GROUP_PRINT)|
> is false, that confirm that no print will be added later.
> 
> Above is the whole explanation & reason for my changes.
> I am changing the comments for OOP to un-embedded one.
> PTAL.

Ok I think I understand. Please see me inline comment in the code.

https://codereview.chromium.org/632243002/diff/140001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/632243002/diff/140001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:780: // added
by |AppendPrintItem|function call.
Would it be accurate to reword the comment as follows?

// The "Print" menu item should always be included for plugins. If 
// content_type_->SupportsGroup(ContextMenuContentType::ITEM_GROUP_PRINT) is,
// true the item will be added inside AppendPrintItem(). Otherwise we add
"Print" here.

[Deepak, 2014-10-14 04:08:16]

On 2014/10/13 16:53:29, raymes wrote:
> Thanks for explanation :) Please see comments inline.
> 
> On 2014/10/11 07:00:02, Deepak wrote:
> > On 2014/10/10 19:52:26, raymes wrote:
> > > Prior to this CL we used to always add "Print" to the context menu (there
> was
> > no
> > > extra condition). Why do we need to add this condition now?
> > > 
> > > Sorry I'm a bit slow on this because I don't know this code at all :(
> > > 
> > > Also note that the order isn't differing because it is out-of-process
per-se
> > but
> > > I think it differs as a result of the fact that it is an embedded plugin.
> > Could
> > > you verify that? I think we should avoid distinguishing between in-process
> vs
> > > out of process in the code/comments where possible. I think in this case
> it's
> > > better to talk about the embedded vs un-embedded case.
> > 
> > ok, @raymes, I am explaining in more detail now.Launching browser with OOP
> flags
> > that is un-embedded case,
> 
> With the out of process plugin, even though the PDF viewer is opened at the
top
> level, the pepper plugin is still embedded. I'm guessing that this is what is
> causing the difference in ordering, not the fact that it is running
> out-of-process. The context menu shouldn't be able to tell the difference
> between an in-process and out-of-process plugin. (Take a look at how we always
> create an object tag in:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> ) 
> 
> My guess is that you can reproduce this bug with an embedded plugin running
> in-process.
> 
> > 
> > I agree that earlier we are adding |Print| in the AppendPluginItems() but
that
> > is not good, this is due to 2 reasons.
> > 1) When we add |Print.| in AppendPluginItems() then later we are also
calling
> > AppendPrintItem(), and that is adding |Print.| 2 times.
> >    I have already put a check for avoiding this in AppendPrintItem().
> 
> Does that mean that we can remove that check now and instead rely on this
check?

  Yes, You are right, We don't need this now.
  I have removed this check.
 
> > 2) we want menu item sequence when their is selection in pdf case almost
same
> as
> > html case.
> > html : copy search print|inspect
> > pdf  : SaveAs copy search print |rotatecw rotateccw|inspect
> > 
> > So earlier when we are adding |Print.| in AppendPluginItems() along with
|Save
> > As.| then it will become.
> > html : copy search print|inspect
> > pdf  : SaveAs print copy search |rotatecw rotateccw|inspect
> > 
> > that is wrong, as |Print| should be after |copy search |. So I am not adding
> > |Print.| in AppendPluginItems(), when it will get added as a part of
> > AppendPrintItem(), that get called after AppendPluginItems() as
> > |content_type_->SupportsGroup(ContextMenuContentType::ITEM_GROUP_PRINT)| is
> > true.
> > 
> > 
> > And When we does not have any selection, Then 
> > 
> > html:back forward reload|saveas print translate viewpagesource
> > viewpageinfo|inspect
> > pdf :saveas print|rotatecw rotateccw|inspect
> > 
> > here we should add |Print| in AppendPluginItems() as we have no selection
and
> > |copy search print| will not be getting added later.
> > And as
> |content_type_->SupportsGroup(ContextMenuContentType::ITEM_GROUP_PRINT)|
> > is false, that confirm that no print will be added later.
> > 
> > Above is the whole explanation & reason for my changes.
> > I am changing the comments for OOP to un-embedded one.
> > PTAL.
> 
> Ok I think I understand. Please see me inline comment in the code.
> 
>
https://codereview.chromium.org/632243002/diff/140001/chrome/browser/renderer...
> File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):
> 
>
https://codereview.chromium.org/632243002/diff/140001/chrome/browser/renderer...
> chrome/browser/renderer_context_menu/render_view_context_menu.cc:780: // added
> by |AppendPrintItem|function call.
> Would it be accurate to reword the comment as follows?
> 
> // The "Print" menu item should always be included for plugins. If 
> // content_type_->SupportsGroup(ContextMenuContentType::ITEM_GROUP_PRINT) is,
> // true the item will be added inside AppendPrintItem(). Otherwise we add
> "Print" here.
  Thanks , I have updated comments.

PTAL.

[raymes, 2014-10-14 16:58:49]

lgtm with one nit

https://codereview.chromium.org/632243002/diff/190001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/632243002/diff/190001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:779: // is,
true the item will be added inside AppendPrintItem(). Otherwise we
nit: no , after "is" (this was my fault, sorry!)

[Deepak, 2014-10-15 05:24:25]

nit given by raymes is addressed..
@avi PTAL.

https://codereview.chromium.org/632243002/diff/190001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/632243002/diff/190001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:779: // is,
true the item will be added inside AppendPrintItem(). Otherwise we
On 2014/10/14 16:58:49, raymes wrote:
> nit: no , after "is" (this was my fault, sorry!)

Done.

[Avi (use Gerrit), 2014-10-15 05:39:09]

lgtm

Nice!

[Deepak, 2014-10-15 05:39:46]

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

[commit-bot: I haz the power, 2014-10-15 05:40:16]

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

[commit-bot: I haz the power, 2014-10-15 08:46:36]

Committed patchset #9 (id:210001)

[commit-bot: I haz the power, 2014-10-15 08:47:15]

Patchset 9 (id:??) landed as
https://crrev.com/57cd2b4e34ec3e480f4c5d19d8719e197c081684
Cr-Commit-Position: refs/heads/master@{#299665}