Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2793103002:
Parse JSON in Blink for CopylessPaste. (Closed)
|
Created:
3 years, 8 months ago by dproctor Modified:
3 years, 8 months ago CC:
Aaron Boodman, abarth-chromium, agrieve+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), dglazkov+blink, haraken, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionParse JSON in Blink for CopylessPaste.
BUG=693633
Review-Url: https://codereview.chromium.org/2793103002
Cr-Commit-Position: refs/heads/master@{#463560}
Committed: https://chromium.googlesource.com/chromium/src/+/83c1b269482c2c824cdc9bb38641bec5f94755a8
Patch Set 1 #
Total comments: 18
Patch Set 2 : rebase to 2789313002 #Patch Set 3 : rebase to 2789313002 #Patch Set 4 : address wychen comments #Patch Set 5 : parse object best-effort up to max nesting depth #
Total comments: 28
Patch Set 6 : address wychen comments #
Total comments: 2
Patch Set 7 : remove using from namespace in header #Patch Set 8 : move private member functions to local functions #
Total comments: 2
Patch Set 9 : update to use url in mojo type #
Total comments: 2
Patch Set 10 : add const to JSONArray instances, now that JSONArray::at is const #Patch Set 11 : split AppIndexingReporter change into new cl #
Total comments: 6
Patch Set 12 : more const #
Total comments: 4
Patch Set 13 : address wychen comments #
Total comments: 1
Patch Set 14 : udpate to new mojo namespace #
Total comments: 12
Patch Set 15 : address esprehn comments #
Total comments: 15
Patch Set 16 : update for blink rename #Patch Set 17 : address dcheng comments #
Messages
Total messages: 47 (7 generated)
wychen@chromium.org changed reviewers: + wychen@chromium.org
https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:37: // TODO(dproctor): Do we want to fail parsing, or (more likely) only pass the Best-effort parsing sounds good. https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:52: void extractEntity(JSONObject* val, EntityPtr* entity) { Sorry for the confusion. It might be better to pass Entity& here instead. And const JSONObject& as well. Same for most of the JSON types below. https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:75: property->values->get_bool_values().push_back(v); Why is this different from the rest? https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:118: addProperty = false; Add a comment about mixed types. https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:178: bool isWhitelistedType(String type) { Would AtomicString be a premature optimization? https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:225: bool extractMetadata(const Element& root, Vector<EntityPtr>* entities) { Similarly, Vector<>& might be better. https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:232: LOG(ERROR) << "Failed to parse json."; Needs to be more specific. https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:254: mojom::blink::WebPagePtr* page) { Similarly, using WebPage& here might be better. https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp (right): https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:32: class CopylessPasteExtractorTest : public ::testing::Test { Might make sense to add tests that trigger addProperty = false, like empty array, mixed typed array, and nested array.
https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:52: void extractEntity(JSONObject* val, EntityPtr* entity) { On 2017/04/04 02:07:14, wychen wrote: > Sorry for the confusion. It might be better to pass Entity& here instead. > And const JSONObject& as well. Same for most of the JSON types below. Done. https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:75: property->values->get_bool_values().push_back(v); On 2017/04/04 02:07:14, wychen wrote: > Why is this different from the rest? Done. https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:118: addProperty = false; On 2017/04/04 02:07:14, wychen wrote: > Add a comment about mixed types. Done. https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:178: bool isWhitelistedType(String type) { On 2017/04/04 02:07:15, wychen wrote: > Would AtomicString be a premature optimization? Done. https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:225: bool extractMetadata(const Element& root, Vector<EntityPtr>* entities) { On 2017/04/04 02:07:15, wychen wrote: > Similarly, Vector<>& might be better. Done. https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:232: LOG(ERROR) << "Failed to parse json."; On 2017/04/04 02:07:14, wychen wrote: > Needs to be more specific. What did you have in mind? I don't believe that the webkit json parser exposes any kind of error message. https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:254: mojom::blink::WebPagePtr* page) { On 2017/04/04 02:07:14, wychen wrote: > Similarly, using WebPage& here might be better. Done. https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp (right): https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:32: class CopylessPasteExtractorTest : public ::testing::Test { On 2017/04/04 02:07:15, wychen wrote: > Might make sense to add tests that trigger addProperty = false, like empty > array, mixed typed array, and nested array. Done.
https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:37: // TODO(dproctor): Do we want to fail parsing, or (more likely) only pass the On 2017/04/04 02:07:14, wychen wrote: > Best-effort parsing sounds good. Done.
https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:55: void extractEntity(const JSONObject& val, Entity& entity, int recursionLevel) { This function feels a bit long. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:68: const JSONObject::Entry entry = val.at(i); Would Entry& work? https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:74: JSONValue::ValueType type = entry.second->getType(); inline in the switch to avoid confusion from String type above, and another type below. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:114: case JSONValue::ValueType::TypeArray: { Probably split this part out. The nested switch and for loops make the part below harder to read, especially what "break" would do. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:115: JSONArray* arr = val.getArray(entry.first); const https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:221: void extractEntitiesFromArray(JSONArray& arr, Vector<EntityPtr>& entities) { const arr https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:223: JSONValue* val = arr.at(i); const https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:232: JSONArray* graph = val.getArray(kJSONLDKeyGraph); const https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:243: std::unique_ptr<JSONValue> json = parseJSON(element.textContent()); We could use the maxDepth version. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:284: page.url = document.url().getString(); The mojo type will change from string to GURL. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h (right): https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h:5: #ifndef THIRD_PARTY_WEBKIT_SOURCE_MODULES_DOCUMENT_METADATA_COPYLESSPASTEEXTRACTOR_H_ Looks like CopylessPasteExtractor_h is more consistent in WebKit.
https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:55: void extractEntity(const JSONObject& val, Entity& entity, int recursionLevel) { On 2017/04/04 18:40:19, wychen wrote: > This function feels a bit long. Split out the nested switch statement. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:68: const JSONObject::Entry entry = val.at(i); On 2017/04/04 18:40:19, wychen wrote: > Would Entry& work? Done. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:74: JSONValue::ValueType type = entry.second->getType(); On 2017/04/04 18:40:19, wychen wrote: > inline in the switch to avoid confusion from String type above, and another type > below. Done. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:114: case JSONValue::ValueType::TypeArray: { On 2017/04/04 18:40:19, wychen wrote: > Probably split this part out. The nested switch and for loops make the part > below harder to read, especially what "break" would do. Done. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:115: JSONArray* arr = val.getArray(entry.first); On 2017/04/04 18:40:19, wychen wrote: > const JSONArray::at isn't const. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:221: void extractEntitiesFromArray(JSONArray& arr, Vector<EntityPtr>& entities) { On 2017/04/04 18:40:19, wychen wrote: > const arr JSONArray::at isn't const. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:223: JSONValue* val = arr.at(i); On 2017/04/04 18:40:19, wychen wrote: > const Done. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:232: JSONArray* graph = val.getArray(kJSONLDKeyGraph); On 2017/04/04 18:40:19, wychen wrote: > const Done. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:243: std::unique_ptr<JSONValue> json = parseJSON(element.textContent()); On 2017/04/04 18:40:19, wychen wrote: > We could use the maxDepth version. So, there are actually two different notions of max depth, as discussed in the comment on the constant. 1. Max depth for parsing json. If the json exceeds this depth, the JSONParser library fails, and we won't parse anything. We would want to set this to something other than the default to avoid the cost of parsing really deeply nested objects. 2. Max depth for converting to webpage representation. If the json exceeds this (but is less than 1.) then we will truncate the nested objects, and only report down to the specified level. We almost certainly don't want 1 == 2, and I'm inclined to just leave 1. at the default for now to avoid premature optimization. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:284: page.url = document.url().getString(); On 2017/04/04 18:40:19, wychen wrote: > The mojo type will change from string to GURL. Acknowledged. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h (right): https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h:5: #ifndef THIRD_PARTY_WEBKIT_SOURCE_MODULES_DOCUMENT_METADATA_COPYLESSPASTEEXTRACTOR_H_ On 2017/04/04 18:40:19, wychen wrote: > Looks like CopylessPasteExtractor_h is more consistent in WebKit. Done, though git cl lint gives me a warning that it should be the other way. Deferring to your suggestion.
https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:221: void extractEntitiesFromArray(JSONArray& arr, Vector<EntityPtr>& entities) { On 2017/04/04 20:44:04, dproctor wrote: > On 2017/04/04 18:40:19, wychen wrote: > > const arr > > JSONArray::at isn't const. Hmm. This is unexpected, but OK. Let's see how fast https://codereview.chromium.org/2795393002/ can land. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:243: std::unique_ptr<JSONValue> json = parseJSON(element.textContent()); On 2017/04/04 20:44:04, dproctor wrote: > On 2017/04/04 18:40:19, wychen wrote: > > We could use the maxDepth version. > > So, there are actually two different notions of max depth, as discussed in the > comment on the constant. > > 1. Max depth for parsing json. If the json exceeds this depth, the JSONParser > library fails, and we won't parse anything. We would want to set this to > something other than the default to avoid the cost of parsing really deeply > nested objects. > > 2. Max depth for converting to webpage representation. If the json exceeds this > (but is less than 1.) then we will truncate the nested objects, and only report > down to the specified level. > > We almost certainly don't want 1 == 2, and I'm inclined to just leave 1. at the > default for now to avoid premature optimization. My bad. JSONParser returns nullptr when exceeding max depth. https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h (right): https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h:5: #ifndef THIRD_PARTY_WEBKIT_SOURCE_MODULES_DOCUMENT_METADATA_COPYLESSPASTEEXTRACTOR_H_ On 2017/04/04 20:44:05, dproctor wrote: > On 2017/04/04 18:40:19, wychen wrote: > > Looks like CopylessPasteExtractor_h is more consistent in WebKit. > > Done, though git cl lint gives me a warning that it should be the other way. > Deferring to your suggestion. My guess is that not all git cl lint suggestions apply to Blink code. https://codereview.chromium.org/2793103002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h (right): https://codereview.chromium.org/2793103002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h:31: static bool extractMetadata(const Element&, Vector<EntityPtr>&); Can these be non-member local functions like before?
https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h (right): https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h:5: #ifndef THIRD_PARTY_WEBKIT_SOURCE_MODULES_DOCUMENT_METADATA_COPYLESSPASTEEXTRACTOR_H_ On 2017/04/04 23:15:32, wychen wrote: > On 2017/04/04 20:44:05, dproctor wrote: > > On 2017/04/04 18:40:19, wychen wrote: > > > Looks like CopylessPasteExtractor_h is more consistent in WebKit. > > > > Done, though git cl lint gives me a warning that it should be the other way. > > Deferring to your suggestion. > > My guess is that not all git cl lint suggestions apply to Blink code. Acknowledged, thanks. https://codereview.chromium.org/2793103002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h (right): https://codereview.chromium.org/2793103002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h:31: static bool extractMetadata(const Element&, Vector<EntityPtr>&); On 2017/04/04 23:15:32, wychen wrote: > Can these be non-member local functions like before? Done.
https://codereview.chromium.org/2793103002/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:286: page.url = document.url().getString(); After https://codereview.chromium.org/2789313002/#ps40001, it would be document.url().
https://codereview.chromium.org/2793103002/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:286: page.url = document.url().getString(); On 2017/04/05 00:59:11, wychen wrote: > After https://codereview.chromium.org/2789313002/#ps40001, it would be > document.url(). Done.
https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:221: void extractEntitiesFromArray(JSONArray& arr, Vector<EntityPtr>& entities) { On 2017/04/04 23:15:32, wychen wrote: > On 2017/04/04 20:44:04, dproctor wrote: > > On 2017/04/04 18:40:19, wychen wrote: > > > const arr > > > > JSONArray::at isn't const. > > Hmm. This is unexpected, but OK. > Let's see how fast https://codereview.chromium.org/2795393002/ can land. https://codereview.chromium.org/2795393002/ has landed, so we can use const when applicable. https://codereview.chromium.org/2793103002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/historyreport/AppIndexingReporter.java (right): https://codereview.chromium.org/2793103002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/historyreport/AppIndexingReporter.java:27: public void reportWebPage(WebPage webpage) { Can we split this file to a separate CL? We can do this in 3 steps for easier landing: - Add the new function in upstream. - Override the new function and remove the old function in downstream. - Remove the old function in upstream.
wychen@chromium.org changed reviewers: + esprehn@chromium.org
PTAL
https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/50005/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:221: void extractEntitiesFromArray(JSONArray& arr, Vector<EntityPtr>& entities) { On 2017/04/05 16:29:41, wychen wrote: > On 2017/04/04 23:15:32, wychen wrote: > > On 2017/04/04 20:44:04, dproctor wrote: > > > On 2017/04/04 18:40:19, wychen wrote: > > > > const arr > > > > > > JSONArray::at isn't const. > > > > Hmm. This is unexpected, but OK. > > Let's see how fast https://codereview.chromium.org/2795393002/ can land. > > https://codereview.chromium.org/2795393002/ has landed, so we can use const when > applicable. Done.
https://codereview.chromium.org/2793103002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/historyreport/AppIndexingReporter.java (right): https://codereview.chromium.org/2793103002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/historyreport/AppIndexingReporter.java:27: public void reportWebPage(WebPage webpage) { On 2017/04/05 16:29:41, wychen wrote: > Can we split this file to a separate CL? > We can do this in 3 steps for easier landing: > - Add the new function in upstream. > - Override the new function and remove the old function in downstream. > - Remove the old function in upstream. Done. moved to https://codereview.chromium.org/2800813003
https://codereview.chromium.org/2793103002/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:80: JSONValue* innerVal = arr.at(j); Nit: this can also be const after this CL: https://codereview.chromium.org/2805473002/ https://codereview.chromium.org/2793103002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:228: JSONValue* val = arr.at(i); ditto
https://codereview.chromium.org/2793103002/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:80: JSONValue* innerVal = arr.at(j); On 2017/04/05 18:48:26, wychen wrote: > Nit: this can also be const after this CL: > https://codereview.chromium.org/2805473002/ JSONObject::cast isn't const either
https://codereview.chromium.org/2793103002/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:80: JSONValue* innerVal = arr.at(j); On 2017/04/05 18:51:41, dproctor wrote: > On 2017/04/05 18:48:26, wychen wrote: > > Nit: this can also be const after this CL: > > https://codereview.chromium.org/2805473002/ > > JSONObject::cast isn't const either Yeah, so I fixed it in the CL above.
https://codereview.chromium.org/2793103002/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:80: JSONValue* innerVal = arr.at(j); On 2017/04/05 18:55:33, wychen wrote: > On 2017/04/05 18:51:41, dproctor wrote: > > On 2017/04/05 18:48:26, wychen wrote: > > > Nit: this can also be const after this CL: > > > https://codereview.chromium.org/2805473002/ > > > > JSONObject::cast isn't const either > > Yeah, so I fixed it in the CL above. Done.
https://codereview.chromium.org/2793103002/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:228: JSONValue* val = arr.at(i); On 2017/04/05 18:48:26, wychen wrote: > ditto Done.
https://codereview.chromium.org/2793103002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:278: DCHECK(document.hasFinishedParsing()); Oops. This doesn't hold anymore, since it is no longer "pushed" from Blink. https://codereview.chromium.org/2793103002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h (right): https://codereview.chromium.org/2793103002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h:20: static bool extract(const Document&, mojom::blink::WebPage&); We can use nullptr to denote there's nothing to report. According to https://codereview.chromium.org/2787703002/#ps140001, could you merge https://codereview.chromium.org/2803873002/ here?
https://codereview.chromium.org/2793103002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:278: DCHECK(document.hasFinishedParsing()); On 2017/04/05 23:07:48, wychen wrote: > Oops. This doesn't hold anymore, since it is no longer "pushed" from Blink. Done. https://codereview.chromium.org/2793103002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h (right): https://codereview.chromium.org/2793103002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h:20: static bool extract(const Document&, mojom::blink::WebPage&); On 2017/04/05 23:07:48, wychen wrote: > We can use nullptr to denote there's nothing to report. > According to https://codereview.chromium.org/2787703002/#ps140001, could you > merge https://codereview.chromium.org/2803873002/ here? Done.
A lot of the names you're using in the mojo API are way too generic, youve used Values and Property and WebPage for our entire public API. I'd like to see those names made less generic and into something that's clearly about this feature instead.
On 2017/04/06 18:09:50, esprehn wrote: > A lot of the names you're using in the mojo API are way too generic, youve used > Values and Property and WebPage for our entire public API. > > I'd like to see those names made less generic and into something that's clearly > about this feature instead. Does it help if we put it in a different namespace?
https://codereview.chromium.org/2793103002/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h (right): https://codereview.chromium.org/2793103002/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h:5: #ifndef CopylessPasteExtractor_h Keeping it as-is is correct. Ref: [names-header-guards] in https://www.chromium.org/blink/coding-style "git cl lint" is confused and gives invalid suggestion.
On 2017/04/06 at 18:12:53, wychen wrote: > On 2017/04/06 18:09:50, esprehn wrote: > > A lot of the names you're using in the mojo API are way too generic, youve used > > Values and Property and WebPage for our entire public API. > > > > I'd like to see those names made less generic and into something that's clearly > > about this feature instead. > > Does it help if we put it in a different namespace? Sure.
On 2017/04/06 18:20:51, esprehn wrote: > On 2017/04/06 at 18:12:53, wychen wrote: > > On 2017/04/06 18:09:50, esprehn wrote: > > > A lot of the names you're using in the mojo API are way too generic, youve > used > > > Values and Property and WebPage for our entire public API. > > > > > > I'd like to see those names made less generic and into something that's > clearly > > > about this feature instead. > > > > Does it help if we put it in a different namespace? > > Sure. Wei-Yin, do you want to update the mojo namespace, and I'll update this (and Java-side) change?
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
drive-by: looking at this and how the mojo was defined... can we take advantage of the mojo Value structs we already have in https://cs.chromium.org/chromium/src/mojo/common/values.mojom? It seems like the data we're passing back is pretty close to JSON, so I'm wondering if we can just use that directly.
On 2017/04/06 18:26:26, dcheng wrote: > drive-by: looking at this and how the mojo was defined... can we take advantage > of the mojo Value structs we already have in > https://cs.chromium.org/chromium/src/mojo/common/values.mojom It seems like the > data we're passing back is pretty close to JSON, so I'm wondering if we can just > use that directly. I don't see any reason why that shouldn't work. wychen?
On 2017/04/06 18:28:17, dproctor wrote: > On 2017/04/06 18:26:26, dcheng wrote: > > drive-by: looking at this and how the mojo was defined... can we take > advantage > > of the mojo Value structs we already have in > > https://cs.chromium.org/chromium/src/mojo/common/values.mojom It seems like > the > > data we're passing back is pretty close to JSON, so I'm wondering if we can > just > > use that directly. > > I don't see any reason why that shouldn't work. wychen? We could. I thought about it, but what I tried to do was to define a data structure that's guaranteed to be valid for app indexing. For example, no mixed types in the array, and no unsupported types.
On 2017/04/06 18:42:26, wychen wrote: > On 2017/04/06 18:28:17, dproctor wrote: > > On 2017/04/06 18:26:26, dcheng wrote: > > > drive-by: looking at this and how the mojo was defined... can we take > > advantage > > > of the mojo Value structs we already have in > > > https://cs.chromium.org/chromium/src/mojo/common/values.mojom It seems like > > the > > > data we're passing back is pretty close to JSON, so I'm wondering if we can > > just > > > use that directly. > > > > I don't see any reason why that shouldn't work. wychen? > > We could. > > I thought about it, but what I tried to do was to define a data structure that's > guaranteed to be valid for app indexing. For example, no mixed types in the > array, and no unsupported types. The CL moving it to a new namespace is here: https://codereview.chromium.org/2803943004/
On 2017/04/06 20:16:18, wychen wrote: > On 2017/04/06 18:42:26, wychen wrote: > > On 2017/04/06 18:28:17, dproctor wrote: > > > On 2017/04/06 18:26:26, dcheng wrote: > > > > drive-by: looking at this and how the mojo was defined... can we take > > > advantage > > > > of the mojo Value structs we already have in > > > > https://cs.chromium.org/chromium/src/mojo/common/values.mojom It seems > like > > > the > > > > data we're passing back is pretty close to JSON, so I'm wondering if we > can > > > just > > > > use that directly. > > > > > > I don't see any reason why that shouldn't work. wychen? > > > > We could. > > > > I thought about it, but what I tried to do was to define a data structure > that's > > guaranteed to be valid for app indexing. For example, no mixed types in the > > array, and no unsupported types. > > The CL moving it to a new namespace is here: > https://codereview.chromium.org/2803943004/ Yep, just as soon as you update the namespace there to document_metadata, I'll update this change.
On 2017/04/06 18:20:51, esprehn wrote: > On 2017/04/06 at 18:12:53, wychen wrote: > > On 2017/04/06 18:09:50, esprehn wrote: > > > A lot of the names you're using in the mojo API are way too generic, youve > used > > > Values and Property and WebPage for our entire public API. > > > > > > I'd like to see those names made less generic and into something that's > clearly > > > about this feature instead. > > > > Does it help if we put it in a different namespace? > > Sure. Alright, mojo types are now in a namespace qualified by document_metadata. Elliott, PTAL.
lgtm w/ a bunch of comments, you should also get a review from dcheng@ https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp (right): https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:43: bool extract(WebPagePtr& page) { Can you return WebPagePtr and let the caller do the is_null check? https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:50: void setURL(const std::string); Can you use WTF::String instead? https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:54: PropertyPtr createStringProperty(const String&, const String&); argument names would be nice here for all these https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:56: PropertyPtr createBooleanProperty(const String&, const bool&); argument names for primitives like bools are very helpful, can you add them? https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:62: WebPagePtr createWebPage(const std::string&, const String&); Can you use WTF::String instead? https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:116: PropertyPtr p = Property::New(); we usually don't abbreviate, so all these variables would be |property| in these functions
On 2017/04/07 01:05:30, esprehn wrote: > lgtm w/ a bunch of comments, you should also get a review from dcheng@ Thanks. Daniel, PTAL. Thanks! > > https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp > (right): > > https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:43: > bool extract(WebPagePtr& page) { > Can you return WebPagePtr and let the caller do the is_null check? > > https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:50: > void setURL(const std::string); > Can you use WTF::String instead? > > https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:54: > PropertyPtr createStringProperty(const String&, const String&); > argument names would be nice here for all these > > https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:56: > PropertyPtr createBooleanProperty(const String&, const bool&); > argument names for primitives like bools are very helpful, can you add them? > > https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:62: > WebPagePtr createWebPage(const std::string&, const String&); > Can you use WTF::String instead? > > https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:116: > PropertyPtr p = Property::New(); > we usually don't abbreviate, so all these variables would be |property| in these > functions
https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp (right): https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:43: bool extract(WebPagePtr& page) { On 2017/04/07 01:05:30, esprehn wrote: > Can you return WebPagePtr and let the caller do the is_null check? Done. https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:50: void setURL(const std::string); On 2017/04/07 01:05:30, esprehn wrote: > Can you use WTF::String instead? Done. https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:54: PropertyPtr createStringProperty(const String&, const String&); On 2017/04/07 01:05:30, esprehn wrote: > argument names would be nice here for all these Done. https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:56: PropertyPtr createBooleanProperty(const String&, const bool&); On 2017/04/07 01:05:30, esprehn wrote: > argument names for primitives like bools are very helpful, can you add them? Done. https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:62: WebPagePtr createWebPage(const std::string&, const String&); On 2017/04/07 01:05:30, esprehn wrote: > Can you use WTF::String instead? Done. https://codereview.chromium.org/2793103002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:116: PropertyPtr p = Property::New(); On 2017/04/07 01:05:30, esprehn wrote: > we usually don't abbreviate, so all these variables would be |property| in these > functions Done.
https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:9: #include <utility> Nit: newline https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:50: constexpr size_t kMaxRepeatedSize = 100; Do we have browser side checks for this as well? Or will AppIndexing error out gracefully when any of these conditions are violated? https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:89: values.set_bool_values(Vector<bool>()); Maybe we should just do this on line 75 instead of having it inside the loop. https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:168: property->values->set_bool_values(Vector<bool>(1, v)); Does initializer list syntax work here? e.g. property->values->set_bool_values({v}); I think that would be more readable. https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:221: extractEntity(val, *(entity.get()), 0); I think this can just be written *entity https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:249: if (!json.get()) { Nit: omit .get() https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp (right): https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:86: property->values->set_string_values(Vector<String>(1, value)); Ditto: consider using initializer list syntax here if it works.
https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:9: #include <utility> On 2017/04/10 22:57:11, dcheng wrote: > Nit: newline Done. https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:50: constexpr size_t kMaxRepeatedSize = 100; On 2017/04/10 22:57:11, dcheng wrote: > Do we have browser side checks for this as well? Or will AppIndexing error out > gracefully when any of these conditions are violated? AppIndexing enforces these conditions as the corresponding Java objects are created. kMaxRepeatedSize, kMaxStringLength are enforced by truncating objects. kMaxDepth, kMaxNumFields are enforced by dropping the object. (e.g. if there's a nested object, the nested object won't be included in the higher level object). https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:89: values.set_bool_values(Vector<bool>()); On 2017/04/10 22:57:11, dcheng wrote: > Maybe we should just do this on line 75 instead of having it inside the loop. Done. https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:168: property->values->set_bool_values(Vector<bool>(1, v)); On 2017/04/10 22:57:11, dcheng wrote: > Does initializer list syntax work here? e.g. > > property->values->set_bool_values({v}); > > I think that would be more readable. Done. https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:221: extractEntity(val, *(entity.get()), 0); On 2017/04/10 22:57:12, dcheng wrote: > I think this can just be written *entity Done. https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:249: if (!json.get()) { On 2017/04/10 22:57:11, dcheng wrote: > Nit: omit .get() Done. https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp (right): https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp:86: property->values->set_string_values(Vector<String>(1, value)); On 2017/04/10 22:57:12, dcheng wrote: > Ditto: consider using initializer list syntax here if it works. Done.
Thanks, Daniel. PTAL.
LGTM https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2793103002/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:50: constexpr size_t kMaxRepeatedSize = 100; On 2017/04/11 00:22:01, dproctor wrote: > On 2017/04/10 22:57:11, dcheng wrote: > > Do we have browser side checks for this as well? Or will AppIndexing error out > > gracefully when any of these conditions are violated? > > AppIndexing enforces these conditions as the corresponding Java objects are > created. > > kMaxRepeatedSize, kMaxStringLength are enforced by truncating objects. > > kMaxDepth, kMaxNumFields are enforced by dropping the object. (e.g. if there's a > nested object, the nested object won't be included in the higher level object). OK, as long as the browser expects and handles this case somehow, that's fine with me.
The CQ bit was checked by dproctor@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2793103002/#ps310001 (title: "address dcheng comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 310001, "attempt_start_ts": 1491887679447550,
"parent_rev": "09853278f973fdfafbfffdd695cff8d7e4cc3aeb", "commit_rev":
"83c1b269482c2c824cdc9bb38641bec5f94755a8"}
Message was sent while issue was closed.
Description was changed from ========== Parse JSON in Blink for CopylessPaste. BUG=693633 ========== to ========== Parse JSON in Blink for CopylessPaste. BUG=693633 Review-Url: https://codereview.chromium.org/2793103002 Cr-Commit-Position: refs/heads/master@{#463560} Committed: https://chromium.googlesource.com/chromium/src/+/83c1b269482c2c824cdc9bb38641... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:310001) as https://chromium.googlesource.com/chromium/src/+/83c1b269482c2c824cdc9bb38641... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
