PDF Plugin: Swallow Invalidate calls before getting initial size

PDF Plugin: Swallow Invalidate calls before getting initial size

The PDF Plugin current expects the size to be set before any invalidate operations, and will DCHECK that.

https://crrev.com/323331 broke this, as the initial size is given to the plugin is now deferred to give the DOM a chance to 'settle down'.

Instead, we can just ignore invalidate calls before the size is set. When the size is actually set, the whole page will be invalidated anyways, so there's no harm.

BUG=474428
Committed: https://crrev.com/4d903a9645edd964b26e2afc4391aa9949c3c0c8
Cr-Commit-Position: refs/heads/master@{#324947}

[tommycli, 2015-04-07 20:50:32]

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

[tommycli, 2015-04-07 20:53:18]

thestig: PTAL. You may want to double-check my reasoning here. I take it that
the fix will need to be merged into M42 yes?

[tommycli, 2015-04-07 20:58:46]

BTW: I checked, and these 'dchecks' were around since the initial inclusion into
google3. [1]

If you have further revision history we might be able to figure out why they
were put in there in the first place.

[1] https://cs.corp.google.com/#piper///depot/google3/third_party/foxit/files/

[Lei Zhang, 2015-04-07 23:21:33]

The DCHECKs is copied from http://codereview.chromium.org/7215030/

and they still exist in ppapi/utility/graphics/paint_manager.cc. I don't know if
it is ok to ignore the DCHECKs. Maybe the ppapi/OWNERS know more about this.

[raymes, 2015-04-08 06:32:28]

I'm not sure about this. It seems innocuous. Possibly we should make the calling
code bail out before calling invalidate in the case that the size isn't set.

[tommycli, 2015-04-09 18:43:35]

On 2015/04/08 06:32:28, raymes wrote:
> I'm not sure about this. It seems innocuous. Possibly we should make the
calling
> code bail out before calling invalidate in the case that the size isn't set.

I thought about that. It wouldn't be too different. You would basically have to
add an extra boolean flag and put the same logic in both instance.cc and
out_of_process_instance.cc.

It seemed cleaner to put it in it the paint manager. 'Invalidate' should be
something you can call on PaintManager even before you call 'SetSize', imo.

[tommycli, 2015-04-10 20:30:50]

tommycli@chromium.org changed reviewers:
+ raymes@chromium.org

[tommycli, 2015-04-10 20:30:51]

raymes: I accidentally dropped you off the list when I replied. Pasted below:

---

I thought about that. It wouldn't be too different. You would basically have to
add an extra boolean flag and put the same logic in both instance.cc and
out_of_process_instance.cc.

It seemed cleaner to put it in it the paint manager. 'Invalidate' should be
something you can call on PaintManager even before you call 'SetSize', imo.

[raymes, 2015-04-13 00:29:27]

That seems reasonable to me. 
lgtm

[tommycli, 2015-04-13 16:53:55]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[tommycli, 2015-04-13 16:54:11]

thestig: I sent to CQ dry run. You okay with this solution?

[commit-bot: I haz the power, 2015-04-13 16:55:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1067043002/20001

[commit-bot: I haz the power, 2015-04-13 17:58:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-13 17:58:22]

This issue passed the CQ dry run.

[Lei Zhang, 2015-04-13 22:17:45]

Should the changes apply to ppapi/utility/graphics/paint_manager.cc as well?
Otherwise, I defer to raymes@

[raymes, 2015-04-13 23:34:44]

Those two files have diverged so much now. I think if the DCHECK is still valid
in the other version, we can keep it there.

[tommycli, 2015-04-13 23:49:38]

On 2015/04/13 23:34:44, raymes wrote:
> Those two files have diverged so much now. I think if the DCHECK is still
valid
> in the other version, we can keep it there.

Alright. I'll leave it as it is then. Sending to CQ. Thanks!

[tommycli, 2015-04-13 23:49:41]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2015-04-13 23:53:12]

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

[commit-bot: I haz the power, 2015-04-13 23:55:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-13 23:56:44]

Patchset 2 (id:??) landed as
https://crrev.com/4d903a9645edd964b26e2afc4391aa9949c3c0c8
Cr-Commit-Position: refs/heads/master@{#324947}

[tommycli, 2015-04-14 00:00:16]

thestig/raymes: Does this need a merge to M43?

I believe the Release behavior of skipping the DCHECK and just moving on is also
harmless.

[raymes, 2015-04-14 00:37:38]

I don't think it would matter, but you could merge it just to be safe.

On Tue, 14 Apr 2015 at 10:00 <tommycli@chromium.org> wrote:

> thestig/raymes: Does this need a merge to M43?
>
> I believe the Release behavior of skipping the DCHECK and just moving on is
> also
> harmless.
>
> https://codereview.chromium.org/1067043002/
>

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