Update the HTML Media Capture implementation.

Update the HTML Media Capture implementation.

In the latest version of the spec, the `capture' attribute is now a boolean
which simply indicates whether the device's capabilities should be used or
not (choosing between cameras, microphones etc is up to the user agent based
on the contents of the `accept' attribute).

This particular patch updates the type of the `capture' member of
content::FileChooserParams by passing an std::pair consisting of a vector of
MIME types passed to the `accept' attribute of the <input> tag and a boolean
corresponding to the `capture' attribute.

The behavior in Android has been slightly changed due to how the spec
change. Namely:
 o Sites that specify only the "capture" attribute setting the "accept"
   attribute will now get a generic picker.
 o Sites that specify the "capture" attribute and pass "*/*" to "accept"
   will now get a generic picker.
 o Sites that specify the "capture" and multiple MIME types for the "accept"
   attribute will now get a generic picker.

R=abarth@chromium.org,beverloo@chromium.org,bulach@chromium.org,tedchoc@chromium.org,eseidel@chromium.org
BUG=240252
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210553

[Peter Beverloo, 2013-05-13 15:37:57]

+joth for WebView

As far as I understand it, Safari never supported any values for the "capture"
attribute.  Furthermore, any mime type specified by the "accept" attribute will
take precedence over "capture" values as well.

I am a little bit concerned about the compatibility impact for websites which
were only created for usage on Android.  This also applies to APK applications
containing WebViews to display local pages.  Adam basically upstreamed the
Android Browser implementation to WebKit when he landed this implementation.

Do we know whether using <input type="file" capture="image" /> (without the
accept attribute!) is a common convention?  Many examples on the internet seem
to be using both @capture and @accept to make the implementation usable for both
Android and iOS.

I'm hoping the risk will be minor and we can align with the specification :-).

[Peter Beverloo, 2013-05-13 15:40:12]

I replied to the bug because that's probably a better forum to discuss that
concern.

[joth, 2013-05-13 16:24:20]

+benm

[Raphael Kubo da Costa (rakuco), 2013-05-14 15:32:53]

Patch v3 is up, and makes the behavior a bit more compatible with the existing
Android implementation.

I have updated the CL's description with some notes on what has been done and in
which cases it is not possible to be compatible with the current implementation.

[joth, 2013-05-15 20:03:59]

https://codereview.chromium.org/14758008/diff/10001/content/public/common/fil...
File content/public/common/file_chooser_params.h (right):

https://codereview.chromium.org/14758008/diff/10001/content/public/common/fil...
content/public/common/file_chooser_params.h:52: // If true, the data should be
obtained using the device's camera/mic/etc.
nit: position this comment inside the #if below immediately above the var
declaration

https://codereview.chromium.org/14758008/diff/10001/content/public/common/fil...
content/public/common/file_chooser_params.h:53: // TODO(jrg): upstream
SelectFileDialog.java!  Currently lives in chrome/.
this TODO is obsolete and can go. (FTR - that file is now in
ui/android/java/src/org/chromium/ui/SelectFileDialog.java)

https://codereview.chromium.org/14758008/diff/10001/content/public/common/fil...
content/public/common/file_chooser_params.h:54: #if
defined(NEW_MEDIA_CAPTURE_IMPLEMENTATION)
rather than spluging usage of this marco all over the code, how about:
- unconditionally change this struct to have a bool
- in content/renderer/render_view_impl.cc where the 'capture' value is copied
over, put one piece of #if conditional code there that knows how to deal with
the legacy case (i.e. if the old capture string is not empty, synthesize the
bool and accept values as appropriate)
- in the rest of this patch, you'd then just include the code to do it the new
way.

btw do you have a link to the blink patch? It's hard to read this one-sided

https://codereview.chromium.org/14758008/diff/10001/ui/DEPS
File ui/DEPS (right):

https://codereview.chromium.org/14758008/diff/10001/ui/DEPS#newcode15
ui/DEPS:15: "+third_party/WebKit/Source/WebKit/chromium/public",
use ! instead of + for temporary includes

https://codereview.chromium.org/14758008/diff/10001/ui/shell_dialogs/select_f...
File ui/shell_dialogs/select_file_dialog_android.cc (right):

https://codereview.chromium.org/14758008/diff/10001/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog_android.cc:83: // some point.
We don't use FIXME in chromium. You could link to the bug.

However, as a broader point, I don't see what further phasing out is needed?
AIUI as soon as we start to define the 'NEW_MEDIA_CAPTURE_IMPLEMENTATION' we'll
already be treating any 'capture' tag in the html as a pure boolean, and
disregarding any string value that was associated with it (camera, microphone,
etc), and so from that point on we're already potentially broken pages?

Put another way, to make a more phased migration, I'd expect not to see and
compile time #if in the chromium code at all. Instead we might do something
like:
- leave the 'capture' param as a string in FileChooserParams forever.
- define a special case value, e.g. capture='true' or '*' to indicate the new
boolean param was present, otherwise it will contain the old camera or
microphone etc values as appropriate. (and empty string indicates no capture
param present at all)
- here, we'd pass through the old value direct if present, else if it's 'true'
use your logic below to try and synthesize a value.

[Raphael Kubo da Costa (rakuco), 2013-05-15 22:05:19]

> content/public/common/file_chooser_params.h:53: // TODO(jrg): upstream
> SelectFileDialog.java!  Currently lives in chrome/.
> this TODO is obsolete and can go. (FTR - that file is now in
> ui/android/java/src/org/chromium/ui/SelectFileDialog.java)

Addressed separately in CL 15144003.

> btw do you have a link to the blink patch? It's hard to read this one-sided

I was waiting for this part to get in before uploading the Blink CL, but
here it goes: https://codereview.chromium.org/15015006

> However, as a broader point, I don't see what further phasing out is needed?
> AIUI as soon as we start to define the 'NEW_MEDIA_CAPTURE_IMPLEMENTATION'
we'll
> already be treating any 'capture' tag in the html as a pure boolean, and
> disregarding any string value that was associated with it (camera, microphone,
> etc), and so from that point on we're already potentially broken pages?

The phasing out I refer to is what I've been discussing with benm in the
related bug report: my original idea was to simply implement the newer version
of the spec, treat the "capture" attribute as a boolean and be done with it
(the whole #ifdef dance is just a way to avoid breaking things while pushing
code to Chromium's and Blink's repositories separately).

benm suggested trying to keep compatibility with the existing implementation to
avoid breaking existing pages by synthesizing the Java string from the boolean.
"Phasing out" in this case means getting rid of this compatibility altogether
at some point in the future; by then we will simply implement what's in the
spec.

> Put another way, to make a more phased migration, I'd expect not to see and
> compile time #if in the chromium code at all. Instead we might do something
> like:
> - leave the 'capture' param as a string in FileChooserParams forever.
> - define a special case value, e.g. capture='true' or '*' to indicate the new
> boolean param was present, otherwise it will contain the old camera or
> microphone etc values as appropriate. (and empty string indicates no capture
> param present at all)
> - here, we'd pass through the old value direct if present, else if it's 'true'
> use your logic below to try and synthesize a value.

Given that the Blink side will also pass only a boolean (ie. any value
attributed to the "capture" tag will not be even read), I don't see how your
second point would be possible -- there's no old values to refer to at all.

[joth, 2013-05-22 21:19:54]

On 15 May 2013 15:05, <raphael.kubo.da.costa@intel.com> wrote:

> content/public/common/file_**chooser_params.h:53: // TODO(jrg): upstream
>> SelectFileDialog.java!  Currently lives in chrome/.
>> this TODO is obsolete and can go. (FTR - that file is now in
>> ui/android/java/src/org/**chromium/ui/SelectFileDialog.**java)
>>
>
> Addressed separately in CL 15144003.
>
>
>  btw do you have a link to the blink patch? It's hard to read this
>> one-sided
>>
>
> I was waiting for this part to get in before uploading the Blink CL, but
> here it goes:
https://codereview.chromium.**org/15015006<https://codereview.chromium.org/15...
>
>
>  However, as a broader point, I don't see what further phasing out is
>> needed?
>> AIUI as soon as we start to define the 'NEW_MEDIA_CAPTURE_**
>> IMPLEMENTATION'
>>
> we'll
>
>> already be treating any 'capture' tag in the html as a pure boolean, and
>> disregarding any string value that was associated with it (camera,
>> microphone,
>> etc), and so from that point on we're already potentially broken pages?
>>
>
> The phasing out I refer to is what I've been discussing with benm in the
> related bug report: my original idea was to simply implement the newer
> version
> of the spec, treat the "capture" attribute as a boolean and be done with it
> (the whole #ifdef dance is just a way to avoid breaking things while
> pushing
> code to Chromium's and Blink's repositories separately).
>

FWIW, you could probably avoid the ifdefs altogether if you first land a
simple part 1 in blink that only adds a new field in WebFileChooserParams
"bool mediaCapture" with the new semantics, part 2 switch chromium to use
it, and part 3 delete the old 'capture' field from blink.
(The important bit is the new field has a unique name so it can co-exist
with the capture string field :-)



> benm suggested trying to keep compatibility with the existing
> implementation to
> avoid breaking existing pages by synthesizing the Java string from the
> boolean.
> "Phasing out" in this case means getting rid of this compatibility
> altogether
> at some point in the future; by then we will simply implement what's in the
> spec.
>
>
>  Put another way, to make a more phased migration, I'd expect not to see
>> and
>> compile time #if in the chromium code at all. Instead we might do
>> something
>> like:
>> - leave the 'capture' param as a string in FileChooserParams forever.
>> - define a special case value, e.g. capture='true' or '*' to indicate the
>> new
>> boolean param was present, otherwise it will contain the old camera or
>> microphone etc values as appropriate. (and empty string indicates no
>> capture
>> param present at all)
>> - here, we'd pass through the old value direct if present, else if it's
>> 'true'
>> use your logic below to try and synthesize a value.
>>
>
> Given that the Blink side will also pass only a boolean (ie. any value
> attributed to the "capture" tag will not be even read), I don't see how
> your
> second point would be possible -- there's no old values to refer to at all.
>
>
So this is the bit that confused me most: If blink will only be passing a
boolean, then how is this retaining compatibility with a site that
specifies capture=camera (and does not specify any accept types at all) ?

But re-reading the bug... I see benm is talking
about retaining compatibility with existing content/ layer embedders, not
web-pages. Reading the comment in your code, I thought you were saying you
were retaining compatibility with existing web pages. Maybe you can clarify
that.


Anyway, my preference would be that if possible the value synthesis
happened before the WebContentsDelegate::RunFileChooser() call is made as
this means we can share the same synthesis logic in webview. And  *if* it
happened make sense to do the synthis over on the blink side, then it has
added benefits: it can retain compat with existing pages (if this is
relevant/desirable?), could be done entirely in one patch, and minimizes
the churn needed through the stack should the spec ever change again :)
I'm not on OWNER list for any of the files touched though, so treat this
just as a suggestion rather than absolute guidance.

[benm (inactive), 2013-05-23 15:08:06]

On 2013/05/22 21:19:54, joth wrote:
> On 15 May 2013 15:05, <mailto:raphael.kubo.da.costa@intel.com> wrote:
> 
> > content/public/common/file_**chooser_params.h:53: // TODO(jrg): upstream
> >> SelectFileDialog.java!  Currently lives in chrome/.
> >> this TODO is obsolete and can go. (FTR - that file is now in
> >> ui/android/java/src/org/**chromium/ui/SelectFileDialog.**java)
> >>
> >
> > Addressed separately in CL 15144003.
> >
> >
> >  btw do you have a link to the blink patch? It's hard to read this
> >> one-sided
> >>
> >
> > I was waiting for this part to get in before uploading the Blink CL, but
> > here it goes:
>
https://codereview.chromium.**org/15015006%3Chttps://codereview.chromium.org/...>
> >
> >
> >  However, as a broader point, I don't see what further phasing out is
> >> needed?
> >> AIUI as soon as we start to define the 'NEW_MEDIA_CAPTURE_**
> >> IMPLEMENTATION'
> >>
> > we'll
> >
> >> already be treating any 'capture' tag in the html as a pure boolean, and
> >> disregarding any string value that was associated with it (camera,
> >> microphone,
> >> etc), and so from that point on we're already potentially broken pages?
> >>
> >
> > The phasing out I refer to is what I've been discussing with benm in the
> > related bug report: my original idea was to simply implement the newer
> > version
> > of the spec, treat the "capture" attribute as a boolean and be done with it
> > (the whole #ifdef dance is just a way to avoid breaking things while
> > pushing
> > code to Chromium's and Blink's repositories separately).
> >
> 
> FWIW, you could probably avoid the ifdefs altogether if you first land a
> simple part 1 in blink that only adds a new field in WebFileChooserParams
> "bool mediaCapture" with the new semantics, part 2 switch chromium to use
> it, and part 3 delete the old 'capture' field from blink.
> (The important bit is the new field has a unique name so it can co-exist
> with the capture string field :-)
> 
> 
> 
> > benm suggested trying to keep compatibility with the existing
> > implementation to
> > avoid breaking existing pages by synthesizing the Java string from the
> > boolean.
> > "Phasing out" in this case means getting rid of this compatibility
> > altogether
> > at some point in the future; by then we will simply implement what's in the
> > spec.
> >
> >
> >  Put another way, to make a more phased migration, I'd expect not to see
> >> and
> >> compile time #if in the chromium code at all. Instead we might do
> >> something
> >> like:
> >> - leave the 'capture' param as a string in FileChooserParams forever.
> >> - define a special case value, e.g. capture='true' or '*' to indicate the
> >> new
> >> boolean param was present, otherwise it will contain the old camera or
> >> microphone etc values as appropriate. (and empty string indicates no
> >> capture
> >> param present at all)
> >> - here, we'd pass through the old value direct if present, else if it's
> >> 'true'
> >> use your logic below to try and synthesize a value.
> >>
> >
> > Given that the Blink side will also pass only a boolean (ie. any value
> > attributed to the "capture" tag will not be even read), I don't see how
> > your
> > second point would be possible -- there's no old values to refer to at all.
> >
> >
> So this is the bit that confused me most: If blink will only be passing a
> boolean, then how is this retaining compatibility with a site that
> specifies capture=camera (and does not specify any accept types at all) ?
> 
> But re-reading the bug... I see benm is talking
> about retaining compatibility with existing content/ layer embedders, not
> web-pages. Reading the comment in your code, I thought you were saying you
> were retaining compatibility with existing web pages. Maybe you can clarify
> that.

Given that Android WebView/Browser/Chrome are the only platforms (I believe) to
support the capture attribute, I think it would be desirable to retain
compatibility with what legacy web pages are doing; sorry if that wasn't clear
on the bug. If we were to keep the attribute as a DOMString in the IDL, what
would be the behavior for apps using the new spec (i.e. treating it as a
boolean)? Would that convert to an empty string or cause an error?

Playing devil's advocate, as there has been fairly significant churn on this
spec (this is the third time the attribute has been re-purposed) would it be
prudent to hold off on changes until it's further along the standardisation
track?

> 
> 
> Anyway, my preference would be that if possible the value synthesis
> happened before the WebContentsDelegate::RunFileChooser() call is made as
> this means we can share the same synthesis logic in webview. And  *if* it
> happened make sense to do the synthis over on the blink side, then it has
> added benefits: it can retain compat with existing pages (if this is
> relevant/desirable?), could be done entirely in one patch, and minimizes
> the churn needed through the stack should the spec ever change again :)
> I'm not on OWNER list for any of the files touched though, so treat this
> just as a suggestion rather than absolute guidance.

[Raphael Kubo da Costa (rakuco), 2013-05-23 21:59:32]

It's easier to address both comments together:

> FWIW, you could probably avoid the ifdefs altogether if you first land a
> simple part 1 in blink that only adds a new field in WebFileChooserParams
> "bool mediaCapture" with the new semantics, part 2 switch chromium to use it,
> and part 3 delete the old 'capture' field from blink. (The important bit is
> the new field has a unique name so it can co-exist with the capture string
> field :-)

This... is so obvious I'm embarassed :-) Thanks for the suggestion.

> So this is the bit that confused me most: If blink will only be passing a
> boolean, then how is this retaining compatibility with a site that specifies
> capture=camera (and does not specify any accept types at all) ?

To summarize what I mentioned in my comments in the associated bug report: both

  <input type="file" capture="{camera,camcorder,microphone}" />
  and
  <input type="file" capture="{image,audio,video}" />

should keep working the same way they currently do in SelectFileDialog.java.
That is, the default intents dialog for mime type "*/*" with some additional
options for camera, camcorder and microphone would be shown.

It is things like the following that _do_ cause compatibility problems if
`capture' becomes a boolean:

  <input type="file" accept="image/*; audio/*" capture="camera" />
  and
  <input type="file" accept="*/*" capture="microphone" />

I do not know how common these non-compliant use cases are, and was waiting for
benm to reply in the bug report to my comment 10.

> Given that Android WebView/Browser/Chrome are the only platforms (I believe)
> to support the capture attribute, I think it would be desirable to retain
> compatibility with what legacy web pages are doing; sorry if that wasn't
> clear on the bug.

> Anyway, my preference would be that if possible the value synthesis
> happened before the WebContentsDelegate::RunFileChooser() call is made as
> this means we can share the same synthesis logic in webview. And  *if* it
> happened make sense to do the synthis over on the blink side

To be fair, at least BlackBerry and Tizen should have support for the capture
attribute as well -- they did seem to enable and use the code that's in WebKit,
but I did not test how they implement it myself.

While it _could_ be possible to make the value synthesis (if we're really
interested in that) as early as in Blink itself, I understood this was against
Blink's idea of implementing the specs as faithfully as possible. Specifically,
since Android seems to be the only Blink downstream that has support for that,
it would make sense to let Chromium for other platforms support the attribute
the way it is specified from the start, so restricting any workarounds to
Android sounded sensible.

> If we were to keep the attribute as a DOMString in the IDL, what would be the
> behavior for apps using the new spec (i.e. treating it as a boolean)? Would
> that convert to an empty string or cause an error?

That's up to us. Even if we don't throw any error, an empty string would not
help with compatibility in the troublesome cases I mentioned above.

> Playing devil's advocate, as there has been fairly significant churn on this
> spec (this is the third time the attribute has been re-purposed) would it be
> prudent to hold off on changes until it's further along the standardisation
> track?

The spec should be fairly stable right now; it is a Candidate Recommendation
moving towards becoming a Proposed Recommentation soon. In fact, that's why I
chose to update the implementation now.

In any case, Anssi Kostiainen, the spec editor, sits right next to me, and is a
better person to comment on this side. I think he's subscribed to this CL, so
I'll defer a more detailed explanation to him.

[anssik, 2013-05-24 08:50:23]

On 2013/05/23 21:59:32, Raphael Kubo da Costa (rakuco) wrote:

> > Playing devil's advocate, as there has been fairly significant churn on this
> > spec (this is the third time the attribute has been re-purposed) would it be
> > prudent to hold off on changes until it's further along the standardisation
> > track?
> 
> The spec should be fairly stable right now; it is a Candidate Recommendation
> moving towards becoming a Proposed Recommentation soon. In fact, that's why I
> chose to update the implementation now.
> 
> In any case, Anssi Kostiainen, the spec editor, sits right next to me, and is
a
> better person to comment on this side. I think he's subscribed to this CL, so
> I'll defer a more detailed explanation to him.

As Raphael mentioned, W3C considers the spec stable and appropriate for
implementation.

It may seem there has been churn around this spec, but that's not quite
accurate. Here's a short summary of the spec's recent history from the first
Last Call onwards:

The spec was widely reviewed (e.g. by HTML, WebApps and Device APIs groups,
browser vendors, and the public) when it hit its first Last Call in July 2012.
Earlier versions of the spec were mere Working Drafts i.e. subject to change. To
address Last Call comments around @accept's relationship with @capture, the
consensus of the group was to make @capture boolean.

To gather feedback on the new design, the group first published a "heartbeat"
draft in December 2012, and issued a second Last Call in March 2013. The second
Last Call ended with no concerns raised, thus the spec was published as a
Candidate Recommendation in May 2013.

The spec has followed the typical process of standards development at W3C and
the group has strived to publish new versions early and often to keep the wider
community informed. Please note that no normative changes to the spec were made
between December 2012 "hearbeat" and May 2013 Candidate Recommendation.

If you wish to make comments regarding the spec, e.g. concerning compatibility
with legacy content, please send your feedback to:

http://lists.w3.org/Archives/Public/public-device-apis/

[joth, 2013-05-25 02:20:34]

On 23 May 2013 14:59, <raphael.kubo.da.costa@intel.com> wrote:

> It's easier to address both comments together:
>
>
>  FWIW, you could probably avoid the ifdefs altogether if you first land a
>> simple part 1 in blink that only adds a new field in WebFileChooserParams
>> "bool mediaCapture" with the new semantics, part 2 switch chromium to use
>> it,
>> and part 3 delete the old 'capture' field from blink. (The important bit
>> is
>> the new field has a unique name so it can co-exist with the capture string
>> field :-)
>>
>
> This... is so obvious I'm embarassed :-) Thanks for the suggestion.
>
>
>  So this is the bit that confused me most: If blink will only be passing a
>> boolean, then how is this retaining compatibility with a site that
>> specifies
>> capture=camera (and does not specify any accept types at all) ?
>>
>
> To summarize what I mentioned in my comments in the associated bug report:
> both
>
>   <input type="file" capture="{camera,camcorder,**microphone}" />
>   and
>   <input type="file" capture="{image,audio,video}" />
>
> should keep working the same way they currently do in
> SelectFileDialog.java.
> That is, the default intents dialog for mime type "*/*" with some
> additional
> options for camera, camcorder and microphone would be shown.
>
>
I must be missing something obvious.
But AFAICT, with you new implementation upon hitting
   <input type="file" capture="camera">
then blink will send FileChooserParams with no specific accept type (empty
or "*/*" ? whatever), and 'camera' = true.
select_file_dialog.cc will then have no accept types to synthesize a
capture value from, so SelectFileDialog.java will fallback to the generic
'choose your input source' intent dialog rather than going straight to the
camera, as it currently would. Right?



> It is things like the following that _do_ cause compatibility problems if
> `capture' becomes a boolean:
>
>   <input type="file" accept="image/*; audio/*" capture="camera" />
>   and
>   <input type="file" accept="*/*" capture="microphone" />
>
> I do not know how common these non-compliant use cases are, and was
> waiting for
> benm to reply in the bug report to my comment 10.
>
>
>  Given that Android WebView/Browser/Chrome are the only platforms (I
>> believe)
>> to support the capture attribute, I think it would be desirable to retain
>> compatibility with what legacy web pages are doing; sorry if that wasn't
>> clear on the bug.
>>
>
>  Anyway, my preference would be that if possible the value synthesis
>> happened before the WebContentsDelegate::**RunFileChooser() call is made
>> as
>> this means we can share the same synthesis logic in webview. And  *if* it
>> happened make sense to do the synthis over on the blink side
>>
>
> To be fair, at least BlackBerry and Tizen should have support for the
> capture
> attribute as well -- they did seem to enable and use the code that's in
> WebKit,
> but I did not test how they implement it myself.
>
> While it _could_ be possible to make the value synthesis (if we're really
> interested in that) as early as in Blink itself, I understood this was
> against
> Blink's idea of implementing the specs as faithfully as possible.
> Specifically,
> since Android seems to be the only Blink downstream that has support for
> that,
> it would make sense to let Chromium for other platforms support the
> attribute
> the way it is specified from the start, so restricting any workarounds to
> Android sounded sensible.
>
>
>  If we were to keep the attribute as a DOMString in the IDL, what would be
>> the
>> behavior for apps using the new spec (i.e. treating it as a boolean)?
>> Would
>> that convert to an empty string or cause an error?
>>
>
> That's up to us. Even if we don't throw any error, an empty string would
> not
> help with compatibility in the troublesome cases I mentioned above.
>
>
>  Playing devil's advocate, as there has been fairly significant churn on
>> this
>> spec (this is the third time the attribute has been re-purposed) would it
>> be
>> prudent to hold off on changes until it's further along the
>> standardisation
>> track?
>>
>
> The spec should be fairly stable right now; it is a Candidate
> Recommendation
> moving towards becoming a Proposed Recommentation soon. In fact, that's
> why I
> chose to update the implementation now.
>
> In any case, Anssi Kostiainen, the spec editor, sits right next to me, and
> is a
> better person to comment on this side. I think he's subscribed to this CL,
> so
> I'll defer a more detailed explanation to him.
>
>
>
https://codereview.chromium.**org/14758008/<https://codereview.chromium.org/1...
>

[Raphael Kubo da Costa (rakuco), 2013-05-25 09:24:26]

> I must be missing something obvious.
> But AFAICT, with you new implementation upon hitting
>    <input type="file" capture="camera">
> then blink will send FileChooserParams with no specific accept type (empty
> or "*/*" ? whatever), and 'camera' = true.
> select_file_dialog.cc will then have no accept types to synthesize a capture
> value from, so SelectFileDialog.java will fallback to the generic 'choose
> your input source' intent dialog rather than going straight to the camera, as
> it currently would. Right?

That's right; I misread the current code, sorry about that.

[Raphael Kubo da Costa (rakuco), 2013-07-02 15:39:36]

Let's try to get the ball rolling again.

I have uploaded new patch versions to the Blink side on
https://codereview.chromium.org/15015006 and here as well. I've followed some of
the advice and got rid of the #ifdef dance by moving some of the compatibility
code to Blink.

We seem to have stalled because the current version of the spec is simply
incompatible with everything that was possible before (it's not that features
got removed, but things just need to be done differently). In practice this
means that in the worst case the Intents dialog will be shown instead of the
final application (like the camera one) on Android.

Personally (as someone who's more used to working on Blink than Chromium), I
think this is a valid trade-off for spec compliance, but the Android people
disagree. I'm not really sure how to reach consensus here, so pointers would be
appreciated. Should we, for example, stay compatible with the old version (ie.
treat the "capture" attribute as an enum) and provide deprecation warnings for a
certain period of time?

[benm (inactive), 2013-07-02 20:45:29]

Looking on this with fresh eyes from last time, I think this more or less lg2m.
Some thoughts:

With this change it seems like this will be the behavior for sites that follow
the old version of the spec:

1 Sites that specify a capture enum and an accept type that 'matches' will see
no change in behavior;
2 Sites that specify capture and an accept type that doesn't match the capture
type (I'd argue broken anyway); they will now get the native app inferred from
their accept type rather than capture value;
3 Sites that specified capture and no accept type will now get a generic picker;
4 Sites that specify capture and a generic i.e. */* accept type will now get a
generic picker
5 Sites that specify capture and a mixed accept type will now get a generic
picker
6 Sites that do not specify capture will see no change in behavior

Cases 3, 4 and 5 seem sub-optimal yet not completely broken as the generic
picker will still include an option for the native capture app they would have
expected to jump right into. The trade off seems reasonable to me for spec
compliance. If we could log a console warning in this case, that'd be helpful
for developers that may notice the change in behavior and may help to move folks
toward the new version of the spec. Perhaps we could do that on the Blink side
of the patch in ChromeClientImpl.cpp if we see capture != true or false?
Something like "The value of the capture attribute should be true or false,
media capture behavior will be inferred from accept type instead".

And then at some point in the future, we can remove the console logging and
switch the attribute from a DOMString to a boolean inside blink.

WDYT?

Cheers

[Raphael Kubo da Costa (rakuco), 2013-07-03 13:44:48]

On 2013/07/02 20:45:29, benm wrote:
> WDYT?

Sounds entirely reasonable to me :-)

I've updated the patch here and the one in http://crrev.com/15015006 with some
minor fixes. I have also uploaded http://crrev.com/18332015 to show what the
rest of the Blink changes (and the deprecation code) would look like.

Thanks for the quick feedback!

[joth, 2013-07-03 17:41:42]

https://codereview.chromium.org/14758008/diff/32001/ui/shell_dialogs/select_f...
File ui/shell_dialogs/select_file_dialog_android.cc (right):

https://codereview.chromium.org/14758008/diff/32001/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog_android.cc:75: // The latest version of the
HTML Media Capture spec says the "capture"
nit: 'latest' is ambiguous when read in future. Include a date, or say "As of
spec version x.y..."

https://codereview.chromium.org/14758008/diff/32001/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog_android.cc:78: // attribute. However, since
we have already shipped an implementation of
likewise, could say versions of chrome up to M29 shipped with....

https://codereview.chromium.org/14758008/diff/32001/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog_android.cc:87: // of the spec at some point.
I'm still feeling mislead by this comment. An old website that says
"capture=camera" will have the 'camera' value ignored, regardless of the block
of code here.
I think that's acceptable. But I just think we need to update the comment to not
suggest this if/else ladder is trying to retain site backwards compat. I think
it's really just retaining Java SelectFileDialog backwards compat. (And we can
remove that... see next comment)

https://codereview.chromium.org/14758008/diff/32001/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog_android.cc:113: capture_value.obj(),
Can't we go the full way and pass 'has_capture_attribute' here as the 4th param,
and move all the policy about which source to use when into the Java side? It's
hard to read having it split part here, and part in Java.

e.g. modify all the Java side helper methods like so:-

    private boolean specificTypeIs(String type) {
        return mFileTypes.size() == 1 && mFileTypes.at(0).equals(type);
    }

    private boolean captureCamcorder() {
        return mCapture && specificTypeIs(ALL_VIDEO_TYPES);
    }

etc...

[Raphael Kubo da Costa (rakuco), 2013-07-04 15:05:34]

On 2013/07/03 17:41:42, joth wrote:
> Can't we go the full way and pass 'has_capture_attribute' here as the 4th
param,
> and move all the policy about which source to use when into the Java side?
It's
> hard to read having it split part here, and part in Java.

Thanks for the suggestion; I've implemented it in patch set 8 (and also changed
the commit message/CL description to incorporate benm's summary of the behavior
changes).

[joth, 2013-07-04 16:55:21]

lgtm thanks!

https://codereview.chromium.org/14758008/diff/40001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/SelectFileDialog.java (right):

https://codereview.chromium.org/14758008/diff/40001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/SelectFileDialog.java:88:
getContentIntent.setType("image/*");
(patch creep) this could be ALL_IMAGE_TYPES etc here

https://codereview.chromium.org/14758008/diff/40001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/SelectFileDialog.java:100:
getContentIntent.setType("*/*");
ALL_TYPES

[Raphael Kubo da Costa (rakuco), 2013-07-04 17:45:06]

On 2013/07/04 16:55:21, joth wrote:
> lgtm thanks!
> 
>
https://codereview.chromium.org/14758008/diff/40001/ui/android/java/src/org/c...
> File ui/android/java/src/org/chromium/ui/SelectFileDialog.java (right):
> 
>
https://codereview.chromium.org/14758008/diff/40001/ui/android/java/src/org/c...
> ui/android/java/src/org/chromium/ui/SelectFileDialog.java:88:
> getContentIntent.setType("image/*");
> (patch creep) this could be ALL_IMAGE_TYPES etc here
> 
>
https://codereview.chromium.org/14758008/diff/40001/ui/android/java/src/org/c...
> ui/android/java/src/org/chromium/ui/SelectFileDialog.java:100:
> getContentIntent.setType("*/*");
> ALL_TYPES

Thanks, I've addressed those separately in
https://codereview.chromium.org/18696003

I guess I just need an lgtm from the appropriate OWNERS in all the directories
touched by the patch now...

[Raphael Kubo da Costa (rakuco), 2013-07-04 17:46:11]

+jam,ben,sky

[joth, 2013-07-04 18:09:06]

jam@ for content/,
sky@ can cover chrome/ and ui/ I think ?  (So ben G may get off lightly)

https://codereview.chromium.org/14758008/diff/40001/ui/shell_dialogs/select_f...
File ui/shell_dialogs/select_file_dialog_android.cc (right):

https://codereview.chromium.org/14758008/diff/40001/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog_android.cc:70: if (params) accept_types =
*(reinterpret_cast<AcceptTypes*>(params));
this should be on two lines.  or use 3 if you use { }

[jam, 2013-07-08 21:09:29]

lgtm

[sky, 2013-07-08 21:13:50]

https://codereview.chromium.org/14758008/diff/49001/ui/shell_dialogs/select_f...
File ui/shell_dialogs/select_file_dialog_android.cc (right):

https://codereview.chromium.org/14758008/diff/49001/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog_android.cc:72: accept_types =
*(reinterpret_cast<AcceptTypes*>(params));
spacing is off here.

https://codereview.chromium.org/14758008/diff/49001/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog_android.cc:77: bool has_capture_attribute =
accept_types.second;
Is there a reason for the local rather then accepot_types.second below?

[Raphael Kubo da Costa (rakuco), 2013-07-08 21:27:00]

https://codereview.chromium.org/14758008/diff/49001/ui/shell_dialogs/select_f...
File ui/shell_dialogs/select_file_dialog_android.cc (right):

https://codereview.chromium.org/14758008/diff/49001/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog_android.cc:72: accept_types =
*(reinterpret_cast<AcceptTypes*>(params));
On 2013/07/08 21:13:51, sky wrote:
> spacing is off here.

Done.

https://codereview.chromium.org/14758008/diff/49001/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog_android.cc:77: bool has_capture_attribute =
accept_types.second;
On 2013/07/08 21:13:51, sky wrote:
> Is there a reason for the local rather then accepot_types.second below?

It seemed to be easier to understand that way. Would you like me to change that?

[sky, 2013-07-08 21:32:32]

https://codereview.chromium.org/14758008/diff/49001/ui/shell_dialogs/select_f...
File ui/shell_dialogs/select_file_dialog_android.cc (right):

https://codereview.chromium.org/14758008/diff/49001/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog_android.cc:77: bool has_capture_attribute =
accept_types.second;
On 2013/07/08 21:27:01, Raphael Kubo da Costa (rakuco) wrote:
> On 2013/07/08 21:13:51, sky wrote:
> > Is there a reason for the local rather then accepot_types.second below?
> 
> It seemed to be easier to understand that way. Would you like me to change
that?

I like consistency. Since you don't declare a local variable for
accept_types.first above don't for accept_types.second.

[Raphael Kubo da Costa (rakuco), 2013-07-08 21:38:12]

https://codereview.chromium.org/14758008/diff/49001/ui/shell_dialogs/select_f...
File ui/shell_dialogs/select_file_dialog_android.cc (right):

https://codereview.chromium.org/14758008/diff/49001/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog_android.cc:77: bool has_capture_attribute =
accept_types.second;
On 2013/07/08 21:32:32, sky wrote:
> On 2013/07/08 21:27:01, Raphael Kubo da Costa (rakuco) wrote:
> > On 2013/07/08 21:13:51, sky wrote:
> > > Is there a reason for the local rather then accepot_types.second below?
> > 
> > It seemed to be easier to understand that way. Would you like me to change
> that?
> 
> I like consistency. Since you don't declare a local variable for
> accept_types.first above don't for accept_types.second.

Done.

[Ted C, 2013-07-08 21:50:56]

https://codereview.chromium.org/14758008/diff/58001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/SelectFileDialog.java (right):

https://codereview.chromium.org/14758008/diff/58001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/SelectFileDialog.java:215: return
mFileTypes.size() == 1 && mFileTypes.at(0).equals(type);
I don't think "at(...)" exists for ArrayList...that I'm aware of.  get(...) is
probably what you want.

Also, I would do TextUtils.equals(mFileTypes.get(0), type) because it will
nicely handle null.

[Raphael Kubo da Costa (rakuco), 2013-07-08 22:45:19]

Thanks for the comments. My ISP has decided to go offline in the middle of all
this, so I'll only be able to update the CL tomorrow, hopefully to get the final
acks :-)

[sky, 2013-07-09 00:14:10]

LGTM

[commit-bot: I haz the power, 2013-07-09 11:05:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raphael.kubo.da.costa@intel.com/147580...

[commit-bot: I haz the power, 2013-07-09 14:06:53]

Change committed as 210553