Add capture attribute for HTML Media Capture

• Component: DOM
• Status: RESOLVED DUPLICATE

[Jongseok Yang]

HTML Media Capture is the HTML Form Based specification that allows the user to take a picture, record a sound file, or record a video like  the file picker interface.
The "capture" attribute should be added for the feature. 
Webkit should check the attribute and execute the proper application or interface.

[Jongseok Yang]

Created attachment 97971 [details]
add capture attribute and functions to get the attribute from FileChooser

[Wonsuk Lee]

I think this is duplicated with Bug 58184(Add support for HTML media capture). So it should be merged to one bug.

[Gyuyoung Kim]

(In reply to comment #2)
> I think this is duplicated with Bug 58184(Add support for HTML media capture). So it should be merged to one bug.

I add a comment regarding this issue. Let's wait for his response.

[Gyuyoung Kim]

Comment on attachment 97971 [details]
add capture attribute and functions to get the attribute from FileChooser

View in context: https://bugs.webkit.org/attachment.cgi?id=97971&action=review

I'm wondering if media capture feature can work on this patch.

> Source/WebCore/ChangeLog:16
> +        No new tests.

If you don't have test cases, remove above line or add the reason why your patch doesn't have test case.

> Source/WebCore/html/HTMLInputElement.cpp:1235
> +#if ENABLE(MEDIA_CAPTURE)

IMO, you have to define disabled ENABLE_MEDIA_CAPTURE. And also, this feature can be adjusted to all ports. Thus, you SHOULD add the macro to configuration files of all ports.

[Gyuyoung Kim]

CC'ing Rodger Combs.

[Sam Weinig]

Comment on attachment 97971 [details]
add capture attribute and functions to get the attribute from FileChooser

I don't think it makes sense for this to go through the open panel interface.

[Jongseok Yang]

(In reply to comment #6)
> (From update of attachment 97971 [details])
> I don't think it makes sense for this to go through the open panel interface.

The capture attribute indicates which source the file picker interface should preferably present to the user by default. I think that it means the user can change the source from the open panel.
You check the use case from "A. User Interface Examples" in http://www.w3.org/TR/2011/WD-html-media-capture-20110414/

The operation for HTML Media Capture is same as open panel except that the capture attribute should be check in open panel operation.
So, I think that the patch is very simple implementation for HTML Media Capture.

[Jongseok Yang]

Created attachment 98147 [details]
Patch v2 : fix "no new tests", remove macro

[Sam Weinig]

Comment on attachment 98147 [details]
Patch v2 : fix "no new tests", remove macro

I think my old comment still holds. Even if the API for this feature is on <input type=file>, I see no reason for the implementation to go through the FileChooser.  I would much rather see a new dedicated "capture" chooser, and new chrome client calls.  It should also not use RenderFileUploadControl.

[Jongseok Yang]

(In reply to comment #9)
> (From update of attachment 98147 [details])
> I think my old comment still holds. Even if the API for this feature is on <input type=file>, I see no reason for the implementation to go through the FileChooser.  I would much rather see a new dedicated "capture" chooser, and new chrome client calls.  It should also not use RenderFileUploadControl.

Thanks for your feedback.

Could you explain the reason for your proposal?

I thought that using the original logic (FileChooser, RenderFileUploadControl) was the advantage because I thought that there was no notable difference except executing camera, camcorder or voice recorder.

I'd like to know what the point overlooked.

[Sam Weinig]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 98147 [details] [details])
> > I think my old comment still holds. Even if the API for this feature is on <input type=file>, I see no reason for the implementation to go through the FileChooser.  I would much rather see a new dedicated "capture" chooser, and new chrome client calls.  It should also not use RenderFileUploadControl.
> 
> Thanks for your feedback.
> 
> Could you explain the reason for your proposal?
> 
> I thought that using the original logic (FileChooser, RenderFileUploadControl) was the advantage because I thought that there was no notable difference except executing camera, camcorder or voice recorder.

My objection is that the difference between choosing a file and using a camera is a not a small one and the implementation should reflect that.

[Eric Carlson]

(In reply to comment #11)
> (In reply to comment #10)
> > 
> > Thanks for your feedback.
> > 
> > Could you explain the reason for your proposal?
> > 
> > I thought that using the original logic (FileChooser, RenderFileUploadControl) was the advantage because I thought that there was no notable difference except executing camera, camcorder or voice recorder.
> 
> My objection is that the difference between choosing a file and using a camera is a not a small one and the implementation should reflect that.

I agree with Sam. While some platforms may want to present a simplistic UI like the mockup shown in http://www.w3.org/TR/2011/WD-html-media-capture-20110414/, I suspect that many will require very different UI for file, camera, and microphone access. Lumping everything into the same classes will make this more difficult and messy.

[Dimitri Glazkov (Google)]

Listen to the Elders!

BTW, I completely reworked FileChooser stuff since you posted this patch, which should make it easier for you to add new type of a chooser.

[Ridley Combs]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (From update of attachment 98147 [details] [details] [details])
> > > I think my old comment still holds. Even if the API for this feature is on <input type=file>, I see no reason for the implementation to go through the FileChooser.  I would much rather see a new dedicated "capture" chooser, and new chrome client calls.  It should also not use RenderFileUploadControl.
> > 
> > Thanks for your feedback.
> > 
> > Could you explain the reason for your proposal?
> > 
> > I thought that using the original logic (FileChooser, RenderFileUploadControl) was the advantage because I thought that there was no notable difference except executing camera, camcorder or voice recorder.
> 
> My objection is that the difference between choosing a file and using a camera is a not a small one and the implementation should reflect that.

I agree that the difference between using a camera and choosing a file is a large change that should be noticeable and easily usable, both should definitely be usable from the same element. I don't think "File Upload" is necessarily the correct text to display in the window; perhaps "Media Upload" or "Audio|Video|Image Upload" (depending on the media type requested). As for the UI to choose between use of a file and use of a capture device, I suggest that before displaying a file chooser, a dialog of some sort should appear that asks the user where they want to get the requested media from: the filesystem, or a capture device. I think this dialog should list all available options (the filesystem and EACH INDIVIDUAL CAPTURE DEVICE that can be used). This should be bypassed when no capture attribute is specified. The dialog text could be something along the lines of this: "The page at www.example.com has requested that you upload a media file. Which source would you like to upload this file from? [Hard Disk], [Canon HD-AV470], [Sony Capture-Card-530] [Cancel]" Any takers?

[Jongseok Yang]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > 
> > > Thanks for your feedback.
> > > 
> > > Could you explain the reason for your proposal?
> > > 
> > > I thought that using the original logic (FileChooser, RenderFileUploadControl) was the advantage because I thought that there was no notable difference except executing camera, camcorder or voice recorder.
> > 
> > My objection is that the difference between choosing a file and using a camera is a not a small one and the implementation should reflect that.
> 
> I agree with Sam. While some platforms may want to present a simplistic UI like the mockup shown in http://www.w3.org/TR/2011/WD-html-media-capture-20110414/, I suspect that many will require very different UI for file, camera, and microphone access. Lumping everything into the same classes will make this more difficult and messy.

Thanks for your interest in my patch.
Actually, I had no plan for more modification. of FileChooser, RenderFileUploadControl. The patch was just to get "capture" attribute.
I did not think that any more patch for "WebCore" were needed just to launch a camera interface. I thought that the next patch for it was just the implementation for each port. So, I could not detect that the patch was more difficult and messy.

I'll trying to find out new implementation to accept your proposal.

[Jongseok Yang]

(In reply to comment #14)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > (From update of attachment 98147 [details] [details] [details] [details])
> > > > I think my old comment still holds. Even if the API for this feature is on <input type=file>, I see no reason for the implementation to go through the FileChooser.  I would much rather see a new dedicated "capture" chooser, and new chrome client calls.  It should also not use RenderFileUploadControl.
> > > 
> > > Thanks for your feedback.
> > > 
> > > Could you explain the reason for your proposal?
> > > 
> > > I thought that using the original logic (FileChooser, RenderFileUploadControl) was the advantage because I thought that there was no notable difference except executing camera, camcorder or voice recorder.
> > 
> > My objection is that the difference between choosing a file and using a camera is a not a small one and the implementation should reflect that.
> 
> I agree that the difference between using a camera and choosing a file is a large change that should be noticeable and easily usable, both should definitely be usable from the same element. I don't think "File Upload" is necessarily the correct text to display in the window; perhaps "Media Upload" or "Audio|Video|Image Upload" (depending on the media type requested). As for the UI to choose between use of a file and use of a capture device, I suggest that before displaying a file chooser, a dialog of some sort should appear that asks the user where they want to get the requested media from: the filesystem, or a capture device. I think this dialog should list all available options (the filesystem and EACH INDIVIDUAL CAPTURE DEVICE that can be used). This should be bypassed when no capture attribute is specified. The dialog text could be something along the lines of this: "The page at www.example.com has requested that you upload a media file. Which source would you like to upload this file from? [Hard Disk], [Canon HD-AV470], [Sony Capture-Card-530] [Cancel]" Any takers?

That good idea. 
I thought that your proposal could be implemented within each port and it's just the implementation choice of each port. Also the display text can be changed by the implementation of each port.

But, I'll trying to find out new implementation to accept reviewers' proposal.

[Wonsuk Lee]

(In reply to comment #15)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > 
> > > > Thanks for your feedback.
> > > > 
> > > > Could you explain the reason for your proposal?
> > > > 
> > > > I thought that using the original logic (FileChooser, RenderFileUploadControl) was the advantage because I thought that there was no notable difference except executing camera, camcorder or voice recorder.
> > > 
> > > My objection is that the difference between choosing a file and using a camera is a not a small one and the implementation should reflect that.
> > 
> > I agree with Sam. While some platforms may want to present a simplistic UI like the mockup shown in http://www.w3.org/TR/2011/WD-html-media-capture-20110414/, I suspect that many will require very different UI for file, camera, and microphone access. Lumping everything into the same classes will make this more difficult and messy.
> 
> Thanks for your interest in my patch.
> Actually, I had no plan for more modification. of FileChooser, RenderFileUploadControl. The patch was just to get "capture" attribute.
> I did not think that any more patch for "WebCore" were needed just to launch a camera interface. I thought that the next patch for it was just the implementation for each port. So, I could not detect that the patch was more difficult and messy.
> 
> I'll trying to find out new implementation to accept your proposal.

For the clarification, Do you mean current version of WebCore is ready to support W3C HTML Media Capture spec ?? Only remaining stuff is related with the port layer. right?
(In reply to comment #15)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > 
> > > > Thanks for your feedback.
> > > > 
> > > > Could you explain the reason for your proposal?
> > > > 
> > > > I thought that using the original logic (FileChooser, RenderFileUploadControl) was the advantage because I thought that there was no notable difference except executing camera, camcorder or voice recorder.
> > > 
> > > My objection is that the difference between choosing a file and using a camera is a not a small one and the implementation should reflect that.
> > 
> > I agree with Sam. While some platforms may want to present a simplistic UI like the mockup shown in http://www.w3.org/TR/2011/WD-html-media-capture-20110414/, I suspect that many will require very different UI for file, camera, and microphone access. Lumping everything into the same classes will make this more difficult and messy.
> 
> Thanks for your interest in my patch.
> Actually, I had no plan for more modification. of FileChooser, RenderFileUploadControl. The patch was just to get "capture" attribute.
> I did not think that any more patch for "WebCore" were needed just to launch a camera interface. I thought that the next patch for it was just the implementation for each port. So, I could not detect that the patch was more difficult and messy.
> 
> I'll trying to find out new implementation to accept your proposal.

For the clarification, do you mean current version of WebCore is ready to support W3C HTML Media Capture spec ?? And only remaining stuff is related with the port layer. right?

[Jongseok Yang]

(In reply to comment #17)
> (In reply to comment #15)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > (In reply to comment #10)
> > > > > 
> > > > > Thanks for your feedback.
> > > > > 
> > > > > Could you explain the reason for your proposal?
> > > > > 
> > > > > I thought that using the original logic (FileChooser, RenderFileUploadControl) was the advantage because I thought that there was no notable difference except executing camera, camcorder or voice recorder.
> > > > 
> > > > My objection is that the difference between choosing a file and using a camera is a not a small one and the implementation should reflect that.
> > > 
> > > I agree with Sam. While some platforms may want to present a simplistic UI like the mockup shown in http://www.w3.org/TR/2011/WD-html-media-capture-20110414/, I suspect that many will require very different UI for file, camera, and microphone access. Lumping everything into the same classes will make this more difficult and messy.
> > 
> > Thanks for your interest in my patch.
> > Actually, I had no plan for more modification. of FileChooser, RenderFileUploadControl. The patch was just to get "capture" attribute.
> > I did not think that any more patch for "WebCore" were needed just to launch a camera interface. I thought that the next patch for it was just the implementation for each port. So, I could not detect that the patch was more difficult and messy.
> > 
> > I'll trying to find out new implementation to accept your proposal.
> 
> For the clarification, Do you mean current version of WebCore is ready to support W3C HTML Media Capture spec ?? Only remaining stuff is related with the port layer. right?
> (In reply to comment #15)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > (In reply to comment #10)
> > > > > 
> > > > > Thanks for your feedback.
> > > > > 
> > > > > Could you explain the reason for your proposal?
> > > > > 
> > > > > I thought that using the original logic (FileChooser, RenderFileUploadControl) was the advantage because I thought that there was no notable difference except executing camera, camcorder or voice recorder.
> > > > 
> > > > My objection is that the difference between choosing a file and using a camera is a not a small one and the implementation should reflect that.
> > > 
> > > I agree with Sam. While some platforms may want to present a simplistic UI like the mockup shown in http://www.w3.org/TR/2011/WD-html-media-capture-20110414/, I suspect that many will require very different UI for file, camera, and microphone access. Lumping everything into the same classes will make this more difficult and messy.
> > 
> > Thanks for your interest in my patch.
> > Actually, I had no plan for more modification. of FileChooser, RenderFileUploadControl. The patch was just to get "capture" attribute.
> > I did not think that any more patch for "WebCore" were needed just to launch a camera interface. I thought that the next patch for it was just the implementation for each port. So, I could not detect that the patch was more difficult and messy.
> > 
> > I'll trying to find out new implementation to accept your proposal.
> 
> For the clarification, do you mean current version of WebCore is ready to support W3C HTML Media Capture spec ?? And only remaining stuff is related with the port layer. right?

I thought that it's right because I thought that it's just the implementation issue for each port and the "capture" attribute was optional from the below statement of HTML Media Capture spec. 
"If an input element in the File Upload state [HTML5] contains accept attribute with values image/*, audio/*, or video/*, the user agent can invoke a file picker that allows respectively the user to take a picture, record a sound file, or record a video in addition to selecting an existing file from the file system."

But, it's just my idea and I should need new implementation to accept reviewers' proposal.

[Jongseok Yang]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #15)
> > > (In reply to comment #12)
> > > > (In reply to comment #11)
> > > > > (In reply to comment #10)
> > > > > > 
> > > > > > Thanks for your feedback.
> > > > > > 
> > > > > > Could you explain the reason for your proposal?
> > > > > > 
> > > > > > I thought that using the original logic (FileChooser, RenderFileUploadControl) was the advantage because I thought that there was no notable difference except executing camera, camcorder or voice recorder.
> > > > > 
> > > > > My objection is that the difference between choosing a file and using a camera is a not a small one and the implementation should reflect that.
> > > > 
> > > > I agree with Sam. While some platforms may want to present a simplistic UI like the mockup shown in http://www.w3.org/TR/2011/WD-html-media-capture-20110414/, I suspect that many will require very different UI for file, camera, and microphone access. Lumping everything into the same classes will make this more difficult and messy.
> > > 
> > > Thanks for your interest in my patch.
> > > Actually, I had no plan for more modification. of FileChooser, RenderFileUploadControl. The patch was just to get "capture" attribute.
> > > I did not think that any more patch for "WebCore" were needed just to launch a camera interface. I thought that the next patch for it was just the implementation for each port. So, I could not detect that the patch was more difficult and messy.
> > > 
> > > I'll trying to find out new implementation to accept your proposal.
> > 
> > For the clarification, Do you mean current version of WebCore is ready to support W3C HTML Media Capture spec ?? Only remaining stuff is related with the port layer. right?
> > (In reply to comment #15)
> > > (In reply to comment #12)
> > > > (In reply to comment #11)
> > > > > (In reply to comment #10)
> > > > > > 
> > > > > > Thanks for your feedback.
> > > > > > 
> > > > > > Could you explain the reason for your proposal?
> > > > > > 
> > > > > > I thought that using the original logic (FileChooser, RenderFileUploadControl) was the advantage because I thought that there was no notable difference except executing camera, camcorder or voice recorder.
> > > > > 
> > > > > My objection is that the difference between choosing a file and using a camera is a not a small one and the implementation should reflect that.
> > > > 
> > > > I agree with Sam. While some platforms may want to present a simplistic UI like the mockup shown in http://www.w3.org/TR/2011/WD-html-media-capture-20110414/, I suspect that many will require very different UI for file, camera, and microphone access. Lumping everything into the same classes will make this more difficult and messy.
> > > 
> > > Thanks for your interest in my patch.
> > > Actually, I had no plan for more modification. of FileChooser, RenderFileUploadControl. The patch was just to get "capture" attribute.
> > > I did not think that any more patch for "WebCore" were needed just to launch a camera interface. I thought that the next patch for it was just the implementation for each port. So, I could not detect that the patch was more difficult and messy.
> > > 
> > > I'll trying to find out new implementation to accept your proposal.
> > 
> > For the clarification, do you mean current version of WebCore is ready to support W3C HTML Media Capture spec ?? And only remaining stuff is related with the port layer. right?
> 
> I thought that it's right because I thought that it's just the implementation issue for each port and the "capture" attribute was optional from the below statement of HTML Media Capture spec. 
> "If an input element in the File Upload state [HTML5] contains accept attribute with values image/*, audio/*, or video/*, the user agent can invoke a file picker that allows respectively the user to take a picture, record a sound file, or record a video in addition to selecting an existing file from the file system."
> 
> But, it's just my idea and I should need new implementation to accept reviewers' proposal.

For the clarification, it's just to launch camera.
MediaFileData should be implemented to support HTML Media Capture spec.

[Ben Murdoch]

Hey guys, I'm interested in reviving this. As there hasn't been any update for six months are you happy if I grab the bug Jongseok Yang?

[Jongseok Yang]

(In reply to comment #20)
> Hey guys, I'm interested in reviving this. As there hasn't been any update for six months are you happy if I grab the bug Jongseok Yang?

I'm sorry that I have not been updated this bug.
I'll upload new patch next week. Just when it never be acceptable, I'll be happy for you to grab this bug.

And I think that HTML Media Capture has two independent implementation.
One is just for "capture" attribute. The other is the WebIDL implementation for getFormatData.

My new patch will be just for "capture" attribute.
For WebIDL for getFormatData, I'll make new bug.

[Jongseok Yang]

Created attachment 123233 [details]
Patch

[Jongseok Yang]

Created attachment 123245 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 123245 [details]
Patch

Attachment 123245 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11127328

[Jongseok Yang]

Created attachment 123253 [details]
Patch

[Ben Murdoch]

Thanks for the new patch Jongseok. I'm not a reviewer, but FWIW I don't think the patch addresses the previous review comments.

I think it'd be best to follow the previous review advice and create a new Capture chooser that inherits from FileChooser and adds the capture attribute value.

Then add a new method on Chrome[Client] to start a capture panel passing in the capture attribute - it can be called from FileInputType at the same place runOpenPanel is called - we determine which one to call based on whether a capture attribute is set or not. Then each port can handle the runCapturePanel as they see fit (which may be very similar to what they do in runOpenPanel, but might not too).

What do you think?

[Jongseok Yang]

(In reply to comment #26)
> Thanks for the new patch Jongseok. I'm not a reviewer, but FWIW I don't think the patch addresses the previous review comments.
> 
> I think it'd be best to follow the previous review advice and create a new Capture chooser that inherits from FileChooser and adds the capture attribute value.
> 
> Then add a new method on Chrome[Client] to start a capture panel passing in the capture attribute - it can be called from FileInputType at the same place runOpenPanel is called - we determine which one to call based on whether a capture attribute is set or not. Then each port can handle the runCapturePanel as they see fit (which may be very similar to what they do in runOpenPanel, but might not too).
> 
> What do you think?


Thanks for your review. 
I have implemented it to accept you. I'll upload new patch next week after making up for the week points.

[Jongseok Yang]

Created attachment 127553 [details]
Patch

[Jongseok Yang]

I uploaded new patch. Could you please review that?

I tried to reflect Ben Murdoch's review. I created new chooser "MediaCaptureChooser" inheriting from FileChooser and add new method "runCapturePanel" to launch a capture panel.

However, I did not create new renderer because I think input element for Media Capture should use the same renderer as input element with file type. If it is not acceptable, I'll try to create new renderer.

[Ben Murdoch]

Comment on attachment 127553 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127553&action=review

Thanks for the latest patch Jongseok! This looks closer to me to what the original reviewers were looking for. I am not a reviewer so cannot formally review the patch, but I have a few comments and will defer back to the original reviewers for their thoughts. Sam, would you mind taking another look at this?

Cheers, Ben

> Source/WebCore/platform/FileChooser.cpp:64
> +    return static_cast<MediaCaptureChooser*>(m_chooser.get());

Don't think the static_cast is needed?

> Source/WebCore/platform/FileChooser.cpp:73
> +        for (Vector<String>::const_iterator iterator = input->acceptMIMETypes().begin(); iterator != input->acceptMIMETypes().end(); ++iterator)

Could this be replaced with something like:
if (MIMETypeRegistry::isSupportCaptureType("image/*") && input->acceptMIMETypes().contains("image/*)) ?

> Source/WebCore/platform/FileChooser.h:44
> +#endif

Maybe merge these two #ifdef blocks?

> Source/WebCore/platform/FileChooser.h:51
> +    Microphone,

These should probably be in alphabetical order.

[Jongseok Yang]

(In reply to comment #30)

Thank you for your review. It was a good help in my patch. 

I have some question about your comment. Could you please give me an answer?


> (From update of attachment 127553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127553&action=review
> 
> Thanks for the latest patch Jongseok! This looks closer to me to what the original reviewers were looking for. I am not a reviewer so cannot formally review the patch, but I have a few comments and will defer back to the original reviewers for their thoughts. Sam, would you mind taking another look at this?
> 
> Cheers, Ben
> 
> > Source/WebCore/platform/FileChooser.cpp:64
> > +    return static_cast<MediaCaptureChooser*>(m_chooser.get());
> 
> Don't think the static_cast is needed?
> 
I'll fix that.


> > Source/WebCore/platform/FileChooser.cpp:73
> > +        for (Vector<String>::const_iterator iterator = input->acceptMIMETypes().begin(); iterator != input->acceptMIMETypes().end(); ++iterator)
> 
> Could this be replaced with something like:
> if (MIMETypeRegistry::isSupportCaptureType("image/*") && input->acceptMIMETypes().contains("image/*)) ?
> 
Could you please check the URL? 
(http://lists.w3.org/Archives/Public/public-device-apis/2011Apr/0013.html)
I'd like to implement a good solution to meet the editor's idea.
Could you please give me your idea?


> > Source/WebCore/platform/FileChooser.h:44
> > +#endif
> 
> Maybe merge these two #ifdef blocks?
> 
> > Source/WebCore/platform/FileChooser.h:51
> > +    Microphone,
> 
> These should probably be in alphabetical order.
>
Do you mean that named types in enum should be sorted? I observed that there are many enum types that they don't meet the coding style. 
Could you please clarify the coding style?

[Ben Murdoch]

> > > Source/WebCore/platform/FileChooser.cpp:73
> > > +        for (Vector<String>::const_iterator iterator = input->acceptMIMETypes().begin(); iterator != input->acceptMIMETypes().end(); ++iterator)
> > 
> > Could this be replaced with something like:
> > if (MIMETypeRegistry::isSupportCaptureType("image/*") && input->acceptMIMETypes().contains("image/*)) ?
> > 
> Could you please check the URL? 
> (http://lists.w3.org/Archives/Public/public-device-apis/2011Apr/0013.html)
> I'd like to implement a good solution to meet the editor's idea.
> Could you please give me your idea?

Oh, I see. It covers the case that the accept type was image/svg mentioned in that email.


> > > Source/WebCore/platform/FileChooser.h:44
> > > +#endif
> > 
> > Maybe merge these two #ifdef blocks?
> > 
> > > Source/WebCore/platform/FileChooser.h:51
> > > +    Microphone,
> > 
> > These should probably be in alphabetical order.
> >
> Do you mean that named types in enum should be sorted? I observed that there are many enum types that they don't meet the coding style. 
> Could you please clarify the coding style?

That's what I meant, but you're right, the style guide doesn't enforce it. IMO I think it looks neater, but whichever you prefer.

[Kent Tamura]

Comment on attachment 127553 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127553&action=review

Please announce this feature to webkit-dev.
http://www.webkit.org/coding/adding-features.html

Does MedieaCaptureChooser set a file name to FileChooerClient?  If so, who removes the file? <input type=file> shows a file name for a captured data?

> Source/WebCore/platform/FileChooser.cpp:72
> +    if (equalIgnoringCase(input->capture(), MediaCaptureNames::camera())) {

input->capture() should be input->fastGetAttribute(captureAttr), and should remove HTMLInputElement::capture().
Anyway, this is a layering violation.

> Source/WebCore/platform/FileChooser.h:74
> +    MediaCaptureType determineCaptureType(HTMLInputElement*);

Layering violation.  We must not use WebCore/html/* classes in WebCore/platform/* files.

[Jongseok Yang]

(In reply to comment #32)
> > > > Source/WebCore/platform/FileChooser.cpp:73
> > > > +        for (Vector<String>::const_iterator iterator = input->acceptMIMETypes().begin(); iterator != input->acceptMIMETypes().end(); ++iterator)
> > > 
> > > Could this be replaced with something like:
> > > if (MIMETypeRegistry::isSupportCaptureType("image/*") && input->acceptMIMETypes().contains("image/*)) ?
> > > 
> > Could you please check the URL? 
> > (http://lists.w3.org/Archives/Public/public-device-apis/2011Apr/0013.html)
> > I'd like to implement a good solution to meet the editor's idea.
> > Could you please give me your idea?
> 
> Oh, I see. It covers the case that the accept type was image/svg mentioned in that email.
> 

In the case of "<input type=file accept="image/jpeg" capture="camera">, A platform might want to support to run the capture panel. If so, it can be supported re-defining supportedCapturedMIMETypes in MIMETypeRegistery for the platform. My patch is for this concept.

What do you think?

> 
> > > > Source/WebCore/platform/FileChooser.h:44
> > > > +#endif
> > > 
> > > Maybe merge these two #ifdef blocks?
> > > 
> > > > Source/WebCore/platform/FileChooser.h:51
> > > > +    Microphone,
> > > 
> > > These should probably be in alphabetical order.
> > >
> > Do you mean that named types in enum should be sorted? I observed that there are many enum types that they don't meet the coding style. 
> > Could you please clarify the coding style?
> 
> That's what I meant, but you're right, the style guide doesn't enforce it. IMO I think it looks neater, but whichever you prefer.

[Jongseok Yang]

(In reply to comment #31)
> (In reply to comment #30)
> 
> Thank you for your review. It was a good help in my patch. 
> 
> I have some question about your comment. Could you please give me an answer?
> 
> 
> > (From update of attachment 127553 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=127553&action=review
> > 
> > Thanks for the latest patch Jongseok! This looks closer to me to what the original reviewers were looking for. I am not a reviewer so cannot formally review the patch, but I have a few comments and will defer back to the original reviewers for their thoughts. Sam, would you mind taking another look at this?
> > 
> > Cheers, Ben
> > 
> > > Source/WebCore/platform/FileChooser.cpp:64
> > > +    return static_cast<MediaCaptureChooser*>(m_chooser.get());
> > 
> > Don't think the static_cast is needed?
> > 
> I'll fix that.
> 

Well, I got the build break when removing static_cast. Please check it again.

> 
> > > Source/WebCore/platform/FileChooser.cpp:73
> > > +        for (Vector<String>::const_iterator iterator = input->acceptMIMETypes().begin(); iterator != input->acceptMIMETypes().end(); ++iterator)
> > 
> > Could this be replaced with something like:
> > if (MIMETypeRegistry::isSupportCaptureType("image/*") && input->acceptMIMETypes().contains("image/*)) ?
> > 
> Could you please check the URL? 
> (http://lists.w3.org/Archives/Public/public-device-apis/2011Apr/0013.html)
> I'd like to implement a good solution to meet the editor's idea.
> Could you please give me your idea?
> 
> 
> > > Source/WebCore/platform/FileChooser.h:44
> > > +#endif
> > 
> > Maybe merge these two #ifdef blocks?
> > 
> > > Source/WebCore/platform/FileChooser.h:51
> > > +    Microphone,
> > 
> > These should probably be in alphabetical order.
> >
> Do you mean that named types in enum should be sorted? I observed that there are many enum types that they don't meet the coding style. 
> Could you please clarify the coding style?

[Jongseok Yang]

(In reply to comment #33)

Thank you for your review. 
Could you please check my comment?


> (From update of attachment 127553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127553&action=review
> 
> Please announce this feature to webkit-dev.
> http://www.webkit.org/coding/adding-features.html
> 

I will that.

> Does MedieaCaptureChooser set a file name to FileChooerClient?  If so, who removes the file? <input type=file> shows a file name for a captured data?
> 

Yes. a file name is set using chooseFile() of MedieaCaptureChooser.
I think that WebKit need not consider who removes the file. WebKit supports download feature but does not manage the downloaded files. 
I think that the feature is similar to the function of "photo" menu in facebook mobile application. The application like facebook mobile application don't care to manage the taken picture. I think it is just the check point of the platform.

> > Source/WebCore/platform/FileChooser.cpp:72
> > +    if (equalIgnoringCase(input->capture(), MediaCaptureNames::camera())) {
> 
> input->capture() should be input->fastGetAttribute(captureAttr), and should remove HTMLInputElement::capture().
> Anyway, this is a layering violation.

capture() is the wrapper function for fastGetAttribute(captureAttr). I observed the same logic from HTMLInputElement::accept(), HTMLInputElement::alt() and HTMLInputElement::multiple(). Do you mean that these functions are also layering violation?

> 
> > Source/WebCore/platform/FileChooser.h:74
> > +    MediaCaptureType determineCaptureType(HTMLInputElement*);
> 
> Layering violation.  We must not use WebCore/html/* classes in WebCore/platform/* files.

I will fix that.

[Kent Tamura]

(In reply to comment #36)

I took a look at the draft.
 - The draft is old.  Is the WG working on it actively?
 - We should support media capture regardless of existence of 'capture' attribute.
  http://www.w3.org/TR/2011/WD-html-media-capture-20110414/#formaccess
 - I don't think we need to use both of Chrome::runOpenPanel and Chrome::runCapturePanel. runCapturePanel should support file choosing, and we should always use Chrome::runCapturePanel if ENABLE_HTML_MEDIA_CAPTURE.
 - I don't think showing a captured file name makes sense. We had better show a captured media information such as:
   "1280x768 still image (123KB)" (with a thumbnail?)
   "35 second audio (63KB)"

> > > Source/WebCore/platform/FileChooser.cpp:72
> > > +    if (equalIgnoringCase(input->capture(), MediaCaptureNames::camera())) {
> > 
> > input->capture() should be input->fastGetAttribute(captureAttr), and should remove HTMLInputElement::capture().
> > Anyway, this is a layering violation.
> 
> capture() is the wrapper function for fastGetAttribute(captureAttr). I observed the same logic from HTMLInputElement::accept(), HTMLInputElement::alt() and HTMLInputElement::multiple(). Do you mean that these functions are also layering violation?

I meant using HTMLInputElement in platform/FileChooser.cpp was a layering violation.

[Jongseok Yang]

(In reply to comment #37)

Thank you for your review. Could you please check my comment?

> (In reply to comment #36)
> 
> I took a look at the draft.
>  - The draft is old.  Is the WG working on it actively?

 I'll check it.

>  - We should support media capture regardless of existence of 'capture' attribute.
>   http://www.w3.org/TR/2011/WD-html-media-capture-20110414/#formaccess

 I agree with you.

>  - I don't think we need to use both of Chrome::runOpenPanel and Chrome::runCapturePanel. runCapturePanel should support file choosing, and we should always use Chrome::runCapturePanel if ENABLE_HTML_MEDIA_CAPTURE.

 I agree with you. I'll fix that
 
>  - I don't think showing a captured file name makes sense. We had better show a captured media information such as:
>    "1280x768 still image (123KB)" (with a thumbnail?)
>    "35 second audio (63KB)"

 I think that WebIDL interface in the spec is for the media information you mentioned. 
 Please check the example : http://www.w3.org/TR/2011/WD-html-media-capture-20110414/#jsexample
 So, I think we need not show the media information.

> 
> > > > Source/WebCore/platform/FileChooser.cpp:72
> > > > +    if (equalIgnoringCase(input->capture(), MediaCaptureNames::camera())) {
> > > 
> > > input->capture() should be input->fastGetAttribute(captureAttr), and should remove HTMLInputElement::capture().
> > > Anyway, this is a layering violation.
> > 
> > capture() is the wrapper function for fastGetAttribute(captureAttr). I observed the same logic from HTMLInputElement::accept(), HTMLInputElement::alt() and HTMLInputElement::multiple(). Do you mean that these functions are also layering violation?
> 
> I meant using HTMLInputElement in platform/FileChooser.cpp was a layering violation.

OK, I will fix.

[Masataka Yakura]

(In reply to comment #37)
> (In reply to comment #36)
> 
> I took a look at the draft.
>  - The draft is old.  Is the WG working on it actively?

I think the DAP WG was agreed to abondon the draft in favor of getUserMedia.
http://lists.w3.org/Archives/Public/public-device-apis/2011Nov/att-0177/minutes-2011-11-03.html#item05
http://lists.w3.org/Archives/Public/public-media-capture/2011Dec/0029.html

http://dev.w3.org/2009/dap/camera/Overview-API.html redirects to the gUM editor's draft.

[Wonsuk Lee]

(In reply to comment #39)
> (In reply to comment #37)
> > (In reply to comment #36)
> > 
> > I took a look at the draft.
> >  - The draft is old.  Is the WG working on it actively?
> I think the DAP WG was agreed to abondon the draft in favor of getUserMedia.
> http://lists.w3.org/Archives/Public/public-device-apis/2011Nov/att-0177/minutes-2011-11-03.html#item05
> http://lists.w3.org/Archives/Public/public-media-capture/2011Dec/0029.html
> http://dev.w3.org/2009/dap/camera/Overview-API.html redirects to the gUM editor's draft.

I think you were confused for the specs related with Media Capture in W3C DAP WG.

Before starting gUM draft, there were two different specs for media capture in W3C DAP WG.
First one is HTML Media Capture[1] and the Second is Media Capture API. Above all things you mentioned are exactly related with Media Capture API. and As you mentioned, it replaced with gUM draft. 

By the way, this bug is exactly related with HTML Media Capture.

[1] http://www.w3.org/TR/2011/WD-html-media-capture-20110414/

[Jongseok Yang]

(In reply to comment #40)
> (In reply to comment #39)
> > (In reply to comment #37)
> > > (In reply to comment #36)
> > > 
> > > I took a look at the draft.
> > >  - The draft is old.  Is the WG working on it actively?
> > I think the DAP WG was agreed to abondon the draft in favor of getUserMedia.
> > http://lists.w3.org/Archives/Public/public-device-apis/2011Nov/att-0177/minutes-2011-11-03.html#item05
> > http://lists.w3.org/Archives/Public/public-media-capture/2011Dec/0029.html
> > http://dev.w3.org/2009/dap/camera/Overview-API.html redirects to the gUM editor's draft.
> 
> I think you were confused for the specs related with Media Capture in W3C DAP WG.
> 
> Before starting gUM draft, there were two different specs for media capture in W3C DAP WG.
> First one is HTML Media Capture[1] and the Second is Media Capture API. Above all things you mentioned are exactly related with Media Capture API. and As you mentioned, it replaced with gUM draft. 
> 
> By the way, this bug is exactly related with HTML Media Capture.
> 
> [1] http://www.w3.org/TR/2011/WD-html-media-capture-20110414/

Thank you for your comment.

This bug is just for HTML Media Capture as Wonsuk Lee mentioned. It's to invoke a capture panel with <input type=file>.
Media Capture API is the spec to launch camera, video or recorder application using APIs.

The first spec - HTML Media Capture is still available.

[Jongseok Yang]

Created attachment 127914 [details]
Patch

[Jongseok Yang]

(In reply to comment #38)
> (In reply to comment #37)
> 
> Thank you for your review. Could you please check my comment?
> 
> > (In reply to comment #36)
> > 
> > I took a look at the draft.
> >  - The draft is old.  Is the WG working on it actively?
> 
>  I'll check it.
> 
> >  - We should support media capture regardless of existence of 'capture' attribute.
> >   http://www.w3.org/TR/2011/WD-html-media-capture-20110414/#formaccess
> 
>  I agree with you.
> 
> >  - I don't think we need to use both of Chrome::runOpenPanel and Chrome::runCapturePanel. runCapturePanel should support file choosing, and we should always use Chrome::runCapturePanel if ENABLE_HTML_MEDIA_CAPTURE.
> 
>  I agree with you. I'll fix that

http://www.w3.org/TR/html-media-capture/#formaccess
I re-checked the spec. The panel for HTML Media Capture should be enabled only if the accept accept attribute with values image/*, audio/*, or video/*.

> 
> >  - I don't think showing a captured file name makes sense. We had better show a captured media information such as:
> >    "1280x768 still image (123KB)" (with a thumbnail?)
> >    "35 second audio (63KB)"
> 
>  I think that WebIDL interface in the spec is for the media information you mentioned. 
>  Please check the example : http://www.w3.org/TR/2011/WD-html-media-capture-20110414/#jsexample
>  So, I think we need not show the media information.
> 
> > 
> > > > > Source/WebCore/platform/FileChooser.cpp:72
> > > > > +    if (equalIgnoringCase(input->capture(), MediaCaptureNames::camera())) {
> > > > 
> > > > input->capture() should be input->fastGetAttribute(captureAttr), and should remove HTMLInputElement::capture().
> > > > Anyway, this is a layering violation.
> > > 
> > > capture() is the wrapper function for fastGetAttribute(captureAttr). I observed the same logic from HTMLInputElement::accept(), HTMLInputElement::alt() and HTMLInputElement::multiple(). Do you mean that these functions are also layering violation?
> > 
> > I meant using HTMLInputElement in platform/FileChooser.cpp was a layering violation.
> 
> OK, I will fix.

[Ben Murdoch]

Comment on attachment 127914 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127914&action=review

Thanks for the latest patch Jongseok!

> Source/WebCore/html/FileInputType.cpp:163
> +            chrome->runOpenPanel(input->document()->frame(), newFileChooser(settings));

As per Kent Tamura's comment, we should only use runCapturePanel #if ENABLE(MEDIA_CAPTURE). Each platform can implement runCapturePanel as it wants to, which may then call runOpenPanel if type == filesystem for example. That's up to the embedder.

> Source/WebCore/platform/FileChooser.cpp:70
> +        return NonCapture;

return FileSystem here as that is the default according to the spec?

> Source/WebCore/platform/FileChooser.cpp:76
> +            if ((capture.isEmpty() || equalIgnoringCase(capture, MediaCaptureNames::camera()))

Why if capture.isEmpty()? This means that if you specify accept="image/*" then we'll always default to the camera? I can't see where the spec says to do that.

> Source/WebCore/platform/FileChooser.cpp:77
> +                && (*iterator).startsWith("image/"))

I think this might be a bit easier to read if you checked the mime type first, i.e.
if (*iterator.startsWith("image/*") && equalsIgnoringCase(capture, MediaCaptureNames::camera()) ...

What do you think?

> Source/WebCore/platform/FileChooser.cpp:86
> +            if (type == NonCapture)

initialise type to FileSystem on line 72 and no need for this?

> Source/WebCore/platform/FileChooser.h:46
> +    NonCapture,

I don't think we need this one.

[Jongseok Yang]

Created attachment 129020 [details]
Patch

[Jongseok Yang]

(In reply to comment #44)

Thank you for your review.

I uploaded new patch as your review. Could you please check it?


> (From update of attachment 127914 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127914&action=review
> 
> Thanks for the latest patch Jongseok!
> 
> > Source/WebCore/html/FileInputType.cpp:163
> > +            chrome->runOpenPanel(input->document()->frame(), newFileChooser(settings));
> 
> As per Kent Tamura's comment, we should only use runCapturePanel #if ENABLE(MEDIA_CAPTURE). Each platform can implement runCapturePanel as it wants to, which may then call runOpenPanel if type == filesystem for example. That's up to the embedder.
> 
> > Source/WebCore/platform/FileChooser.cpp:70
> > +        return NonCapture;
> 
> return FileSystem here as that is the default according to the spec?
> 
> > Source/WebCore/platform/FileChooser.cpp:76
> > +            if ((capture.isEmpty() || equalIgnoringCase(capture, MediaCaptureNames::camera()))
> 
> Why if capture.isEmpty()? This means that if you specify accept="image/*" then we'll always default to the camera? I can't see where the spec says to do that.
> 
> > Source/WebCore/platform/FileChooser.cpp:77
> > +                && (*iterator).startsWith("image/"))
> 
> I think this might be a bit easier to read if you checked the mime type first, i.e.
> if (*iterator.startsWith("image/*") && equalsIgnoringCase(capture, MediaCaptureNames::camera()) ...
> 
> What do you think?
> 
> > Source/WebCore/platform/FileChooser.cpp:86
> > +            if (type == NonCapture)
> 
> initialise type to FileSystem on line 72 and no need for this?
> 
> > Source/WebCore/platform/FileChooser.h:46
> > +    NonCapture,
> 
> I don't think we need this one.

[Jongseok Yang]

Created attachment 129021 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 129021 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129021&action=review

Just some minor comment while skimming the patch

> Source/WebCore/platform/FileChooser.cpp:59
> +MediaCaptureChooser* FileChooserClient::newMediaCaptureChooser(const FileChooserSettings& settings, MediaCaptureType device)

It almost looks like the name of the class doesn't fit anymore? ContentChooserClient?

> Source/WebCore/platform/FileChooser.cpp:72
> +    for (Vector<String>::const_iterator iterator = acceptMIMETypes.begin(); iterator != acceptMIMETypes.end(); ++iterator)

We normally call the iterator for "it" and put the end outside the look.

> Source/WebCore/platform/FileChooser.h:71
>      FileChooser* newFileChooser(const FileChooserSettings&);
>  
> +#if ENABLE(HTML_MEDIA_CAPTURE)
> +    MediaCaptureChooser* newMediaCaptureChooser(const FileChooserSettings&, MediaCaptureType);

Could the newFileChooser and newMediaCaptureChooser be merged? Would it make sense?

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:730
> +void ChromeClientImpl::runCapturePanel(Frame*, PassRefPtr<MediaCaptureChooser>)

im not sure this is always a panel, so maybe runCaptureChooser makes more sense? Though that would mean we would have to rename the runFilePanel as well

[Hajime Morrita]

Comment on attachment 129021 [details]
Patch

I don't want to have ENABLE ifdef anymore if possible.
And this is a typical module-able/supplement-able feature.
Please consider to:
- Adopt the supplment-style IDL
- Introducing Supplementable<T> for Chrome and attach the separate client objectas a supplement.
   Please don't let ChromeClient fat.

[Jongseok Yang]

HTML Media Capture was implemented by Adam Barth.
Please check the bug:
https://bugs.webkit.org/show_bug.cgi?id=85958

*** This bug has been marked as a duplicate of bug 85958 ***