Move WebView.captureVisibleRegion to stable API.

Move WebView.captureVisibleRegion to stable API.

WebView.captureVisibleRegion has been available in the experimental API
since 2016.02.01. It seems like it's time to make it available on the
stable channel.

BUG=635549
Committed: https://crrev.com/2b6427807476027a8b1e5fa2ee3036305a49d64e
Cr-Commit-Position: refs/heads/master@{#410154}

[wjmaclean, 2016-08-04 14:41:48]

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

[wjmaclean, 2016-08-04 14:42:16]

Description was changed from

==========
Move WebView.captureVisibleRegion to stable API.

WebView.captureVisibleRegion has been available in the experimental API
since 2016.02.01. It seems like it's time to make it available on the
stable channel.

BUG=none
==========

to

==========
Move WebView.captureVisibleRegion to stable API.

WebView.captureVisibleRegion has been available in the experimental API
since 2016.02.01. It seems like it's time to make it available on the
stable channel.

BUG=none
==========

[wjmaclean, 2016-08-04 14:42:16]

wjmaclean@chromium.org changed reviewers:
+ nasko@chromium.org

[commit-bot: I haz the power, 2016-08-04 14:42:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2016-08-04 14:42:40]

wjmaclean@chromium.org changed reviewers:
+ rockot@chromium.org

[wjmaclean, 2016-08-04 14:43:28]

rockot@, nasko@ - tiny CL ... any objections to making
WebView.captureVisibleRegion available on stable now?

[nasko, 2016-08-04 14:46:30]

I'm not sure why my review is needed here, since I'm not an owner in extension
APIs.
In general, we should only expose APIs that are really needed to solve problems
that can't be done any other way. How has been the usage of this API on
experimental extensions so far?

[wjmaclean, 2016-08-04 14:49:13]

On 2016/08/04 14:46:30, nasko (slow) wrote:
> I'm not sure why my review is needed here, since I'm not an owner in extension
> APIs.
> In general, we should only expose APIs that are really needed to solve
problems
> that can't be done any other way. How has been the usage of this API on
> experimental extensions so far?

This has been a long requested feature, and has been trouble-free since it
landed. It mirrors an existing api for extensions that isn't available on
WebView. (Sorry for the noise, I just thought you might want to know given
WebView's relation to OOPIF.)

[Fady Samuel, 2016-08-04 15:00:24]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2016-08-04 15:00:24]

Drive-by OWNER review:

not lgtm

https://developer.chrome.com/apps/tags/webview

Making this API public without documenting it seems unfortunate. Also, I'm kind
of surprised this hasn't broken things in the past since I don't believe
experimental APIs make it to stable. How are our usage counters?

Also I'd prefer not to keep around dead code, could we please delete this file
along with the associated API permission? 

https://cs.chromium.org/chromium/src/extensions/common/api/_api_features.json...

And its usage elsewhere:

https://cs.chromium.org/chromium/src/extensions/renderer/dispatcher.cc?q=webV...

Thanks!

[wjmaclean, 2016-08-04 15:09:42]

On 2016/08/04 15:00:24, Fady Samuel wrote:
> Drive-by OWNER review:
> 
> not lgtm
> 
> https://developer.chrome.com/apps/tags/webview
> 
> Making this API public without documenting it seems unfortunate. Also, I'm
kind
> of surprised this hasn't broken things in the past since I don't believe
> experimental APIs make it to stable. How are our usage counters?
> 
> Also I'd prefer not to keep around dead code, could we please delete this file
> along with the associated API permission? 
> 
>
https://cs.chromium.org/chromium/src/extensions/common/api/_api_features.json...
> 
> And its usage elsewhere:
> 
>
https://cs.chromium.org/chromium/src/extensions/renderer/dispatcher.cc?q=webV...
> 
> Thanks!

I'm not sure I understand the comment "I don't believe experimental APIs make it
to stable". This was implemented as an experimental API as a way of allowing it
to be tested by users without potentially breaking stable channel ... was that
the wrong thing to do. If we weren't planning on releasing this as a public API,
then I'm not sure I understand the point of
https://bugs.chromium.org/p/chromium/issues/detail?id=326755.

The documentation issue can be fixed (I thought there was documentation, so my
mistake).

We do have consumers for this (and we have received requests to make it stable),
but I'll have to look up numbers to give specific values.

[Fady Samuel, 2016-08-04 15:19:22]

We only install experimental APIs up to dev:
https://cs.chromium.org/chromium/src/extensions/common/api/_api_features.json...

What that means is this API actually isn't exposed to stable. I'm not
opposed to exposing it to stable, but I was just surprised that we haven't
broken consumers of this API as a result of the API disappearing on stable
channel. We have had this problem before where a(n internal) consumer
relies on a new API in dev and it doesn't make it to stable and things
break. This led me to believe that perhaps we don't have much usage now for
this? That's not a blocker to bringing to stable. I'm just curious if we've
had feedback on this API in dev?

On Thu, Aug 4, 2016 at 8:09 AM <wjmaclean@chromium.org> wrote:

> On 2016/08/04 15:00:24, Fady Samuel wrote:
> > Drive-by OWNER review:
> >
> > not lgtm
> >
> > https://developer.chrome.com/apps/tags/webview
> >
> > Making this API public without documenting it seems unfortunate. Also,
> I'm
> kind
> > of surprised this hasn't broken things in the past since I don't believe
> > experimental APIs make it to stable. How are our usage counters?
> >
> > Also I'd prefer not to keep around dead code, could we please delete
> this file
> > along with the associated API permission?
> >
> >
>
>
https://cs.chromium.org/chromium/src/extensions/common/api/_api_features.json...
> >
> > And its usage elsewhere:
> >
> >
>
>
https://cs.chromium.org/chromium/src/extensions/renderer/dispatcher.cc?q=webV...
> >
> > Thanks!
>
> I'm not sure I understand the comment "I don't believe experimental APIs
> make it
> to stable". This was implemented as an experimental API as a way of
> allowing it
> to be tested by users without potentially breaking stable channel ... was
> that
> the wrong thing to do. If we weren't planning on releasing this as a
> public API,
> then I'm not sure I understand the point of
> https://bugs.chromium.org/p/chromium/issues/detail?id=326755.
>
> The documentation issue can be fixed (I thought there was documentation,
> so my
> mistake).
>
> We do have consumers for this (and we have received requests to make it
> stable),
> but I'll have to look up numbers to give specific values.
>
> https://codereview.chromium.org/2210333002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wjmaclean, 2016-08-04 15:47:17]

On 2016/08/04 15:19:22, Fady Samuel wrote:
> We only install experimental APIs up to dev:
>
https://cs.chromium.org/chromium/src/extensions/common/api/_api_features.json...
> 
> What that means is this API actually isn't exposed to stable. I'm not
> opposed to exposing it to stable, but I was just surprised that we haven't
> broken consumers of this API as a result of the API disappearing on stable
> channel. We have had this problem before where a(n internal) consumer
> relies on a new API in dev and it doesn't make it to stable and things
> break. This led me to believe that perhaps we don't have much usage now for
> this? That's not a blocker to bringing to stable. I'm just curious if we've
> had feedback on this API in dev?

I know people are using it based on direct interactions with them (both internal
and external), but when I look up histogram stats for it
(WEBVIEWINTERNAL_CAPTUREVISIBLEREGION) I don't see it anywhere; I'm not sure
what to make of that, as it would be one thing if there were low numbers, but
none at all seems surprising.

By comparison, something like WEBVIEWINTERNAL_SETZOOM has a low, but non-zero
value.

So what's the right way to proceed here then? Do we make this available in order
to collect stats? (But then it seems hard to turn it off afterwards without
breaking someone.)

As for the docs, it was put in webview_tag.json, but it doesn't show up on
https://developer.chrome.com/apps/tags/webview. I'm not sure why that happens,
but perhaps it's related to the fact that it has been experimental up to now? Or
maybe the webview_tag.json entry is just not configured properly.

[commit-bot: I haz the power, 2016-08-04 17:25:58]

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

[commit-bot: I haz the power, 2016-08-04 17:25:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wjmaclean, 2016-08-04 17:38:46]

wjmaclean@chromium.org changed reviewers:
- nasko@chromium.org

[wjmaclean, 2016-08-04 17:46:48]

PTAL

- I've removed the WebView experimental support files as requested (it was a
pain adding them back last time which is why I oiiginally left them in place,
but I can see the point about removing dead code).

- I've verified running chrome/common/extensions/docs/server2/preview.py against
the CL that the documentation is being shown now the function is in the
non-experimental API

[wjmaclean, 2016-08-04 18:30:02]

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

[commit-bot: I haz the power, 2016-08-04 18:31:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 19:57:28]

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

[commit-bot: I haz the power, 2016-08-04 19:57:28]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-08-05 03:23:57]

Thanks for addressing my concerns! As there are known clients of this API then
lgtm.

[Ken Rockot(use gerrit already), 2016-08-05 18:26:55]

lgtm

[wjmaclean, 2016-08-05 18:31:04]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2016-08-05 18:31:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 20:35:12]

Description was changed from

==========
Move WebView.captureVisibleRegion to stable API.

WebView.captureVisibleRegion has been available in the experimental API
since 2016.02.01. It seems like it's time to make it available on the
stable channel.

BUG=none
==========

to

==========
Move WebView.captureVisibleRegion to stable API.

WebView.captureVisibleRegion has been available in the experimental API
since 2016.02.01. It seems like it's time to make it available on the
stable channel.

BUG=none
==========

[commit-bot: I haz the power, 2016-08-05 20:35:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-05 20:36:50]

Description was changed from

==========
Move WebView.captureVisibleRegion to stable API.

WebView.captureVisibleRegion has been available in the experimental API
since 2016.02.01. It seems like it's time to make it available on the
stable channel.

BUG=none
==========

to

==========
Move WebView.captureVisibleRegion to stable API.

WebView.captureVisibleRegion has been available in the experimental API
since 2016.02.01. It seems like it's time to make it available on the
stable channel.

BUG=none

Committed: https://crrev.com/2b6427807476027a8b1e5fa2ee3036305a49d64e
Cr-Commit-Position: refs/heads/master@{#410154}
==========

[commit-bot: I haz the power, 2016-08-05 20:36:51]

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

[wjmaclean, 2016-08-08 15:37:19]

Description was changed from

==========
Move WebView.captureVisibleRegion to stable API.

WebView.captureVisibleRegion has been available in the experimental API
since 2016.02.01. It seems like it's time to make it available on the
stable channel.

BUG=none

Committed: https://crrev.com/2b6427807476027a8b1e5fa2ee3036305a49d64e
Cr-Commit-Position: refs/heads/master@{#410154}
==========

to

==========
Move WebView.captureVisibleRegion to stable API.

WebView.captureVisibleRegion has been available in the experimental API
since 2016.02.01. It seems like it's time to make it available on the
stable channel.

BUG=635549

Committed: https://crrev.com/2b6427807476027a8b1e5fa2ee3036305a49d64e
Cr-Commit-Position: refs/heads/master@{#410154}
==========