Bug 138325 - Web Inspector: Fix RWIProtocol 64-to-32 bit conversion warnings
Summary: Web Inspector: Fix RWIProtocol 64-to-32 bit conversion warnings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-11-03 13:26 PST by Joseph Pecoraro
Modified: 2014-11-03 15:02 PST (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (2.19 KB, patch)
2014-11-03 13:28 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (29.88 KB, patch)
2014-11-03 13:30 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-11-03 13:26:44 PST
When building the ObjC Generated protocol files with warnings I see:

.../JavaScriptCore.framework/PrivateHeaders/InspectorValues.h:265:38: error: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'unsigned int' [-Werror,-Wshorten-64-to-32]
.../DerivedSources/WebInspector/RWIProtocolBackendDispatchers.mm:397:58: error: implicit conversion loses integer precision: 'NSInteger' (aka 'long') to 'int' [-Werror,-Wshorten-64-to-32]
.../DerivedSources/WebInspector/RWIProtocolEventDispatchers.mm:109:62: error: implicit conversion loses integer precision: 'NSInteger' (aka 'long') to 'int' [-Werror,-Wshorten-64-to-32]
.../DerivedSources/WebInspector/RWIProtocolTypes.mm:70:23: error: implicit conversion loses integer precision: 'NSInteger' (aka 'long') to 'int' [-Werror,-Wshorten-64-to-32]

Instead of NSInteger for "integer" protocol types, we should just use "int" to match InspectorValue/InspectorObject's interface.
Note we can keep NS_ENUM(NSInteger) because those always get converted to/from WTF::Strings on the internal InspectorObject.
Comment 1 Radar WebKit Bug Importer 2014-11-03 13:27:01 PST
<rdar://problem/18857253>
Comment 2 Joseph Pecoraro 2014-11-03 13:28:48 PST
Created attachment 240873 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2014-11-03 13:29:57 PST
Oops, I forgot to add script tests rebaselines.
Comment 4 WebKit Commit Bot 2014-11-03 13:30:31 PST
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)
Comment 5 Joseph Pecoraro 2014-11-03 13:30:57 PST
Created attachment 240874 [details]
[PATCH] Proposed Fix
Comment 6 Timothy Hatcher 2014-11-03 13:45:52 PST
Comment on attachment 240874 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=240874&action=review

> Source/JavaScriptCore/ChangeLog:14
> +        Use int instead of NSInteger for APIs that eventually map to
> +        InspectorObject's setInteger, which takes an int.

Feels like a step backwards. Can we fix InspectorObject's setInteger instead?
Comment 7 Joseph Pecoraro 2014-11-03 14:23:13 PST
(In reply to comment #6)
> Comment on attachment 240874 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240874&action=review
> 
> > Source/JavaScriptCore/ChangeLog:14
> > +        Use int instead of NSInteger for APIs that eventually map to
> > +        InspectorObject's setInteger, which takes an int.
> 
> Feels like a step backwards. Can we fix InspectorObject's setInteger instead?

If we changed setInteger to support NSInteger it likely be size_t.

However, there is a potential real issue here. These "int" values get JSON serialized, and JavaScript Integers can not be a full 64bit integer:

> JavaScript represents numbers using IEEE-754 double-precision (64 bit) format.
> As I understand it this gives you 53 bits precision

The compromise that the JSC JSValue API went with was using an explicit int32_t for creating a JSValue with an integer value. We could move our Internal APIs to that type, but I think just using int for now is fine.
Comment 8 WebKit Commit Bot 2014-11-03 15:02:22 PST
Comment on attachment 240874 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 240874

Committed r175493: <http://trac.webkit.org/changeset/175493>
Comment 9 WebKit Commit Bot 2014-11-03 15:02:26 PST
All reviewed patches have been landed.  Closing bug.