Factor parsing "ln=" to SuggestionAnswer

components/omnibox/browser/suggestion_answer.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_OMNIBOX_BROWSER_SUGGESTION_ANSWER_H_
 #define COMPONENTS_OMNIBOX_BROWSER_SUGGESTION_ANSWER_H_
 
 #include <memory>
 #include <string>
 #include <vector>
@@ skipping 57 matching lines @@
     ~TextField();
 
     // Parses |field_json| and populates |text_field| with the contents.  If any
     // of the required elements is missing, returns false and leaves text_field
     // in a partially populated state.
     static bool ParseTextField(const base::DictionaryValue* field_json,
                                TextField* text_field);
 
     const base::string16& text() const { return text_; }
     int type() const { return type_; }
     bool has_num_lines() const { return has_num_lines_; }

[groby-ooo-7-16, 2016/06/23 20:49:27]

Since we discussed this as well: Will you kill this in a separate CL?

[Kevin Bailey, 2016/06/23 21:49:23]

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.

[groby-ooo-7-16, 2016/06/28 17:20:04]

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.

[Kevin Bailey, 2016/06/28 18:46:23]

'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.

     int num_lines() const { return num_lines_; }
 
     bool Equals(const TextField& field) const;
 
    private:
     base::string16 text_;
     int type_;
     bool has_num_lines_;
     int num_lines_;
 
@@ skipping 51 matching lines @@
   // TODO(jdonnelly): Once something like std::optional<T> is available in base/
   // (see discussion at http://goo.gl/zN2GNy) remove this in favor of having
   // SuggestResult and AutocompleteMatch use optional<SuggestionAnswer>.
   static std::unique_ptr<SuggestionAnswer> copy(
       const SuggestionAnswer* source) {
     return base::WrapUnique(source ? new SuggestionAnswer(*source) : nullptr);
   }
 
   const ImageLine& first_line() const { return first_line_; }
   const ImageLine& second_line() const { return second_line_; }
+  int SecondLineSize() const { return second_line_size_; }

[groby-ooo-7-16, 2016/06/23 20:49:27]

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)

[Kevin Bailey, 2016/06/23 21:49:22]

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()'.

[Kevin Bailey, 2016/06/28 15:59:32]

Done.

 
   // Answer type accessors.  Valid types are non-negative and defined at
   // https://goto.google.com/visual_element_configuration.
   int type() const { return type_; }
   void set_type(int type) { type_ = type; }
 
   bool Equals(const SuggestionAnswer& answer) const;
 
   // Retrieves any image URLs appearing in this answer and adds them to |urls|.
   void AddImageURLsTo(URLs* urls) const;
 
  private:
   // Forbid assignment.
   SuggestionAnswer& operator=(const SuggestionAnswer&);
 
   ImageLine first_line_;
   ImageLine second_line_;
   int type_;
+  int second_line_size_;
 
   FRIEND_TEST_ALL_PREFIXES(SuggestionAnswerTest, DifferentValuesAreUnequal);
 };
 
 #endif  // COMPONENTS_OMNIBOX_BROWSER_SUGGESTION_ANSWER_H_