Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2275513002:
Support having javascript return dictionaries. (Closed)
DescriptionSupport having javascript return dictionaries.
BUG=None
Committed: https://crrev.com/047b06f0b384ff843f2c81d453cb81bfaed2f056
Cr-Commit-Position: refs/heads/master@{#413725}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comments. #
Total comments: 6
Messages
Total messages: 18 (9 generated)
Description was changed from ========== Create base::Dictionary from NSDictionary BUG=None ========== to ========== Create base::Dictionary from NSDictionary BUG=None ==========
jif@chromium.org changed reviewers: + marq@chromium.org
Description was changed from ========== Create base::Dictionary from NSDictionary BUG=None ========== to ========== Support having javascript return dictionaries. BUG=None ==========
ptal
LGTM with nits. https://codereview.chromium.org/2275513002/diff/1/ios/web/web_state/ui/web_vi... File ios/web/web_state/ui/web_view_js_utils.mm (right): https://codereview.chromium.org/2275513002/diff/1/ios/web/web_state/ui/web_vi... ios/web/web_state/ui/web_view_js_utils.mm:67: std::unique_ptr<base::DictionaryValue> dictionnary = Spelling: "dictionary". https://codereview.chromium.org/2275513002/diff/1/ios/web/web_state/ui/web_vi... File ios/web/web_state/ui/web_view_js_utils_unittest.mm (right): https://codereview.chromium.org/2275513002/diff/1/ios/web/web_state/ui/web_vi... ios/web/web_state/ui/web_view_js_utils_unittest.mm:106: NSDictionary* dictionnary = Spelling: "dictionary".
Thanks. https://codereview.chromium.org/2275513002/diff/1/ios/web/web_state/ui/web_vi... File ios/web/web_state/ui/web_view_js_utils.mm (right): https://codereview.chromium.org/2275513002/diff/1/ios/web/web_state/ui/web_vi... ios/web/web_state/ui/web_view_js_utils.mm:67: std::unique_ptr<base::DictionaryValue> dictionnary = On 2016/08/23 12:28:26, marq wrote: > Spelling: "dictionary". Done. https://codereview.chromium.org/2275513002/diff/1/ios/web/web_state/ui/web_vi... File ios/web/web_state/ui/web_view_js_utils_unittest.mm (right): https://codereview.chromium.org/2275513002/diff/1/ios/web/web_state/ui/web_vi... ios/web/web_state/ui/web_view_js_utils_unittest.mm:106: NSDictionary* dictionnary = On 2016/08/23 12:28:26, marq wrote: > Spelling: "dictionary". Done.
The CQ bit was checked by jif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org Link to the patchset: https://codereview.chromium.org/2275513002/#ps20001 (title: "Addressed comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Support having javascript return dictionaries. BUG=None ========== to ========== Support having javascript return dictionaries. BUG=None ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Support having javascript return dictionaries. BUG=None ========== to ========== Support having javascript return dictionaries. BUG=None Committed: https://crrev.com/047b06f0b384ff843f2c81d453cb81bfaed2f056 Cr-Commit-Position: refs/heads/master@{#413725} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/047b06f0b384ff843f2c81d453cb81bfaed2f056 Cr-Commit-Position: refs/heads/master@{#413725}
Message was sent while issue was closed.
eugenebut@chromium.org changed reviewers: + eugenebut@chromium.org
Message was sent while issue was closed.
What was the purpose of this CL? The description does not explain anything and there is no bug associated. https://codereview.chromium.org/2275513002/diff/20001/ios/web/web_state/ui/we... File ios/web/web_state/ui/web_view_js_utils.mm (right): https://codereview.chromium.org/2275513002/diff/20001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:71: const std::string& path([key UTF8String]); Please use sys_string_conversions instead of |UTF8String|. https://codereview.chromium.org/2275513002/diff/20001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:73: ValueResultFromWKResult([wk_result objectForKey:key])); This JSON is coming from internet, which means that it can be almost infinitely deep and cause stack overflow.
Message was sent while issue was closed.
jif@google.com changed reviewers: + jif@google.com
Message was sent while issue was closed.
The purpose of this CL was to be able to write (downstream) a helper that computed the location of an HTML element. https://codereview.chromium.org/2275513002/diff/20001/ios/web/web_state/ui/we... File ios/web/web_state/ui/web_view_js_utils.mm (right): https://codereview.chromium.org/2275513002/diff/20001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:71: const std::string& path([key UTF8String]); On 2016/09/09 15:26:21, Eugene But wrote: > Please use sys_string_conversions instead of |UTF8String|. Acknowledged. https://codereview.chromium.org/2275513002/diff/20001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:73: ValueResultFromWKResult([wk_result objectForKey:key])); On 2016/09/09 15:26:21, Eugene But wrote: > This JSON is coming from internet, which means that it can be almost infinitely > deep and cause stack overflow. When does the JSON come from the internet?
Message was sent while issue was closed.
Thanks! So since this is needed only for EG test, we could simply use different API (left suggestion in a bug). https://codereview.chromium.org/2275513002/diff/20001/ios/web/web_state/ui/we... File ios/web/web_state/ui/web_view_js_utils.mm (right): https://codereview.chromium.org/2275513002/diff/20001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:71: const std::string& path([key UTF8String]); On 2016/09/09 17:54:45, jif-google wrote: > On 2016/09/09 15:26:21, Eugene But wrote: > > Please use sys_string_conversions instead of |UTF8String|. > > Acknowledged. [key UTF8String] may return nil, which will crash the app. Using sys_string_conversions is not just nice to have thing, it's a question of correctness. https://codereview.chromium.org/2275513002/diff/20001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:73: ValueResultFromWKResult([wk_result objectForKey:key])); On 2016/09/09 17:54:45, jif-google wrote: > On 2016/09/09 15:26:21, Eugene But wrote: > > This JSON is coming from internet, which means that it can be almost > infinitely > > deep and cause stack overflow. > > When does the JSON come from the internet? wk_result is the result of script evaluation, for example a result of __crWeb function call. Web pages can easily override those calls and provide a dictionary which can overflow the stack. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
