WebIDL to C++ values mapping correction (issue 1129663004 by pranay.kumar@samsung.com)

1 view
Skip to first unread message

pranay...@samsung.com

unread,
May 19, 2015, 5:12:56 AM5/19/15
to har...@chromium.org, blink-...@chromium.org, tz...@chromium.org, nhi...@chromium.org, rt...@chromium.org, dgr...@chromium.org, cmum...@chromium.org, jsbel...@chromium.org, kinuko+...@chromium.org
Reviewers: haraken,

Message:
Hi Haraken, PTAL

-Thanks

Description:
WebIDL defines |long| as being a 32 bit unsigned value. However in
C++, "long"
is a 64-bit value.

There is confusion between Blink code between the *.idl files and *.h.
* using "long" instead of "int"
* using "long long" instead of "long"
* using "unsigned long" instead of "unsigned"
* using "unsigned long long" instead of "unsigned long"

BUG=267360

Please review this at https://codereview.chromium.org/1129663004/

Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Affected files (+11, -11 lines):
M Source/modules/filesystem/FileWriterSync.h
M Source/modules/filesystem/FileWriterSync.cpp
M Source/modules/gamepad/Gamepad.h
M Source/modules/indexeddb/IDBFactory.h
M Source/modules/indexeddb/IDBFactory.cpp
M Source/modules/webaudio/AudioDestinationNode.h
M Source/modules/webaudio/AudioDestinationNode.cpp


Index: Source/modules/filesystem/FileWriterSync.cpp
diff --git a/Source/modules/filesystem/FileWriterSync.cpp
b/Source/modules/filesystem/FileWriterSync.cpp
index
656f144cb22eaf3c6dec8a2357843a12190f8c11..c2e2f7979b9fa568dd80a4b9424e2d1101a9c9c9
100644
--- a/Source/modules/filesystem/FileWriterSync.cpp
+++ b/Source/modules/filesystem/FileWriterSync.cpp
@@ -61,14 +61,14 @@ void FileWriterSync::write(Blob* data, ExceptionState&
exceptionState)
setLength(position());
}

-void FileWriterSync::seek(long long position, ExceptionState&
exceptionState)
+void FileWriterSync::seek(long position, ExceptionState& exceptionState)
{
ASSERT(writer());
ASSERT(m_complete);
seekInternal(position);
}

-void FileWriterSync::truncate(long long offset, ExceptionState&
exceptionState)
+void FileWriterSync::truncate(long offset, ExceptionState& exceptionState)
{
ASSERT(writer());
ASSERT(m_complete);
Index: Source/modules/filesystem/FileWriterSync.h
diff --git a/Source/modules/filesystem/FileWriterSync.h
b/Source/modules/filesystem/FileWriterSync.h
index
82bd91c59fa4264831eb7e19704a230ad5fb8ca7..7a1710f509dc9f626fa4d7447ad19555ab795c04
100644
--- a/Source/modules/filesystem/FileWriterSync.h
+++ b/Source/modules/filesystem/FileWriterSync.h
@@ -63,8 +63,8 @@ public:

// FileWriterBase
void write(Blob*, ExceptionState&);
- void seek(long long position, ExceptionState&);
- void truncate(long long length, ExceptionState&);
+ void seek(long position, ExceptionState&);
+ void truncate(long length, ExceptionState&);

// WebFileWriterClient, via FileWriterBase
virtual void didWrite(long long bytes, bool complete) override;
Index: Source/modules/gamepad/Gamepad.h
diff --git a/Source/modules/gamepad/Gamepad.h
b/Source/modules/gamepad/Gamepad.h
index
b9ac12c8f19f75e5612e7d73bf6ca6d113e6630b..2162a8f9e061aa26d0314422562b48dbaff94d82
100644
--- a/Source/modules/gamepad/Gamepad.h
+++ b/Source/modules/gamepad/Gamepad.h
@@ -55,8 +55,8 @@ public:
bool connected() const { return m_connected; }
void setConnected(bool val) { m_connected = val; }

- unsigned long long timestamp() const { return m_timestamp; }
- void setTimestamp(unsigned long long val) { m_timestamp = val; }
+ unsigned long timestamp() const { return m_timestamp; }
+ void setTimestamp(unsigned long val) { m_timestamp = val; }

const String& mapping() const { return m_mapping; }
void setMapping(const String& val) { m_mapping = val; }
@@ -75,7 +75,7 @@ private:
String m_id;
unsigned m_index;
bool m_connected;
- unsigned long long m_timestamp;
+ unsigned long m_timestamp;
String m_mapping;
DoubleVector m_axes;
GamepadButtonVector m_buttons;
Index: Source/modules/indexeddb/IDBFactory.cpp
diff --git a/Source/modules/indexeddb/IDBFactory.cpp
b/Source/modules/indexeddb/IDBFactory.cpp
index
4bbe913ae25b48147e7a4c78cb44cefdfbc53bea..75417034650471fc2dac682366edd9b0795f7f2c
100644
--- a/Source/modules/indexeddb/IDBFactory.cpp
+++ b/Source/modules/indexeddb/IDBFactory.cpp
@@ -91,7 +91,7 @@ IDBRequest* IDBFactory::getDatabaseNames(ScriptState*
scriptState, ExceptionStat
return request;
}

-IDBOpenDBRequest* IDBFactory::open(ScriptState* scriptState, const String&
name, unsigned long long version, ExceptionState& exceptionState)
+IDBOpenDBRequest* IDBFactory::open(ScriptState* scriptState, const String&
name, unsigned long version, ExceptionState& exceptionState)
{
IDB_TRACE("IDBFactory::open");
if (!version) {
Index: Source/modules/indexeddb/IDBFactory.h
diff --git a/Source/modules/indexeddb/IDBFactory.h
b/Source/modules/indexeddb/IDBFactory.h
index
8b2c0d17c14308e7fe45c94a47162fa97a18700b..1095e775128abfc45242b33601b8ac648fed1236
100644
--- a/Source/modules/indexeddb/IDBFactory.h
+++ b/Source/modules/indexeddb/IDBFactory.h
@@ -51,7 +51,7 @@ public:
IDBRequest* getDatabaseNames(ScriptState*, ExceptionState&);

IDBOpenDBRequest* open(ScriptState*, const String& name,
ExceptionState&);
- IDBOpenDBRequest* open(ScriptState*, const String& name, unsigned long
long version, ExceptionState&);
+ IDBOpenDBRequest* open(ScriptState*, const String& name, unsigned long
version, ExceptionState&);
IDBOpenDBRequest* deleteDatabase(ScriptState*, const String& name,
ExceptionState&);

short cmp(ScriptState*, const ScriptValue& first, const ScriptValue&
second, ExceptionState&);
Index: Source/modules/webaudio/AudioDestinationNode.cpp
diff --git a/Source/modules/webaudio/AudioDestinationNode.cpp
b/Source/modules/webaudio/AudioDestinationNode.cpp
index
538a366a9c16e6424baac2ee29d0c6c9190e4e4e..319b51432f990d483a2326636e08cda4d141dfe8
100644
--- a/Source/modules/webaudio/AudioDestinationNode.cpp
+++ b/Source/modules/webaudio/AudioDestinationNode.cpp
@@ -105,7 +105,7 @@ AudioDestinationHandler&
AudioDestinationNode::audioDestinationHandler() const
return static_cast<AudioDestinationHandler&>(handler());
}

-unsigned long AudioDestinationNode::maxChannelCount() const
+unsigned AudioDestinationNode::maxChannelCount() const
{
return audioDestinationHandler().maxChannelCount();
}
Index: Source/modules/webaudio/AudioDestinationNode.h
diff --git a/Source/modules/webaudio/AudioDestinationNode.h
b/Source/modules/webaudio/AudioDestinationNode.h
index
bba01f5021d2364fab223bf6c45267f0cf820274..7594a74b4c6a854566e6c3813cf855deccd8446b
100644
--- a/Source/modules/webaudio/AudioDestinationNode.h
+++ b/Source/modules/webaudio/AudioDestinationNode.h
@@ -96,7 +96,7 @@ class AudioDestinationNode : public AudioNode {
public:
AudioDestinationHandler& audioDestinationHandler() const;

- unsigned long maxChannelCount() const;
+ unsigned maxChannelCount() const;

protected:
AudioDestinationNode(AudioContext&);


har...@chromium.org

unread,
May 19, 2015, 5:20:03 AM5/19/15
to pranay...@samsung.com, blink-...@chromium.org, tz...@chromium.org, nhi...@chromium.org, rt...@chromium.org, dgr...@chromium.org, cmum...@chromium.org, jsbel...@chromium.org, kinuko+...@chromium.org
I don't think this CL is right. Isn't "unsigned long" 32 bit in a 32 bit
architecture?


https://codereview.chromium.org/1129663004/

pranay...@samsung.com

unread,
May 19, 2015, 5:40:11 AM5/19/15
to har...@chromium.org, blink-...@chromium.org, tz...@chromium.org, nhi...@chromium.org, rt...@chromium.org, dgr...@chromium.org, cmum...@chromium.org, jsbel...@chromium.org, kinuko+...@chromium.org

https://codereview.chromium.org/1129663004/diff/1/Source/modules/webaudio/AudioDestinationNode.cpp
File Source/modules/webaudio/AudioDestinationNode.cpp (left):

https://codereview.chromium.org/1129663004/diff/1/Source/modules/webaudio/AudioDestinationNode.cpp#oldcode108
Source/modules/webaudio/AudioDestinationNode.cpp:108: unsigned long
AudioDestinationNode::maxChannelCount() const
Hm..but AFAIK architecture decides for the lower bounds, implementations
are free to use any representations so no upper bounds should be there
on size. Is your concern referenced to here only or for other changes as
well?

https://codereview.chromium.org/1129663004/

har...@chromium.org

unread,
May 19, 2015, 5:43:07 AM5/19/15
to pranay...@samsung.com, yu...@chromium.org, ba...@chromium.org, blink-...@chromium.org, tz...@chromium.org, nhi...@chromium.org, rt...@chromium.org, dgr...@chromium.org, cmum...@chromium.org, jsbel...@chromium.org, kinuko+...@chromium.org
https://codereview.chromium.org/1129663004/

yu...@chromium.org

unread,
May 19, 2015, 5:47:51 AM5/19/15
to pranay...@samsung.com, har...@chromium.org, ba...@chromium.org, blink-...@chromium.org, tz...@chromium.org, nhi...@chromium.org, rt...@chromium.org, dgr...@chromium.org, cmum...@chromium.org, jsbel...@chromium.org, kinuko+...@chromium.org
NOT LGTM

> However in C++, "long" is a 64-bit value.

This is plain wrong. long is 32-bit if:

- you are on 32-bit architecture; or
- you are on Windows; or
- your compiler is directed to do so.

https://codereview.chromium.org/1129663004/

pranay...@samsung.com

unread,
May 19, 2015, 5:51:45 AM5/19/15
to har...@chromium.org, yu...@chromium.org, ba...@chromium.org, blink-...@chromium.org, tz...@chromium.org, nhi...@chromium.org, rt...@chromium.org, dgr...@chromium.org, cmum...@chromium.org, jsbel...@chromium.org, kinuko+...@chromium.org
Hi Yuta,

Sure..Thanks for clearing out


https://codereview.chromium.org/1129663004/

jsb...@chromium.org

unread,
May 19, 2015, 12:01:07 PM5/19/15
to pranay...@samsung.com, har...@chromium.org, yu...@chromium.org, ba...@chromium.org, blink-...@chromium.org, tz...@chromium.org, nhi...@chromium.org, rt...@chromium.org, dgr...@chromium.org, cmum...@chromium.org, jsbel...@chromium.org, kinuko+...@chromium.org
I would be in favor of changing from "unsigned long" to "uint32_t" (etc) in
the
implementations for IDL.


https://codereview.chromium.org/1129663004/

pranay...@samsung.com

unread,
May 22, 2015, 3:41:26 AM5/22/15
to har...@chromium.org, yu...@chromium.org, ba...@chromium.org, blink-...@chromium.org, tz...@chromium.org, nhi...@chromium.org, rt...@chromium.org, dgr...@chromium.org, cmum...@chromium.org, jsbel...@chromium.org, kinuko+...@chromium.org, ero...@chromium.org
Hi jsbell,
Thanks :) Should the related bug
"http://code.google.com/p/chromium/issues/detail?id=267360" be modified as
per
the review comments or a new bug should be raised for use of uint**_t in the
implementations?

@Eroman: What are your views over the same?

https://codereview.chromium.org/1129663004/

pranay...@samsung.com

unread,
May 22, 2015, 7:37:35 AM5/22/15
to har...@chromium.org, yu...@chromium.org, ba...@chromium.org, blink-...@chromium.org, tz...@chromium.org, nhi...@chromium.org, rt...@chromium.org, dgr...@chromium.org, cmum...@chromium.org, jsbel...@chromium.org, kinuko+...@chromium.org, ero...@chromium.org
On 2015/05/22 07:41:26, Pranay wrote:
> Hi jsbell,
> Thanks :) Should the related bug
> "http://code.google.com/p/chromium/issues/detail?id=267360" be modified
> as per
> the review comments or a new bug should be raised for use of uint**_t in
> the
> implementations?

> @Eroman: What are your views over the same?

Thanks all your reviews, Closing this issue now.


https://codereview.chromium.org/1129663004/
Reply all
Reply to author
Forward
0 new messages