Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2153343002:
Implement support for loading font files from outside system directory (Closed)
|
Created:
4 years, 5 months ago by Ilya Kulshin Modified:
4 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, Will Harris Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement support for loading font files from outside system directory
Implement support for loading font files from outside system
directory. When a font file is installed outside the system font
dir, we will now have the browser give the renderer a handle to
the file that it can use to access the font data.
This change will bypass sandbox restrictions that normally
prevents the renderer from opening fonts outside the windows
fonts directory.
BUG=610466
Committed: https://crrev.com/96ce8347f39ce0eebd10de476afb08c0afb406c9
Cr-Commit-Position: refs/heads/master@{#407658}
Patch Set 1 #Patch Set 2 : Delete temp file #Patch Set 3 : Remove more temp files #Patch Set 4 : Add missing include #Patch Set 5 : Unittests #Patch Set 6 : Refactor cast #
Total comments: 22
Patch Set 7 : Review feedback #Patch Set 8 : Test patchset to run try job #
Total comments: 4
Patch Set 9 : Switch to using IPC::PlatformFileForTransit #Patch Set 10 : Add command line flag to control custom font file loading #
Total comments: 4
Patch Set 11 : Switch command flags to use feature API instead #
Total comments: 3
Patch Set 12 : Add some more comments #
Total comments: 1
Messages
Total messages: 62 (46 generated)
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix reference lifetime Basic implementation in progress BUG= TODO: unittests ========== to ========== Implement support for loading font files from outside system directory. When a font file is installed outside the system font dir, we will now have the browser give the renderer a handle to the file that it can use to access the font data. BUG= ==========
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Description was changed from ========== Implement support for loading font files from outside system directory. When a font file is installed outside the system font dir, we will now have the browser give the renderer a handle to the file that it can use to access the font data. BUG= ========== to ========== Implement support for loading font files from outside system directory. When a font file is installed outside the system font dir, we will now have the browser give the renderer a handle to the file that it can use to access the font data. BUG= ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kulshin@chromium.org changed reviewers: + ananta@chromium.org, nasko@chromium.org
ptal ananta@ - content/browser*, content/child/*, content/test/* nasko@ - content/common/dwrite_font_proxy_messages.h
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2153343002/diff/100001/content/browser/render... File content/browser/renderer_host/dwrite_font_proxy_message_filter_win_unittest.cc (right): https://codereview.chromium.org/2153343002/diff/100001/content/browser/render... content/browser/renderer_host/dwrite_font_proxy_message_filter_win_unittest.cc:232: EXPECT_LT(0u, handles.size()); Perhaps using EXPECT_FALSE(handles.empty()) would make this more readable? https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right): https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:545: DWriteFontCollectionProxy::FontFileReference::FontFileReference( newline https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_win.h (right): https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:85: size_t CreateFontFileReference(base::string16 file_path); Newline between functions. Please use different names for these functions. Additionally const reference for string16. Some comments for these functions would be useful for reference. https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:89: class FontFileReference { Please add some comments for this class. https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:97: explicit FontFileReference(base::string16 file_path); const reference for string16. https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:101: const base::string16* Path() const { newline https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:105: const base::File* File() const { newline
https://codereview.chromium.org/2153343002/diff/100001/content/browser/render... File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc (right): https://codereview.chromium.org/2153343002/diff/100001/content/browser/render... content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:549: base::File::FLAG_EXCLUSIVE_WRITE); Why do we need write access? Shouldn't the renderer be able to only read font data and not write back? https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right): https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:354: font_files_.emplace_back(file_path); This is confusing for me. Why are we stashing string paths into a vector of size_t type? https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:359: font_files_.emplace_back(file_handle); Mixing paths and handles in the same type seems very wrong to me. https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_win.h (right): https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:205: std::vector<size_t> file_references_; Since this is Windows specific code, why not use HANDLE?
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal. I changed my approach, so that instead of storing both paths and handles I now convert everything into handles and just store those. This makes the code simpler, but means that the font code could keep the file handles open for longer. It should only open handles to files that are actually used on the page, so it shouldn't be too bad. https://codereview.chromium.org/2153343002/diff/100001/content/browser/render... File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc (right): https://codereview.chromium.org/2153343002/diff/100001/content/browser/render... content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:549: base::File::FLAG_EXCLUSIVE_WRITE); On 2016/07/20 16:40:03, nasko wrote: > Why do we need write access? Shouldn't the renderer be able to only read font > data and not write back? FLAG_EXCLUSIVE_WRITE doesn't grant write access - it instead prevents the handle from being opened with FILE_SHARE_WRITE access. See https://cs.chromium.org/chromium/src/base/files/file_win.cc?q=FLAG_EXCLUSIVE_.... I updated the comment a bit. https://codereview.chromium.org/2153343002/diff/100001/content/browser/render... File content/browser/renderer_host/dwrite_font_proxy_message_filter_win_unittest.cc (right): https://codereview.chromium.org/2153343002/diff/100001/content/browser/render... content/browser/renderer_host/dwrite_font_proxy_message_filter_win_unittest.cc:232: EXPECT_LT(0u, handles.size()); On 2016/07/20 01:34:59, ananta wrote: > Perhaps using EXPECT_FALSE(handles.empty()) would make this more readable? Done. https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right): https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:354: font_files_.emplace_back(file_path); On 2016/07/20 16:40:03, nasko wrote: > This is confusing for me. Why are we stashing string paths into a vector of > size_t type? font_files_ is of type FontFileReference. emplace_back will call the FontFileReference constructor to construct it in place from a string16. I can see how this is confusing, but in any case this code was removed due to a refactor. https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:359: font_files_.emplace_back(file_handle); On 2016/07/20 16:40:03, nasko wrote: > Mixing paths and handles in the same type seems very wrong to me. After thinking about it a bit, I changed the code to use handles throughout. This will now open a handle after getting the path from the browser process, and then use just the handles in the rest of the code. https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:545: DWriteFontCollectionProxy::FontFileReference::FontFileReference( On 2016/07/20 01:34:59, ananta wrote: > newline Done. https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_win.h (right): https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:85: size_t CreateFontFileReference(base::string16 file_path); On 2016/07/20 01:34:59, ananta wrote: > Newline between functions. Please use different names for these functions. > Additionally const reference for string16. Some comments for these functions > would be useful for reference. Removed this code due to a refactor. https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:89: class FontFileReference { On 2016/07/20 01:34:59, ananta wrote: > Please add some comments for this class. Removed this class due to a refactor. https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:97: explicit FontFileReference(base::string16 file_path); On 2016/07/20 01:34:59, ananta wrote: > const reference for string16. Done. https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:101: const base::string16* Path() const { On 2016/07/20 01:34:59, ananta wrote: > newline Done. https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:105: const base::File* File() const { On 2016/07/20 01:34:59, ananta wrote: > newline Done. https://codereview.chromium.org/2153343002/diff/100001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:205: std::vector<size_t> file_references_; On 2016/07/20 16:40:03, nasko wrote: > Since this is Windows specific code, why not use HANDLE? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
Patchset #8 (id:160001) has been deleted
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
I don't know much about fonts, so can't sign off on how the font-specific changes look. However the IPC change looks fine to me, so I'm happy to stamp the CL once someone with more font knowledge has reviewed it. https://codereview.chromium.org/2153343002/diff/180001/content/browser/render... File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc (right): https://codereview.chromium.org/2153343002/diff/180001/content/browser/render... content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:279: // https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2... Can we shorten the link? https://codereview.chromium.org/2153343002/diff/180001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right): https://codereview.chromium.org/2153343002/diff/180001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:212: OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); Do we allow the renderer process to open font files directly?! I thought the whole idea of the font proxy is to open those files in the browser process and hand off handles to the renderer.
I think this code should be using handle attachment brokering - exposed via IPC::GetPlatformFileForTransit interface. It shouldn't be doing the DuplicateHandle manually, as this is done by the IPC subsystem.
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal. Thanks for the tip about PlatformFileForTransit. https://codereview.chromium.org/2153343002/diff/180001/content/browser/render... File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc (right): https://codereview.chromium.org/2153343002/diff/180001/content/browser/render... content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:279: // https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2... On 2016/07/21 21:45:55, nasko wrote: > Can we shorten the link? I removed this code and the comment after switching to IPC::PlatformFileForTransit. https://codereview.chromium.org/2153343002/diff/180001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right): https://codereview.chromium.org/2153343002/diff/180001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:212: OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); On 2016/07/21 21:45:56, nasko wrote: > Do we allow the renderer process to open font files directly?! I thought the > whole idea of the font proxy is to open those files in the browser process and > hand off handles to the renderer. Since these files are in the system fonts directory, we allow the renderer to open them directly. Files outside the system directory will be blocked by sandbox policy. The real gain from the font proxy is in font enumeration. Before, each renderer would need to read all the font files to discover the installed fonts. Now, the proxy allows the renderer to use the shared/cached system collection to get that info, so the renderer only needs to read just the fonts that are used on the page.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Implement support for loading font files from outside system directory. When a font file is installed outside the system font dir, we will now have the browser give the renderer a handle to the file that it can use to access the font data. BUG= ========== to ========== Implement support for loading font files from outside system directory Implement support for loading font files from outside system directory. When a font file is installed outside the system font dir, we will now have the browser give the renderer a handle to the file that it can use to access the font data. BUG=610466 ==========
Much better! Thanks for converting it. IPC LGTM, though I'd still like someone more familiar with fonts to take a look. https://codereview.chromium.org/2153343002/diff/220001/content/public/common/... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2153343002/diff/220001/content/public/common/... content/public/common/content_switches.cc:122: // Force: forces all fonts to use the custom font file codepath. What is the default behavior when the flag isn't present? https://codereview.chromium.org/2153343002/diff/220001/content/public/common/... content/public/common/content_switches.cc:125: const char kForceDirectWriteCustomFonts[] = "force-directwrite-custom-fonts"; This needs to be put in alphabetical order.
ptal, sorry for the churn. I switched from using command line flags to the Chromium features API. This will allow better control in the field, and allow me to run an experiment on canary to verify stability before this goes live. https://codereview.chromium.org/2153343002/diff/220001/content/public/common/... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2153343002/diff/220001/content/public/common/... content/public/common/content_switches.cc:122: // Force: forces all fonts to use the custom font file codepath. On 2016/07/22 23:51:29, nasko wrote: > What is the default behavior when the flag isn't present? Acknowledged. https://codereview.chromium.org/2153343002/diff/220001/content/public/common/... content/public/common/content_switches.cc:125: const char kForceDirectWriteCustomFonts[] = "force-directwrite-custom-fonts"; On 2016/07/22 23:51:29, nasko wrote: > This needs to be put in alphabetical order. Acknowledged.
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2153343002/diff/240001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right): https://codereview.chromium.org/2153343002/diff/240001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:208: // CreateFontFileStream, and DirectWrite requires the reference keys to How was this working previously. Comparing the old code, it looks like the file_names are not being leaked?
https://codereview.chromium.org/2153343002/diff/240001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right): https://codereview.chromium.org/2153343002/diff/240001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:208: // CreateFontFileStream, and DirectWrite requires the reference keys to On 2016/07/25 22:21:39, ananta wrote: > How was this working previously. Comparing the old code, it looks like the > file_names are not being leaked? First, I should point out that we aren't leaking the *memory* for storing the handles, we're leaking the underlying operating system objects. That set should be bounded in size, since we only load families that Skia asks for, we only load each family once, and all the files are unique within the family (there are some cases when a single file is needed for multiple families - in that case, each family would create a reference to that file). As for how it was working before... As far as I can tell, when CreateStreamFromKey is called DirectWrite internally copies the fontFileReferenceKeySize bytes pointed to by fontFileReferenceKey. Since the data was previously just strings, there is no additional cleanup required and the memory (from our side) could be freed immediately after the CreateStreamFromKey call. With handles, closing the handle would invalidate it, causing future attempts by DirectWrite to create the stream to fail (yes, I've seen calls when sometimes DirectWrite asks the loader for multiple streams per CreateStreamFromKey invocation). We could free the handles in DWriteFontCollectionProxy::Unregister(), but that isn't called until process shutdown and at that point we might as well let the OS handle it, but I can add the code if you think that's worthwhile. (no, the IDWriteFontFileLoader does not have a "FreeKey" API)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % comment request https://codereview.chromium.org/2153343002/diff/240001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right): https://codereview.chromium.org/2153343002/diff/240001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:208: // CreateFontFileStream, and DirectWrite requires the reference keys to On 2016/07/25 22:38:52, Ilya Kulshin wrote: > On 2016/07/25 22:21:39, ananta wrote: > > How was this working previously. Comparing the old code, it looks like the > > file_names are not being leaked? > > First, I should point out that we aren't leaking the *memory* for storing the > handles, we're leaking the underlying operating system objects. That set should > be bounded in size, since we only load families that Skia asks for, we only load > each family once, and all the files are unique within the family (there are some > cases when a single file is needed for multiple families - in that case, each > family would create a reference to that file). > > As for how it was working before... > As far as I can tell, when CreateStreamFromKey is called DirectWrite internally > copies the fontFileReferenceKeySize bytes pointed to by fontFileReferenceKey. > Since the data was previously just strings, there is no additional cleanup > required and the memory (from our side) could be freed immediately after the > CreateStreamFromKey call. With handles, closing the handle would invalidate it, > causing future attempts by DirectWrite to create the stream to fail (yes, I've > seen calls when sometimes DirectWrite asks the loader for multiple streams per > CreateStreamFromKey invocation). We could free the handles in > DWriteFontCollectionProxy::Unregister(), but that isn't called until process > shutdown and at that point we might as well let the OS handle it, but I can add > the code if you think that's worthwhile. > > (no, the IDWriteFontFileLoader does not have a "FreeKey" API) Thanks. Please add some comments in the patchset and here, which indicate that having font handles outside windows\fonts opened by the browser avoids file access denied errors in the renderer as the default policies won't allow these file opens.
Description was changed from ========== Implement support for loading font files from outside system directory Implement support for loading font files from outside system directory. When a font file is installed outside the system font dir, we will now have the browser give the renderer a handle to the file that it can use to access the font data. BUG=610466 ========== to ========== Implement support for loading font files from outside system directory Implement support for loading font files from outside system directory. When a font file is installed outside the system font dir, we will now have the browser give the renderer a handle to the file that it can use to access the font data. This change will bypass sandbox restrictions that normally prevents the renderer from opening fonts outside the windows fonts directory. BUG=610466 ==========
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
The CQ bit was unchecked by kulshin@chromium.org
The CQ bit was checked by kulshin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org, ananta@chromium.org Link to the patchset: https://codereview.chromium.org/2153343002/#ps260001 (title: "Add some more comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement support for loading font files from outside system directory Implement support for loading font files from outside system directory. When a font file is installed outside the system font dir, we will now have the browser give the renderer a handle to the file that it can use to access the font data. This change will bypass sandbox restrictions that normally prevents the renderer from opening fonts outside the windows fonts directory. BUG=610466 ========== to ========== Implement support for loading font files from outside system directory Implement support for loading font files from outside system directory. When a font file is installed outside the system font dir, we will now have the browser give the renderer a handle to the file that it can use to access the font data. This change will bypass sandbox restrictions that normally prevents the renderer from opening fonts outside the windows fonts directory. BUG=610466 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Implement support for loading font files from outside system directory Implement support for loading font files from outside system directory. When a font file is installed outside the system font dir, we will now have the browser give the renderer a handle to the file that it can use to access the font data. This change will bypass sandbox restrictions that normally prevents the renderer from opening fonts outside the windows fonts directory. BUG=610466 ========== to ========== Implement support for loading font files from outside system directory Implement support for loading font files from outside system directory. When a font file is installed outside the system font dir, we will now have the browser give the renderer a handle to the file that it can use to access the font data. This change will bypass sandbox restrictions that normally prevents the renderer from opening fonts outside the windows fonts directory. BUG=610466 Committed: https://crrev.com/96ce8347f39ce0eebd10de476afb08c0afb406c9 Cr-Commit-Position: refs/heads/master@{#407658} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/96ce8347f39ce0eebd10de476afb08c0afb406c9 Cr-Commit-Position: refs/heads/master@{#407658}
Message was sent while issue was closed.
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
Message was sent while issue was closed.
Drive-by nit... https://codereview.chromium.org/2153343002/diff/260001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right): https://codereview.chromium.org/2153343002/diff/260001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:542: reinterpret_cast<const void*>(&files_[current_file_]), This cast isn't actually needed, is it? Pointers decay naturally to void*/const void*. The code certainly compiles without it, and extraneous casts confuse me. It looks like the cast was unnecessary in the previous version as well, although I didn't test that. |
