Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2961383002:
Show a new placeholder for PDF's when plugins are disabled. (Closed)
DescriptionShow a new placeholder for PDF's when plugins are disabled.
Created PDFPluginPlaceholder class that is used to create the
placeholder in ChromeContentRendererClient, when plugins are
disabled and a PDF is supposed to be displayed. The current placeholder
says "This plugin is not supported," but does not allow the user to
open the PDF. The new placeholder is blank, for now, but will later
allow users to open PDF's by clicking a button.
BUG=737787
Review-Url: https://codereview.chromium.org/2961383002
Cr-Commit-Position: refs/heads/master@{#483895}
Committed: https://chromium.googlesource.com/chromium/src/+/daee0ce8d5a883fd879a1d6988333f5efb21dc3a
Patch Set 1 #Patch Set 2 : Forgot parentheses #
Total comments: 6
Patch Set 3 : Add comments, change function name and arguments #
Total comments: 2
Patch Set 4 : comment #
Total comments: 16
Patch Set 5 : . #
Total comments: 4
Patch Set 6 : . #
Messages
Total messages: 46 (26 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 ========== Show a new placeholder for PDF's when plugins are disabled. The current placeholder says "This plugin is not supported," but does not allow the user to open the PDF's. The new placeholder is blank, for now, but will later allow users to open PDF's by clicking a button. BUG=737787 ========== to ========== Show a new placeholder for PDF's when plugins are disabled. Created PDFPluginPlaceholder class that is used to create the placeholder in chrome_content_renderer_client, when plugins are disabled and a PDF is supposed to be displayed. The current placeholder says "This plugin is not supported," but does not allow the user to open the PDF. The new placeholder is blank, for now, but will later allow users to open PDF's by clicking a button. BUG=737787 ==========
amberwon@google.com changed reviewers: + tommycli@chromium.org
some fairly trivial comments below https://codereview.chromium.org/2961383002/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2961383002/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:892: ->plugin(); Worth adding a comment indicating that the placeholder is owned by the returned plugin. https://codereview.chromium.org/2961383002/diff/20001/chrome/renderer/plugins... File chrome/renderer/plugins/pdf_plugin_placeholder.h (right): https://codereview.chromium.org/2961383002/diff/20001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:10: class PDFPluginPlaceholder final : public plugins::PluginPlaceholderBase, Add a comment as to the purpose of the class. And in that comment probably add a TODO for you to flesh out the class more. https://codereview.chromium.org/2961383002/diff/20001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:14: static PDFPluginPlaceholder* CreateBlockedPlugin( Should probably be CreatePDFPlaceholder ... and there's no reason to take the template_id as a parameter, since it will always be fixed. One more nit: one space above this method plz
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
https://codereview.chromium.org/2961383002/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2961383002/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:892: ->plugin(); On 2017/06/29 23:31:41, tommycli wrote: > Worth adding a comment indicating that the placeholder is owned by the returned > plugin. Done. https://codereview.chromium.org/2961383002/diff/20001/chrome/renderer/plugins... File chrome/renderer/plugins/pdf_plugin_placeholder.h (right): https://codereview.chromium.org/2961383002/diff/20001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:10: class PDFPluginPlaceholder final : public plugins::PluginPlaceholderBase, On 2017/06/29 23:31:41, tommycli wrote: > Add a comment as to the purpose of the class. And in that comment probably add a > TODO for you to flesh out the class more. Done. https://codereview.chromium.org/2961383002/diff/20001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:14: static PDFPluginPlaceholder* CreateBlockedPlugin( On 2017/06/29 23:31:41, tommycli wrote: > Should probably be CreatePDFPlaceholder ... and there's no reason to take the > template_id as a parameter, since it will always be fixed. > > One more nit: one space above this method plz Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with one nit below https://codereview.chromium.org/2961383002/diff/40001/chrome/renderer/plugins... File chrome/renderer/plugins/pdf_plugin_placeholder.h (right): https://codereview.chromium.org/2961383002/diff/40001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:12: // TODO(amberwon): Flesh out the class more. "...to download an embedded PDF when the PDF plugin is disabled or unavailable."
https://codereview.chromium.org/2961383002/diff/40001/chrome/renderer/plugins... File chrome/renderer/plugins/pdf_plugin_placeholder.h (right): https://codereview.chromium.org/2961383002/diff/40001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:12: // TODO(amberwon): Flesh out the class more. On 2017/06/30 15:31:18, tommycli wrote: > "...to download an embedded PDF when the PDF plugin is disabled or unavailable." Done.
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 Link to the patchset: https://codereview.chromium.org/2961383002/#ps60001 (title: "comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
amberwon@google.com changed reviewers: + thestig@chromium.org
https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... File chrome/renderer/plugins/pdf_plugin_placeholder.h (right): https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:10: // Creates a placeholder that allows users to click to download a PDF for when nit: The class dosen't create a placeholder. It is the placeholder, right? https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:17: static gin::WrapperInfo kWrapperInfo; Does this need the const keyword? https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:19: static PDFPluginPlaceholder* CreatePDFPlaceholder( Can you add a comment to explain the ownership of the returned pointer? https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:27: ~PDFPluginPlaceholder() override; We usually don't bother marking classes / methods as final, unless there's some specific reason. If we are going to use final for a class, then all of its overrides should be final too. https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/resourc... File chrome/renderer/resources/renderer_resources.grd (right): https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/renderer_resources.grd:17: <include name="IDR_BLOCKED_PLUGIN_HTML" file="plugins/blocked_plugin.html" flattenhtml="true" type="BINDATA" /> tommycli: FYI, you may want to wrap these in <if expr="enable_plugins"> in a separate CL. https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/renderer_resources.grd:21: <include name="IDR_PDF_PLUGIN_HTML" file="plugins/pdf_plugin.html" flattenhtml="true" type="BINDATA" /> nit: The rest of the list is in alphabetical order. Let's keep it that way.
Also, can you wrap the commit message to 72 columns?
thanks lei https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/resourc... File chrome/renderer/resources/renderer_resources.grd (right): https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/renderer_resources.grd:17: <include name="IDR_BLOCKED_PLUGIN_HTML" file="plugins/blocked_plugin.html" flattenhtml="true" type="BINDATA" /> On 2017/06/30 19:35:22, Lei Zhang wrote: > tommycli: FYI, you may want to wrap these in <if expr="enable_plugins"> in a > separate CL. Hmm. yeah some of these will need to be in that... but disabled_plugin.html and pdf_plugin.html at least we will definitely need even with enable_plugins = false.
Description was changed from ========== Show a new placeholder for PDF's when plugins are disabled. Created PDFPluginPlaceholder class that is used to create the placeholder in chrome_content_renderer_client, when plugins are disabled and a PDF is supposed to be displayed. The current placeholder says "This plugin is not supported," but does not allow the user to open the PDF. The new placeholder is blank, for now, but will later allow users to open PDF's by clicking a button. BUG=737787 ========== to ========== Show a new placeholder for PDF's when plugins are disabled. Created PDFPluginPlaceholder class that is used to create the placeholder in chrome_content_renderer_client, when plugins are disabled and a PDF is supposed to be displayed. The current placeholder says "This plugin is not supported," but does not allow the user to open the PDF. The new placeholder is blank, for now, but will later allow users to open PDF's by clicking a button. BUG=737787 ==========
https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... File chrome/renderer/plugins/pdf_plugin_placeholder.h (right): https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:10: // Creates a placeholder that allows users to click to download a PDF for when On 2017/06/30 19:35:21, Lei Zhang wrote: > nit: The class dosen't create a placeholder. It is the placeholder, right? Done. https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:17: static gin::WrapperInfo kWrapperInfo; On 2017/06/30 19:35:21, Lei Zhang wrote: > Does this need the const keyword? No, in the Wrappable class, kWrapperInfo is passed in a function call that requires a non-const value. https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:19: static PDFPluginPlaceholder* CreatePDFPlaceholder( On 2017/06/30 19:35:21, Lei Zhang wrote: > Can you add a comment to explain the ownership of the returned pointer? Done. https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:27: ~PDFPluginPlaceholder() override; On 2017/06/30 19:35:21, Lei Zhang wrote: > We usually don't bother marking classes / methods as final, unless there's some > specific reason. If we are going to use final for a class, then all of its > overrides should be final too. Done. https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/resourc... File chrome/renderer/resources/renderer_resources.grd (right): https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/renderer_resources.grd:21: <include name="IDR_PDF_PLUGIN_HTML" file="plugins/pdf_plugin.html" flattenhtml="true" type="BINDATA" /> On 2017/06/30 19:35:22, Lei Zhang wrote: > nit: The rest of the list is in alphabetical order. Let's keep it that way. Done.
The commit message refers to chrome_content_renderer_client, which sounds like a variable for a class instance, except there is no variable with that name. Did you mean ChromeContentRendererClient (the class) instead?
https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... File chrome/renderer/plugins/pdf_plugin_placeholder.h (right): https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:17: static gin::WrapperInfo kWrapperInfo; On 2017/06/30 21:59:22, amberwon wrote: > On 2017/06/30 19:35:21, Lei Zhang wrote: > > Does this need the const keyword? > > No, in the Wrappable class, kWrapperInfo is passed in a function call that > requires a non-const value. Ok, I guess this is a weird Gin thing. Let's leave it be. https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:27: ~PDFPluginPlaceholder() override; On 2017/06/30 21:59:22, amberwon wrote: > On 2017/06/30 19:35:21, Lei Zhang wrote: > > We usually don't bother marking classes / methods as final, unless there's > some > > specific reason. If we are going to use final for a class, then all of its > > overrides should be final too. > > Done. So they are now consistent, but how about we make them finals instead of overrides? https://codereview.chromium.org/2961383002/diff/80001/chrome/renderer/plugins... File chrome/renderer/plugins/pdf_plugin_placeholder.h (right): https://codereview.chromium.org/2961383002/diff/80001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:19: // Returned pointer is owned by plugin, which can be returned by the member Nobody actually owns the pointer. It's the object whose lifetime is actually important. https://codereview.chromium.org/2961383002/diff/80001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:20: // function plugin(). Except I don't see a plugin() function in this file, so as a reader of this comment, it's not obvious what it is referring to. So putting it all together, how about: Returned placeholder is owned by the associated plugin, which can be retrieved with PluginPlaceholderBase::plugin().
https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... File chrome/renderer/plugins/pdf_plugin_placeholder.h (right): https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:17: static gin::WrapperInfo kWrapperInfo; On 2017/06/30 22:45:11, Lei Zhang wrote: > On 2017/06/30 21:59:22, amberwon wrote: > > On 2017/06/30 19:35:21, Lei Zhang wrote: > > > Does this need the const keyword? > > > > No, in the Wrappable class, kWrapperInfo is passed in a function call that > > requires a non-const value. > > Ok, I guess this is a weird Gin thing. Let's leave it be. Acknowledged. https://codereview.chromium.org/2961383002/diff/60001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:27: ~PDFPluginPlaceholder() override; On 2017/06/30 22:45:11, Lei Zhang wrote: > On 2017/06/30 21:59:22, amberwon wrote: > > On 2017/06/30 19:35:21, Lei Zhang wrote: > > > We usually don't bother marking classes / methods as final, unless there's > > some > > > specific reason. If we are going to use final for a class, then all of its > > > overrides should be final too. > > > > Done. > > So they are now consistent, but how about we make them finals instead of > overrides? Done. https://codereview.chromium.org/2961383002/diff/80001/chrome/renderer/plugins... File chrome/renderer/plugins/pdf_plugin_placeholder.h (right): https://codereview.chromium.org/2961383002/diff/80001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:19: // Returned pointer is owned by plugin, which can be returned by the member On 2017/06/30 22:45:11, Lei Zhang wrote: > Nobody actually owns the pointer. It's the object whose lifetime is actually > important. Done. https://codereview.chromium.org/2961383002/diff/80001/chrome/renderer/plugins... chrome/renderer/plugins/pdf_plugin_placeholder.h:20: // function plugin(). On 2017/06/30 22:45:11, Lei Zhang wrote: > Except I don't see a plugin() function in this file, so as a reader of this > comment, it's not obvious what it is referring to. So putting it all together, > how about: > > Returned placeholder is owned by the associated plugin, which can be retrieved > with PluginPlaceholderBase::plugin(). Done.
Description was changed from ========== Show a new placeholder for PDF's when plugins are disabled. Created PDFPluginPlaceholder class that is used to create the placeholder in chrome_content_renderer_client, when plugins are disabled and a PDF is supposed to be displayed. The current placeholder says "This plugin is not supported," but does not allow the user to open the PDF. The new placeholder is blank, for now, but will later allow users to open PDF's by clicking a button. BUG=737787 ========== to ========== Show a new placeholder for PDF's when plugins are disabled. Created PDFPluginPlaceholder class that is used to create the placeholder in ChromeContentRendererClient, when plugins are disabled and a PDF is supposed to be displayed. The current placeholder says "This plugin is not supported," but does not allow the user to open the PDF. The new placeholder is blank, for now, but will later allow users to open PDF's by clicking a button. BUG=737787 ==========
On 2017/06/30 22:36:08, Lei Zhang wrote: > The commit message refers to chrome_content_renderer_client, which sounds like a > variable for a class instance, except there is no variable with that name. Did > you mean ChromeContentRendererClient (the class) instead? I made this change. lgtm
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 Link to the patchset: https://codereview.chromium.org/2961383002/#ps100001 (title: ".")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by thestig@chromium.org
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": 100001, "attempt_start_ts": 1498871688512030,
"parent_rev": "de4949205312c49050928fa46e45759bdb8cc2da", "commit_rev":
"daee0ce8d5a883fd879a1d6988333f5efb21dc3a"}
Message was sent while issue was closed.
Description was changed from ========== Show a new placeholder for PDF's when plugins are disabled. Created PDFPluginPlaceholder class that is used to create the placeholder in ChromeContentRendererClient, when plugins are disabled and a PDF is supposed to be displayed. The current placeholder says "This plugin is not supported," but does not allow the user to open the PDF. The new placeholder is blank, for now, but will later allow users to open PDF's by clicking a button. BUG=737787 ========== to ========== Show a new placeholder for PDF's when plugins are disabled. Created PDFPluginPlaceholder class that is used to create the placeholder in ChromeContentRendererClient, when plugins are disabled and a PDF is supposed to be displayed. The current placeholder says "This plugin is not supported," but does not allow the user to open the PDF. The new placeholder is blank, for now, but will later allow users to open PDF's by clicking a button. BUG=737787 Review-Url: https://codereview.chromium.org/2961383002 Cr-Commit-Position: refs/heads/master@{#483895} Committed: https://chromium.googlesource.com/chromium/src/+/daee0ce8d5a883fd879a1d698833... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/daee0ce8d5a883fd879a1d698833... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
