Add functions to collect bookmarks from Pdfium

Add functions to collect bookmarks from Pdfium

TraverseBookmarks() collects the bookmarks into a VarDictionary, with the following key/values:
 - title
 - page (if one is available)
 - children, a VarArray of bookmarks (each a VarDictionary with the same structure)

BUG=110020
Committed: https://crrev.com/6a36d4d1cc5e7d31d578c077bbf92a84d02a7578
Cr-Commit-Position: refs/heads/master@{#312059}

[Alexandre Carlton, 2014-12-16 04:17:37]

alexandrec@chromium.org changed reviewers:
+ raymes@chromium.org, sammc@chromium.org

[Alexandre Carlton, 2014-12-16 04:17:38]

Collect bookmarks from Foxit API.

[Sam McNally, 2014-12-16 05:58:35]

https://codereview.chromium.org/810623003/diff/30001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/810623003/diff/30001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:406: console.log('pdf.js: Retrieved
bookmarks.');
Remove these before landing.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdf_engine.h
File pdf/pdf_engine.h (right):

https://codereview.chromium.org/810623003/diff/30001/pdf/pdf_engine.h#newcode251
pdf/pdf_engine.h:251: // Returns a nested Dictionary of Bookmarks
Document the structure of the dictionary.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2312: void TraverseBookmarks(FPDF_BOOKMARK bookmark,
pp::VarDictionary *dict,
Put this in the anonymous namespace above.

Return a pp::VarDictionary instead of taking one to fill. Also, to be
consistent, you should take |doc| before |bookmark|.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2318: FPDFBookmark_GetTitle(bookmark, data,
buffer_size);
I would pass the result of WriteInto(...) directly into FPDFBookmark_GetTitle.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2321: dict->Set(pp::Var("title"),
pp::Var(shortTitle));
Remove shortTitle and pass base::UTF16ToUTF8(title).

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2324: int pageIndex = dest ?
FPDFDest_GetPageIndex(doc, dest) : -1;
How about only setting "page" for bookmarks that have a destination page?

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2328: pp::VarArray *children = new pp::VarArray();
Put this on the stack. Otherwise, it will leak.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2329: children->SetLength(length);
You don't need this. VarArray resizes to fit on calls to Set().

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2332: pp::VarDictionary *childDict;
Declare this inside the loop (or remove it entirely once this returns the
dictionary instead of taking one). See above about leaking too.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2333: bookmark = FPDFBookmark_GetFirstChild(doc,
bookmark);
Use a different variable for the child bookmarks.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2334: while (bookmark != NULL) {
I think this would be nicer as a for loop.

[Alexandre Carlton, 2014-12-16 22:14:51]

https://codereview.chromium.org/810623003/diff/30001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/810623003/diff/30001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:406: console.log('pdf.js: Retrieved
bookmarks.');
On 2014/12/16 05:58:34, Sam McNally wrote:
> Remove these before landing.

Done.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdf_engine.h
File pdf/pdf_engine.h (right):

https://codereview.chromium.org/810623003/diff/30001/pdf/pdf_engine.h#newcode251
pdf/pdf_engine.h:251: // Returns a nested Dictionary of Bookmarks
On 2014/12/16 05:58:34, Sam McNally wrote:
> Document the structure of the dictionary.

Done.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2312: void TraverseBookmarks(FPDF_BOOKMARK bookmark,
pp::VarDictionary *dict,
On 2014/12/16 05:58:34, Sam McNally wrote:
> Put this in the anonymous namespace above.
> 
> Return a pp::VarDictionary instead of taking one to fill. Also, to be
> consistent, you should take |doc| before |bookmark|.

Done.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2318: FPDFBookmark_GetTitle(bookmark, data,
buffer_size);
On 2014/12/16 05:58:34, Sam McNally wrote:
> I would pass the result of WriteInto(...) directly into FPDFBookmark_GetTitle.

Done.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2321: dict->Set(pp::Var("title"),
pp::Var(shortTitle));
On 2014/12/16 05:58:34, Sam McNally wrote:
> Remove shortTitle and pass base::UTF16ToUTF8(title).

Done.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2324: int pageIndex = dest ?
FPDFDest_GetPageIndex(doc, dest) : -1;
On 2014/12/16 05:58:34, Sam McNally wrote:
> How about only setting "page" for bookmarks that have a destination page?

Done.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2328: pp::VarArray *children = new pp::VarArray();
On 2014/12/16 05:58:35, Sam McNally wrote:
> Put this on the stack. Otherwise, it will leak.

Done.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2329: children->SetLength(length);
On 2014/12/16 05:58:34, Sam McNally wrote:
> You don't need this. VarArray resizes to fit on calls to Set().

Done.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2332: pp::VarDictionary *childDict;
On 2014/12/16 05:58:34, Sam McNally wrote:
> Declare this inside the loop (or remove it entirely once this returns the
> dictionary instead of taking one). See above about leaking too.

Done.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2333: bookmark = FPDFBookmark_GetFirstChild(doc,
bookmark);
On 2014/12/16 05:58:35, Sam McNally wrote:
> Use a different variable for the child bookmarks.

Done.

https://codereview.chromium.org/810623003/diff/30001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2334: while (bookmark != NULL) {
On 2014/12/16 05:58:34, Sam McNally wrote:
> I think this would be nicer as a for loop.

Done.

[raymes, 2014-12-16 23:35:54]

https://codereview.chromium.org/810623003/diff/50001/pdf/pdf_engine.h
File pdf/pdf_engine.h (right):

https://codereview.chromium.org/810623003/diff/50001/pdf/pdf_engine.h#newcode251
pdf/pdf_engine.h:251: // Returns a nested Dictionary of Bookmarks, containing:
nit "containing the following key/values:"

https://codereview.chromium.org/810623003/diff/50001/pdf/pdf_engine.h#newcode256
pdf/pdf_engine.h:256: virtual pp::VarDictionary GetBookmarks() = 0;
Let's return a pp::VarArray here.

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:531: pp::VarDictionary
TraverseBookmarks(FPDF_DOCUMENT doc, FPDF_BOOKMARK bookmark) {
Add a comment about what this returns (include details about the "root"
bookmark) and what happens when "NULL" is passed in as |bookmark|.

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:532: 
nit: remove this blank line

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:536: if (buffer_size > 1) {
buffer_size > 0

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:538: WriteInto(&title, buffer_size + 1),
buffer_size);
-buffer_size should already include the null characters, so remove the + 1
-use a 4 space indent here

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:542: 
nit: remove a blink line here

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:545: int pageIndex = FPDFDest_GetPageIndex(doc,
dest);
we don't use camel case in chrome C++ so this should be page_index

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:547: }
if the dest is invalid, should we still set "page" to 0?

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:551: FPDF_BOOKMARK childBookmark =
FPDFBookmark_GetFirstChild(doc, bookmark);
child_bookmark

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:552: for (int i = 0; childBookmark != NULL; i++) {
This is a little unconventional. Maybe just have a while loop.

int child_index = 0;
while (child_bookmark != NULL) {
  ...
  child_index++;
}

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2333: pp::VarDictionary dict =
TraverseBookmarks(doc_, NULL);
as mentioned, let's pull out the array from the root bookmark and return that.

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2334: return dict;
or just:
return TraverseBookmarks(doc_, NULL);

[Alexandre Carlton, 2014-12-17 00:51:13]

https://codereview.chromium.org/810623003/diff/50001/pdf/pdf_engine.h
File pdf/pdf_engine.h (right):

https://codereview.chromium.org/810623003/diff/50001/pdf/pdf_engine.h#newcode251
pdf/pdf_engine.h:251: // Returns a nested Dictionary of Bookmarks, containing:
On 2014/12/16 23:35:53, raymes wrote:
> nit "containing the following key/values:"

Done.

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:531: pp::VarDictionary
TraverseBookmarks(FPDF_DOCUMENT doc, FPDF_BOOKMARK bookmark) {
On 2014/12/16 23:35:53, raymes wrote:
> Add a comment about what this returns (include details about the "root"
> bookmark) and what happens when "NULL" is passed in as |bookmark|.

Done.

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:532: 
On 2014/12/16 23:35:53, raymes wrote:
> nit: remove this blank line

Done.

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:536: if (buffer_size > 1) {
On 2014/12/16 23:35:54, raymes wrote:
> buffer_size > 0

Done.

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:538: WriteInto(&title, buffer_size + 1),
buffer_size);
On 2014/12/16 23:35:53, raymes wrote:
> -buffer_size should already include the null characters, so remove the + 1
> -use a 4 space indent here

Done.

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:542: 
On 2014/12/16 23:35:54, raymes wrote:
> nit: remove a blink line here

Done.

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:545: int pageIndex = FPDFDest_GetPageIndex(doc,
dest);
On 2014/12/16 23:35:53, raymes wrote:
> we don't use camel case in chrome C++ so this should be page_index

Done.

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:547: }
On 2014/12/16 23:35:53, raymes wrote:
> if the dest is invalid, should we still set "page" to 0?

As discussed, some bookmarks don't have pages at all (they can open urls in a
browser, for example).

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:551: FPDF_BOOKMARK childBookmark =
FPDFBookmark_GetFirstChild(doc, bookmark);
On 2014/12/16 23:35:53, raymes wrote:
> child_bookmark

Done.

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:552: for (int i = 0; childBookmark != NULL; i++) {
On 2014/12/16 23:35:53, raymes wrote:
> This is a little unconventional. Maybe just have a while loop.
> 
> int child_index = 0;
> while (child_bookmark != NULL) {
>   ...
>   child_index++;
> }

Done.

https://codereview.chromium.org/810623003/diff/50001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:2333: pp::VarDictionary dict =
TraverseBookmarks(doc_, NULL);
On 2014/12/16 23:35:53, raymes wrote:
> as mentioned, let's pull out the array from the root bookmark and return that.

Done.

[Sam McNally, 2014-12-17 02:48:45]

https://codereview.chromium.org/810623003/diff/110001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/810623003/diff/110001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:557: pp::VarDictionary childDict =
TraverseBookmarks(doc, child_bookmark);
child_dict or better yet, pass this directly to children.Set().

In general, prefer not creating variables that are only used once.

[Alexandre Carlton, 2014-12-17 02:57:09]

https://codereview.chromium.org/810623003/diff/110001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/810623003/diff/110001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:557: pp::VarDictionary childDict =
TraverseBookmarks(doc, child_bookmark);
On 2014/12/17 02:48:45, Sam McNally wrote:
> child_dict or better yet, pass this directly to children.Set().
> 
> In general, prefer not creating variables that are only used once.

Done.

[raymes, 2015-01-05 02:34:39]

lgtm

[raymes, 2015-01-13 07:13:26]

https://codereview.chromium.org/810623003/diff/130001/chrome/browser/resource...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/810623003/diff/130001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:405: this.bookmarks =
message.data.bookmarks;
nit: make this a private instance variable (this.bookmarks_)

https://codereview.chromium.org/810623003/diff/130001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/810623003/diff/130001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:542: }
See https://codereview.chromium.org/833263003/. We may have to resize the string
to avoid the trailing NULLs

[raymes, 2015-01-14 00:26:06]

https://codereview.chromium.org/810623003/diff/130001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/810623003/diff/130001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:538: size_t buffer_size =
FPDFBookmark_GetTitle(bookmark, NULL, 0);
-We should use unsigned long here rather than size_t (the same as the api).
-The problem here is that we are passing the buffer size to WriteInto rather
than the length of the text. We should use

title_length = buffer_size / sizeof(base::string16::value_type)

to compute the title length and then 

if (title_length > 1) 
......WriteInto(... title_length)

[Alexandre Carlton, 2015-01-15 22:53:49]

https://codereview.chromium.org/810623003/diff/130001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/810623003/diff/130001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:538: size_t buffer_size =
FPDFBookmark_GetTitle(bookmark, NULL, 0);
On 2015/01/14 00:26:06, raymes wrote:
> -We should use unsigned long here rather than size_t (the same as the api).
> -The problem here is that we are passing the buffer size to WriteInto rather
> than the length of the text. We should use
> 
> title_length = buffer_size / sizeof(base::string16::value_type)
> 
> to compute the title length and then 
> 
> if (title_length > 1) 
> ......WriteInto(... title_length)

Done. This resolves the trailing \u0000 characters.

https://codereview.chromium.org/810623003/diff/130001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:542: }
On 2015/01/13 07:13:26, raymes wrote:
> See https://codereview.chromium.org/833263003/. We may have to resize the
string
> to avoid the trailing NULLs

Done.

[raymes, 2015-01-16 02:44:23]

Looks good with a few nits to fix

https://codereview.chromium.org/810623003/diff/250001/pdf/out_of_process_inst...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/810623003/diff/250001/pdf/out_of_process_inst...
pdf/out_of_process_instance.cc:1118:
bookmarksMessage.Set(pp::Var(kJSBookmarksType), engine_->GetBookmarks());
We should have a separate constant here as the key in the dictionary:
kJSBookmarks. The constant can also have the value "bookmarks" though.

https://codereview.chromium.org/810623003/diff/250001/pdf/pdf_engine.h
File pdf/pdf_engine.h (right):

https://codereview.chromium.org/810623003/diff/250001/pdf/pdf_engine.h#newcod...
pdf/pdf_engine.h:251: // containing the following key/values:
nit: Make sure you fill the full 80chars before adding a line break

https://codereview.chromium.org/810623003/diff/250001/pdf/pdf_engine.h#newcod...
pdf/pdf_engine.h:253: // - "page" of type Var(int)
nit:
-"title" - a string Var.
-"page" - an int Var.

https://codereview.chromium.org/810623003/diff/250001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/810623003/diff/250001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:535: api_string_adapter.Close(bytes_written /
character_size);
this should just be 
api_string_adapter.Close(title_length);
you could just DCHECK bytes_written == buffer_size if you want

https://codereview.chromium.org/810623003/diff/250001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:538: 
nit: remove the above 2 blank lines

[Alexandre Carlton, 2015-01-16 03:01:40]

https://codereview.chromium.org/810623003/diff/250001/pdf/out_of_process_inst...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/810623003/diff/250001/pdf/out_of_process_inst...
pdf/out_of_process_instance.cc:1118:
bookmarksMessage.Set(pp::Var(kJSBookmarksType), engine_->GetBookmarks());
On 2015/01/16 02:44:23, raymes wrote:
> We should have a separate constant here as the key in the dictionary:
> kJSBookmarks. The constant can also have the value "bookmarks" though.

Done.

https://codereview.chromium.org/810623003/diff/250001/pdf/pdf_engine.h
File pdf/pdf_engine.h (right):

https://codereview.chromium.org/810623003/diff/250001/pdf/pdf_engine.h#newcod...
pdf/pdf_engine.h:251: // containing the following key/values:
On 2015/01/16 02:44:23, raymes wrote:
> nit: Make sure you fill the full 80chars before adding a line break

Done.

https://codereview.chromium.org/810623003/diff/250001/pdf/pdf_engine.h#newcod...
pdf/pdf_engine.h:253: // - "page" of type Var(int)
On 2015/01/16 02:44:23, raymes wrote:
> nit:
> -"title" - a string Var.
> -"page" - an int Var.

Done.

https://codereview.chromium.org/810623003/diff/250001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/810623003/diff/250001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:535: api_string_adapter.Close(bytes_written /
character_size);
On 2015/01/16 02:44:23, raymes wrote:
> this should just be 
> api_string_adapter.Close(title_length);
> you could just DCHECK bytes_written == buffer_size if you want

Done.

https://codereview.chromium.org/810623003/diff/250001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:538: 
On 2015/01/16 02:44:23, raymes wrote:
> nit: remove the above 2 blank lines

Done.

[raymes, 2015-01-18 21:33:14]

lgtm

[Sam McNally, 2015-01-18 23:42:39]

lgtm

[Alexandre Carlton, 2015-01-19 01:40:59]

The CQ bit was checked by alexandrec@chromium.org

[commit-bot: I haz the power, 2015-01-19 01:41:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/810623003/290001

[commit-bot: I haz the power, 2015-01-19 03:44:15]

Committed patchset #16 (id:290001)

[commit-bot: I haz the power, 2015-01-19 03:45:21]

Patchset 16 (id:??) landed as
https://crrev.com/6a36d4d1cc5e7d31d578c077bbf92a84d02a7578
Cr-Commit-Position: refs/heads/master@{#312059}