Re: [3/3] ole32: Use a shortcut in DataCache_Save only if fSameAsLoad set to TRUE.
On Wed, Sep 20, 2017 at 02:54:21PM +0300, Dmitry Timoshkov wrote:
This looks like a typo, and this patch makes saving data in a pretty complex application work.
Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/ole32/datacache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/ole32/datacache.c b/dlls/ole32/datacache.c index 4e684b4189..c3383176f8 100644 --- a/dlls/ole32/datacache.c +++ b/dlls/ole32/datacache.c @@ -1663,7 +1663,7 @@ static HRESULT WINAPI DataCache_Save( }
/* this is a shortcut if nothing changed */ - if (!dirty && !fSameAsLoad && This->presentationStorage) + if (!dirty && fSameAsLoad && This->presentationStorage) { return IStorage_CopyTo(This->presentationStorage, 0, NULL, NULL, pStg); }
No, this isn't a typo. The idea is to copy the loaded storage to a fresh storage that is passed when fSameAsLoad is FALSE. I'm aware that there are issues related to the storage handling in both the cache and the default handler. That's on my list of things to look into, but feel free to investigate further. Huw.
Hi Huw, Huw Davies <huw(a)codeweavers.com> wrote:
No, this isn't a typo. The idea is to copy the loaded storage to a fresh storage that is passed when fSameAsLoad is FALSE.
I'm aware that there are issues related to the storage handling in both the cache and the default handler. That's on my list of things to look into, but feel free to investigate further.
I just sent some tests for data cache behaviour when saving a loaded storage, and according to the tests the cache should not blindly copy the whole original storage (i.e. should not call IStorage_CopyTo) in its Save implementation. Moreover it looks like the cache never saves "Contents" streams, and saves only very first "OlePres" stream it has encountered for special storage classes. I'm attaching a preliminary patch that removes the IStorage_CopyTo optimization and solves some of the new tests. What is more important for me this patch also fixes the application that I have here (it's an OLE control that creates data cache storage with custom content, it uses MFC and according to MFC sources it doesn't expect to see "Contents" streams after OleSave()). What do you think? -- Dmitry.
On Thu, Oct 19, 2017 at 03:52:11PM +0800, Dmitry Timoshkov wrote:
Hi Huw,
Huw Davies <huw(a)codeweavers.com> wrote:
No, this isn't a typo. The idea is to copy the loaded storage to a fresh storage that is passed when fSameAsLoad is FALSE.
I'm aware that there are issues related to the storage handling in both the cache and the default handler. That's on my list of things to look into, but feel free to investigate further.
I just sent some tests for data cache behaviour when saving a loaded storage, and according to the tests the cache should not blindly copy the whole original storage (i.e. should not call IStorage_CopyTo) in its Save implementation. Moreover it looks like the cache never saves "Contents" streams, and saves only very first "OlePres" stream it has encountered for special storage classes. I'm attaching a preliminary patch that removes the IStorage_CopyTo optimization and solves some of the new tests. What is more important for me this patch also fixes the application that I have here (it's an OLE control that creates data cache storage with custom content, it uses MFC and according to MFC sources it doesn't expect to see "Contents" streams after OleSave()).
What do you think?
There are a few things going on in that patch, so it would obviously need splitting, but removing the CopyTo() shortcut is fine. Note also that the cache will save to a CONTENTS stream (and not a presentation stream) if the class id is one of the static picture classes. Huw.
Huw Davies <huw(a)codeweavers.com> wrote:
I just sent some tests for data cache behaviour when saving a loaded storage, and according to the tests the cache should not blindly copy the whole original storage (i.e. should not call IStorage_CopyTo) in its Save implementation. Moreover it looks like the cache never saves "Contents" streams, and saves only very first "OlePres" stream it has encountered for special storage classes. I'm attaching a preliminary patch that removes the IStorage_CopyTo optimization and solves some of the new tests. What is more important for me this patch also fixes the application that I have here (it's an OLE control that creates data cache storage with custom content, it uses MFC and according to MFC sources it doesn't expect to see "Contents" streams after OleSave()).
What do you think?
There are a few things going on in that patch, so it would obviously need splitting, but removing the CopyTo() shortcut is fine.
Thanks.
Note also that the cache will save to a CONTENTS stream (and not a presentation stream) if the class id is one of the static picture classes.
Perhaps I'm missing something obvious, but the tests show that the cache never creates a Contents stream on save. -- Dmitry.
On Thu, Oct 19, 2017 at 05:55:34PM +0800, Dmitry Timoshkov wrote:
Huw Davies <huw(a)codeweavers.com> wrote:
Note also that the cache will save to a CONTENTS stream (and not a presentation stream) if the class id is one of the static picture classes.
Perhaps I'm missing something obvious, but the tests show that the cache never creates a Contents stream on save.
Do the tests currently test saving one of the CLSID_Picture classes? Huw.
Huw Davies <huw(a)codeweavers.com> wrote:
Note also that the cache will save to a CONTENTS stream (and not a presentation stream) if the class id is one of the static picture classes.
Perhaps I'm missing something obvious, but the tests show that the cache never creates a Contents stream on save.
Do the tests currently test saving one of the CLSID_Picture classes?
Yes, three of them are being tested. -- Dmitry.
On Thu, Oct 19, 2017 at 06:15:10PM +0800, Dmitry Timoshkov wrote:
Huw Davies <huw(a)codeweavers.com> wrote:
Note also that the cache will save to a CONTENTS stream (and not a presentation stream) if the class id is one of the static picture classes.
Perhaps I'm missing something obvious, but the tests show that the cache never creates a Contents stream on save.
Do the tests currently test saving one of the CLSID_Picture classes?
Yes, three of them are being tested.
But all of them with some strange mixture of presentation and contents streams. What about one with just a contents stream? Huw.
Huw Davies <huw(a)codeweavers.com> wrote:
Note also that the cache will save to a CONTENTS stream (and not a presentation stream) if the class id is one of the static picture classes.
Perhaps I'm missing something obvious, but the tests show that the cache never creates a Contents stream on save.
Do the tests currently test saving one of the CLSID_Picture classes?
Yes, three of them are being tested.
But all of them with some strange mixture of presentation and contents streams. What about one with just a contents stream?
Good point, I'll add one, the tests are easily extendable. Thanks. -- Dmitry.
participants (2)
-
Dmitry Timoshkov -
Huw Davies