Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2091473003:
Factor parsing "ln=" to SuggestionAnswer (Closed)
|
Created:
4 years, 6 months ago by Kevin Bailey Modified:
4 years, 5 months ago CC:
chromium-reviews, tfarina, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFactor parsing "ln=" to SuggestionAnswer
Multiple parts of the code need to access the "ln="
field, if any, in the second line text fields of a
SuggestionAnswer. Move the parse and access method
to a central place, and fix up the callers.
BUG=
Committed: https://crrev.com/e959e110424ef144189fa2046e046f6a5003d5ee
Cr-Commit-Position: refs/heads/master@{#403169}
Patch Set 1 #
Total comments: 12
Patch Set 2 : SuggestionAnswer::SecondLineSize -> ImageLine::TextNumLines #
Total comments: 2
Patch Set 3 : TextNumLines -> num_text_lines #
Total comments: 2
Patch Set 4 : Reorder lines #
Messages
Total messages: 21 (8 generated)
Description was changed from ========== Factor parsing "ln=" to SuggestionAnswer Multiple parts of the code need to access the "ln=" field, if any, in the second line text fields of a SuggetsionAnswer. Move the parse and access method to a central place, and fix up the callers. BUG= ========== to ========== Factor parsing "ln=" to SuggestionAnswer Multiple parts of the code need to access the "ln=" field, if any, in the second line text fields of a SuggestionAnswer. Move the parse and access method to a central place, and fix up the callers. BUG= ==========
krb@chromium.org changed reviewers: + groby@chromium.org, pkasting@chromium.org
Hi Peter, It was suggested by a couple people that we consolidate the methods from each platform that dig out the "ln=" parameter from a SuggestionAnswer. A method on that structure seemed like the logical place, but it's not quite so simple. The field can theoretically exist in each of multiple text fields, on each of 4 (!) lines. Therefore, having a single "GetNumLines()" method didn't seem appropriate. It is only expected on the first field of the second line. I named this method "SecondLineSize()", to indicate which one it was digging out, but if you can think of a better name, great. I'm including the Mac change too, just to take care of all these platforms at once.
Thanks for tackling the cleanup. I'm slightly confused on where ln actually shows up - see my comments on the test. https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... File components/omnibox/browser/suggestion_answer.cc (right): https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... components/omnibox/browser/suggestion_answer.cc:186: second_line_size_(answer.second_line_size_) {} Since this is just a default copy ctor, let's do the right thing: SuggestionAnswer::SuggestionAnswer(const SuggestionAnswer& answer) = default; https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... File components/omnibox/browser/suggestion_answer.h (right): https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... components/omnibox/browser/suggestion_answer.h:78: bool has_num_lines() const { return has_num_lines_; } Since we discussed this as well: Will you kill this in a separate CL? https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... components/omnibox/browser/suggestion_answer.h:150: int SecondLineSize() const { return second_line_size_; } Why not simply have a LineSize() function on ImageLine? While we only care about it for the first line, it makes more sense as a location. (To me) https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... File components/omnibox/browser/suggestion_answer_unittest.cc (left): https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... components/omnibox/browser/suggestion_answer_unittest.cc:218: EXPECT_TRUE(first_line.text_fields()[0].has_num_lines()); Why did this check move from the first to the second line? Does the protocol spec actually say that ln can show up anywhere, but affects only the second line? (In which case we should fix the spec)
Some quick replies. "ln" is supposed to be in the first text field of the second (image) line. https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... File components/omnibox/browser/suggestion_answer.cc (right): https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... components/omnibox/browser/suggestion_answer.cc:186: second_line_size_(answer.second_line_size_) {} On 2016/06/23 20:49:27, groby wrote: > Since this is just a default copy ctor, let's do the right thing: > > SuggestionAnswer::SuggestionAnswer(const SuggestionAnswer& answer) = default; sgtm https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... File components/omnibox/browser/suggestion_answer.h (right): https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... components/omnibox/browser/suggestion_answer.h:78: bool has_num_lines() const { return has_num_lines_; } On 2016/06/23 20:49:27, groby wrote: > Since we discussed this as well: Will you kill this in a separate CL? I actually like this method, since it closely correlates with Protobuf methods. Also, without it, TextField::ParseTextField() would have to return any "ln" that it found to ImageLine. We could do this; it just sounds hacky to me. https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... components/omnibox/browser/suggestion_answer.h:150: int SecondLineSize() const { return second_line_size_; } On 2016/06/23 20:49:27, groby wrote: > Why not simply have a LineSize() function on ImageLine? While we only care about > it for the first line, it makes more sense as a location. (To me) Yes, I like the idea of moving it to ImageLine, but I'd still like to distinguish it from 'additional text' and 'status text'. Maybe 'TextNumLines()'. https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... File components/omnibox/browser/suggestion_answer_unittest.cc (left): https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... components/omnibox/browser/suggestion_answer_unittest.cc:218: EXPECT_TRUE(first_line.text_fields()[0].has_num_lines()); On 2016/06/23 20:49:27, groby wrote: > Why did this check move from the first to the second line? > > Does the protocol spec actually say that ln can show up anywhere, but affects > only the second line? (In which case we should fix the spec) Since ParseTextField is looking for it, we can handle it in any text field, including 'additional text' and 'status text'. Thus, I had no qualms about testing for it in the first line. But practically, it will only be in the answer of a multiline answer. They only specified it in a reply to jdonnely@. (He asked, how are we supposed to know that an answer is a dictionary definition and that we're supposed to allow it go beyond one line, and they said, it will have a "ln" field.) Here is the example that they give in the document: [ {"il": {"at": {"t": "\u2022 /KHsEMSHIYKLlizIYm/", "tt": 8}, "t": [ {"t": "definition of socialism", "tt": 8}]}}, {"il": {"t": [ {"t": "a political and economic theory ..." "tt": 8, "ln": 3}]}}]}, Moving it to the second line also makes it more clear to a reader where to expect it.
https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... File components/omnibox/browser/suggestion_answer.cc (right): https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... components/omnibox/browser/suggestion_answer.cc:186: second_line_size_(answer.second_line_size_) {} On 2016/06/23 21:49:22, Kevin Bailey wrote: > On 2016/06/23 20:49:27, groby wrote: > > Since this is just a default copy ctor, let's do the right thing: > > > > SuggestionAnswer::SuggestionAnswer(const SuggestionAnswer& answer) = default; > > sgtm Done. https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... File components/omnibox/browser/suggestion_answer.h (right): https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... components/omnibox/browser/suggestion_answer.h:150: int SecondLineSize() const { return second_line_size_; } On 2016/06/23 21:49:22, Kevin Bailey wrote: > On 2016/06/23 20:49:27, groby wrote: > > Why not simply have a LineSize() function on ImageLine? While we only care > about > > it for the first line, it makes more sense as a location. (To me) > > Yes, I like the idea of moving it to ImageLine, but I'd still like to > distinguish it from 'additional text' and 'status text'. Maybe 'TextNumLines()'. Done.
LGTM % nit https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... File components/omnibox/browser/suggestion_answer.h (right): https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... components/omnibox/browser/suggestion_answer.h:78: bool has_num_lines() const { return has_num_lines_; } On 2016/06/23 21:49:23, Kevin Bailey wrote: > On 2016/06/23 20:49:27, groby wrote: > > Since we discussed this as well: Will you kill this in a separate CL? > > I actually like this method, since it closely correlates with Protobuf methods. > Also, without it, TextField::ParseTextField() would have to return any "ln" that > it found to ImageLine. We could do this; it just sounds hacky to me. It is *implicit* that a TextField has a single line unless otherwise specified. And hence, this function provides no practical use. (Or at least I can't find a use of it that wouldn't be equally addressed by just returning "1" for num lines if the parameter is not present) Going further - this class doesn't really need to be a class, it can be a struct. With the data fields public. But, again, separate CL. I don't want to derail this. https://codereview.chromium.org/2091473003/diff/20001/components/omnibox/brow... File components/omnibox/browser/suggestion_answer.h (right): https://codereview.chromium.org/2091473003/diff/20001/components/omnibox/brow... components/omnibox/browser/suggestion_answer.h:111: int TextNumLines() const { return text_num_lines_; } Nit: NumTextLines? Also, since it's a cheap function, probably num_text_lines().
https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... File components/omnibox/browser/suggestion_answer.h (right): https://codereview.chromium.org/2091473003/diff/1/components/omnibox/browser/... components/omnibox/browser/suggestion_answer.h:78: bool has_num_lines() const { return has_num_lines_; } On 2016/06/28 17:20:04, groby wrote: > > It is *implicit* that a TextField has a single line unless otherwise specified. > And hence, this function provides no practical use. (Or at least I can't find a > use of it that wouldn't be equally addressed by just returning "1" for num lines > if the parameter is not present) 'ParseImageLine()' uses the first "ln" field that it finds. (This is my preference, since the AiS spec doesn't promise that it will be in the first text field.) If 'num_lines()' returned '1' whether present or not, 'ParseImageLine()' wouldn't know if it found one. We could have 'ParseImageLine()' only look at the first text field, but I think that gives up compatibility unnecessarily. Alternatively, 'ParseTextField()' could pass it back. Maybe Peter has a preference. https://codereview.chromium.org/2091473003/diff/20001/components/omnibox/brow... File components/omnibox/browser/suggestion_answer.h (right): https://codereview.chromium.org/2091473003/diff/20001/components/omnibox/brow... components/omnibox/browser/suggestion_answer.h:111: int TextNumLines() const { return text_num_lines_; } On 2016/06/28 17:20:04, groby wrote: > Nit: NumTextLines? Also, since it's a cheap function, probably num_text_lines(). Done.
LGTM https://codereview.chromium.org/2091473003/diff/40001/components/omnibox/brow... File components/omnibox/browser/suggestion_answer.h (right): https://codereview.chromium.org/2091473003/diff/40001/components/omnibox/brow... components/omnibox/browser/suggestion_answer.h:111: int num_text_lines() const { return num_text_lines_; } Nit: Make accessor order and member declaration order below match
https://codereview.chromium.org/2091473003/diff/40001/components/omnibox/brow... File components/omnibox/browser/suggestion_answer.h (right): https://codereview.chromium.org/2091473003/diff/40001/components/omnibox/brow... components/omnibox/browser/suggestion_answer.h:111: int num_text_lines() const { return num_text_lines_; } On 2016/06/29 23:08:57, Peter Kasting (slow) wrote: > Nit: Make accessor order and member declaration order below match Made this one match rest of CL.
The CQ bit was checked by krb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by krb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2091473003/#ps60001 (title: "Reorder lines")
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 ========== Factor parsing "ln=" to SuggestionAnswer Multiple parts of the code need to access the "ln=" field, if any, in the second line text fields of a SuggestionAnswer. Move the parse and access method to a central place, and fix up the callers. BUG= ========== to ========== Factor parsing "ln=" to SuggestionAnswer Multiple parts of the code need to access the "ln=" field, if any, in the second line text fields of a SuggestionAnswer. Move the parse and access method to a central place, and fix up the callers. BUG= ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Factor parsing "ln=" to SuggestionAnswer Multiple parts of the code need to access the "ln=" field, if any, in the second line text fields of a SuggestionAnswer. Move the parse and access method to a central place, and fix up the callers. BUG= ========== to ========== Factor parsing "ln=" to SuggestionAnswer Multiple parts of the code need to access the "ln=" field, if any, in the second line text fields of a SuggestionAnswer. Move the parse and access method to a central place, and fix up the callers. BUG= Committed: https://crrev.com/e959e110424ef144189fa2046e046f6a5003d5ee Cr-Commit-Position: refs/heads/master@{#403169} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e959e110424ef144189fa2046e046f6a5003d5ee Cr-Commit-Position: refs/heads/master@{#403169} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
