Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
157763002:
[Mac] In +[CustomFrameView load], do not perform any work if the process has a --type flag. (Closed)
|
Created:
6 years, 10 months ago by Robert Sesek Modified:
6 years, 10 months ago Reviewers:
Mark Mentovai CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Mac] In +[CustomFrameView load], do not perform any work if the process has a --type flag.
This flag indicates that it is a non-browser process and so does not need to
use NSWindow. By interacting with the NSThemeFrame class, its +initialize method
runs. This initializer communicates with the WindowServer. Since +load is run
from dyld as a module initializer, this effectively connects to the WindowServer
via static initialization.
BUG=306348
R=mark@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250112
Patch Set 1 #
Total comments: 10
Patch Set 2 : '' #
Total comments: 2
Patch Set 3 : duh #
Total comments: 4
Patch Set 4 : Comments #Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/157763002/diff/1/chrome/browser/ui/cocoa/cust... File chrome/browser/ui/cocoa/custom_frame_view.mm (right): https://codereview.chromium.org/157763002/diff/1/chrome/browser/ui/cocoa/cust... chrome/browser/ui/cocoa/custom_frame_view.mm:10: #import <objc/runtime.h> Why are these in a separate section from the ones above? https://codereview.chromium.org/157763002/diff/1/chrome/browser/ui/cocoa/cust... chrome/browser/ui/cocoa/custom_frame_view.mm:50: // +initialize methods in the renderer may establish Mach IPC with This comment should say the thing about +[NSThemeFrame initialize] being the evil you’re trying to avoid. That bit of wisdom shouldn’t be relegated just to the commit log. (You don’t have to bring over the whole thing about dyld. Those are just mechanics that aren’t really germane to what you’re doing.) https://codereview.chromium.org/157763002/diff/1/chrome/browser/ui/cocoa/cust... chrome/browser/ui/cocoa/custom_frame_view.mm:55: for (int i = 0; i < *_NSGetArgc(); ++i) { Don’t keep calling this and dereferencing it. int argc = *_NSGetArgc() only once outside the loop. https://codereview.chromium.org/157763002/diff/1/chrome/browser/ui/cocoa/cust... chrome/browser/ui/cocoa/custom_frame_view.mm:55: for (int i = 0; i < *_NSGetArgc(); ++i) { argv[0] is the executable name. Command-line argument processing always starts at argv[1]. https://codereview.chromium.org/157763002/diff/1/chrome/browser/ui/cocoa/cust... chrome/browser/ui/cocoa/custom_frame_view.mm:57: if (strstr(arg, "--type=") != NULL) Don’t scan the whole |arg|, --type= is only significant to you if it appears at the beginning of it. Use strncmp.
https://codereview.chromium.org/157763002/diff/1/chrome/browser/ui/cocoa/cust... File chrome/browser/ui/cocoa/custom_frame_view.mm (right): https://codereview.chromium.org/157763002/diff/1/chrome/browser/ui/cocoa/cust... chrome/browser/ui/cocoa/custom_frame_view.mm:10: #import <objc/runtime.h> On 2014/02/07 18:49:39, Mark Mentovai wrote: > Why are these in a separate section from the ones above? Done. https://codereview.chromium.org/157763002/diff/1/chrome/browser/ui/cocoa/cust... chrome/browser/ui/cocoa/custom_frame_view.mm:50: // +initialize methods in the renderer may establish Mach IPC with On 2014/02/07 18:49:39, Mark Mentovai wrote: > This comment should say the thing about +[NSThemeFrame initialize] being the > evil you’re trying to avoid. That bit of wisdom shouldn’t be relegated just to > the commit log. > > (You don’t have to bring over the whole thing about dyld. Those are just > mechanics that aren’t really germane to what you’re doing.) Done. https://codereview.chromium.org/157763002/diff/1/chrome/browser/ui/cocoa/cust... chrome/browser/ui/cocoa/custom_frame_view.mm:55: for (int i = 0; i < *_NSGetArgc(); ++i) { On 2014/02/07 18:49:39, Mark Mentovai wrote: > Don’t keep calling this and dereferencing it. > > int argc = *_NSGetArgc() > > only once outside the loop. Done. https://codereview.chromium.org/157763002/diff/1/chrome/browser/ui/cocoa/cust... chrome/browser/ui/cocoa/custom_frame_view.mm:55: for (int i = 0; i < *_NSGetArgc(); ++i) { On 2014/02/07 18:49:39, Mark Mentovai wrote: > argv[0] is the executable name. Command-line argument processing always starts > at argv[1]. Done. https://codereview.chromium.org/157763002/diff/1/chrome/browser/ui/cocoa/cust... chrome/browser/ui/cocoa/custom_frame_view.mm:57: if (strstr(arg, "--type=") != NULL) On 2014/02/07 18:49:39, Mark Mentovai wrote: > Don’t scan the whole |arg|, --type= is only significant to you if it appears at > the beginning of it. Use strncmp. Done.
https://codereview.chromium.org/157763002/diff/50001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/custom_frame_view.mm (right): https://codereview.chromium.org/157763002/diff/50001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/custom_frame_view.mm:57: if (strcmp(arg, "--type=") >= 0) Gotta use strNcmp.
https://codereview.chromium.org/157763002/diff/50001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/custom_frame_view.mm (right): https://codereview.chromium.org/157763002/diff/50001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/custom_frame_view.mm:57: if (strcmp(arg, "--type=") >= 0) On 2014/02/07 19:06:26, Mark Mentovai wrote: > Gotta use strNcmp. I don't even know what this code does. What stupid git wrote this.
LGTMarco https://codereview.chromium.org/157763002/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/custom_frame_view.mm (right): https://codereview.chromium.org/157763002/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/custom_frame_view.mm:55: const char kType[] = "--type"; Missing = relative to what you had before. Put it back! https://codereview.chromium.org/157763002/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/custom_frame_view.mm:58: if (strncmp(arg, kType, sizeof(kType) - 1) == 0) Tip: strlen(kType) is fine too. The compiler is smart enough to that strlen of a const char[] is also a compile-time constant. I used to use sizeof-minus-one in this case because of its obvious compile-time constness, but I’ve switched to using strlen everywhere: it’s less error-prone because you don’t need to remember the -1, and if the const char[] turns into some kind of char* (const or otherwise), the strlen will still be correct, where the sizeof will be incorrect in the worst possible way, the silent one.
https://codereview.chromium.org/157763002/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/custom_frame_view.mm (right): https://codereview.chromium.org/157763002/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/custom_frame_view.mm:55: const char kType[] = "--type"; On 2014/02/07 19:58:00, Mark Mentovai wrote: > Missing = relative to what you had before. Put it back! Done. https://codereview.chromium.org/157763002/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/custom_frame_view.mm:58: if (strncmp(arg, kType, sizeof(kType) - 1) == 0) On 2014/02/07 19:58:00, Mark Mentovai wrote: > Tip: strlen(kType) is fine too. The compiler is smart enough to that strlen of a > const char[] is also a compile-time constant. I used to use sizeof-minus-one in > this case because of its obvious compile-time constness, but I’ve switched to > using strlen everywhere: it’s less error-prone because you don’t need to > remember the -1, and if the const char[] turns into some kind of char* (const or > otherwise), the strlen will still be correct, where the sizeof will be incorrect > in the worst possible way, the silent one. Nice tip. Compiler so smart.
lgtMARCUS
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/157763002/150001
The CQ bit was unchecked by commit-bot@chromium.org
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Message was sent while issue was closed.
Committed patchset #4 manually as r250112 (presubmit successful). |
