Integrate MapData into JSMap and JSSet
Created attachment 248382 [details] Patch
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.
Created attachment 248406 [details] Patch
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.
Created attachment 248407 [details] Patch
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.
Created attachment 248422 [details] Patch
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.
Created attachment 248425 [details] Patch
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.
Created attachment 248430 [details] Patch
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.
Created attachment 248532 [details] Patch
Created attachment 248533 [details] Patch
Comment on attachment 248533 [details] Patch Nice!
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.
(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.
(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.
Created attachment 248536 [details] Patch
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.
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.
Ah, sorry, I'll update once more for windows build. Dropping MapData.cpp from vcxproj.
Created attachment 248539 [details] Patch
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.
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.
(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.
(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. ;-)
Committed r181458: <http://trac.webkit.org/changeset/181458>
(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!