Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
913663002:
Fix for Incomplete context menu is shown on pdf files and reload,back,forward options are missing (Closed)
DescriptionFix for incomplete context menu in pdf page.
when we are viewing the PDF in a MimeHandlerViewGuest then we should
use its embedder WebContents.
Changes done to use the embedder WebContents if we have guest view.
BUG=449919
Committed: https://crrev.com/0c6616522ca22a77e444c4b4f57b3b71baf30cdb
Cr-Commit-Position: refs/heads/master@{#316187}
Patch Set 1 #Patch Set 2 : Fixing error. #
Total comments: 7
Patch Set 3 : Changes as per review comments. #
Total comments: 2
Patch Set 4 : Removing comment. #
Total comments: 1
Messages
Total messages: 21 (5 generated)
deepak.m1@samsung.com changed reviewers: + lazyboy@chromium.org
PTAL at my changes. Thanks
raymes@chromium.org changed reviewers: + raymes@chromium.org
https://codereview.chromium.org/913663002/diff/20001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/913663002/diff/20001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu.cc:765: // Full page plugin, so show page menu items. This breaks the embedded case. The question is what to do here, because the architecture of PDF has changed. Now we always embed a plugin in the extension. Let's get another opinion on this. https://codereview.chromium.org/913663002/diff/20001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1783: ->ExecutePluginActionAtLocation(location, action); Which WebContents does source_web_contents_ represent - the inner one or the outer one? If it's the inner one, could we call a function like GetWebContentsToUse in the next file to get the embedder? It's quite hard to understand why this code works now. https://codereview.chromium.org/913663002/diff/20001/components/renderer_cont... File components/renderer_context_menu/render_view_context_menu_base.cc (right): https://codereview.chromium.org/913663002/diff/20001/components/renderer_cont... components/renderer_context_menu/render_view_context_menu_base.cc:133: // If we're viewing the PDF in a MimeHandlerViewGuest, use its embedder Please don't mention PDF here, just mention MimeHandlerViewGuest
PTAL https://codereview.chromium.org/913663002/diff/20001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/913663002/diff/20001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu.cc:765: // Full page plugin, so show page menu items. On 2015/02/11 00:45:39, raymes wrote: > This breaks the embedded case. The question is what to do here, because the > architecture of PDF has changed. Now we always embed a plugin in the extension. > Let's get another opinion on this. Acknowledged. https://codereview.chromium.org/913663002/diff/20001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1783: ->ExecutePluginActionAtLocation(location, action); On 2015/02/11 00:45:39, raymes wrote: > Which WebContents does source_web_contents_ represent - the inner one or the > outer one? If it's the inner one, could we call a function like > GetWebContentsToUse in the next file to get the embedder? It's quite hard to > understand why this code works now. source_web_contents_ represents here guest view embedded web contents. and when we make call using this webContent then plugin->plugin()->rotateView() from WebViewImpl::performPluginAction() does not lands in PepperWebPluginImpl::rotateView(). So we need to use WebContents::FromRenderFrameHost(render_frame_host) for getting inner WebContents, like the case of inspectElement. https://codereview.chromium.org/913663002/diff/20001/components/renderer_cont... File components/renderer_context_menu/render_view_context_menu_base.cc (right): https://codereview.chromium.org/913663002/diff/20001/components/renderer_cont... components/renderer_context_menu/render_view_context_menu_base.cc:133: // If we're viewing the PDF in a MimeHandlerViewGuest, use its embedder On 2015/02/11 00:45:39, raymes wrote: > Please don't mention PDF here, just mention MimeHandlerViewGuest Done.
https://codereview.chromium.org/913663002/diff/20001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/913663002/diff/20001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1783: ->ExecutePluginActionAtLocation(location, action); Ah, I see. Thanks!
On 2015/02/11 23:03:35, raymes wrote: > https://codereview.chromium.org/913663002/diff/20001/chrome/browser/renderer_... > File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): > > https://codereview.chromium.org/913663002/diff/20001/chrome/browser/renderer_... > chrome/browser/renderer_context_menu/render_view_context_menu.cc:1783: > ->ExecutePluginActionAtLocation(location, action); > Ah, I see. Thanks! To @lazyboy Friendly ping!
@lazyboy probably isn't a good reviewer. We need to hear back from jam@ or avi@ on http://code.google.com/p/chromium/issues/detail?id=449919 first.
deepak.m1@samsung.com changed reviewers: + avi@chromium.org
On 2015/02/12 03:43:02, raymes wrote: > @lazyboy probably isn't a good reviewer. We need to hear back from jam@ or avi@ > on http://code.google.com/p/chromium/issues/detail?id=449919 first. ++Adding Avi in review.
Given avi@'s opinion on the bug, I think this looks good and don't have a problem with it. avi: WDYT? lgtm Thanks! https://codereview.chromium.org/913663002/diff/40001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/913663002/diff/40001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu.cc:765: // Full page plugin, so show page menu items. Please remove this comment as it's inaccurate now.
New patchsets have been uploaded after l-g-t-m from raymes@chromium.org
Thanks for review. PTAL https://codereview.chromium.org/913663002/diff/40001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/913663002/diff/40001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu.cc:765: // Full page plugin, so show page menu items. On 2015/02/13 00:40:27, raymes wrote: > Please remove this comment as it's inaccurate now. Done.
lgtm
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/913663002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0c6616522ca22a77e444c4b4f57b3b71baf30cdb Cr-Commit-Position: refs/heads/master@{#316187}
Message was sent while issue was closed.
https://codereview.chromium.org/913663002/diff/60001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left): https://codereview.chromium.org/913663002/diff/60001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu.cc:778: } I had more of a think about this and I think the change we have made makes sense for MimeHandlerViews which are more like iframes but it makes less sense for other plugins. Could we keep this code and add an exception for the case of MimeHandlerViews?
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/935863002/ by raymes@chromium.org. The reason for reverting is: This caused http://code.google.com/p/chromium/issues/detail?id=458927 . There is a followup CL: https://codereview.chromium.org/931073002/ which almost does a revert of this anyway. So I'm reverting here just to be safe and make sure this doesn't make its way to a branch..
Message was sent while issue was closed.
On 2015/02/18 00:43:01, raymes wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/935863002/ by mailto:raymes@chromium.org. > > The reason for reverting is: This caused > http://code.google.com/p/chromium/issues/detail?id=458927 . There is a followup > CL: https://codereview.chromium.org/931073002/ which almost does a revert of > this anyway. So I'm reverting here just to be safe and make sure this doesn't > make its way to a branch.. https://codereview.chromium.org/931073002/ had been raised for this again. That fix this issue. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
