Android WebView: Make a custom Picture subclass

Android WebView: Make a custom Picture subclass

This allows the capturePicture path to work fully even when using
mismatched skia version to the system. AwCreatePictureFunction is now
obsolete and can be removed in a followup.

BUG=b/9814370

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216667

[joth, 2013-08-03 17:38:12]

(notes from the author)

https://codereview.chromium.org/22035002/diff/14001/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.h (right):

https://codereview.chromium.org/22035002/diff/14001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.h:39: // TODO(joth): consider
extracting this to its own utility class.
I left in this .h/.cc for this patch as it makes the diff much easier to follow.
(see the .cc)

https://codereview.chromium.org/22035002/diff/14001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwPicture.java
(right):

https://codereview.chromium.org/22035002/diff/14001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwPicture.java:107: }
else Log.e ?

[joth, 2013-08-08 20:59:31]

+sgurun as kristianm is ignoring me.

[Kristian Monsen, 2013-08-08 21:28:58]

lgtm

https://codereview.chromium.org/22035002/diff/14001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwPicture.java
(right):

https://codereview.chromium.org/22035002/diff/14001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwPicture.java:107: }
On 2013/08/03 17:38:12, joth wrote:
> else Log.e ?

It would potentially crash the app. It either happens all the time or never on a
system as far as I understand? Maybe best the keep the exception.

[sgurun-gerrit only, 2013-08-08 22:11:19]

https://codereview.chromium.org/22035002/diff/14001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwPicture.java
(right):

https://codereview.chromium.org/22035002/diff/14001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwPicture.java:85: Rect
mClipBoundsTemporary = new Rect();
Is this intentional? why is it not private and defined at the top?

[joth, 2013-08-08 22:13:15]

Thanks!


On 8 August 2013 15:11, <sgurun@chromium.org> wrote:

>
> https://codereview.chromium.**org/22035002/diff/14001/**
>
android_webview/java/src/org/**chromium/android_webview/**AwPicture.java<https://codereview.chromium.org/22035002/diff/14001/android_webview/java/src/org/chromium/android_webview/AwPicture.java>
> File
> android_webview/java/src/org/**chromium/android_webview/**AwPicture.java
> (right):
>
> https://codereview.chromium.**org/22035002/diff/14001/**
> android_webview/java/src/org/**chromium/android_webview/**
>
AwPicture.java#newcode85<https://codereview.chromium.org/22035002/diff/14001/android_webview/java/src/org/chromium/android_webview/AwPicture.java#newcode85>
> android_webview/java/src/org/**chromium/android_webview/**
> AwPicture.java:85:
> Rect mClipBoundsTemporary = new Rect();
> Is this intentional?

yes

> why is it not private

good catch


> and defined at the top?
>
they don't have to be. when a private member is only used by one (or two)
methods  it's ok to declare it with them. I'll comment that this is a
temporary used in one method, held as member just to avoid GC


>
https://codereview.chromium.**org/22035002/<https://codereview.chromium.org/2...
>

[sgurun-gerrit only, 2013-08-08 22:14:31]

lgtm

k

[joth, 2013-08-08 22:39:01]

Thanks

+boliu for a committer stamp of approval

https://codereview.chromium.org/22035002/diff/14001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwPicture.java
(right):

https://codereview.chromium.org/22035002/diff/14001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwPicture.java:107: }
On 2013/08/08 21:28:59, Kristian Monsen wrote:
> On 2013/08/03 17:38:12, joth wrote:
> > else Log.e ?
> 
> It would potentially crash the app. It either happens all the time or never on
a
> system as far as I understand? Maybe best the keep the exception.

As per verbal discussion, I'll remove the mCompatibilityMode and change to
always throw for incorrect usage at this layer

[boliu, 2013-08-08 23:05:10]

lgtm

https://codereview.chromium.org/22035002/diff/14001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwPicture.java
(right):

https://codereview.chromium.org/22035002/diff/14001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwPicture.java:107: }
On 2013/08/08 22:39:01, joth wrote:
> On 2013/08/08 21:28:59, Kristian Monsen wrote:
> > On 2013/08/03 17:38:12, joth wrote:
> > > else Log.e ?
> > 
> > It would potentially crash the app. It either happens all the time or never
on
> a
> > system as far as I understand? Maybe best the keep the exception.
> 
> As per verbal discussion, I'll remove the mCompatibilityMode and change to
> always throw for incorrect usage at this layer

What is that discussion? It's probably a bad idea for apps to call these
methods, but they didn't through exception before?

[joth, 2013-08-08 23:10:10]

On 8 August 2013 16:05, <boliu@chromium.org> wrote:

> lgtm
>
>
>
>
>
> https://codereview.chromium.**org/22035002/diff/14001/**
>
android_webview/java/src/org/**chromium/android_webview/**AwPicture.java<https://codereview.chromium.org/22035002/diff/14001/android_webview/java/src/org/chromium/android_webview/AwPicture.java>
> File
> android_webview/java/src/org/**chromium/android_webview/**AwPicture.java
> (right):
>
> https://codereview.chromium.**org/22035002/diff/14001/**
> android_webview/java/src/org/**chromium/android_webview/**
>
AwPicture.java#newcode107<https://codereview.chromium.org/22035002/diff/14001/android_webview/java/src/org/chromium/android_webview/AwPicture.java#newcode107>
> android_webview/java/src/org/**chromium/android_webview/**
> AwPicture.java:107:
> }
> On 2013/08/08 22:39:01, joth wrote:
>
>> On 2013/08/08 21:28:59, Kristian Monsen wrote:
>> > On 2013/08/03 17:38:12, joth wrote:
>> > > else Log.e ?
>> >
>> > It would potentially crash the app. It either happens all the time
>>
> or never on
>
>> a
>> > system as far as I understand? Maybe best the keep the exception.
>>
>
>  As per verbal discussion, I'll remove the mCompatibilityMode and
>>
> change to
>
>> always throw for incorrect usage at this layer
>>
>
> What is that discussion? It's probably a bad idea for apps to call these
> methods, but they didn't through exception before?
>
> ah sorry. yes so two steps
- land this now, get some stats on how often it happens
- when needed (or anyway) make a wrapper class in the glue for legacy apps
that has whatever compat logic



>
https://codereview.chromium.**org/22035002/<https://codereview.chromium.org/2...
>

[boliu, 2013-08-08 23:11:35]

sg

[commit-bot: I haz the power, 2013-08-09 00:48:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/22035002/36001

[commit-bot: I haz the power, 2013-08-09 12:35:14]

Change committed as 216667