Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1085993003:
Plugin Power Saver: Make posters work right when image is 404 Not Found. (Closed)
DescriptionPlugin Power Saver: Make posters work right when image is 404 Not Found.
Currently, when a poster image is specified that's 404 Not Found, or
too small, the placeholder can become too small and unclickable.
This change forces the placeholder HTML to expand to the size of the
WebViewPlugin.
BUG=475653
Committed: https://crrev.com/8d89f69f21d06ab3cd611a4c38eefc22ca64bbbe
Cr-Commit-Position: refs/heads/master@{#326897}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Messages
Total messages: 22 (5 generated)
tommycli@chromium.org changed reviewers: + thestig@chromium.org
thestig: This patch requires https://codereview.chromium.org/1088763002/ to go in first, but can I have a review? Thanks
Is the behavior consistent with the video tag's poster behavior? How about the case where the poster is bigger than the <object> size? Are there any visual changes to the looks of blocked plugins? https://codereview.chromium.org/1085993003/diff/1/chrome/browser/plugins/plug... File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right): https://codereview.chromium.org/1085993003/diff/1/chrome/browser/plugins/plug... chrome/browser/plugins/plugin_power_saver_browsertest.cc:82: IN_PROC_BROWSER_TEST_F(PluginPowerSaverBrowserTest, LargePluginsUsePosters) { Just name this correctly in the CL that adds it.
On 2015/04/15 07:39:07, Lei Zhang (slow to respond) wrote: > Is the behavior consistent with the video tag's poster behavior? > > How about the case where the poster is bigger than the <object> size? > > Are there any visual changes to the looks of blocked plugins? > > https://codereview.chromium.org/1085993003/diff/1/chrome/browser/plugins/plug... > File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right): > > https://codereview.chromium.org/1085993003/diff/1/chrome/browser/plugins/plug... > chrome/browser/plugins/plugin_power_saver_browsertest.cc:82: > IN_PROC_BROWSER_TEST_F(PluginPowerSaverBrowserTest, LargePluginsUsePosters) { > Just name this correctly in the CL that adds it. Good call: I've modified the patch to make the behavior exactly consistent with the video tag's posters. (Prevent image aspect ratio change, hide broken image links).
On 2015/04/15 23:44:10, tommycli wrote: > On 2015/04/15 07:39:07, Lei Zhang (slow to respond) wrote: > > Is the behavior consistent with the video tag's poster behavior? > > > > How about the case where the poster is bigger than the <object> size? > > > > Are there any visual changes to the looks of blocked plugins? > > > > > https://codereview.chromium.org/1085993003/diff/1/chrome/browser/plugins/plug... > > File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right): > > > > > https://codereview.chromium.org/1085993003/diff/1/chrome/browser/plugins/plug... > > chrome/browser/plugins/plugin_power_saver_browsertest.cc:82: > > IN_PROC_BROWSER_TEST_F(PluginPowerSaverBrowserTest, LargePluginsUsePosters) { > > Just name this correctly in the CL that adds it. > > Good call: I've modified the patch to make the behavior exactly consistent with > the video tag's posters. (Prevent image aspect ratio change, hide broken image > links). ... and a broken image url still counts as a poster, so the plugin is throttled? Ask Dan to take a look at the JS/CSS. I'm not familiar with object-fit.
tommycli@chromium.org changed reviewers: + dbeam@chromium.org
thestig: Yes, broken image link is still counts as a poster. On <video>, it's just a blank white poster, which is what we do here too. dbeam: Do the CSS changes seem reasonable? specifically the 'object-fit' usage. https://codereview.chromium.org/1085993003/diff/1/chrome/browser/plugins/plug... File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right): https://codereview.chromium.org/1085993003/diff/1/chrome/browser/plugins/plug... chrome/browser/plugins/plugin_power_saver_browsertest.cc:82: IN_PROC_BROWSER_TEST_F(PluginPowerSaverBrowserTest, LargePluginsUsePosters) { On 2015/04/15 07:39:07, Lei Zhang (slow to respond) wrote: > Just name this correctly in the CL that adds it. Done.
Generally lgtm (haven't used object-fit but in reading the MDN page you seem to bebDoing It Right) https://codereview.chromium.org/1085993003/diff/40001/chrome/renderer/resourc... File chrome/renderer/resources/plugin_placeholders.css (right): https://codereview.chromium.org/1085993003/diff/40001/chrome/renderer/resourc... chrome/renderer/resources/plugin_placeholders.css:5: html { Nit: instead of: html { ... } body { ... } just html, body { ... } https://codereview.chromium.org/1085993003/diff/40001/chrome/renderer/resourc... chrome/renderer/resources/plugin_placeholders.css:16: height: 100%; Can you alpha sort the rules in each block? https://codereview.chromium.org/1085993003/diff/40001/chrome/renderer/resourc... File chrome/renderer/resources/plugin_poster.html (right): https://codereview.chromium.org/1085993003/diff/40001/chrome/renderer/resourc... chrome/renderer/resources/plugin_poster.html:68: onerror="this.style.display = 'none';"> nit: instead of this.style.display = 'none'; Use this.hidden = true; Also, try not to use onerror or other event handler attributes in HTML. Better: <script> poster.onerror = function() { poster.hidden = true; }; </script> Or something along these lines.
thestig/dbeam: done! https://codereview.chromium.org/1085993003/diff/40001/chrome/renderer/resourc... File chrome/renderer/resources/plugin_placeholders.css (right): https://codereview.chromium.org/1085993003/diff/40001/chrome/renderer/resourc... chrome/renderer/resources/plugin_placeholders.css:5: html { On 2015/04/17 03:53:53, Dan Beam wrote: > Nit: instead of: > > html { > ... > } > > body { > ... > } > > just > > html, > body { > ... > } Done. https://codereview.chromium.org/1085993003/diff/40001/chrome/renderer/resourc... chrome/renderer/resources/plugin_placeholders.css:16: height: 100%; On 2015/04/17 03:53:53, Dan Beam wrote: > Can you alpha sort the rules in each block? Done. https://codereview.chromium.org/1085993003/diff/40001/chrome/renderer/resourc... File chrome/renderer/resources/plugin_poster.html (right): https://codereview.chromium.org/1085993003/diff/40001/chrome/renderer/resourc... chrome/renderer/resources/plugin_poster.html:68: onerror="this.style.display = 'none';"> On 2015/04/17 03:53:53, Dan Beam wrote: > nit: instead of > > this.style.display = 'none'; > > Use > > this.hidden = true; > > Also, try not to use onerror or other event handler attributes in HTML. > > Better: > > <script> > poster.onerror = function() { > poster.hidden = true; > }; > </script> > > Or something along these lines. Done.
https://codereview.chromium.org/1085993003/diff/40001/chrome/renderer/resourc... File chrome/renderer/resources/plugin_placeholders.css (right): https://codereview.chromium.org/1085993003/diff/40001/chrome/renderer/resourc... chrome/renderer/resources/plugin_placeholders.css:16: height: 100%; On 2015/04/22 20:11:33, tommycli wrote: > On 2015/04/17 03:53:53, Dan Beam wrote: > > Can you alpha sort the rules in each block? > > Done. missed this https://codereview.chromium.org/1085993003/diff/60001/chrome/renderer/resourc... File chrome/renderer/resources/plugin_poster.html (right): https://codereview.chromium.org/1085993003/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/plugin_poster.html:77: <script> nit: +\s\s indent, e.g. <script> // start here, \s\s more than <script> https://codereview.chromium.org/1085993003/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/plugin_poster.html:84: }; you may need to defer calling getElementById() until the document is read (e.g. 'load' or 'DOMContentLoaded' events) or null might be returned
https://codereview.chromium.org/1085993003/diff/60001/chrome/renderer/resourc... File chrome/renderer/resources/plugin_poster.html (right): https://codereview.chromium.org/1085993003/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/plugin_poster.html:84: }; On 2015/04/22 20:19:17, Dan Beam wrote: > you may need to defer calling getElementById() until the document is read (e.g. > 'load' or 'DOMContentLoaded' events) or null might be returned eh, actually seems fine: http://stackoverflow.com/questions/2024018/using-domcontentready-considered-a...
dbeam: addressed your coments below. Thanks! https://codereview.chromium.org/1085993003/diff/40001/chrome/renderer/resourc... File chrome/renderer/resources/plugin_placeholders.css (right): https://codereview.chromium.org/1085993003/diff/40001/chrome/renderer/resourc... chrome/renderer/resources/plugin_placeholders.css:16: height: 100%; On 2015/04/22 20:19:17, Dan Beam wrote: > On 2015/04/22 20:11:33, tommycli wrote: > > On 2015/04/17 03:53:53, Dan Beam wrote: > > > Can you alpha sort the rules in each block? > > > > Done. > > missed this Done. Could of sworn I did this. maybe I didn't check it in. https://codereview.chromium.org/1085993003/diff/60001/chrome/renderer/resourc... File chrome/renderer/resources/plugin_poster.html (right): https://codereview.chromium.org/1085993003/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/plugin_poster.html:77: <script> On 2015/04/22 20:19:17, Dan Beam wrote: > nit: +\s\s indent, e.g. > > <script> > // start here, \s\s more than <script> Done.
lgtm
lgtm
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1085993003/80001
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 tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1085993003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8d89f69f21d06ab3cd611a4c38eefc22ca64bbbe Cr-Commit-Position: refs/heads/master@{#326897} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
