Unncessary 'View source code' option is enabled in context menu of the 'Pdf Viewer' app.

Unncessary 'View source code' option is enabled in context menu of the 'Pdf Viewer' app.

If we're viewing in a MimeHandlerViewGuest, that is we have plugin , view source should not be shown.

BUG=523254
Committed: https://crrev.com/467ac7794852a11645ef172205d9a668c84523fd
Cr-Commit-Position: refs/heads/master@{#347598}

[Deepak, 2015-08-23 05:58:49]

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

[Deepak, 2015-08-23 05:58:50]

PTAL at this small change.
Thanks

[Deepak, 2015-08-25 12:40:51]

PTAL at this small change.
Thanks

[Deepak, 2015-09-01 02:50:13]

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

[Deepak, 2015-09-01 02:50:13]

@avi
PTAL at this small change.
Thanks

[Avi (use Gerrit), 2015-09-01 04:06:12]

I think this is OK. Does this prevent "view source" for any <webview>?

https://codereview.chromium.org/1305043003/diff/1/chrome/browser/renderer_con...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1305043003/diff/1/chrome/browser/renderer_con...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1149: if
(chrome::FindBrowserWithWebContents(embedder_web_contents_) == NULL)
nullptr

[Deepak, 2015-09-01 05:24:59]

Thanks for review.
This will prevent "view source" for app's that have no browser associated.
Changes done as suggested.
PTAL

https://codereview.chromium.org/1305043003/diff/1/chrome/browser/renderer_con...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1305043003/diff/1/chrome/browser/renderer_con...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1149: if
(chrome::FindBrowserWithWebContents(embedder_web_contents_) == NULL)
On 2015/09/01 04:06:12, Avi wrote:
> nullptr

Done.

[Deepak, 2015-09-03 15:05:41]

@avi
PTAL, 
Thanks

[Avi (use Gerrit), 2015-09-03 15:13:40]

I'm trying to understand how this regressed. The call to the embedder's
CanViewSource ends up at NavigationControllerImpl::CanViewSource(), which says
"no" after consulting mime_util::IsSupportedNonImageMimeType and
media::IsSupportedMediaMimeType. Is the moving of the PDF out-of-process how
this happened? What is the top-level mime type now?

[Avi (use Gerrit), 2015-09-03 19:32:40]

On 2015/09/03 15:13:40, Avi wrote:
> I'm trying to understand how this regressed. The call to the embedder's
> CanViewSource ends up at NavigationControllerImpl::CanViewSource(), which says
> "no" after consulting mime_util::IsSupportedNonImageMimeType and
> media::IsSupportedMediaMimeType. Is the moving of the PDF out-of-process how
> this happened? What is the top-level mime type now?

FYI, there's a bug to add "view source" to <webview>s (http://crbug.com/479175)
so this likely won't fly as-is.

[Deepak, 2015-09-04 05:45:50]

On 2015/09/03 19:32:40, Avi wrote:
> On 2015/09/03 15:13:40, Avi wrote:
> > I'm trying to understand how this regressed. The call to the embedder's
> > CanViewSource ends up at NavigationControllerImpl::CanViewSource(), which
says
> > "no" after consulting mime_util::IsSupportedNonImageMimeType and
> > media::IsSupportedMediaMimeType. Is the moving of the PDF out-of-process how
> > this happened? What is the top-level mime type now?
> 
> FYI, there's a bug to add "view source" to <webview>s
(http://crbug.com/479175)
> so this likely won't fly as-is.

ok, Let me answer your query below:
1) Yes, I agree that NavigationControllerImpl::CanViewSource() get called.
In issue We are getting mime_type as "text/html"
so mime_util::IsSupportedNonImageMimeType() and
!media::IsSupportedMediaMimeType(mime_type) are true,
so is_viewable_mime_type become true and CanViewSource() return true, and "view
Source" get visible in the
context menu. But selecting this option from context menu opens empty page in
browser.

2) I think mime_type should be "application/pdf" that will make 
CanViewSource() return false.

This may be due to moving of the PDF out-of-process.

3) Just a observation:
For IDC_CONTENT_CONTEXT_VIEWFRAMEINFO we are checking association of browser as
chrome::FindBrowserWithWebContents() and if browser is not associated then "view
frameInfo" will
be disabled.
In our case also browser is not associated with pdf app window.

Further As add "view source" to <webview>s (http://crbug.com/479175) bug
is it also for app's that does not have browser associated!!

Please correct me if I misunderstood something.

[Avi (use Gerrit), 2015-09-04 15:39:23]

On 2015/09/04 05:45:50, Deepak wrote:
> 1) In issue We are getting mime_type as "text/html"

That's unfortunate; yes, technically at the top-level it's an HTML page, but
that doesn't help. Sigh.

> 2) I think mime_type should be "application/pdf" that will make 
> CanViewSource() return false.

Although the top-level isn't a PDF... Can we set some kind of flag to say "even
though this is an HTML page, don't show the source"? I'm wondering what the
right way of doing this is.

> Further As add "view source" to <webview>s (http://crbug.com/479175) bug
> is it also for app's that does not have browser associated!!

This is my question. Right now, you're proposing only showing the "view source"
for pages that are top-level browser tabs. It seems to me that the bug I
referenced is trying to do the opposite, add _more_ cases that have "view
source", and in particular, in cases that are very similar to yours. Is there a
way to harmonize?

[Deepak, 2015-09-05 08:04:49]

On 2015/09/04 15:39:23, Avi wrote:
> On 2015/09/04 05:45:50, Deepak wrote:
> > 1) In issue We are getting mime_type as "text/html"
> 
> That's unfortunate; yes, technically at the top-level it's an HTML page, but
> that doesn't help. Sigh.

 Acknowledged.

> > 2) I think mime_type should be "application/pdf" that will make 
> > CanViewSource() return false.
> 
> Although the top-level isn't a PDF... Can we set some kind of flag to say
"even
> though this is an HTML page, don't show the source"? I'm wondering what the
> right way of doing this is.

I think "if we're viewing in a MimeHandlerViewGuest,view source should not be
shown".
So Even we have top level as HTML then we will not show "view Source".
I have done code changes for this.

> > Further As add "view source" to <webview>s (http://crbug.com/479175) bug
> > is it also for app's that does not have browser associated!!
> 
> This is my question. Right now, you're proposing only showing the "view
source"
> for pages that are top-level browser tabs. It seems to me that the bug I
> referenced is trying to do the opposite, add _more_ cases that have "view
> source", and in particular, in cases that are very similar to yours. Is there
a
> way to harmonize?

Now when we have the top-level page as HTML page But if we are viewing in a 
MimeHandlerViewGuest, then "ViewSource" will be disabled.
else it will return result of CanViewSource() call.
Is this approach ok?

[AKV, 2015-09-05 11:15:31]

ajith.v@chromium.org changed reviewers:
+ ajith.v@chromium.org

[AKV, 2015-09-05 11:15:32]

Please check inline comments.

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

https://codereview.chromium.org/1305043003/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1149: // If
we're viewing in a MimeHandlerViewGuest source should not be shown.
Please protect it using ENABLE_EXTENSIONS flag.

https://codereview.chromium.org/1305043003/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1151:
source_web_contents_)) {
Please indent to 4 spaces instead of 8

https://codereview.chromium.org/1305043003/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1152: return
false;
nit: single line body block doesn't need braces.

[Deepak, 2015-09-05 11:52:15]

@AKV
PTAL

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

https://codereview.chromium.org/1305043003/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1149: // If
we're viewing in a MimeHandlerViewGuest source should not be shown.
On 2015/09/05 11:15:32, AKV wrote:
> Please protect it using ENABLE_EXTENSIONS flag.

Done.

https://codereview.chromium.org/1305043003/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1151:
source_web_contents_)) {
On 2015/09/05 11:15:32, AKV wrote:
> Please indent to 4 spaces instead of 8

Done.

https://codereview.chromium.org/1305043003/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1152: return
false;
On 2015/09/05 11:15:32, AKV wrote:
> nit: single line body block doesn't need braces.

Done.

[AKV, 2015-09-05 12:53:10]

Please check inline comment.

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

https://codereview.chromium.org/1305043003/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1151: if
(extensions::MimeHandlerViewGuest::FromWebContents(
Can we use ContextMenuParams MediaType to check whether this is going to be
rendered on a plugin ?

[Deepak, 2015-09-05 12:55:08]

@AKV
PTAL

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

https://codereview.chromium.org/1305043003/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1151: if
(extensions::MimeHandlerViewGuest::FromWebContents(
On 2015/09/05 12:53:09, AKV wrote:
> Can we use ContextMenuParams MediaType to check whether this is going to be
> rendered on a plugin ?

Done.

[AKV, 2015-09-05 13:12:48]

peer review looks good to me!

[Avi (use Gerrit), 2015-09-05 20:29:14]

lgtm

That actually looks like a really good way to do this. Thanks Ajith for the
suggestion.

[Peter Kasting, 2015-09-05 20:42:51]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2015-09-05 20:42:51]

Drive-by (no signoff from me needed)

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

https://codereview.chromium.org/1305043003/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1151: return
embedder_web_contents_->GetController().CanViewSource();
Nit: Simpler:

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

[Deepak, 2015-09-07 03:00:41]

Thanks @peter for review.
Changes done as suggested.
Thanks

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

https://codereview.chromium.org/1305043003/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1151: return
embedder_web_contents_->GetController().CanViewSource();
On 2015/09/05 20:42:51, Peter Kasting wrote:
> Nit: Simpler:
> 
>       return (params_.media_type != WebContextMenuData::MediaTypePlugin) &&
>           embedder_web_contents_->GetController().CanViewSource();

Done.

[Deepak, 2015-09-07 03:00:48]

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

[Deepak, 2015-09-07 03:00:48]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1305043003/#ps100001
(title: "Changes as per review comments.")

[commit-bot: I haz the power, 2015-09-07 03:00:53]

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

[commit-bot: I haz the power, 2015-09-07 03:43:41]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-07 03:44:18]

Patchset 6 (id:??) landed as
https://crrev.com/467ac7794852a11645ef172205d9a668c84523fd
Cr-Commit-Position: refs/heads/master@{#347598}