Summary: | The WebContent process should not use NSScreen in the screenDepth implementation. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, commit-queue, thorton, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 182747 | ||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2018-02-22 11:26:57 PST
Created attachment 334461 [details]
Patch
Created attachment 334464 [details]
Patch
Comment on attachment 334464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334464&action=review This patch looks good, but I think it would be better if you got rid of the duplicated code. Can you please revise that? > Source/WebCore/platform/mac/PlatformScreenMac.mm:140 > + return screenProperties().get(iter->key).screenDepth; This snippet of logic is also in 'screenRect' and 'screenAvailableRect', (and perhaps others?). Can you encapsulate it in a common function that returns the appropriate screenProperties, then each of these methods can just return the member we are interested in? Also: Should we ASSERT if the depth is zero, just in case the IPC didn't happen for some reason? Created attachment 334469 [details]
Patch
Created attachment 334470 [details]
Patch
Comment on attachment 334470 [details]
Patch
Looks great! r=me.
Comment on attachment 334470 [details]
Patch
Thanks for reviewing!
Comment on attachment 334470 [details] Patch Clearing flags on attachment: 334470 Committed r228940: <https://trac.webkit.org/changeset/228940> All reviewed patches have been landed. Closing bug. |