Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
423283003:
Fix userVisibility() for files dragged into the renderer. (Closed)
|
Created:
6 years, 4 months ago by pwnall-personal Modified:
6 years, 4 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, dglazkov+blink, kinuko+fileapi, nhiroki, tzik Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionFix userVisibility() for files dragged into the renderer.
While trying to fix drag-and-drop for JS-created files, I broke
drag-and-drop for real files. This change fixes userVisibility() for
real files dragged into the renderer. This change also renames a couple
of the File::create methods, to clarify their usage.
BUG=398366
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179285
Patch Set 1 #Patch Set 2 : Added test. #
Total comments: 16
Patch Set 3 : Addressed feedback. #
Total comments: 12
Patch Set 4 : Addressed 2nd round of feedback. #
Messages
Total messages: 23 (0 generated)
Kent-san, can you please take a look? Sorry for breaking this for everyone! I'll look into cleaning up all the File::create methods, so this kind of bug will not happen again. I'll also look into creating a LayoutTest that covers dragging outside files into the renderer.
Please add an automated test (layout test or unit test) Please do not use the word `Chrome' in the CL description. We might be Opera :)
On 2014/07/30 07:47:08, tkent wrote: > Please add an automated test (layout test or unit test) > Please do not use the word `Chrome' in the CL description. We might be Opera :) I've been working on a layout test that covers this situation, but I'm having trouble. https://codereview.chromium.org/428733008/ The test fails when ran manually on a ChromeOS-like browser (linux build of chrome with chromeos=1 in GYP_DEFINES), where the file is dragged from Files.app. However, it passes when ran as a layout test. It also passes when ran as a browser test. I'll keep investigating.
On 2014/07/30 13:31:03, pwnall wrote: > On 2014/07/30 07:47:08, tkent wrote: > > Please add an automated test (layout test or unit test) > > Please do not use the word `Chrome' in the CL description. We might be Opera > :) > > I've been working on a layout test that covers this situation, but I'm having > trouble. > https://codereview.chromium.org/428733008/ > > The test fails when ran manually on a ChromeOS-like browser (linux build of > chrome with chromeos=1 in GYP_DEFINES), where the file is dragged from > Files.app. However, it passes when ran as a layout test. It also passes when ran > as a browser test. > > I'll keep investigating. If it's hard to write a layout test, please make a C++ unit test. It should be something like: - Call DataObject::addFilename() - Confirm the last item of DataObject::m_itemList is FileKind and the File::userVisibility is IsUserVisible.
On 2014/07/30 23:32:09, tkent wrote: > If it's hard to write a layout test, please make a C++ unit test. It should be > something like: > - Call DataObject::addFilename() > - Confirm the last item of DataObject::m_itemList is FileKind and the > File::userVisibility is IsUserVisible. I just finished writing a unit test. It is uploaded in the latest patch. I'm a bit concerned about the amount of stubbing that I had to do. On the other hand, I can't figure out a way to check for userVisibility() in layout tests. As far as I know, it only impacts what folder is used when the file picker is invoked. So, the only alternative would be to add a special function for exposing a File's UserVisibility to testRunner, and I think that's even dirtier than all that stubbing. What do you think?
On 2014/07/30 at 23:38:44, costan wrote: > On 2014/07/30 23:32:09, tkent wrote: > > If it's hard to write a layout test, please make a C++ unit test. It should be > > something like: > > - Call DataObject::addFilename() > > - Confirm the last item of DataObject::m_itemList is FileKind and the > > File::userVisibility is IsUserVisible. > > I just finished writing a unit test. It is uploaded in the latest patch. I'm a bit concerned about the amount of stubbing that I had to do. > > On the other hand, I can't figure out a way to check for userVisibility() in layout tests. As far as I know, it only impacts what folder is used when the file picker is invoked. So, the only alternative would be to add a special function for exposing a File's UserVisibility to testRunner, and I think that's even dirtier than all that stubbing. > > What do you think? How come you can't use eventSender.beginDragWithFiles() in conjunction with a <input type="file">?
> On the other hand, I can't figure out a way to check for userVisibility() in > layout tests. As far as I know, it only impacts what folder is used when the > file picker is invoked. So, the only alternative would be to add a special > function for exposing a File's UserVisibility to testRunner, and I think that's > even dirtier than all that stubbing. I don't think exposing UserVisibility is reasonable. https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... File Source/web/WebDragDataTest.cpp (right): https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. We should not put a test not for Source/web/ to Source/web. IMO this should be Source/core/clipboard/DataObjectTest.cpp. https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:19: namespace { We should use |namespace blink| for a test for blink namespace. (We have a lot of wrong examples.) https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:114: WebBlobRegistry mockBlobRegistry; mockBlobRegistry -> m_mockBlobRegistry https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:115: MockMimeRegistry mockMimeRegistry; mockMimeRegistry -> m_mockMimeRegistry https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:116: MockFileUtilities mockFileUtilities; mockFileUtilities -> m_mockFileUtilities https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:122: WebDragDataTest() : m_dragData() { } |m_dragData()| is unnecessary. https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:141: OwnPtrWillBeRawPtr<WebDragData> m_dragData; OwnPtrWillBeRawPtr -> OwnPtr https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:142: OwnPtrWillBeRawPtr<RegistryMockPlatform> m_mockPlatform; OwnPtrWillBeRawPtr -> OwnPtr
On 2014/07/30 23:48:08, dcheng (OOO) wrote: > How come you can't use eventSender.beginDragWithFiles() in conjunction with a > <input type="file">? I tried this in a layout test: https://codereview.chromium.org/428733008/ In there, I'm dragging a file onto a <div> and assigning the FileList from the DataTransfer to an <input type="file">, to make the test robust against special drag-and-drop handling inside HTMLInputElement. Then I simulate going to another page and hitting the Back button, which should run through the form serialization/deserialization code in DocumentState. That test passes on all the platforms, and passes on Mac when ran manually. It does fail when ran manually on a Linux build with chromeos=1. Also, as far as I can see, this CL doesn't make the issue go away, so there's something left. I'm guessing it has to do with File serialization, and I'm looking into that now.
Thank you very much for your quick feedback, Kent-san! I'll clean up this patch and keep looking into the root cause for the Files.app breakage. On 2014/07/30 23:56:02, tkent wrote: > https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... > File Source/web/WebDragDataTest.cpp (right): > > https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... > Source/web/WebDragDataTest.cpp:1: // Copyright 2014 The Chromium Authors. All > rights reserved. > We should not put a test not for Source/web/ to Source/web. > IMO this should be Source/core/clipboard/DataObjectTest.cpp. FWIW, this is what led me to put the test there: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
I hope that I have addressed all the feedback. Can you please take another look? https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... File Source/web/WebDragDataTest.cpp (right): https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/07/30 23:56:01, tkent wrote: > We should not put a test not for Source/web/ to Source/web. > IMO this should be Source/core/clipboard/DataObjectTest.cpp. Done. https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:19: namespace { On 2014/07/30 23:56:02, tkent wrote: > We should use |namespace blink| for a test for blink namespace. > (We have a lot of wrong examples.) Done. Thank you for teaching me! https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:114: WebBlobRegistry mockBlobRegistry; On 2014/07/30 23:56:01, tkent wrote: > mockBlobRegistry -> m_mockBlobRegistry Done! Thank you, and sorry! https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:115: MockMimeRegistry mockMimeRegistry; On 2014/07/30 23:56:02, tkent wrote: > mockMimeRegistry -> m_mockMimeRegistry Done. https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:116: MockFileUtilities mockFileUtilities; On 2014/07/30 23:56:01, tkent wrote: > mockFileUtilities -> m_mockFileUtilities Done. https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:122: WebDragDataTest() : m_dragData() { } On 2014/07/30 23:56:01, tkent wrote: > |m_dragData()| is unnecessary. Done. https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:141: OwnPtrWillBeRawPtr<WebDragData> m_dragData; On 2014/07/30 23:56:01, tkent wrote: > OwnPtrWillBeRawPtr -> OwnPtr Done. Thank you! https://codereview.chromium.org/423283003/diff/20001/Source/web/WebDragDataTe... Source/web/WebDragDataTest.cpp:142: OwnPtrWillBeRawPtr<RegistryMockPlatform> m_mockPlatform; On 2014/07/30 23:56:01, tkent wrote: > OwnPtrWillBeRawPtr -> OwnPtr Done.
> FWIW, this is what led me to put the test there: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... I added this FIXME comment. It doesn't mean all of tests should be in Source/web/. https://codereview.chromium.org/423283003/diff/40001/Source/core/clipboard/Da... File Source/core/clipboard/DataObjectTest.cpp (right): https://codereview.chromium.org/423283003/diff/40001/Source/core/clipboard/Da... Source/core/clipboard/DataObjectTest.cpp:122: virtual void SetUp() add OVERRIDE https://codereview.chromium.org/423283003/diff/40001/Source/core/clipboard/Da... Source/core/clipboard/DataObjectTest.cpp:130: virtual void TearDown() Ditto. https://codereview.chromium.org/423283003/diff/40001/Source/core/clipboard/Da... Source/core/clipboard/DataObjectTest.cpp:138: RefPtr<DataObject> m_dataObject; RefPtr -> RefPtrWillBePersistent https://codereview.chromium.org/423283003/diff/40001/Source/core/clipboard/Da... Source/core/clipboard/DataObjectTest.cpp:145: WTF::String filePath = Platform::current()->unitTestSupport()->webKitRootDir(); WTF:: prefix is unnecessary. https://codereview.chromium.org/423283003/diff/40001/Source/core/clipboard/Da... Source/core/clipboard/DataObjectTest.cpp:146: filePath.append("/Source/web/tests/data/dragdata/file.txt"); Dependency to Source/web/ from Source/core is a layering violation. Let's use "/Source/core/clipboard/DataObjectTest.cpp". https://codereview.chromium.org/423283003/diff/40001/Source/core/clipboard/Da... Source/core/clipboard/DataObjectTest.cpp:148: m_dataObject->addFilename(filePath, WTF::String()); WTF:: prefix is unnecessary.
Thank you for the quick and thorough 2nd round of feedback, Kent-san! Can you please take another look? https://codereview.chromium.org/423283003/diff/40001/Source/core/clipboard/Da... File Source/core/clipboard/DataObjectTest.cpp (right): https://codereview.chromium.org/423283003/diff/40001/Source/core/clipboard/Da... Source/core/clipboard/DataObjectTest.cpp:122: virtual void SetUp() On 2014/07/31 01:41:15, tkent wrote: > add OVERRIDE Done. Thank you! https://codereview.chromium.org/423283003/diff/40001/Source/core/clipboard/Da... Source/core/clipboard/DataObjectTest.cpp:130: virtual void TearDown() On 2014/07/31 01:41:15, tkent wrote: > Ditto. Done. https://codereview.chromium.org/423283003/diff/40001/Source/core/clipboard/Da... Source/core/clipboard/DataObjectTest.cpp:138: RefPtr<DataObject> m_dataObject; On 2014/07/31 01:41:15, tkent wrote: > RefPtr -> RefPtrWillBePersistent Done. Thank you! https://codereview.chromium.org/423283003/diff/40001/Source/core/clipboard/Da... Source/core/clipboard/DataObjectTest.cpp:145: WTF::String filePath = Platform::current()->unitTestSupport()->webKitRootDir(); On 2014/07/31 01:41:14, tkent wrote: > WTF:: prefix is unnecessary. Done. https://codereview.chromium.org/423283003/diff/40001/Source/core/clipboard/Da... Source/core/clipboard/DataObjectTest.cpp:146: filePath.append("/Source/web/tests/data/dragdata/file.txt"); On 2014/07/31 01:41:15, tkent wrote: > Dependency to Source/web/ from Source/core is a layering violation. > Let's use "/Source/core/clipboard/DataObjectTest.cpp". Done. Thank you! https://codereview.chromium.org/423283003/diff/40001/Source/core/clipboard/Da... Source/core/clipboard/DataObjectTest.cpp:148: m_dataObject->addFilename(filePath, WTF::String()); On 2014/07/31 01:41:15, tkent wrote: > WTF:: prefix is unnecessary. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/costan@gmail.com/423283003/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/17805)
Message was sent while issue was closed.
Change committed as 179285
Message was sent while issue was closed.
On 2014/07/31 at 04:10:18, commit-bot wrote: > Change committed as 179285 Not sure how I got dropped from replies on this patch... I realize the ship has sailed, but I'm not enthusiastic about the unit test. Why does so much stuff need to be stubbed out?
Message was sent while issue was closed.
On 2014/07/31 04:48:36, dcheng (OOO) wrote: > On 2014/07/31 at 04:10:18, commit-bot wrote: > > Change committed as 179285 > > Not sure how I got dropped from replies on this patch... > > I realize the ship has sailed, but I'm not enthusiastic about the unit test. Why > does so much stuff need to be stubbed out? I'd be happy to write a better version, if possible! Every stub in there is needed so that File creation doesn't crash. File creation uses createBlobDataForFile() in File.cpp and BlobDataHandle::create() https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Among other things, createBlobDataForFile() calls to MIMETypeRegistry::getMIMETypeForExtension() https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... BlobDataHandle::create() calls to BlobRegistry::registerBlobData() https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... The calls above use blobRegistry() and mimeRegistry() on the current platform. They return 0 (should be nullptr?) by default, so I had to stub them to avoid crashing. While WebBlobRegistry has a reasonable implementation, WebMimeRegistry declares all its methods pure virtual, so I need to stub out every one of them. The only implementations I could find are outside blink. https://code.google.com/p/chromium/codesearch#chromium/src/content/child/simp... https://code.google.com/p/chromium/codesearch#chromium/src/mojo/services/html... Last, the variant of the File constructor that gets created calls fileUtilities() on the current platform, to extract the base name in a path. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... WebFileUtilities has a protected destructor, so I had to subclass it in order to expose the destructor. I agree that it's a lot of stubbing. I don't see a better solution, though. I think that the stubbing I'm doing is typical dependency injection. I'd argue that we should have a blink::Platform subclass that can stubs everything, for use in all unit tests. If other people think this is the right direction, I'd be happy to prepare a CL that pulls out the stubbing code in standalone files, and look into getting that plugged into the test setup. What do you think?
Message was sent while issue was closed.
On 2014/07/31 at 15:46:41, costan wrote: > On 2014/07/31 04:48:36, dcheng (OOO) wrote: > > On 2014/07/31 at 04:10:18, commit-bot wrote: > > > Change committed as 179285 > > > > Not sure how I got dropped from replies on this patch... > > > > I realize the ship has sailed, but I'm not enthusiastic about the unit test. Why > > does so much stuff need to be stubbed out? > > I'd be happy to write a better version, if possible! > > Every stub in there is needed so that File creation doesn't crash. > > File creation uses createBlobDataForFile() in File.cpp and BlobDataHandle::create() > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Among other things, createBlobDataForFile() calls to MIMETypeRegistry::getMIMETypeForExtension() > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > BlobDataHandle::create() calls to BlobRegistry::registerBlobData() > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > The calls above use blobRegistry() and mimeRegistry() on the current platform. They return 0 (should be nullptr?) by default, so I had to stub them to avoid crashing. > > While WebBlobRegistry has a reasonable implementation, WebMimeRegistry declares all its methods pure virtual, so I need to stub out every one of them. The only implementations I could find are outside blink. > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/simp... > https://code.google.com/p/chromium/codesearch#chromium/src/mojo/services/html... > > > Last, the variant of the File constructor that gets created calls fileUtilities() on the current platform, to extract the base name in a path. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > WebFileUtilities has a protected destructor, so I had to subclass it in order to expose the destructor. > > > I agree that it's a lot of stubbing. I don't see a better solution, though. I think that the stubbing I'm doing is typical dependency injection. I'd argue that we should have a blink::Platform subclass that can stubs everything, for use in all unit tests. If other people think this is the right direction, I'd be happy to prepare a CL that pulls out the stubbing code in standalone files, and look into getting that plugged into the test setup. > > What do you think? There's already a fake test platform installed for unit tests (https://code.google.com/p/chromium/codesearch#chromium/src/content/test/test_...). I'm guessing this complexity is mainly due to the fact that the default MIME registry doesn't recognize the .cpp extension. If the test file was a .txt file instead of a .cpp, would that eliminate the need for all these stubs?
Message was sent while issue was closed.
On 2014/07/31 17:23:48, dcheng (OOO) wrote: > There's already a fake test platform installed for unit tests > (https://code.google.com/p/chromium/codesearch#chromium/src/content/test/test_...). > I'm guessing this complexity is mainly due to the fact that the default MIME > registry doesn't recognize the .cpp extension. If the test file was a .txt file > instead of a .cpp, would that eliminate the need for all these stubs? Ah, is that the platform used by the blink unit tests? If so, I think it only needs a mock blob registry. I was tempted to inherit from it, but I thought I can't depend on content/ from blink.
Message was sent while issue was closed.
On 2014/07/31 at 17:41:10, costan wrote: > On 2014/07/31 17:23:48, dcheng (OOO) wrote: > > There's already a fake test platform installed for unit tests > > (https://code.google.com/p/chromium/codesearch#chromium/src/content/test/test_...). > > I'm guessing this complexity is mainly due to the fact that the default MIME > > registry doesn't recognize the .cpp extension. If the test file was a .txt file > > instead of a .cpp, would that eliminate the need for all these stubs? > > Ah, is that the platform used by the blink unit tests? > If so, I think it only needs a mock blob registry. > > I was tempted to inherit from it, but I thought I can't depend on content/ from blink. I would just update content to instantiate a blob registry for tests as well. It will be useful for other tests too.
Message was sent while issue was closed.
On 2014/07/31 17:42:07, dcheng (OOO) wrote: > On 2014/07/31 at 17:41:10, costan wrote: > > On 2014/07/31 17:23:48, dcheng (OOO) wrote: > > > There's already a fake test platform installed for unit tests > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/content/test/test_...). > > > I'm guessing this complexity is mainly due to the fact that the default MIME > > > registry doesn't recognize the .cpp extension. If the test file was a .txt > file > > > instead of a .cpp, would that eliminate the need for all these stubs? > > > > Ah, is that the platform used by the blink unit tests? > > If so, I think it only needs a mock blob registry. > > > > I was tempted to inherit from it, but I thought I can't depend on content/ > from blink. > > I would just update content to instantiate a blob registry for tests as well. It > will be useful for other tests too. For sure. I'll go that route. Thank you!
Message was sent while issue was closed.
On 2014/07/31 17:44:37, pwnall wrote: > On 2014/07/31 17:42:07, dcheng (OOO) wrote: > > On 2014/07/31 at 17:41:10, costan wrote: > > > On 2014/07/31 17:23:48, dcheng (OOO) wrote: > > > > There's already a fake test platform installed for unit tests > > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/content/test/test_...). > > > > I'm guessing this complexity is mainly due to the fact that the default > MIME > > > > registry doesn't recognize the .cpp extension. If the test file was a .txt > > file > > > > instead of a .cpp, would that eliminate the need for all these stubs? > > > > > > Ah, is that the platform used by the blink unit tests? > > > If so, I think it only needs a mock blob registry. > > > > > > I was tempted to inherit from it, but I thought I can't depend on content/ > > from blink. > > > > I would just update content to instantiate a blob registry for tests as well. > It > > will be useful for other tests too. > > For sure. I'll go that route. Thank you! For posterity, this is happening. Chromium-side CL: https://codereview.chromium.org/434833002/ Blink-side CL: https://codereview.chromium.org/434823002/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
