Summary: | Avoid warning if a process does not have access to com.apple.webinspector | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
Component: | Web Inspector | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, commit-queue, graouts, joepeck, timothy, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Joseph Pecoraro
2014-09-02 18:34:11 PDT
Created attachment 237539 [details]
[PATCH] Proposed Fix
Still don't know how I feel about this but here is the patch.
Attachment 237539 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:69: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 237539 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=237539&action=review > Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:59 > +#if __has_include(<sandbox/private.h>) > +#import <sandbox/private.h> > +#else > +enum sandbox_filter_type { > + SANDBOX_FILTER_GLOBAL_NAME, > +}; > +extern "C" { > +int sandbox_check(pid_t, const char *operation, enum sandbox_filter_type, ...); > +} > +#endif Sandbox SPIs seem like a prime target for the new technique Maciej is suggesting, as we use them in many places. Instead of declaring the SPIs in each .mm file, they would go into SandboxSPI.h. In any case, sandbox_check declaration should not be inside the #else. (In reply to comment #3) > (From update of attachment 237539 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=237539&action=review > > > Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:59 > > +#if __has_include(<sandbox/private.h>) > > +#import <sandbox/private.h> > > +#else > > +enum sandbox_filter_type { > > + SANDBOX_FILTER_GLOBAL_NAME, > > +}; > > +extern "C" { > > +int sandbox_check(pid_t, const char *operation, enum sandbox_filter_type, ...); > > +} > > +#endif > > Sandbox SPIs seem like a prime target for the new technique Maciej is suggesting, as we use them in many places. Instead of declaring the SPIs in each .mm file, they would go into SandboxSPI.h. I suppose I could. This would be the first in JavaScriptCore so I opted for the easier path. > In any case, sandbox_check declaration should not be inside the #else. I don't follow. Why not? > > In any case, sandbox_check declaration should not be inside the #else.
>
> I don't follow. Why not?
This way, we'll get a compile error internally if the function signature changes in the future. That's much better than having to chase a mysterious behavior difference between internal and open source builds.
(In reply to comment #5) > > > In any case, sandbox_check declaration should not be inside the #else. > > > > I don't follow. Why not? > > This way, we'll get a compile error internally if the function signature changes in the future. That's much better than having to chase a mysterious behavior difference between internal and open source builds. Internally we should just get the include, so if the signature changes we would fail immediately. If the file changes, then we'd be out of luck. #if __has_include(<sandbox/private.h>) #import <sandbox/private.h> ... I think we have to put some signature in the else, so that open source developers can compile the file. This pattern is repeated in a couple places (WebKit2). > if the signature changes we would fail immediately
This depends on how the signature changes - it can be source compatible, but not binary compatible. Having an unconditional declaration after the conditional include would catch that.
Created attachment 237587 [details]
[PATCH] For Landing
Comment on attachment 237587 [details] [PATCH] For Landing Clearing flags on attachment: 237587 Committed r173238: <http://trac.webkit.org/changeset/173238> All reviewed patches have been landed. Closing bug. |