Plugin Power Saver: Throttle 'large' plugins that appear small.

Plugin Power Saver: Throttle 'large' plugins that appear small.

Some plugins are large, but only a small portion of it is visible to
the user. An example might be a large SWF in a small div container.

This is to allow the SWF to dynamically expand via JS. However, the
throttling criteria should be based on the visible size rather than the SWF size.

This patch fixes that by walking up the DOM tree and looking for smaller
parent elements and using that as the visible size.

This patch also centers the 'play' button on the visible area rather
than the whole keyframe area, which may be much larger than the visible
area.

BUG=468142, 403800
Committed: https://crrev.com/c3ace4f15c91440c2dad238731422fe29b1b4373
Cr-Commit-Position: refs/heads/master@{#323331}

[tommycli, 2015-03-18 00:06:46]

tommycli@chromium.org changed reviewers:
+ piman@chromium.org, thestig@chromium.org

[tommycli, 2015-03-18 00:06:47]

thestig / piman: PTAL

The patch seems unnecessarily large given the apparent simplicity of what I'm
trying to accomplish. I'm happy to take any simplifying suggestions.

Thanks!

[tommycli, 2015-03-18 00:07:38]

+cc groby

Let me know if you have any feedback on this approach (walk up DOM tree looking
for smaller parent elements to find the actual visible size of plugin).

[Lei Zhang, 2015-03-18 00:19:32]

- naive -> native in the CL desc?
- Use a gfx::Size instead of a pair of ints.

https://codereview.chromium.org/1015023002/diff/1/chrome/renderer/plugins/plu...
File chrome/renderer/plugins/plugin_preroller.cc (right):

https://codereview.chromium.org/1015023002/diff/1/chrome/renderer/plugins/plu...
chrome/renderer/plugins/plugin_preroller.cc:57: keyframe_width_ =
bitmap->width();
This is gfx::Size::SetToMin(), right?

https://codereview.chromium.org/1015023002/diff/1/chrome/renderer/plugins/plu...
chrome/renderer/plugins/plugin_preroller.cc:65: int width =
std::min(keyframe_width_, throttler_->GetWidth());
This is gfx::Size::SetToMin() right?

[tommycli, 2015-03-18 00:39:33]

thestig: thanks. That eliminated some sloc

https://codereview.chromium.org/1015023002/diff/1/chrome/renderer/plugins/plu...
File chrome/renderer/plugins/plugin_preroller.cc (right):

https://codereview.chromium.org/1015023002/diff/1/chrome/renderer/plugins/plu...
chrome/renderer/plugins/plugin_preroller.cc:57: keyframe_width_ =
bitmap->width();
On 2015/03/18 00:19:32, Lei Zhang wrote:
> This is gfx::Size::SetToMin(), right?

Done.

https://codereview.chromium.org/1015023002/diff/1/chrome/renderer/plugins/plu...
chrome/renderer/plugins/plugin_preroller.cc:65: int width =
std::min(keyframe_width_, throttler_->GetWidth());
On 2015/03/18 00:19:32, Lei Zhang wrote:
> This is gfx::Size::SetToMin() right?

Done.

[Lei Zhang, 2015-03-18 00:48:12]

Is it possible to add a test for this?

https://codereview.chromium.org/1015023002/diff/20001/content/public/renderer...
File content/public/renderer/plugin_instance_throttler.h (right):

https://codereview.chromium.org/1015023002/diff/20001/content/public/renderer...
content/public/renderer/plugin_instance_throttler.h:91: virtual const gfx::Size&
GetSize() const = 0;
You should comment what this is exactly.

https://codereview.chromium.org/1015023002/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/1015023002/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:821: blink::WebRect
bounds = node.to<WebElement>().boundsInViewportSpace();
You can change the type of |bounds| to gfx::Rect() and use SetToMin() below.

https://codereview.chromium.org/1015023002/diff/20001/content/renderer/pepper...
File content/renderer/pepper/plugin_instance_throttler_impl.cc (right):

https://codereview.chromium.org/1015023002/diff/20001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.cc:118: initialized_size_
= size;
put this in the initializer list?

https://codereview.chromium.org/1015023002/diff/20001/content/renderer/pepper...
File content/renderer/pepper/plugin_instance_throttler_impl.h (right):

https://codereview.chromium.org/1015023002/diff/20001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.h:100: gfx::Size
initialized_size_;
const?

[piman, 2015-03-18 01:58:57]

https://codereview.chromium.org/1015023002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/1015023002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:823: }
This seems crazy. It's also most likely wrong because you're not taking the
overflow property into account. And you shouldn't be trying to duplicate blink's
layout code.

Is the size you want the clip rect passed to ViewChanged?

[tommycli, 2015-03-19 22:10:13]

piman/thestig: addressed your concerns below.

thestig: Will remember to look into some kind of test once piman approves /
disapproves of the overall methodology.

https://codereview.chromium.org/1015023002/diff/20001/content/public/renderer...
File content/public/renderer/plugin_instance_throttler.h (right):

https://codereview.chromium.org/1015023002/diff/20001/content/public/renderer...
content/public/renderer/plugin_instance_throttler.h:91: virtual const gfx::Size&
GetSize() const = 0;
On 2015/03/18 00:48:12, Lei Zhang wrote:
> You should comment what this is exactly.

Done.

https://codereview.chromium.org/1015023002/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/1015023002/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:821: blink::WebRect
bounds = node.to<WebElement>().boundsInViewportSpace();
On 2015/03/18 00:48:12, Lei Zhang wrote:
> You can change the type of |bounds| to gfx::Rect() and use SetToMin() below.

Done.

https://codereview.chromium.org/1015023002/diff/20001/content/renderer/pepper...
File content/renderer/pepper/plugin_instance_throttler_impl.cc (right):

https://codereview.chromium.org/1015023002/diff/20001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.cc:118: initialized_size_
= size;
On 2015/03/18 00:48:12, Lei Zhang wrote:
> put this in the initializer list?

I was not able to, as the Throttler is constructed way before Initialize(...) is
called.

https://codereview.chromium.org/1015023002/diff/20001/content/renderer/pepper...
File content/renderer/pepper/plugin_instance_throttler_impl.h (right):

https://codereview.chromium.org/1015023002/diff/20001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.h:100: gfx::Size
initialized_size_;
On 2015/03/18 00:48:12, Lei Zhang wrote:
> const?

Can not, since it's assigned in Initialize, right?

https://codereview.chromium.org/1015023002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/1015023002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:823: }
On 2015/03/18 01:58:57, piman (Very slow to review) wrote:
> This seems crazy. It's also most likely wrong because you're not taking the
> overflow property into account. And you shouldn't be trying to duplicate
blink's
> layout code.
> 
> Is the size you want the clip rect passed to ViewChanged?

Hi piman,

The clip rect passed to ViewChanged was the first thing (of many) that I tried.
In fact, it's very-very close to what I want.

It fails when the plugin is below the fold when the page loads.

I agree that this logic fails on the overflow property.

Happy to take more suggestions.

Tommy

[Lei Zhang, 2015-03-19 22:26:42]

You should be able to add a test independent of the implementation because you
already know how to trigger the problem and you know the expected behavior.

[Lei Zhang, 2015-03-19 22:29:45]

https://codereview.chromium.org/1015023002/diff/20001/content/renderer/pepper...
File content/renderer/pepper/plugin_instance_throttler_impl.cc (right):

https://codereview.chromium.org/1015023002/diff/20001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.cc:118: initialized_size_
= size;
On 2015/03/19 22:10:12, tommycli wrote:
> On 2015/03/18 00:48:12, Lei Zhang wrote:
> > put this in the initializer list?
> 
> I was not able to, as the Throttler is constructed way before Initialize(...)
is
> called.

Whoops, I was thinking this was the ctor. Ignore.

[piman, 2015-03-19 22:43:04]

On Thu, Mar 19, 2015 at 3:10 PM, <tommycli@chromium.org> wrote:

> piman/thestig: addressed your concerns below.
>
> thestig: Will remember to look into some kind of test once piman approves /
> disapproves of the overall methodology.
>
>
> https://codereview.chromium.org/1015023002/diff/20001/
> content/public/renderer/plugin_instance_throttler.h
> File content/public/renderer/plugin_instance_throttler.h (right):
>
> https://codereview.chromium.org/1015023002/diff/20001/
> content/public/renderer/plugin_instance_throttler.h#newcode91
> content/public/renderer/plugin_instance_throttler.h:91: virtual const
> gfx::Size& GetSize() const = 0;
> On 2015/03/18 00:48:12, Lei Zhang wrote:
>
>> You should comment what this is exactly.
>>
>
> Done.
>
> https://codereview.chromium.org/1015023002/diff/20001/
> content/renderer/pepper/pepper_plugin_instance_impl.cc
> File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):
>
> https://codereview.chromium.org/1015023002/diff/20001/
> content/renderer/pepper/pepper_plugin_instance_impl.cc#newcode821
> content/renderer/pepper/pepper_plugin_instance_impl.cc:821:
> blink::WebRect bounds = node.to<WebElement>().boundsInViewportSpace();
> On 2015/03/18 00:48:12, Lei Zhang wrote:
>
>> You can change the type of |bounds| to gfx::Rect() and use SetToMin()
>>
> below.
>
> Done.
>
> https://codereview.chromium.org/1015023002/diff/20001/
> content/renderer/pepper/plugin_instance_throttler_impl.cc
> File content/renderer/pepper/plugin_instance_throttler_impl.cc (right):
>
> https://codereview.chromium.org/1015023002/diff/20001/
> content/renderer/pepper/plugin_instance_throttler_impl.cc#newcode118
> content/renderer/pepper/plugin_instance_throttler_impl.cc:118:
> initialized_size_ = size;
> On 2015/03/18 00:48:12, Lei Zhang wrote:
>
>> put this in the initializer list?
>>
>
> I was not able to, as the Throttler is constructed way before
> Initialize(...) is called.
>
> https://codereview.chromium.org/1015023002/diff/20001/
> content/renderer/pepper/plugin_instance_throttler_impl.h
> File content/renderer/pepper/plugin_instance_throttler_impl.h (right):
>
> https://codereview.chromium.org/1015023002/diff/20001/
> content/renderer/pepper/plugin_instance_throttler_impl.h#newcode100
> content/renderer/pepper/plugin_instance_throttler_impl.h:100: gfx::Size
> initialized_size_;
> On 2015/03/18 00:48:12, Lei Zhang wrote:
>
>> const?
>>
>
> Can not, since it's assigned in Initialize, right?
>
> https://codereview.chromium.org/1015023002/diff/40001/
> content/renderer/pepper/pepper_plugin_instance_impl.cc
> File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):
>
> https://codereview.chromium.org/1015023002/diff/40001/
> content/renderer/pepper/pepper_plugin_instance_impl.cc#newcode823
> content/renderer/pepper/pepper_plugin_instance_impl.cc:823: }
> On 2015/03/18 01:58:57, piman (Very slow to review) wrote:
>
>> This seems crazy. It's also most likely wrong because you're not
>>
> taking the
>
>> overflow property into account. And you shouldn't be trying to
>>
> duplicate blink's
>
>> layout code.
>>
>
>  Is the size you want the clip rect passed to ViewChanged?
>>
>
> Hi piman,
>
> The clip rect passed to ViewChanged was the first thing (of many) that I
> tried. In fact, it's very-very close to what I want.
>
> It fails when the plugin is below the fold when the page loads.
>
> I agree that this logic fails on the overflow property.
>
> Happy to take more suggestions.
>

Add the logic on the blink side?
The code that clips to the frame is
in
WebPluginContainerImpl::calculateGeometry/WebPluginContainerImpl::windowClipRect.
Maybe the code could separate those 2 and pass them to the WebPlugin?



>
> Tommy
>
> https://codereview.chromium.org/1015023002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2015-03-19 22:47:55]

piman: That seems reasonable. I'll give that a shot.

On Thu, Mar 19, 2015 at 3:42 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Thu, Mar 19, 2015 at 3:10 PM, <tommycli@chromium.org> wrote:
>
>> piman/thestig: addressed your concerns below.
>>
>> thestig: Will remember to look into some kind of test once piman approves
>> /
>> disapproves of the overall methodology.
>>
>>
>> https://codereview.chromium.org/1015023002/diff/20001/
>> content/public/renderer/plugin_instance_throttler.h
>> File content/public/renderer/plugin_instance_throttler.h (right):
>>
>> https://codereview.chromium.org/1015023002/diff/20001/
>> content/public/renderer/plugin_instance_throttler.h#newcode91
>> content/public/renderer/plugin_instance_throttler.h:91: virtual const
>> gfx::Size& GetSize() const = 0;
>> On 2015/03/18 00:48:12, Lei Zhang wrote:
>>
>>> You should comment what this is exactly.
>>>
>>
>> Done.
>>
>> https://codereview.chromium.org/1015023002/diff/20001/
>> content/renderer/pepper/pepper_plugin_instance_impl.cc
>> File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):
>>
>> https://codereview.chromium.org/1015023002/diff/20001/
>> content/renderer/pepper/pepper_plugin_instance_impl.cc#newcode821
>> content/renderer/pepper/pepper_plugin_instance_impl.cc:821:
>> blink::WebRect bounds = node.to<WebElement>().boundsInViewportSpace();
>> On 2015/03/18 00:48:12, Lei Zhang wrote:
>>
>>> You can change the type of |bounds| to gfx::Rect() and use SetToMin()
>>>
>> below.
>>
>> Done.
>>
>> https://codereview.chromium.org/1015023002/diff/20001/
>> content/renderer/pepper/plugin_instance_throttler_impl.cc
>> File content/renderer/pepper/plugin_instance_throttler_impl.cc (right):
>>
>> https://codereview.chromium.org/1015023002/diff/20001/
>> content/renderer/pepper/plugin_instance_throttler_impl.cc#newcode118
>> content/renderer/pepper/plugin_instance_throttler_impl.cc:118:
>> initialized_size_ = size;
>> On 2015/03/18 00:48:12, Lei Zhang wrote:
>>
>>> put this in the initializer list?
>>>
>>
>> I was not able to, as the Throttler is constructed way before
>> Initialize(...) is called.
>>
>> https://codereview.chromium.org/1015023002/diff/20001/
>> content/renderer/pepper/plugin_instance_throttler_impl.h
>> File content/renderer/pepper/plugin_instance_throttler_impl.h (right):
>>
>> https://codereview.chromium.org/1015023002/diff/20001/
>> content/renderer/pepper/plugin_instance_throttler_impl.h#newcode100
>> content/renderer/pepper/plugin_instance_throttler_impl.h:100: gfx::Size
>> initialized_size_;
>> On 2015/03/18 00:48:12, Lei Zhang wrote:
>>
>>> const?
>>>
>>
>> Can not, since it's assigned in Initialize, right?
>>
>> https://codereview.chromium.org/1015023002/diff/40001/
>> content/renderer/pepper/pepper_plugin_instance_impl.cc
>> File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):
>>
>> https://codereview.chromium.org/1015023002/diff/40001/
>> content/renderer/pepper/pepper_plugin_instance_impl.cc#newcode823
>> content/renderer/pepper/pepper_plugin_instance_impl.cc:823: }
>> On 2015/03/18 01:58:57, piman (Very slow to review) wrote:
>>
>>> This seems crazy. It's also most likely wrong because you're not
>>>
>> taking the
>>
>>> overflow property into account. And you shouldn't be trying to
>>>
>> duplicate blink's
>>
>>> layout code.
>>>
>>
>>  Is the size you want the clip rect passed to ViewChanged?
>>>
>>
>> Hi piman,
>>
>> The clip rect passed to ViewChanged was the first thing (of many) that I
>> tried. In fact, it's very-very close to what I want.
>>
>> It fails when the plugin is below the fold when the page loads.
>>
>> I agree that this logic fails on the overflow property.
>>
>> Happy to take more suggestions.
>>
>
> Add the logic on the blink side?
> The code that clips to the frame is
> in
WebPluginContainerImpl::calculateGeometry/WebPluginContainerImpl::windowClipRect.
> Maybe the code could separate those 2 and pass them to the WebPlugin?
>
>
>
>>
>> Tommy
>>
>> https://codereview.chromium.org/1015023002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tommycli, 2015-04-01 00:09:07]

piman: PTAL. I finally got around to trying your advice of delaying using
coordinates by posting a task to the message loop.

It works!

In fact, there was already the ScheduleAsyncDidChangeView() method built into
PepperPluginInstanceImpl. I just re-used that for plugin initialization also.

In my (limited) manual testing, it now seems to handle the re-layoutted Youtube
masthead case well.

[tommycli, 2015-04-01 00:34:11]

thestig:

This patch has changed a lot, so may need another look.

Also: last time you mentioned adding a test. What kind of test were you thinking
of? An end-to-end browser test?

Would you be okay with something like that in a separate CL?

Thanks!

[Lei Zhang, 2015-04-01 00:51:08]

https://codereview.chromium.org/1015023002/diff/120001/chrome/renderer/plugin...
File chrome/renderer/plugins/chrome_plugin_placeholder.h (right):

https://codereview.chromium.org/1015023002/diff/120001/chrome/renderer/plugin...
chrome/renderer/plugins/chrome_plugin_placeholder.h:38: const gfx::Size&
custom_poster_size);
Maybe put all the poster related parameters in a struct? We're at 11
parameters...

https://codereview.chromium.org/1015023002/diff/120001/chrome/renderer/plugin...
File chrome/renderer/plugins/plugin_preroller.h (right):

https://codereview.chromium.org/1015023002/diff/120001/chrome/renderer/plugin...
chrome/renderer/plugins/plugin_preroller.h:13: #include "ui/gfx/geometry/size.h"
not needed?

https://codereview.chromium.org/1015023002/diff/120001/chrome/renderer/resour...
File chrome/renderer/resources/plugin_poster.html (right):

https://codereview.chromium.org/1015023002/diff/120001/chrome/renderer/resour...
chrome/renderer/resources/plugin_poster.html:68: <div id="innerContainer"
"inner_container" to be consistent with "plugin_icon" below.

[piman, 2015-04-01 01:02:41]

https://codereview.chromium.org/1015023002/diff/120001/content/renderer/peppe...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/1015023002/diff/120001/content/renderer/peppe...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1631: if
(!sent_initial_did_change_view_) {
So, you may be getting into this code more than once. SendDidChangeView is
called from various places, but we will just early out if there is a pending
posted task, on l.1655, in which case sent_initial_did_change_view_ will not be
set to true.

The following order is possible:

- Initialization (sent_initial_did_change_view_ is false)
- ScheduleAsyncDidChangeView (sent_initial_did_change_view_ is still false)
- something calls SendDidChangeView directly (e.g. ResetAsProxied or
PageVisibilityChanged), and we get here (sent_initial_did_change_view_ is still
false) but we early exit on l.1655 because a task is pending, so
sent_initial_did_change_view_ is not set to true
- the task executes, and we get here again, sent_initial_did_change_view_ is
still false


Maybe both this code and the code below (the if (throttler_ &&
throttler_->IsThrottled()) block) should move after l.1655, right before
sent_initial_did_change_view_ is set to true (that way you're guaranteed it's
done exactly once).

https://codereview.chromium.org/1015023002/diff/120001/content/renderer/peppe...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/1015023002/diff/120001/content/renderer/peppe...
content/renderer/pepper/pepper_plugin_instance_impl.h:747: gfx::Rect
unobscured_;
nit: maybe unobscured_rect_?
unobscured_ sounds like a bool.

[tommycli, 2015-04-01 18:55:28]

thestig, piman: addressed below. thanks!

https://codereview.chromium.org/1015023002/diff/120001/chrome/renderer/plugin...
File chrome/renderer/plugins/chrome_plugin_placeholder.h (right):

https://codereview.chromium.org/1015023002/diff/120001/chrome/renderer/plugin...
chrome/renderer/plugins/chrome_plugin_placeholder.h:38: const gfx::Size&
custom_poster_size);
On 2015/04/01 00:51:08, Lei Zhang wrote:
> Maybe put all the poster related parameters in a struct? We're at 11
> parameters...

Done.

https://codereview.chromium.org/1015023002/diff/120001/chrome/renderer/plugin...
File chrome/renderer/plugins/plugin_preroller.h (right):

https://codereview.chromium.org/1015023002/diff/120001/chrome/renderer/plugin...
chrome/renderer/plugins/plugin_preroller.h:13: #include "ui/gfx/geometry/size.h"
On 2015/04/01 00:51:08, Lei Zhang wrote:
> not needed?

Done.

https://codereview.chromium.org/1015023002/diff/120001/chrome/renderer/resour...
File chrome/renderer/resources/plugin_poster.html (right):

https://codereview.chromium.org/1015023002/diff/120001/chrome/renderer/resour...
chrome/renderer/resources/plugin_poster.html:68: <div id="innerContainer"
On 2015/04/01 00:51:08, Lei Zhang wrote:
> "inner_container" to be consistent with "plugin_icon" below.

Done.

https://codereview.chromium.org/1015023002/diff/120001/content/renderer/peppe...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/1015023002/diff/120001/content/renderer/peppe...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1631: if
(!sent_initial_did_change_view_) {
On 2015/04/01 01:02:41, piman (Very slow to review) wrote:
> So, you may be getting into this code more than once. SendDidChangeView is
> called from various places, but we will just early out if there is a pending
> posted task, on l.1655, in which case sent_initial_did_change_view_ will not
be
> set to true.
> 
> The following order is possible:
> 
> - Initialization (sent_initial_did_change_view_ is false)
> - ScheduleAsyncDidChangeView (sent_initial_did_change_view_ is still false)
> - something calls SendDidChangeView directly (e.g. ResetAsProxied or
> PageVisibilityChanged), and we get here (sent_initial_did_change_view_ is
still
> false) but we early exit on l.1655 because a task is pending, so
> sent_initial_did_change_view_ is not set to true
> - the task executes, and we get here again, sent_initial_did_change_view_ is
> still false
> 
> 
> Maybe both this code and the code below (the if (throttler_ &&
> throttler_->IsThrottled()) block) should move after l.1655, right before
> sent_initial_did_change_view_ is set to true (that way you're guaranteed it's
> done exactly once).

Done. Thanks for the explanation. That's a pretty tricky setup.

I was unable to move that block under 1655 as that conditional uses the
throttler-modified view data. However, I was able to move the async view change
early exit to the very top.

https://codereview.chromium.org/1015023002/diff/120001/content/renderer/peppe...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/1015023002/diff/120001/content/renderer/peppe...
content/renderer/pepper/pepper_plugin_instance_impl.h:747: gfx::Rect
unobscured_;
On 2015/04/01 01:02:41, piman (Very slow to review) wrote:
> nit: maybe unobscured_rect_?
> unobscured_ sounds like a bool.

Done.

[Lei Zhang, 2015-04-01 19:18:35]

https://codereview.chromium.org/1015023002/diff/160001/chrome/renderer/plugin...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/1015023002/diff/160001/chrome/renderer/plugin...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:161:
base::Int64ToString(poster_info.custom_poster_size.width()) + "px");
Why not just base::IntToString()?

https://codereview.chromium.org/1015023002/diff/160001/chrome/renderer/plugin...
File chrome/renderer/plugins/chrome_plugin_placeholder.h (right):

https://codereview.chromium.org/1015023002/diff/160001/chrome/renderer/plugin...
chrome/renderer/plugins/chrome_plugin_placeholder.h:18: // If |poster_attribute|
contains relative paths, |base_url| must be
If someone reads this for the first time, I don't think they'll understand what
all this means.

https://codereview.chromium.org/1015023002/diff/160001/content/renderer/peppe...
File content/renderer/pepper/plugin_instance_throttler_impl.h (right):

https://codereview.chromium.org/1015023002/diff/160001/content/renderer/peppe...
content/renderer/pepper/plugin_instance_throttler_impl.h:59: const gfx::Size&
size);
The comment above still refers to view bounds, but you've renamed the variable.

https://codereview.chromium.org/1015023002/diff/160001/content/renderer/peppe...
content/renderer/pepper/plugin_instance_throttler_impl.h:105: gfx::Size
initialized_size_;
const

[tommycli, 2015-04-01 19:33:20]

thestig: thanks!

https://codereview.chromium.org/1015023002/diff/160001/chrome/renderer/plugin...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/1015023002/diff/160001/chrome/renderer/plugin...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:161:
base::Int64ToString(poster_info.custom_poster_size.width()) + "px");
On 2015/04/01 19:18:35, Lei Zhang wrote:
> Why not just base::IntToString()?

Done.

https://codereview.chromium.org/1015023002/diff/160001/chrome/renderer/plugin...
File chrome/renderer/plugins/chrome_plugin_placeholder.h (right):

https://codereview.chromium.org/1015023002/diff/160001/chrome/renderer/plugin...
chrome/renderer/plugins/chrome_plugin_placeholder.h:18: // If |poster_attribute|
contains relative paths, |base_url| must be
On 2015/04/01 19:18:35, Lei Zhang wrote:
> If someone reads this for the first time, I don't think they'll understand
what
> all this means.

Done. You're right. I cleaned up the commenting.

https://codereview.chromium.org/1015023002/diff/160001/content/renderer/peppe...
File content/renderer/pepper/plugin_instance_throttler_impl.h (right):

https://codereview.chromium.org/1015023002/diff/160001/content/renderer/peppe...
content/renderer/pepper/plugin_instance_throttler_impl.h:59: const gfx::Size&
size);
On 2015/04/01 19:18:35, Lei Zhang wrote:
> The comment above still refers to view bounds, but you've renamed the
variable.

Done.

https://codereview.chromium.org/1015023002/diff/160001/content/renderer/peppe...
content/renderer/pepper/plugin_instance_throttler_impl.h:105: gfx::Size
initialized_size_;
On 2015/04/01 19:18:35, Lei Zhang wrote:
> const

I cannot, as it's modified after the constructor.

[piman, 2015-04-01 19:37:22]

lgtm

[Lei Zhang, 2015-04-01 21:04:48]

lgtm

[tommycli, 2015-04-01 21:05:13]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2015-04-01 21:05:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1015023002/180001

[commit-bot: I haz the power, 2015-04-01 21:56:16]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-04-01 21:57:13]

Patchset 10 (id:??) landed as
https://crrev.com/c3ace4f15c91440c2dad238731422fe29b1b4373
Cr-Commit-Position: refs/heads/master@{#323331}