Integrate MapData into JSMap and JSSet

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Integrate MapData into JSMap and JSSet

[Yusuke Suzuki]

Created attachment 248382 [details]
Patch

[Yusuke Suzuki]

Created the prototype to integrate MapData into JSMap / JSSet.
In the current patch, MapData is not splitted into MapData / SetData.

MapData's operations are similar to SetData.
If splitting MapData into SetData and MapData is preferable, one possible solution is template parameterizing MapData such as MapData<ValueType, ValueTraits> and using MapData<Pair<Key, Value>, Traits> in JSMap and MapData<Value, Traits> in JSSet.

[Yusuke Suzuki]

Created attachment 248406 [details]
Patch

[WebKit Commit Bot]

Attachment 248406 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/MapData.h:191:  MapDataImpl::const_iterator::const_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/MapData.h:212:  MapDataImpl::const_iterator::end is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 248407 [details]
Patch

[WebKit Commit Bot]

Attachment 248407 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/MapData.h:191:  MapDataImpl::const_iterator::const_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/MapData.h:212:  MapDataImpl::const_iterator::end is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 248422 [details]
Patch

[WebKit Commit Bot]

Attachment 248422 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/MapData.h:193:  MapDataImpl::const_iterator::const_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/MapData.h:214:  MapDataImpl::const_iterator::end is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 248425 [details]
Patch

[WebKit Commit Bot]

Attachment 248425 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/MapData.h:194:  MapDataImpl::const_iterator::const_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/MapData.h:215:  MapDataImpl::const_iterator::end is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 248430 [details]
Patch

[WebKit Commit Bot]

Attachment 248430 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/MapData.h:194:  MapDataImpl::const_iterator::const_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/MapData.h:215:  MapDataImpl::const_iterator::end is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 248532 [details]
Patch

[Yusuke Suzuki]

Created attachment 248533 [details]
Patch

[Filip Pizlo]

Comment on attachment 248533 [details]
Patch

Nice!

[WebKit Commit Bot]

Attachment 248533 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSMap.h:119:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/MapData.h:193:  MapDataImpl::const_iterator::const_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/MapData.h:214:  MapDataImpl::const_iterator::end is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

(In reply to comment #16)
> Attachment 248533 [details] did not pass style-queue:
> 
> 
> ERROR: Source/JavaScriptCore/runtime/JSMap.h:119:  The parameter name
> "value" adds no information, so it should be removed. 

I think that the style bot is wrong here; you're using the word "value" to distinguish from "key".  So, please ignore this style feedback.

> [readability/parameter_name] [5]
> ERROR: Source/JavaScriptCore/runtime/MapData.h:193: 
> MapDataImpl::const_iterator::const_iterator is incorrectly named. Don't use
> underscores in your identifier names.  [readability/naming/underscores] [4]
> ERROR: Source/JavaScriptCore/runtime/MapData.h:214: 
> MapDataImpl::const_iterator::end is incorrectly named. Don't use underscores
> in your identifier names.  [readability/naming/underscores] [4]

These errors are wrong.  You're right to use const_iterator as the name of a const iterator. ;-)

> Total errors found: 3 in 19 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

[Yusuke Suzuki]

(In reply to comment #17)
> I think that the style bot is wrong here; you're using the word "value" to
> distinguish from "key".  So, please ignore this style feedback.
> 
> > [readability/parameter_name] [5]
> > ERROR: Source/JavaScriptCore/runtime/MapData.h:193: 
> > MapDataImpl::const_iterator::const_iterator is incorrectly named. Don't use
> > underscores in your identifier names.  [readability/naming/underscores] [4]
> > ERROR: Source/JavaScriptCore/runtime/MapData.h:214: 
> > MapDataImpl::const_iterator::end is incorrectly named. Don't use underscores
> > in your identifier names.  [readability/naming/underscores] [4]
> 
> These errors are wrong.  You're right to use const_iterator as the name of a
> const iterator. ;-)
> 
> > Total errors found: 3 in 19 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against
> > check-webkit-style.

Thank you for your review.

Oops, one build failure is raised.
This is because JSMap::get conflicts with JSObject::get.
I needed to rebuild with --clean for MapConstructor.cpp, this is because it doesn't use JSMap::get, so rebuild doesn't occur.
I'll fix it with speciflying explicitly by `map->JSObject::get`. And update the patch with it.

The change will become only this from the reviewed patch.

[Yusuke Suzuki]

Created attachment 248536 [details]
Patch

[WebKit Commit Bot]

Attachment 248536 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSMap.h:119:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/MapData.h:193:  MapDataImpl::const_iterator::const_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/MapData.h:214:  MapDataImpl::const_iterator::end is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 248536 [details]
Patch

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

I've updated the patch with a few build fix.
I've only annotated the changes from the previous reviewed patch.

To land it safely, I've updated the patch again.
Could you take a look?

> Source/JavaScriptCore/runtime/MapConstructor.cpp:63
> +    JSValue adderFunction = map->JSObject::get(exec, exec->propertyNames().set);

Explicitly specifying JSObject::get since JSMap also has JSMap::get which signature is different from JSObject::get.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2555
> +            set->add(m_exec, outValue);

I've missed that changing this from set->set to set->add.
Now I've built whole WebKit and make sure that build passed, run-webkit-tests for structured-cloning passed, run-javascriptcore-tests passed on GTK environment.

[Yusuke Suzuki]

Ah, sorry, I'll update once more for windows build.
Dropping MapData.cpp from vcxproj.

[Yusuke Suzuki]

Created attachment 248539 [details]
Patch

[Filip Pizlo]

This probably doesn't need to be reviewed again even if you have to make small changes to pacify the various builds.  That's how we usually work - it's a given that after an r+, additional changes may be necessary, and if they are small, we don't request additional review.

[WebKit Commit Bot]

Attachment 248539 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSMap.h:119:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/MapData.h:193:  MapDataImpl::const_iterator::const_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/MapData.h:214:  MapDataImpl::const_iterator::end is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

(In reply to comment #24)
> This probably doesn't need to be reviewed again even if you have to make
> small changes to pacify the various builds.  That's how we usually work -
> it's a given that after an r+, additional changes may be necessary, and if
> they are small, we don't request additional review.

Thank you :)
I've checked build on gtk, but maybe, iOS seems failed due to the lack of JS_EXPORT_PRIVATE.
So I'll move this patch from my GTK environment to OSX, eusured that build/test passed and land it.

Does Tools/Scripts/webkit-patch land-safely works fine?
If it works, I'm planning to use it to land it safely.

[Filip Pizlo]

(In reply to comment #26)
> (In reply to comment #24)
> > This probably doesn't need to be reviewed again even if you have to make
> > small changes to pacify the various builds.  That's how we usually work -
> > it's a given that after an r+, additional changes may be necessary, and if
> > they are small, we don't request additional review.
> 
> Thank you :)
> I've checked build on gtk, but maybe, iOS seems failed due to the lack of
> JS_EXPORT_PRIVATE.
> So I'll move this patch from my GTK environment to OSX, eusured that
> build/test passed and land it.
> 
> Does Tools/Scripts/webkit-patch land-safely works fine?
> If it works, I'm planning to use it to land it safely.

I don't know. I just svn commit. ;-)

[Yusuke Suzuki]

Committed r181458: <http://trac.webkit.org/changeset/181458>

[Yusuke Suzuki]

(In reply to comment #27)
> I don't know. I just svn commit. ;-)

I see. I've landed with Tools/Scripts/webkit-patch land. Thank you!