Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
148893005:
[Mac] Disable Mach IPC between renderers and cfprefsd. (Closed)
|
Created:
6 years, 10 months ago by Robert Sesek Modified:
6 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Mac] Disable Mach IPC between renderers and cfprefsd.
BUG=306348
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253430
Patch Set 1 #Patch Set 2 : Remove regex #
Total comments: 22
Patch Set 3 : '' #
Total comments: 4
Patch Set 4 : Anchor #
Total comments: 1
Messages
Total messages: 19 (0 generated)
I haven't decided if this CL is a joke or not. But it works. I also considered not using a regex but parsing the line manually. For a PoC, the regex was quicker to write. WDYT?
Gross. I wish I could pick on the regex, but this is so much worse than the regex. If we did this, it might be the worst hack we have.
Now with less regex!
https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... File content/renderer/renderer_main_platform_delegate_mac.mm (right): https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:80: base::ScopedCFTypeRef<CFStringRef> description( You also have port_description in this function. This one is runloop_description. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:81: CFCopyDescription(CFRunLoopGetCurrent())); A comment with the full contents of what this string is expected to look like would be helpful. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:85: CFRange start_range; There are a few different ranges in here. This one is the range for a "context = <CFMachPort " substring. So name it context_range, machport_range, cfmachport_context_range, etc. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:95: for (CFIndex j = start_range.location; i < length - j; ++j) { “i is less than length minus j” Do you think that’s readable? It doesn’t help that the names i and j don’t say anything about the quantities they represent. I don’t think this logic is even correct. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:97: ++address_range.length; Don’t you want to do this after breaking on ' '? https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:98: if (c == ' ') If you never found the space, you might have a truncated string. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:103: if (address_range.length < 1) If you passed through the loop above at all, even if you only found a space, address_range.length will always be at least 1. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:113: uint64 address = 0; Marginal preference for #if __LP64__ // this thing #else // same thing as this but with uint32 and HexStringToUInt #endif https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:118: CFMachPortRef mach_port = reinterpret_cast<CFMachPortRef>(address); if (CFGetTypeID(mach_port) != CFMachPortGetTypeID()) continue; https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:123: CFCopyDescription(mach_port)); Contents comment for this one too? https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:125: CFSTR("callout = __CFXNotificationReceiveFromServer"), Anchor this string with a trailing space. Maybe even ", callout = __CFXNotificationReceiveFromServer ("
https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... File content/renderer/renderer_main_platform_delegate_mac.mm (right): https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:80: base::ScopedCFTypeRef<CFStringRef> description( On 2014/02/25 17:08:41, Mark Mentovai wrote: > You also have port_description in this function. This one is > runloop_description. Done. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:81: CFCopyDescription(CFRunLoopGetCurrent())); On 2014/02/25 17:08:41, Mark Mentovai wrote: > A comment with the full contents of what this string is expected to look like > would be helpful. Done. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:85: CFRange start_range; On 2014/02/25 17:08:41, Mark Mentovai wrote: > There are a few different ranges in here. This one is the range for a "context = > <CFMachPort " substring. So name it context_range, machport_range, > cfmachport_context_range, etc. Done. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:95: for (CFIndex j = start_range.location; i < length - j; ++j) { On 2014/02/25 17:08:41, Mark Mentovai wrote: > “i is less than length minus j” > > Do you think that’s readable? It doesn’t help that the names i and j don’t say > anything about the quantities they represent. > > I don’t think this logic is even correct. Wow that was doltish of me. This is the problem with testing over SneakerNet. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:97: ++address_range.length; On 2014/02/25 17:08:41, Mark Mentovai wrote: > Don’t you want to do this after breaking on ' '? Done. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:98: if (c == ' ') On 2014/02/25 17:08:41, Mark Mentovai wrote: > If you never found the space, you might have a truncated string. I don't think this condition is likely at all, and even if it were, the string-to-address conversion would fail. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:103: if (address_range.length < 1) On 2014/02/25 17:08:41, Mark Mentovai wrote: > If you passed through the loop above at all, even if you only found a space, > address_range.length will always be at least 1. Done. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:113: uint64 address = 0; On 2014/02/25 17:08:41, Mark Mentovai wrote: > Marginal preference for > > #if __LP64__ > // this thing > #else > // same thing as this but with uint32 and HexStringToUInt > #endif Done. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:118: CFMachPortRef mach_port = reinterpret_cast<CFMachPortRef>(address); On 2014/02/25 17:08:41, Mark Mentovai wrote: > if (CFGetTypeID(mach_port) != CFMachPortGetTypeID()) > continue; Done. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:123: CFCopyDescription(mach_port)); On 2014/02/25 17:08:41, Mark Mentovai wrote: > Contents comment for this one too? Done. https://codereview.chromium.org/148893005/diff/70001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:125: CFSTR("callout = __CFXNotificationReceiveFromServer"), On 2014/02/25 17:08:41, Mark Mentovai wrote: > Anchor this string with a trailing space. > > Maybe even ", callout = __CFXNotificationReceiveFromServer (" Done.
LGTM https://codereview.chromium.org/148893005/diff/90001/content/renderer/rendere... File content/renderer/renderer_main_platform_delegate_mac.mm (right): https://codereview.chromium.org/148893005/diff/90001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:94: CFSTR("context = <CFMachPort "), CFRangeMake(i, length - i), Leading commaspace on this one too? https://codereview.chromium.org/148893005/diff/90001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:108: ++port_address_range.length; Maybe don’t even bother with this inside the loop. After the loop, set port_address_range.length = j - port_address_range.location.
+avi for OWNERS https://codereview.chromium.org/148893005/diff/90001/content/renderer/rendere... File content/renderer/renderer_main_platform_delegate_mac.mm (right): https://codereview.chromium.org/148893005/diff/90001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:94: CFSTR("context = <CFMachPort "), CFRangeMake(i, length - i), On 2014/02/25 18:07:47, Mark Mentovai wrote: > Leading commaspace on this one too? Done. https://codereview.chromium.org/148893005/diff/90001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_mac.mm:108: ++port_address_range.length; On 2014/02/25 18:07:47, Mark Mentovai wrote: > Maybe don’t even bother with this inside the loop. After the loop, set > port_address_range.length = j - port_address_range.location. Then j would have to be scoped outside of the loop. I prefer this, if you don't care.
I don’t care, I was just trying to do less math because it’s hard and I’d rather go to the mall.
lgtm https://codereview.chromium.org/148893005/diff/110001/content/renderer/render... File content/renderer/renderer_main_platform_delegate_mac.mm (right): https://codereview.chromium.org/148893005/diff/110001/content/renderer/render... content/renderer/renderer_main_platform_delegate_mac.mm:72: // You are about to read a pretty disgusting hack. In a static initializer, Yes, this is disgusting.
The CQ bit was checked by rsesek@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/148893005/110001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/148893005/110001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/148893005/110001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/148893005/110001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/148893005/110001
The CQ bit was unchecked by phajdan.jr@chromium.org
The CQ bit was checked by phajdan.jr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/148893005/110001
Message was sent while issue was closed.
Change committed as 253430 |
