Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
637873006:
Add a flag to show if the FileChooserCompletion needs local paths or not. (Closed)
DescriptionAdd a flag to show if the FileChooserCompletion needs local paths or not.
BUG=126902
TEST=None
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183798
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fixed. #
Total comments: 7
Patch Set 3 : Add default value. #
Messages
Total messages: 14 (2 generated)
hirono@chromium.org changed reviewers: + tkent@chromium.org
PTAL the CL? The flag will be needed to let FileChooserCompletion handle non-native files that do not have local paths. Design doc: http://goo.gl/NUuNxG Thank you!
https://codereview.chromium.org/637873006/diff/1/Source/web/ChromeClientImpl.cpp File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/637873006/diff/1/Source/web/ChromeClientImpl.... Source/web/ChromeClientImpl.cpp:617: params.needLocalPath = !fileChooser->settings().allowsDirectoryUpload; I don't understand why directory-upload doesn't need local path. https://codereview.chromium.org/637873006/diff/1/public/web/WebFileChooserPar... File public/web/WebFileChooserParams.h (right): https://codereview.chromium.org/637873006/diff/1/public/web/WebFileChooserPar... public/web/WebFileChooserParams.h:74: // Whether FileChooserCompletion needs local paths or not. The What is FileChooserCompletion? Do you mean WebFileChooserCompletion? https://codereview.chromium.org/637873006/diff/1/public/web/WebFileChooserPar... public/web/WebFileChooserParams.h:76: // local path specifies false to the flag. Does it mean |needLocalPath| is updated outside of ChromeClientImpl::runOpenPanel?
Thank you! https://codereview.chromium.org/637873006/diff/1/Source/web/ChromeClientImpl.cpp File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/637873006/diff/1/Source/web/ChromeClientImpl.... Source/web/ChromeClientImpl.cpp:617: params.needLocalPath = !fileChooser->settings().allowsDirectoryUpload; On 2014/10/15 08:58:12, tkent wrote: > I don't understand why directory-upload doesn't need local path. Yes, this was wrong. When uploading directory, we need local paths. For uploading files, I'm going to add non-native file support that does not need local paths. Thank you for catching it. https://codereview.chromium.org/637873006/diff/1/public/web/WebFileChooserPar... File public/web/WebFileChooserParams.h (right): https://codereview.chromium.org/637873006/diff/1/public/web/WebFileChooserPar... public/web/WebFileChooserParams.h:74: // Whether FileChooserCompletion needs local paths or not. The On 2014/10/15 08:58:12, tkent wrote: > What is FileChooserCompletion? Do you mean WebFileChooserCompletion? Yes. Updated the comment. https://codereview.chromium.org/637873006/diff/1/public/web/WebFileChooserPar... public/web/WebFileChooserParams.h:76: // local path specifies false to the flag. On 2014/10/15 08:58:12, tkent wrote: > Does it mean |needLocalPath| is updated outside of > ChromeClientImpl::runOpenPanel? No, if the flag is set once, it will not be updated. WebFileChooserCompletion has two implementations: WebFileChooserCompletionImpl and PepperFileChooserHost. For WebFileChooserCompletionImpl, we specify the value to the flag in ChromeClientImpl::runOpenPanel. For PepperFileChooserHost, I'm going to specify true to the flag at the outside of blink. https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... Updated the comment.
https://codereview.chromium.org/637873006/diff/1/public/web/WebFileChooserPar... File public/web/WebFileChooserParams.h (right): https://codereview.chromium.org/637873006/diff/1/public/web/WebFileChooserPar... public/web/WebFileChooserParams.h:76: // local path specifies false to the flag. On 2014/10/15 09:26:51, hirono wrote: > On 2014/10/15 08:58:12, tkent wrote: > > Does it mean |needLocalPath| is updated outside of > > ChromeClientImpl::runOpenPanel? > > No, if the flag is set once, it will not be updated. > > WebFileChooserCompletion has two implementations: WebFileChooserCompletionImpl > and PepperFileChooserHost. > > For WebFileChooserCompletionImpl, we specify the value to the flag in > ChromeClientImpl::runOpenPanel. > For PepperFileChooserHost, I'm going to specify true to the flag at the outside > of blink. > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... > > Updated the comment. Thanks. I understand. https://codereview.chromium.org/637873006/diff/20001/Source/web/ChromeClientI... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/637873006/diff/20001/Source/web/ChromeClientI... Source/web/ChromeClientImpl.cpp:617: params.needLocalPath = fileChooser->settings().allowsDirectoryUpload; I think we need a local path even if directory uploading is not allowed. I guess this should be always |true|. https://codereview.chromium.org/637873006/diff/20001/public/web/WebFileChoose... File public/web/WebFileChooserParams.h (right): https://codereview.chromium.org/637873006/diff/20001/public/web/WebFileChoose... public/web/WebFileChooserParams.h:84: , useMediaCapture(false) we should initialize needLocalPath here.
https://codereview.chromium.org/637873006/diff/20001/Source/web/ChromeClientI... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/637873006/diff/20001/Source/web/ChromeClientI... Source/web/ChromeClientImpl.cpp:617: params.needLocalPath = fileChooser->settings().allowsDirectoryUpload; On 2014/10/16 00:39:02, tkent wrote: > I think we need a local path even if directory uploading is not allowed. I > guess this should be always |true|. Currently I'm adding the support to upload files without local paths. Should we set true to the flag now and update at the future CL? https://codereview.chromium.org/637873006/diff/20001/public/web/WebFileChoose... File public/web/WebFileChooserParams.h (right): https://codereview.chromium.org/637873006/diff/20001/public/web/WebFileChoose... public/web/WebFileChooserParams.h:84: , useMediaCapture(false) On 2014/10/16 00:39:02, tkent wrote: > we should initialize needLocalPath here. Done.
https://codereview.chromium.org/637873006/diff/20001/Source/web/ChromeClientI... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/637873006/diff/20001/Source/web/ChromeClientI... Source/web/ChromeClientImpl.cpp:617: params.needLocalPath = fileChooser->settings().allowsDirectoryUpload; On 2014/10/16 06:45:07, hirono wrote: > On 2014/10/16 00:39:02, tkent wrote: > > I think we need a local path even if directory uploading is not allowed. I > > guess this should be always |true|. > > Currently I'm adding the support to upload files without local paths. Should we > set true to the flag now and update at the future CL? Do you implement it for WebFileChooserCompletinoImpl and WebViewClient::runFileChooser()?
https://codereview.chromium.org/637873006/diff/20001/Source/web/ChromeClientI... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/637873006/diff/20001/Source/web/ChromeClientI... Source/web/ChromeClientImpl.cpp:617: params.needLocalPath = fileChooser->settings().allowsDirectoryUpload; On 2014/10/16 07:50:30, tkent wrote: > On 2014/10/16 06:45:07, hirono wrote: > > On 2014/10/16 00:39:02, tkent wrote: > > > I think we need a local path even if directory uploading is not allowed. I > > > guess this should be always |true|. > > > > Currently I'm adding the support to upload files without local paths. Should > we > > set true to the flag now and update at the future CL? > > Do you implement it for WebFileChooserCompletinoImpl and > WebViewClient::runFileChooser()? Yes, this is a draft patch. https://codereview.chromium.org/642813009/
On 2014/10/16 07:52:45, hirono wrote: > https://codereview.chromium.org/637873006/diff/20001/Source/web/ChromeClientI... > File Source/web/ChromeClientImpl.cpp (right): > > https://codereview.chromium.org/637873006/diff/20001/Source/web/ChromeClientI... > Source/web/ChromeClientImpl.cpp:617: params.needLocalPath = > fileChooser->settings().allowsDirectoryUpload; > On 2014/10/16 07:50:30, tkent wrote: > > On 2014/10/16 06:45:07, hirono wrote: > > > On 2014/10/16 00:39:02, tkent wrote: > > > > I think we need a local path even if directory uploading is not allowed. > I > > > > guess this should be always |true|. > > > > > > Currently I'm adding the support to upload files without local paths. Should > > we > > > set true to the flag now and update at the future CL? > > > > Do you implement it for WebFileChooserCompletinoImpl and > > WebViewClient::runFileChooser()? > > Yes, this is a draft patch. > https://codereview.chromium.org/642813009/ Also PTAL the design doc for the entire design. - http://goo.gl/NUuNxG
lgtm. > Also PTAL the design doc for the entire design. - http://goo.gl/NUuNxG No. It consumes reviewer's time too much. The CL description should contain necessary justifications of code changes. https://codereview.chromium.org/637873006/diff/20001/Source/web/ChromeClientI... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/637873006/diff/20001/Source/web/ChromeClientI... Source/web/ChromeClientImpl.cpp:617: params.needLocalPath = fileChooser->settings().allowsDirectoryUpload; On 2014/10/16 07:52:45, hirono wrote: > On 2014/10/16 07:50:30, tkent wrote: > > On 2014/10/16 06:45:07, hirono wrote: > > > On 2014/10/16 00:39:02, tkent wrote: > > > > I think we need a local path even if directory uploading is not allowed. > I > > > > guess this should be always |true|. > > > > > > Currently I'm adding the support to upload files without local paths. Should > > we > > > set true to the flag now and update at the future CL? > > > > Do you implement it for WebFileChooserCompletinoImpl and > > WebViewClient::runFileChooser()? > > Yes, this is a draft patch. > https://codereview.chromium.org/642813009/ ok, I understand.
On 2014/10/16 07:59:52, tkent wrote: > lgtm. > > > Also PTAL the design doc for the entire design. - http://goo.gl/NUuNxG > > No. It consumes reviewer's time too much. The CL description should contain > necessary justifications of code changes. > > https://codereview.chromium.org/637873006/diff/20001/Source/web/ChromeClientI... > File Source/web/ChromeClientImpl.cpp (right): > > https://codereview.chromium.org/637873006/diff/20001/Source/web/ChromeClientI... > Source/web/ChromeClientImpl.cpp:617: params.needLocalPath = > fileChooser->settings().allowsDirectoryUpload; > On 2014/10/16 07:52:45, hirono wrote: > > On 2014/10/16 07:50:30, tkent wrote: > > > On 2014/10/16 06:45:07, hirono wrote: > > > > On 2014/10/16 00:39:02, tkent wrote: > > > > > I think we need a local path even if directory uploading is not allowed. > > > I > > > > > guess this should be always |true|. > > > > > > > > Currently I'm adding the support to upload files without local paths. > Should > > > we > > > > set true to the flag now and update at the future CL? > > > > > > Do you implement it for WebFileChooserCompletinoImpl and > > > WebViewClient::runFileChooser()? > > > > Yes, this is a draft patch. > > https://codereview.chromium.org/642813009/ > > ok, I understand. Thanks!
The CQ bit was checked by hirono@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/637873006/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as 183798 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
