'View source code' option should be disabled in context menu for 'Paper Toolbar' of 'Pdf Viewer"

When we have viewing in a MimeHandlerViewGuest then 'View Source Code' will be
disabled. This is needed as when context menu comes on the meterial UI buttons
like On the 'Zoom-out','Zoom-in','Fullscreen' icon or 'Paper Toolbar' of the
pdf viewer then these elements are not HTMLEmbedElements. So data.mediaType
and data.mediaFlags values are not updated for these materiel UI toolbar.

BUG=539269
Committed: https://crrev.com/400dbfa3846a40649f04f75dd65f92917e1e3a45
Cr-Commit-Position: refs/heads/master@{#353982}

[Deepak, 2015-10-08 11:09:17]

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

[Deepak, 2015-10-08 11:09:17]

For Peer review PTAL.
Thanks

[AKV, 2015-10-08 13:45:55]

peer review looks good to me!

[Deepak, 2015-10-08 13:48:03]

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

[Deepak, 2015-10-08 13:48:04]

PTAL

[Deepak, 2015-10-08 13:48:41]

@lazyboy
PTAL
Thanks

[lazyboy, 2015-10-08 18:35:39]

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

https://chromiumcodereview.appspot.com/1389273003/diff/20001/chrome/browser/r...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1157: return
!extensions::MimeHandlerViewGuest::FromWebContents(
Wouldn't this start showing view source for plugin elements? I'd keep the !=
MediaTypePlugin check.

[Deepak, 2015-10-09 05:29:05]

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

https://codereview.chromium.org/1389273003/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1157: return
!extensions::MimeHandlerViewGuest::FromWebContents(
On 2015/10/08 18:35:39, lazyboy wrote:
> Wouldn't this start showing view source for plugin elements? I'd keep the !=
> MediaTypePlugin check.

"view source" for the plugin elements will be disabled.

"params_.media_type != WebContextMenuData::MediaTypePlugin" will not help as 
when user right click on the "Zoom in" , "Zoom out" buttons of the material UI
then as these elements are not embedded in the content so "media_type" value
does not get updated to WebContextMenuData::MediaTypePlugin from
ContextMenuClientImpl::showContextMenu(), so this condition will pass and 
"view source" will remain enabled.

we need to put
!extensions::MimeHandlerViewGuest::FromWebContents(source_web_contents_) check 
then params_.media_type!=WebContextMenuData::MediaTypePlugin not required, as
params_.media_type
value get updated for plugins only.

Please correct me if I misunderstood something.

[lazyboy, 2015-10-09 15:46:38]

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

https://codereview.chromium.org/1389273003/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1157: return
!extensions::MimeHandlerViewGuest::FromWebContents(
On 2015/10/09 05:29:05, Deepak wrote:
> On 2015/10/08 18:35:39, lazyboy wrote:
> > Wouldn't this start showing view source for plugin elements? I'd keep the !=
> > MediaTypePlugin check.
> 
> "view source" for the plugin elements will be disabled.
> 
> "params_.media_type != WebContextMenuData::MediaTypePlugin" will not help as 
> when user right click on the "Zoom in" , "Zoom out" buttons of the material UI
> then as these elements are not embedded in the content so "media_type" value
> does not get updated to WebContextMenuData::MediaTypePlugin from
> ContextMenuClientImpl::showContextMenu(), so this condition will pass and 
> "view source" will remain enabled.
> 
> we need to put
> !extensions::MimeHandlerViewGuest::FromWebContents(source_web_contents_) check

> then params_.media_type!=WebContextMenuData::MediaTypePlugin not required, as
> params_.media_type
> value get updated for plugins only.
> 
> Please correct me if I misunderstood something.

For regular plugins (not PDF, say flash), we'd get
MimeHandlerViewGuest::FromWebContents() == NULL and this will start
showing "view source". This would break existing behavior.
What I suggested is to do both of
if ((media_type != MediaTypePlugin || !MimeHandlerViewGuest:From) &&
  embedder_web_contents_->GetController().CanViewSource()) ...
Does that make sense?

Can you also write a test for this? Thanks.

[Deepak, 2015-10-11 07:15:56]

On 2015/10/09 15:46:38, lazyboy wrote:
>
https://codereview.chromium.org/1389273003/diff/20001/chrome/browser/renderer...
> File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):
> 
>
https://codereview.chromium.org/1389273003/diff/20001/chrome/browser/renderer...
> chrome/browser/renderer_context_menu/render_view_context_menu.cc:1157: return
> !extensions::MimeHandlerViewGuest::FromWebContents(
> On 2015/10/09 05:29:05, Deepak wrote:
> > On 2015/10/08 18:35:39, lazyboy wrote:
> > > Wouldn't this start showing view source for plugin elements? I'd keep the
!=
> > > MediaTypePlugin check.
> > 
> > "view source" for the plugin elements will be disabled.
> > 
> > "params_.media_type != WebContextMenuData::MediaTypePlugin" will not help as

> > when user right click on the "Zoom in" , "Zoom out" buttons of the material
UI
> > then as these elements are not embedded in the content so "media_type" value
> > does not get updated to WebContextMenuData::MediaTypePlugin from
> > ContextMenuClientImpl::showContextMenu(), so this condition will pass and 
> > "view source" will remain enabled.
> > 
> > we need to put
> > !extensions::MimeHandlerViewGuest::FromWebContents(source_web_contents_)
check
> 
> > then params_.media_type!=WebContextMenuData::MediaTypePlugin not required,
as
> > params_.media_type
> > value get updated for plugins only.
> > 
> > Please correct me if I misunderstood something.
> 
> For regular plugins (not PDF, say flash), we'd get
> MimeHandlerViewGuest::FromWebContents() == NULL and this will start
> showing "view source". This would break existing behavior.
> What I suggested is to do both of
> if ((media_type != MediaTypePlugin || !MimeHandlerViewGuest:From) &&
>   embedder_web_contents_->GetController().CanViewSource()) ...
> Does that make sense?
> 
> Can you also write a test for this? Thanks.

I think in that case condition should be "&&" .

For making all case working it should be like:
---------------------------------------------------------------------
// Condition 1
if (!!extensions::MimeHandlerViewGuest::FromWebContents(source_web_contents_)) {
 
  // early return.
  return false;
}
// condition 2 && condition 3
return (params_.media_type != WebContextMenuData::MediaTypePlugin) &&       
 embedder_web_contents_->GetController().CanViewSource();
--------------------------------------------------------------------
So this will take care of all cases, right click on :
1) HTML page. (condition1,condition2,condition3) will be (false,true,true)->true
2) Browser tab PDF material UI (condition1,condition2,condition3) will be
(true,true,false)->false
3) PDF Viewer App material UI (condition1,condition2,condition3) will be
(true,true,true)->false

and for 'flash' plugin as you mentioned that 
extensions::MimeHandlerViewGuest::FromWebContents(source_web_contents_)==NULL
then (condition1,condition2,condition3) will be (false,false,XXX)->false.

We can club these condition, This is just for demonstration purpose.like:

return !extensions::MimeHandlerViewGuest::FromWebContents(source_web_contents_)
&&
       (params_.media_type != WebContextMenuData::MediaTypePlugin) &&
        embedder_web_contents_->GetController().CanViewSource();

secondly Context menu in PDF Viewer App material UI is one of the corner case
as PDF viewer app need to be launched that remain detected from browser and we
get MIME type as "text/html" so CanViewSource() will return true.

please correct me if I missed something.

[lazyboy, 2015-10-13 17:36:48]

On 2015/10/11 07:15:56, Deepak wrote:
> On 2015/10/09 15:46:38, lazyboy wrote:
> >
>
https://codereview.chromium.org/1389273003/diff/20001/chrome/browser/renderer...
> > File chrome/browser/renderer_context_menu/render_view_context_menu.cc
(right):
> > 
> >
>
https://codereview.chromium.org/1389273003/diff/20001/chrome/browser/renderer...
> > chrome/browser/renderer_context_menu/render_view_context_menu.cc:1157:
return
> > !extensions::MimeHandlerViewGuest::FromWebContents(
> > On 2015/10/09 05:29:05, Deepak wrote:
> > > On 2015/10/08 18:35:39, lazyboy wrote:
> > > > Wouldn't this start showing view source for plugin elements? I'd keep
the
> !=
> > > > MediaTypePlugin check.
> > > 
> > > "view source" for the plugin elements will be disabled.
> > > 
> > > "params_.media_type != WebContextMenuData::MediaTypePlugin" will not help
as
> 
> > > when user right click on the "Zoom in" , "Zoom out" buttons of the
material
> UI
> > > then as these elements are not embedded in the content so "media_type"
value
> > > does not get updated to WebContextMenuData::MediaTypePlugin from
> > > ContextMenuClientImpl::showContextMenu(), so this condition will pass and 
> > > "view source" will remain enabled.
> > > 
> > > we need to put
> > > !extensions::MimeHandlerViewGuest::FromWebContents(source_web_contents_)
> check
> > 
> > > then params_.media_type!=WebContextMenuData::MediaTypePlugin not required,
> as
> > > params_.media_type
> > > value get updated for plugins only.
> > > 
> > > Please correct me if I misunderstood something.
> > 
> > For regular plugins (not PDF, say flash), we'd get
> > MimeHandlerViewGuest::FromWebContents() == NULL and this will start
> > showing "view source". This would break existing behavior.
> > What I suggested is to do both of
> > if ((media_type != MediaTypePlugin || !MimeHandlerViewGuest:From) &&
> >   embedder_web_contents_->GetController().CanViewSource()) ...
> > Does that make sense?
> > 
> > Can you also write a test for this? Thanks.
> 
> I think in that case condition should be "&&" .
> 
> For making all case working it should be like:
> ---------------------------------------------------------------------
> // Condition 1
> if (!!extensions::MimeHandlerViewGuest::FromWebContents(source_web_contents_))
{
>  
>   // early return.
>   return false;
> }
> // condition 2 && condition 3
> return (params_.media_type != WebContextMenuData::MediaTypePlugin) &&       
>  embedder_web_contents_->GetController().CanViewSource();
Thanks for explaining, I wasn't thinking too hard.
I think I like this simpler looking approach with early return, keeps
things readable compared to the one statement one.
Please also write a test if possible.
> --------------------------------------------------------------------
> So this will take care of all cases, right click on :
> 1) HTML page. (condition1,condition2,condition3) will be
(false,true,true)->true
> 2) Browser tab PDF material UI (condition1,condition2,condition3) will be
> (true,true,false)->false
> 3) PDF Viewer App material UI (condition1,condition2,condition3) will be
> (true,true,true)->false
> 
> and for 'flash' plugin as you mentioned that 
> extensions::MimeHandlerViewGuest::FromWebContents(source_web_contents_)==NULL
> then (condition1,condition2,condition3) will be (false,false,XXX)->false.
> 
> We can club these condition, This is just for demonstration purpose.like:
> 
> return
!extensions::MimeHandlerViewGuest::FromWebContents(source_web_contents_)
> &&
>        (params_.media_type != WebContextMenuData::MediaTypePlugin) &&
>         embedder_web_contents_->GetController().CanViewSource();
> 
> secondly Context menu in PDF Viewer App material UI is one of the corner case
> as PDF viewer app need to be launched that remain detected from browser and we
> get MIME type as "text/html" so CanViewSource() will return true.
> 
> please correct me if I missed something.

[Deepak, 2015-10-14 04:46:17]

@lazyboy
Test case for current scenario requires:
1)Launching of Browser that have "PDF Viewer Extension app" installed.
2)After Launching "PDF Viewer Extension app",PDF Viewer app will launch in a
separate Window.
3)Then opening a saved PDF(in machine) in that PDF Viewer.
4)Bringing Material UI and then right click to get context menu.
5)Observing "view Source" in the context menu.
I tried to find some reference for above scenario in corresponding test
files,but not able to find any reference.

Other changes has been done.

Please suggest your opinion.
PTAL
Thanks

[lazyboy, 2015-10-14 07:38:36]

On 2015/10/14 04:46:17, Deepak wrote:
> @lazyboy
> Test case for current scenario requires:
> 1)Launching of Browser that have "PDF Viewer Extension app" installed.
> 2)After Launching "PDF Viewer Extension app",PDF Viewer app will launch in a
> separate Window.
> 3)Then opening a saved PDF(in machine) in that PDF Viewer.
> 4)Bringing Material UI and then right click to get context menu.
> 5)Observing "view Source" in the context menu.
> I tried to find some reference for above scenario in corresponding test
> files,but not able to find any reference.
> 
> Other changes has been done.
> 
> Please suggest your opinion.
OK, given the complexity, I'm going to stamp LGTM on this one. Thanks.
> PTAL
> Thanks

[Deepak, 2015-10-14 07:39:09]

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

[commit-bot: I haz the power, 2015-10-14 07:39:35]

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

[commit-bot: I haz the power, 2015-10-14 08:29:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-14 08:30:18]

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