Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2972123002:
Click to Open PDF: Open PDF once View PDF is clicked. (Closed)
DescriptionClick to Open PDF: Open PDF once View PDF is clicked.
pdf_plugin.html shows the file name and a View PDF link. When View PDF is
clicked, the callback function sends a message. When this message is
handled in PluginObserver, the URL for the PDF file is opened in a new
tab. However, in guest view, when the message is handled, nothing is
done.
BUG=737787
Review-Url: https://codereview.chromium.org/2972123002
Cr-Commit-Position: refs/heads/master@{#487235}
Committed: https://chromium.googlesource.com/chromium/src/+/254fa14030667b2a7b707e975a23437d9faca45d
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : remove id since it's not used #
Total comments: 23
Patch Set 4 : . #
Total comments: 2
Patch Set 5 : . #
Total comments: 2
Patch Set 6 : Rename from download to open PDF #
Total comments: 10
Patch Set 7 : Check URL #Patch Set 8 : Change comment; return if URL is invalid #
Total comments: 7
Patch Set 9 : Validate URL; change comment #Patch Set 10 : comment #
Total comments: 1
Patch Set 11 : Grammar #Messages
Total messages: 46 (15 generated)
The CQ bit was checked by amberwon@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== . . BUG= ========== to ========== Click to Download PDF: Download PDF when View PDF is clicked. pdf_plugin.html shows the file name and a View PDF link. When View PDF is clicked, the callback function sends a message. When this message is handled in PluginObserver, the URL for the PDF file is opened in a new tab. However, in guest view, when the message is handled, nothing is done. BUG=737787 ==========
amberwon@google.com changed reviewers: + tommycli@chromium.org
amberwon@google.com changed reviewers: + thestig@chromium.org
thestig: PTAL
https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/guest_vi... File chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/guest_vi... chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:65: IPC_MESSAGE_HANDLER(ChromeViewHostMsg_DownloadPDF, OnDownloadPDF) So what happens if the IPC message isn't handled? https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/guest_vi... chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:117: void ChromeWebViewPermissionHelperDelegate::OnDownloadPDF(const GURL& url) {} Can you explain when we would reach here vs PluginObserver? https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/plugins/... File chrome/browser/plugins/plugin_observer.cc (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/plugins/... chrome/browser/plugins/plugin_observer.cc:341: web_contents()->OpenURL(content::OpenURLParams( Have you looked into OpenURL() vs SaveFrame() ? https://codereview.chromium.org/2972123002/diff/40001/chrome/common/render_me... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/common/render_me... chrome/common/render_messages.h:345: IPC_MESSAGE_ROUTED1(ChromeViewHostMsg_DownloadPDF, GURL /* url */) Can you explain what this does? https://codereview.chromium.org/2972123002/diff/40001/chrome/common/render_me... chrome/common/render_messages.h:345: IPC_MESSAGE_ROUTED1(ChromeViewHostMsg_DownloadPDF, GURL /* url */) Does this belong inside the #if BUILDFLAG(ENABLE_PLUGINS) section? https://codereview.chromium.org/2972123002/diff/40001/chrome/renderer/plugins... File chrome/renderer/plugins/pdf_plugin_placeholder.cc (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.cc:30: values.SetString("fileName", ((GURL)params.url).ExtractFileName()); No C-style casts. https://codereview.chromium.org/2972123002/diff/40001/chrome/renderer/resourc... File chrome/renderer/resources/plugins/pdf_plugin.html (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/renderer/resourc... chrome/renderer/resources/plugins/pdf_plugin.html:11: <a href="#" onclick="plugin.downloadPDF()">View PDF</a> I'm still curious what happens if the href is simlply set to the link of the PDF to download. https://codereview.chromium.org/2972123002/diff/40001/chrome/renderer/resourc... chrome/renderer/resources/plugins/pdf_plugin.html:11: <a href="#" onclick="plugin.downloadPDF()">View PDF</a> Since this uses JS, does that mean if the user has JS disabled in content setting, then this won't work?
https://codereview.chromium.org/2972123002/diff/40001/chrome/common/render_me... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/common/render_me... chrome/common/render_messages.h:345: IPC_MESSAGE_ROUTED1(ChromeViewHostMsg_DownloadPDF, GURL /* url */) On 2017/07/07 17:45:45, Lei Zhang wrote: > Does this belong inside the #if BUILDFLAG(ENABLE_PLUGINS) section? For this particular one -- no - since Android needs this as well. Which brings up another point -- we may need to move this to a different class (other than PluginObserver) if that's not created on Android. For now I'm okay with deferring making it work on !ENABLE_PLUGINS until a followup.
lgtm from my perspective after addressing thestig's concerns. https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/guest_vi... File chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/guest_vi... chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:65: IPC_MESSAGE_HANDLER(ChromeViewHostMsg_DownloadPDF, OnDownloadPDF) On 2017/07/07 17:45:45, Lei Zhang wrote: > So what happens if the IPC message isn't handled? I looked upstream a few callsites and didn't see a crash. However, we thought that it would be odd if it was never handled... and probably better as an explicit no-op in guest view. https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/guest_vi... chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:117: void ChromeWebViewPermissionHelperDelegate::OnDownloadPDF(const GURL& url) {} On 2017/07/07 17:45:45, Lei Zhang wrote: > Can you explain when we would reach here vs PluginObserver? I think this is for guest view https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/plugins/... File chrome/browser/plugins/plugin_observer.h (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/plugins/... chrome/browser/plugins/plugin_observer.h:48: void OnDownloadPDF(const GURL& url); nit: should be end of list right?
https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/guest_vi... File chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/guest_vi... chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:117: void ChromeWebViewPermissionHelperDelegate::OnDownloadPDF(const GURL& url) {} On 2017/07/07 18:35:30, tommycli wrote: > On 2017/07/07 17:45:45, Lei Zhang wrote: > > Can you explain when we would reach here vs PluginObserver? > > I think this is for guest view Yes, that part is obvious. Let me try to ask better questions: - Can you explain why this doesn't do anything? - Is there a test or a demo page where execution would reach this point? - This is specifically in the web_view directory. What about the code in c/b/guest_view/app_view and code for other GuestView subclasses?
https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/plugins/... File chrome/browser/plugins/plugin_observer.cc (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/plugins/... chrome/browser/plugins/plugin_observer.cc:341: web_contents()->OpenURL(content::OpenURLParams( On 2017/07/07 17:45:45, Lei Zhang wrote: > Have you looked into OpenURL() vs SaveFrame() ? I've looked into both. At the moment, both Tommy and I aren't sure which one would be more appropriate to use. For now, I'll stick with OpenURL and if we find that SaveFrame would be better, we can change it then. https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/plugins/... File chrome/browser/plugins/plugin_observer.h (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/plugins/... chrome/browser/plugins/plugin_observer.h:48: void OnDownloadPDF(const GURL& url); On 2017/07/07 18:35:30, tommycli wrote: > nit: should be end of list right? Done. https://codereview.chromium.org/2972123002/diff/40001/chrome/common/render_me... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/common/render_me... chrome/common/render_messages.h:345: IPC_MESSAGE_ROUTED1(ChromeViewHostMsg_DownloadPDF, GURL /* url */) On 2017/07/07 17:45:45, Lei Zhang wrote: > Can you explain what this does? This declares the message (ChromeViewHostMsg_DownloadPDF), with a GURL as an argument. https://codereview.chromium.org/2972123002/diff/40001/chrome/renderer/plugins... File chrome/renderer/plugins/pdf_plugin_placeholder.cc (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.cc:30: values.SetString("fileName", ((GURL)params.url).ExtractFileName()); On 2017/07/07 17:45:45, Lei Zhang wrote: > No C-style casts. Done. https://codereview.chromium.org/2972123002/diff/40001/chrome/renderer/resourc... File chrome/renderer/resources/plugins/pdf_plugin.html (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/renderer/resourc... chrome/renderer/resources/plugins/pdf_plugin.html:11: <a href="#" onclick="plugin.downloadPDF()">View PDF</a> On 2017/07/07 17:45:45, Lei Zhang wrote: > I'm still curious what happens if the href is simlply set to the link of the PDF > to download. Links don't seem to open within the placeholder. https://codereview.chromium.org/2972123002/diff/40001/chrome/renderer/resourc... chrome/renderer/resources/plugins/pdf_plugin.html:11: <a href="#" onclick="plugin.downloadPDF()">View PDF</a> On 2017/07/07 17:45:45, Lei Zhang wrote: > Since this uses JS, does that mean if the user has JS disabled in content > setting, then this won't work? It still seems to work, though not entirely sure why.
https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/plugins/... File chrome/browser/plugins/plugin_observer.cc (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/plugins/... chrome/browser/plugins/plugin_observer.cc:341: web_contents()->OpenURL(content::OpenURLParams( On 2017/07/07 20:44:22, amberwon wrote: > On 2017/07/07 17:45:45, Lei Zhang wrote: > > Have you looked into OpenURL() vs SaveFrame() ? > > I've looked into both. At the moment, both Tommy and I aren't sure > which one would be more appropriate to use. For now, I'll stick with > OpenURL and if we find that SaveFrame would be better, we can change > it then. FWIW, SaveFrame() is used exclusively by the PDF plugin, and it has much few parameters. Internally it just calls SaveFrameWithHeaders() which has a few more callers. SaveFrameWithHeaders() then goes straight to download code, whereas it's not clear what OpenURL() will do. https://codereview.chromium.org/2972123002/diff/40001/chrome/common/render_me... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/common/render_me... chrome/common/render_messages.h:345: IPC_MESSAGE_ROUTED1(ChromeViewHostMsg_DownloadPDF, GURL /* url */) On 2017/07/07 20:44:22, amberwon wrote: > On 2017/07/07 17:45:45, Lei Zhang wrote: > > Can you explain what this does? > > This declares the message (ChromeViewHostMsg_DownloadPDF), with a > GURL as an argument. Can you describe what it does, as a comment in the code? Like all the existing IPC messages above it. https://codereview.chromium.org/2972123002/diff/60001/chrome/renderer/plugins... File chrome/renderer/plugins/pdf_plugin_placeholder.cc (right): https://codereview.chromium.org/2972123002/diff/60001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.cc:30: GURL url = params.url; Can this be written as: values.SetString("fileName", GURL(url).ExtractFileName()); ?
https://codereview.chromium.org/2972123002/diff/40001/chrome/common/render_me... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/common/render_me... chrome/common/render_messages.h:345: IPC_MESSAGE_ROUTED1(ChromeViewHostMsg_DownloadPDF, GURL /* url */) On 2017/07/07 21:08:20, Lei Zhang wrote: > On 2017/07/07 20:44:22, amberwon wrote: > > On 2017/07/07 17:45:45, Lei Zhang wrote: > > > Can you explain what this does? > > > > This declares the message (ChromeViewHostMsg_DownloadPDF), with a > > GURL as an argument. > > Can you describe what it does, as a comment in the code? Like all the existing > IPC messages above it. Done. https://codereview.chromium.org/2972123002/diff/60001/chrome/renderer/plugins... File chrome/renderer/plugins/pdf_plugin_placeholder.cc (right): https://codereview.chromium.org/2972123002/diff/60001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.cc:30: GURL url = params.url; On 2017/07/07 21:08:20, Lei Zhang wrote: > Can this be written as: > > values.SetString("fileName", GURL(url).ExtractFileName()); ? Yes, it can. Done.
https://codereview.chromium.org/2972123002/diff/80001/chrome/common/render_me... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/80001/chrome/common/render_me... chrome/common/render_messages.h:345: // Tells the browser to open a PDF file in a new tab. Used when plugins are OK, so now this says to open a PDF in a new tab, but the message name says "DownloadPDF" and the CL description says "Download PDF". It's unclear what the actual goal is. Maybe clarifying this will also help decide whether to use OpenURL() or SaveFrame().
Description was changed from ========== Click to Download PDF: Download PDF when View PDF is clicked. pdf_plugin.html shows the file name and a View PDF link. When View PDF is clicked, the callback function sends a message. When this message is handled in PluginObserver, the URL for the PDF file is opened in a new tab. However, in guest view, when the message is handled, nothing is done. BUG=737787 ========== to ========== Click to Open PDF: Open PDF once View PDF is clicked. pdf_plugin.html shows the file name and a View PDF link. When View PDF is clicked, the callback function sends a message. When this message is handled in PluginObserver, the URL for the PDF file is opened in a new tab. However, in guest view, when the message is handled, nothing is done. BUG=737787 ==========
https://codereview.chromium.org/2972123002/diff/80001/chrome/common/render_me... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/80001/chrome/common/render_me... chrome/common/render_messages.h:345: // Tells the browser to open a PDF file in a new tab. Used when plugins are On 2017/07/07 21:27:52, Lei Zhang wrote: > OK, so now this says to open a PDF in a new tab, but the message name says > "DownloadPDF" and the CL description says "Download PDF". It's unclear what the > actual goal is. Maybe clarifying this will also help decide whether to use > OpenURL() or SaveFrame(). Done. Chose to go with opening the PDF instead of downloading it.
reminder: The questions regarding guest_view changes are still unresolved. https://codereview.chromium.org/2972123002/diff/100001/chrome/browser/plugins... File chrome/browser/plugins/plugin_observer.cc (right): https://codereview.chromium.org/2972123002/diff/100001/chrome/browser/plugins... chrome/browser/plugins/plugin_observer.cc:347: ui::PAGE_TRANSITION_AUTO_BOOKMARK, false)); I'm not sure if these are correct, but I also don't know the right answers. Can you explain why you chose PAGE_TRANSITION_AUTO_BOOKMARK, and is_renderer_initiated = false? https://codereview.chromium.org/2972123002/diff/100001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/100001/chrome/common/render_m... chrome/common/render_messages.h:345: // Tells the browser to open a PDF file in a new tab. Used when plugins are plugins are disabled -> there is no PDF viewer available ?
https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/guest_vi... File chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc (right): https://codereview.chromium.org/2972123002/diff/40001/chrome/browser/guest_vi... chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:117: void ChromeWebViewPermissionHelperDelegate::OnDownloadPDF(const GURL& url) {} On 2017/07/07 19:13:28, Lei Zhang wrote: > On 2017/07/07 18:35:30, tommycli wrote: > > On 2017/07/07 17:45:45, Lei Zhang wrote: > > > Can you explain when we would reach here vs PluginObserver? > > > > I think this is for guest view > > Yes, that part is obvious. Let me try to ask better questions: > > - Can you explain why this doesn't do anything? > - Is there a test or a demo page where execution would reach this point? > - This is specifically in the web_view directory. What about the code in > c/b/guest_view/app_view and code for other GuestView subclasses? A no-op is appropriate here, since within GuestView, a download should not be triggered. In GuestView, WebViews are on an isolated process. Some things that use GuestView include sign in pages. There is an extension that can be used to test it: https://chrome.google.com/webstore/detail/browser-sample/edggnmnajhcbhlnpjnog... As for the subclasses, none of them seem to deal with plugins. https://codereview.chromium.org/2972123002/diff/100001/chrome/browser/plugins... File chrome/browser/plugins/plugin_observer.cc (right): https://codereview.chromium.org/2972123002/diff/100001/chrome/browser/plugins... chrome/browser/plugins/plugin_observer.cc:347: ui::PAGE_TRANSITION_AUTO_BOOKMARK, false)); On 2017/07/07 22:47:58, Lei Zhang wrote: > I'm not sure if these are correct, but I also don't know the right answers. Can > you explain why you chose PAGE_TRANSITION_AUTO_BOOKMARK, and > is_renderer_initiated = false? They were used for OnOpenAboutPlugins, when the placeholder for PDF's showed a link to chrome://plugins. So I chose them again here. PAGE_TRANSITION_AUTO_BOOKMARK means that the user got to this page through a suggestion in the UI. is_renderer_initiated indicates whether the navigation is initiated by the renderer process. In this case, the browser process initiates it. https://codereview.chromium.org/2972123002/diff/100001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/100001/chrome/common/render_m... chrome/common/render_messages.h:345: // Tells the browser to open a PDF file in a new tab. Used when plugins are On 2017/07/07 22:47:58, Lei Zhang wrote: > plugins are disabled -> there is no PDF viewer available ? I believe just Chrome's PDF Viewer is disabled, but other PDF viewers may be available.
thestig@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/2972123002/diff/100001/chrome/browser/plugins... File chrome/browser/plugins/plugin_observer.cc (right): https://codereview.chromium.org/2972123002/diff/100001/chrome/browser/plugins... chrome/browser/plugins/plugin_observer.cc:347: ui::PAGE_TRANSITION_AUTO_BOOKMARK, false)); On 2017/07/11 18:08:05, amberwon wrote: > On 2017/07/07 22:47:58, Lei Zhang wrote: > > I'm not sure if these are correct, but I also don't know the right answers. > Can > > you explain why you chose PAGE_TRANSITION_AUTO_BOOKMARK, and > > is_renderer_initiated = false? > > They were used for OnOpenAboutPlugins, when the placeholder for PDF's showed a > link to chrome://plugins. So I chose them again here. > PAGE_TRANSITION_AUTO_BOOKMARK means that the user got to this page through a > suggestion in the UI. > is_renderer_initiated indicates whether the navigation is initiated by the > renderer process. In this case, the browser process initiates it. Well, since the code being referred to was removed in https://crrev.com/443581, I'll add dcheng@ as the security reviewer for this CL. Some of the code looks familiar, but there is the addition of a GURL parameter. https://codereview.chromium.org/2972123002/diff/100001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/100001/chrome/common/render_m... chrome/common/render_messages.h:345: // Tells the browser to open a PDF file in a new tab. Used when plugins are On 2017/07/11 18:08:05, amberwon wrote: > On 2017/07/07 22:47:58, Lei Zhang wrote: > > plugins are disabled -> there is no PDF viewer available ? > > I believe just Chrome's PDF Viewer is disabled, but other PDF > viewers may be available. If one disables the internal PDF Viewer and installs a Rob's PDF Viewer extension, [1] then a page that embeds a PDF will still display the PDF, and the pdf_plugin.html placeholder won't show. By plugins, do you really mean PPAPI plugins, or just anything that can be plugged in, which includes extensions? [1] https://chrome.google.com/webstore/detail/pdf-viewer/oemmndcbldboiebfnladdacb...
thestig@chromium.org changed reviewers: + fsamuel@chromium.org
+fsamuel to double check the guest_view changes.
tommycli@chromium.org changed reviewers: + lfg@chromium.org - fsamuel@chromium.org
lfg: PTAL guest_view changes verify thestig: sounds good... I'm substituting in lfg@ for guest_view because we had a VC with him to discuss these changes already.
Just to clear things up from my end: when plugins are disabled, does the PDF viewer still work at the top-level? Otherwise, I'm wondering how this helps. Also, do we need to do any validation on the received URL? Couldn't a page claim that it has a PDF subresource pointing to chrome://foo?
https://codereview.chromium.org/2972123002/diff/100001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/100001/chrome/common/render_m... chrome/common/render_messages.h:345: // Tells the browser to open a PDF file in a new tab. Used when plugins are On 2017/07/12 00:51:37, Lei Zhang wrote: > On 2017/07/11 18:08:05, amberwon wrote: > > On 2017/07/07 22:47:58, Lei Zhang wrote: > > > plugins are disabled -> there is no PDF viewer available ? > > > > I believe just Chrome's PDF Viewer is disabled, but other PDF > > viewers may be available. > > If one disables the internal PDF Viewer and installs a Rob's PDF Viewer > extension, [1] then a page that embeds a PDF will still display the PDF, and the > pdf_plugin.html placeholder won't show. > > By plugins, do you really mean PPAPI plugins, or just anything that can be > plugged in, which includes extensions? > > [1] > https://chrome.google.com/webstore/detail/pdf-viewer/oemmndcbldboiebfnladdacb... Yes I mean PPAPI plugins. I changed the comment to reflect the case where a user has an PDF viewer extension as well.
On 2017/07/12 08:35:52, dcheng wrote: > Just to clear things up from my end: when plugins are disabled, does the PDF > viewer still work at the top-level? Otherwise, I'm wondering how this helps. > > Also, do we need to do any validation on the received URL? Couldn't a page claim > that it has a PDF subresource pointing to chrome://foo? The PDF viewer does not work at the top-level, so the new PDF placeholder will allow the user to be able to open the PDF. Just added URL validation.
https://codereview.chromium.org/2972123002/diff/100001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/100001/chrome/common/render_m... chrome/common/render_messages.h:345: // Tells the browser to open a PDF file in a new tab. Used when plugins are On 2017/07/12 19:12:20, amberwon wrote: > On 2017/07/12 00:51:37, Lei Zhang wrote: > > On 2017/07/11 18:08:05, amberwon wrote: > > > On 2017/07/07 22:47:58, Lei Zhang wrote: > > > > plugins are disabled -> there is no PDF viewer available ? > > > > > > I believe just Chrome's PDF Viewer is disabled, but other PDF > > > viewers may be available. > > > > If one disables the internal PDF Viewer and installs a Rob's PDF Viewer > > extension, [1] then a page that embeds a PDF will still display the PDF, and > the > > pdf_plugin.html placeholder won't show. > > > > By plugins, do you really mean PPAPI plugins, or just anything that can be > > plugged in, which includes extensions? > > > > [1] > > > https://chrome.google.com/webstore/detail/pdf-viewer/oemmndcbldboiebfnladdacb... > > Yes I mean PPAPI plugins. I changed the comment to reflect the case where a user > has an PDF viewer extension as well. So what was wrong with my initial suggestion? https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/plugins... File chrome/browser/plugins/plugin_observer.cc (right): https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/plugins... chrome/browser/plugins/plugin_observer.cc:341: if (!url.SchemeIsHTTPOrHTTPS()) What about ftp://, or file:// ? The latter works from file:///path/to/foo.html but not from a web server, due to same-origin policy or what not.
https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/plugins... File chrome/browser/plugins/plugin_observer.cc (right): https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/plugins... chrome/browser/plugins/plugin_observer.cc:341: if (!url.SchemeIsHTTPOrHTTPS()) On 2017/07/12 22:57:30, Lei Zhang wrote: > What about ftp://, or file:// ? The latter works from file:///path/to/foo.html > but not from a web server, due to same-origin policy or what not. I think we should be using ChildProcessSecurityPolicy::CanRequestURL() here. I think we'll need to pass the RenderFrameHost parameter down from OnMessageReceived() though.
https://codereview.chromium.org/2972123002/diff/100001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/100001/chrome/common/render_m... chrome/common/render_messages.h:345: // Tells the browser to open a PDF file in a new tab. Used when plugins are On 2017/07/12 22:57:30, Lei Zhang wrote: > On 2017/07/12 19:12:20, amberwon wrote: > > On 2017/07/12 00:51:37, Lei Zhang wrote: > > > On 2017/07/11 18:08:05, amberwon wrote: > > > > On 2017/07/07 22:47:58, Lei Zhang wrote: > > > > > plugins are disabled -> there is no PDF viewer available ? > > > > > > > > I believe just Chrome's PDF Viewer is disabled, but other PDF > > > > viewers may be available. > > > > > > If one disables the internal PDF Viewer and installs a Rob's PDF Viewer > > > extension, [1] then a page that embeds a PDF will still display the PDF, and > > the > > > pdf_plugin.html placeholder won't show. > > > > > > By plugins, do you really mean PPAPI plugins, or just anything that can be > > > plugged in, which includes extensions? > > > > > > [1] > > > > > > https://chrome.google.com/webstore/detail/pdf-viewer/oemmndcbldboiebfnladdacb... > > > > Yes I mean PPAPI plugins. I changed the comment to reflect the case where a > user > > has an PDF viewer extension as well. > > So what was wrong with my initial suggestion? I suggested the new version and did not see your original suggestion. The original suggestion seems just fine as well and probably more concise. https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/plugins... File chrome/browser/plugins/plugin_observer.cc (right): https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/plugins... chrome/browser/plugins/plugin_observer.cc:341: if (!url.SchemeIsHTTPOrHTTPS()) On 2017/07/12 23:04:22, dcheng wrote: > On 2017/07/12 22:57:30, Lei Zhang wrote: > > What about ftp://, or file:// ? The latter works from file:///path/to/foo.html > > but not from a web server, due to same-origin policy or what not. > > I think we should be using ChildProcessSecurityPolicy::CanRequestURL() here. I > think we'll need to pass the RenderFrameHost parameter down from > OnMessageReceived() though. Awesome thank dcheng, i did not know about that method.
https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/guest_v... File chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc (right): https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/guest_v... chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:117: void ChromeWebViewPermissionHelperDelegate::OnOpenPDF(const GURL& url) {} Why do we need an empty IPC handler? Can't you just leave the IPC unhandled?
https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/guest_v... File chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc (right): https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/guest_v... chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:117: void ChromeWebViewPermissionHelperDelegate::OnOpenPDF(const GURL& url) {} On 2017/07/13 13:11:12, lfg wrote: > Why do we need an empty IPC handler? Can't you just leave the IPC unhandled? Back in the days when this had an OnOpenAboutPlugins handler, it was also an explicit empty function. IMO simply leaving a message unhandled seems odd because then the next programmer may assume we forgot to handle it. While an explicit empty function does indicate that the programmers explicitly decided not to handle it. It may make sense for the body to contain a comment saying "Guest views should never trigger PDF downloads."
https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/guest_v... File chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc (right): https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/guest_v... chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:117: void ChromeWebViewPermissionHelperDelegate::OnOpenPDF(const GURL& url) {} On 2017/07/13 13:11:12, lfg wrote: > Why do we need an empty IPC handler? Can't you just leave the IPC unhandled? Back in the days when this had an OnOpenAboutPlugins handler, it was also an explicit empty function. IMO simply leaving a message unhandled seems odd because then the next programmer may assume we forgot to handle it. While an explicit empty function does indicate that the programmers explicitly decided not to handle it. It may make sense for the body to contain a comment saying "Guest views should never trigger PDF downloads."
https://codereview.chromium.org/2972123002/diff/100001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/100001/chrome/common/render_m... chrome/common/render_messages.h:345: // Tells the browser to open a PDF file in a new tab. Used when plugins are On 2017/07/12 22:57:30, Lei Zhang (OOO) wrote: > On 2017/07/12 19:12:20, amberwon wrote: > > On 2017/07/12 00:51:37, Lei Zhang wrote: > > > On 2017/07/11 18:08:05, amberwon wrote: > > > > On 2017/07/07 22:47:58, Lei Zhang wrote: > > > > > plugins are disabled -> there is no PDF viewer available ? > > > > > > > > I believe just Chrome's PDF Viewer is disabled, but other PDF > > > > viewers may be available. > > > > > > If one disables the internal PDF Viewer and installs a Rob's PDF Viewer > > > extension, [1] then a page that embeds a PDF will still display the PDF, and > > the > > > pdf_plugin.html placeholder won't show. > > > > > > By plugins, do you really mean PPAPI plugins, or just anything that can be > > > plugged in, which includes extensions? > > > > > > [1] > > > > > > https://chrome.google.com/webstore/detail/pdf-viewer/oemmndcbldboiebfnladdacb... > > > > Yes I mean PPAPI plugins. I changed the comment to reflect the case where a > user > > has an PDF viewer extension as well. > > So what was wrong with my initial suggestion? Done. https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/plugins... File chrome/browser/plugins/plugin_observer.cc (right): https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/plugins... chrome/browser/plugins/plugin_observer.cc:341: if (!url.SchemeIsHTTPOrHTTPS()) On 2017/07/12 23:04:22, dcheng wrote: > On 2017/07/12 22:57:30, Lei Zhang wrote: > > What about ftp://, or file:// ? The latter works from file:///path/to/foo.html > > but not from a web server, due to same-origin policy or what not. > > I think we should be using ChildProcessSecurityPolicy::CanRequestURL() here. I > think we'll need to pass the RenderFrameHost parameter down from > OnMessageReceived() though. Done.
Patchset #9 (id:160001) has been deleted
ipc lgtm
guest_view lgtm with nit. https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/guest_v... File chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc (right): https://codereview.chromium.org/2972123002/diff/140001/chrome/browser/guest_v... chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:117: void ChromeWebViewPermissionHelperDelegate::OnOpenPDF(const GURL& url) {} On 2017/07/13 16:26:29, tommycli wrote: > On 2017/07/13 13:11:12, lfg wrote: > > Why do we need an empty IPC handler? Can't you just leave the IPC unhandled? > > Back in the days when this had an OnOpenAboutPlugins handler, it was also an > explicit empty function. > > IMO simply leaving a message unhandled seems odd because then the next > programmer may assume we forgot to handle it. While an explicit empty function > does indicate that the programmers explicitly decided not to handle it. It may > make sense for the body to contain a comment saying "Guest views should never > trigger PDF downloads." That seems reasonable. Please, add a comment explaining that this is intentionally left blank.
lgtm https://codereview.chromium.org/2972123002/diff/200001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/2972123002/diff/200001/chrome/common/render_m... chrome/common/render_messages.h:345: // Tells the browser to open a PDF file in a new tab. Used when there is no PDF grammar: "there is no PDF Viewer available" or "no PDF Viewer is avaialbe".
The CQ bit was checked by amberwon@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, dcheng@chromium.org, lfg@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2972123002/#ps220001 (title: "Grammar")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1500319362924370,
"parent_rev": "a313c233fa036a1439c203b25d0377d1578dab75", "commit_rev":
"ec2c88974b1a5a58dd5013aa79818ddc5cf823fd"}
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1500319362924370,
"parent_rev": "8ef4d9d1fe146d7f6a5278652768c7bbe21c45dd", "commit_rev":
"254fa14030667b2a7b707e975a23437d9faca45d"}
Message was sent while issue was closed.
Description was changed from ========== Click to Open PDF: Open PDF once View PDF is clicked. pdf_plugin.html shows the file name and a View PDF link. When View PDF is clicked, the callback function sends a message. When this message is handled in PluginObserver, the URL for the PDF file is opened in a new tab. However, in guest view, when the message is handled, nothing is done. BUG=737787 ========== to ========== Click to Open PDF: Open PDF once View PDF is clicked. pdf_plugin.html shows the file name and a View PDF link. When View PDF is clicked, the callback function sends a message. When this message is handled in PluginObserver, the URL for the PDF file is opened in a new tab. However, in guest view, when the message is handled, nothing is done. BUG=737787 Review-Url: https://codereview.chromium.org/2972123002 Cr-Commit-Position: refs/heads/master@{#487235} Committed: https://chromium.googlesource.com/chromium/src/+/254fa14030667b2a7b707e975a23... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/254fa14030667b2a7b707e975a23... |
