Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1767653002:
Implement content_decoder_tool.cc to decode offline any resources. (Closed)
DescriptionImplement content_decoder_tool.cc to decode offline any resources.
Sandwich is going to use the HTMLPreloadScanner to get all the
resources to prefetch instead of using the Clovis' dependency
graph. However resources in the chrome HTTP cache or in the WPR
archive are stored as transport layer content, implying that they
might be stored using a compression algorithm, according to the
Content-Encoding response header.
This tools enable us to decode any resources using the same very
code path used in chrome, implying that we will be able to
uncompressed absolutely all resources that chrome can and is
advertising in its Accept-Encoding request header.
BUG=582080
Committed: https://crrev.com/4dc1dc6668a87620574dd43392046c3691914da2
Cr-Commit-Position: refs/heads/master@{#382599}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addresses pasko's comments. #
Total comments: 1
Patch Set 3 : Addressing comments I have miss in the previous revision. #
Total comments: 6
Patch Set 4 : Addresses gavinp's nits #
Messages
Total messages: 28 (9 generated)
gabadie@chromium.org changed reviewers: + pasko@chromium.org
PTAL.
top-level comment: good catch! we'd indeed need to decompress to run HTMLPreloadScanner. What is the plan? Decompress to a pipe that directs into another tool? I would like to understand whether combining these two tools into one would be simpler. WDYT? Now minor comments: s/Implements/Implement/ I asked to phrase commit messages this way. If you disagree, we should discuss, silently ignoring suggestions is not productive. s/ressources/resources/ Please run the text through the speller. You can do: EDITOR=vim "git cl description" :set spell Easy! s/contentdecodertool/content_decoder_tool/
On 2016/03/04 16:46:31, pasko wrote: > top-level comment: good catch! we'd indeed need to decompress to run > HTMLPreloadScanner. > > What is the plan? Decompress to a pipe that directs into another tool? I would > like to understand whether combining these two tools into one would be simpler. > WDYT? Yes pipe from/to other commands. I don't want to put it in cachetool, because we may need it as well for reading directly from the WPR archive, and also cachetool could be reused for non HTTP caches. And I didn't want it to be in in the HTMLPreloadScanner tool because I have in mind a bigger idea of NoState-Prefetch benchmark that would give us interesting statistics. But it would requires to decode resources as well. I would be happy to share it offline once you are back. > > Now minor comments: > > s/Implements/Implement/ > > I asked to phrase commit messages this way. If you disagree, we should discuss, > silently ignoring suggestions is not productive. My bad. I have complelety forgotten this. I don't even recall you telling me this. I have always did it that way at all my previous employers because when looking at the log or a review, everyone is asking themself, What does this CL do/change? This CL <first line of the commit message>. For instance: This CL "Implements contentdecodertool.cc to decode offline any resources". > > s/ressources/resources/ My bad. > > Please run the text through the speller. You can do: > EDITOR=vim "git cl description" > :set spell > > Easy! > > s/contentdecodertool/content_decoder_tool/ I thought it too. But I wanted to be consistent with cachetool that Gavin named.
Description was changed from ========== Implements contentdecodertool.cc to decode offline any ressources. Sandwich is going to use the HTMLPreloadScanner to get all the ressources to prefetch instead of using the Clovis' dependency graph. However resources in the chrome HTTP cache or in the WPR archive are stored as transport layer content, implying that they might be stored compressed according to the Content-Encoding response header's value. This tools enable us to decode any resources using the same very code path used in chrome, implying that we will be able to uncompress absolutly all resources that chrome can and is advertising in its Accept-Encoding request header. BUG=582080 ========== to ========== Implements contentdecodertool.cc to decode offline any resources. Sandwich is going to use the HTMLPreloadScanner to get all the resources to prefetch instead of using the Clovis' dependency graph. However resources in the chrome HTTP cache or in the WPR archive are stored as transport layer content, implying that they might be stored using a compression algorithm, according to the Content-Encoding response header. This tools enable us to decode any resources using the same very code path used in chrome, implying that we will be able to uncompressed absolutely all resources that chrome can and is advertising in its Accept-Encoding request header. BUG=582080 ==========
Description was changed from ========== Implements contentdecodertool.cc to decode offline any resources. Sandwich is going to use the HTMLPreloadScanner to get all the resources to prefetch instead of using the Clovis' dependency graph. However resources in the chrome HTTP cache or in the WPR archive are stored as transport layer content, implying that they might be stored using a compression algorithm, according to the Content-Encoding response header. This tools enable us to decode any resources using the same very code path used in chrome, implying that we will be able to uncompressed absolutely all resources that chrome can and is advertising in its Accept-Encoding request header. BUG=582080 ========== to ========== Implement contentdecodertool.cc to decode offline any resources. Sandwich is going to use the HTMLPreloadScanner to get all the resources to prefetch instead of using the Clovis' dependency graph. However resources in the chrome HTTP cache or in the WPR archive are stored as transport layer content, implying that they might be stored using a compression algorithm, according to the Content-Encoding response header. This tools enable us to decode any resources using the same very code path used in chrome, implying that we will be able to uncompressed absolutely all resources that chrome can and is advertising in its Accept-Encoding request header. BUG=582080 ==========
I will follow up with review shortly. This is just following the discussion that is already started. These are _not_ major concerns, not really blocking, but I would like to get agreement on them at some point anyway. On 2016/03/04 17:36:05, gabadie wrote: > On 2016/03/04 16:46:31, pasko wrote: > > top-level comment: good catch! we'd indeed need to decompress to run > > HTMLPreloadScanner. > > > > What is the plan? Decompress to a pipe that directs into another tool? I would > > like to understand whether combining these two tools into one would be > simpler. > > WDYT? > Yes pipe from/to other commands. I don't want to put it in cachetool, because we > may need it as well for reading directly from the WPR archive, and also > cachetool could be reused for non HTTP caches. And I didn't want it to be in in > the HTMLPreloadScanner tool because I have in mind a bigger idea of > NoState-Prefetch benchmark that would give us interesting statistics. But it > would requires to decode resources as well. I would be happy to share it offline > once you are back. Good. I did not think about using it for non-http caches. Now I cannot agree more. Also nothing stops us from linking this code into some other tool later. > > Now minor comments: > > > > s/Implements/Implement/ > > > > I asked to phrase commit messages this way. If you disagree, we should > discuss, > > silently ignoring suggestions is not productive. > My bad. I have complelety forgotten this. I don't even recall you telling me > this. I have always did it that way at all my previous employers because when > looking at the log or a review, everyone is asking themself, What does this CL > do/change? This CL <first line of the commit message>. For instance: This CL > "Implements contentdecodertool.cc to decode offline any resources". In fairness, I probably did not formalize this as a rule before, and I failed to find any examples in a few minutes. Moreover, your style is actually _better_ (more consistent with function comments in addition to the reason you provided). However, looking at a recent log of commits in chromium it is either a name of a new thing introduced or something that supports my theory: $ git log origin/master --oneline | head -20 6b8fd36 Include platform/RuntimeEnabledFeature.h if a dictionary has a runtime enabled feature. fdf9da8 off_the_record_chrome_browser_state_io_data.mm: fix case for UIKit. eef246b Update V8 to version 5.1.139.1 (cherry-pick). 048e27b Remove the IME items in the status tray when the IME menu is activated. a0c8fd9 Added FATAL instead of IMMEIDATE_CRASH when PartitionAlloc OOM with < 16M memory usage. 5d19b64 Welcome isSameNode back as a per-spec method 8e242b3 DevTools: roll CodeMirror to e8aaf70 for modes: JS, CSS, XML. Add JSX. 52eaa4e Update libjingle.gyp and BUILD.gn for WebRTC. Files are being added and removed due to cleanup of the public Apis and removing PeerConnection dependencies on cricket::VideoCapturer. b07af05 Cleanup after selection gap painting removal. 2abe9b2f mb_config.pyl cleanup - part two 8df3dd0 Use std::is_base_of to assert WeakPtr is used correctly 4ccd1db Automated Commit: Committing new LKGM version 8069.0.0 for chromeos. 7c11c16 MD History: Refactor how data is created in tests 563e666 [Autofill] Possibly import more than one address profile per form submission. 7f5fd53 Recover kDisableMacOverlays switch. 1974ec7 [Media Router WebUI] Truncate long sink names in route details. e4e1eb6 Clean up lingering references to writing-mode: horizontal-bt. 127acd7 Blink Compositor Animations: Reset delegate in CC on Blink player deletion. 5b7bb09 Add experimental accessibility features to Material Design settings 0905ff8 Disable FontName, FontSize, and FontSizeDelta commands in contenteditable=plaintext-only. Out of these 20 there is no instance of "includes", "updates", "removes", "adds", "welcomes" etc. I don't know why, and I do not think the style is good, but I think consistency is good. Feel free to ask in chromium-dev@ why our style is like this. And this way we can actually change it for the better :) > > s/ressources/resources/ > My bad. > > > > Please run the text through the speller. You can do: > > EDITOR=vim "git cl description" > > :set spell > > > > Easy! > > > > s/contentdecodertool/content_decoder_tool/ > I thought it too. But I wanted to be consistent with cachetool that Gavin named. I did not find a reference where Gavin insisted on naming for cachetool, but consistency would dictate the opposite: $ git ls-files | grep tool.cc components/feedback/anonymizer_tool.cc courgette/courgette_minimal_tool.cc courgette/courgette_tool.cc net/tools/cachetool/cachetool.cc third_party/crashpad/crashpad/tools/mac/catch_exception_tool.cc third_party/crashpad/crashpad/tools/mac/exception_port_tool.cc tools/gn/tool.cc
overall looks good, thanks!! I have only some variable naming and documentation-related suggestions. https://codereview.chromium.org/1767653002/diff/1/net/tools/contentdecodertoo... File net/tools/contentdecodertool/contentdecodertool.cc (right): https://codereview.chromium.org/1767653002/diff/1/net/tools/contentdecodertoo... net/tools/contentdecodertool/contentdecodertool.cc:18: std::cout << "contentdecodertool encoding_type [encoding_type]*" << std::endl let's name it content_encoding (instead of encoding_type), seems less ambiguous and more widely recognizable by people not necessary, but somewhat neat: PrintHelp(argv[0]) could print the exact name of the command that was used to run the command (including the path), no need to maintain consistency with the way we name the build target. another very minor nit: the star ('*') is slightly harder to notice than '...', so various *nix manuals use the syntax like: tail [OPTION]... [FILE]... https://codereview.chromium.org/1767653002/diff/1/net/tools/contentdecodertoo... net/tools/contentdecodertool/contentdecodertool.cc:50: Filter::FilterType encode_type = I think |filter_type| suits better as a name https://codereview.chromium.org/1767653002/diff/1/net/tools/contentdecodertoo... net/tools/contentdecodertool/contentdecodertool.cc:68: int encode_buffer_size = filter->stream_buffer_size(); I do not understand why the word 'encode' is used. There is no 'encode' operation used here. So a more readable name would be 'encoded_buffer_size'. I would actually prefer to name variables as suggested in documentation for net/filter/filter.h: pre_filter_data_len, post_filter_buf, post_filter_data_len, etc. https://codereview.chromium.org/1767653002/diff/1/net/tools/contentdecodertoo... net/tools/contentdecodertool/contentdecodertool.cc:82: std::cerr << "Couldn't decode stdin properly." << std::endl; nit: s/ properly//
Description was changed from ========== Implement contentdecodertool.cc to decode offline any resources. Sandwich is going to use the HTMLPreloadScanner to get all the resources to prefetch instead of using the Clovis' dependency graph. However resources in the chrome HTTP cache or in the WPR archive are stored as transport layer content, implying that they might be stored using a compression algorithm, according to the Content-Encoding response header. This tools enable us to decode any resources using the same very code path used in chrome, implying that we will be able to uncompressed absolutely all resources that chrome can and is advertising in its Accept-Encoding request header. BUG=582080 ========== to ========== Implement content_decoder_tool.cc to decode offline any resources. Sandwich is going to use the HTMLPreloadScanner to get all the resources to prefetch instead of using the Clovis' dependency graph. However resources in the chrome HTTP cache or in the WPR archive are stored as transport layer content, implying that they might be stored using a compression algorithm, according to the Content-Encoding response header. This tools enable us to decode any resources using the same very code path used in chrome, implying that we will be able to uncompressed absolutely all resources that chrome can and is advertising in its Accept-Encoding request header. BUG=582080 ==========
gabadie@chromium.org changed reviewers: + gavinp@chromium.org
Thanks Egor for your review. I have addressed all your comments, including renaming the tool with the underscore. PTAL. Adding gavinp@chromium.org as reviewer: for rubber stamp review. https://codereview.chromium.org/1767653002/diff/1/net/tools/contentdecodertoo... File net/tools/contentdecodertool/contentdecodertool.cc (right): https://codereview.chromium.org/1767653002/diff/1/net/tools/contentdecodertoo... net/tools/contentdecodertool/contentdecodertool.cc:18: std::cout << "contentdecodertool encoding_type [encoding_type]*" << std::endl On 2016/03/16 12:05:53, pasko wrote: > let's name it content_encoding (instead of encoding_type), seems less ambiguous > and more widely recognizable by people > > not necessary, but somewhat neat: > PrintHelp(argv[0]) could print the exact name of the command that was used to > run the command (including the path), no need to maintain consistency with the > way we name the build target. > > another very minor nit: the star ('*') is slightly harder to notice than '...', > so various *nix manuals use the syntax like: > > tail [OPTION]... [FILE]... Done. https://codereview.chromium.org/1767653002/diff/1/net/tools/contentdecodertoo... net/tools/contentdecodertool/contentdecodertool.cc:50: Filter::FilterType encode_type = On 2016/03/16 12:05:53, pasko wrote: > I think |filter_type| suits better as a name Done. https://codereview.chromium.org/1767653002/diff/1/net/tools/contentdecodertoo... net/tools/contentdecodertool/contentdecodertool.cc:68: int encode_buffer_size = filter->stream_buffer_size(); On 2016/03/16 12:05:53, pasko wrote: > I do not understand why the word 'encode' is used. There is no 'encode' > operation used here. So a more readable name would be 'encoded_buffer_size'. > > I would actually prefer to name variables as suggested in documentation for > net/filter/filter.h: > > pre_filter_data_len, post_filter_buf, post_filter_data_len, etc. Done. https://codereview.chromium.org/1767653002/diff/1/net/tools/contentdecodertoo... net/tools/contentdecodertool/contentdecodertool.cc:82: std::cerr << "Couldn't decode stdin properly." << std::endl; On 2016/03/16 12:05:53, pasko wrote: > nit: s/ properly// Done.
lgtm with a small change thank you! https://codereview.chromium.org/1767653002/diff/1/net/tools/contentdecodertoo... File net/tools/contentdecodertool/contentdecodertool.cc (right): https://codereview.chromium.org/1767653002/diff/1/net/tools/contentdecodertoo... net/tools/contentdecodertool/contentdecodertool.cc:18: std::cout << "contentdecodertool encoding_type [encoding_type]*" << std::endl On 2016/03/16 18:09:16, gabadie wrote: > On 2016/03/16 12:05:53, pasko wrote: > > let's name it content_encoding (instead of encoding_type), seems less > ambiguous > > and more widely recognizable by people > > > > not necessary, but somewhat neat: > > PrintHelp(argv[0]) could print the exact name of the command that was used to > > run the command (including the path), no need to maintain consistency with the > > way we name the build target. > > > > another very minor nit: the star ('*') is slightly harder to notice than > '...', > > so various *nix manuals use the syntax like: > > > > tail [OPTION]... [FILE]... > > Done. OK, the 2 out of 3 suggestions were not necessary, but your thoughts about why not taking them would have been appreciated :) https://codereview.chromium.org/1767653002/diff/20001/net/tools/content_decod... File net/tools/content_decoder_tool/content_decoder_tool.cc (right): https://codereview.chromium.org/1767653002/diff/20001/net/tools/content_decod... net/tools/content_decoder_tool/content_decoder_tool.cc:18: std::cout << "contentdecodertool content_encoding [content_encoding]*" so now it should say content_decoder_tool :/ ... or argv[0]
So sorry, I have completely missed these comments. I have just uploaded a new CL with these changes. Pinging gavinp@chromium.org for review. =) https://codereview.chromium.org/1767653002/diff/1/net/tools/contentdecodertoo... File net/tools/contentdecodertool/contentdecodertool.cc (right): https://codereview.chromium.org/1767653002/diff/1/net/tools/contentdecodertoo... net/tools/contentdecodertool/contentdecodertool.cc:18: std::cout << "contentdecodertool encoding_type [encoding_type]*" << std::endl On 2016/03/16 18:30:59, pasko wrote: > On 2016/03/16 18:09:16, gabadie wrote: > > On 2016/03/16 12:05:53, pasko wrote: > > > let's name it content_encoding (instead of encoding_type), seems less > > ambiguous > > > and more widely recognizable by people > > > > > > not necessary, but somewhat neat: > > > PrintHelp(argv[0]) could print the exact name of the command that was used > to > > > run the command (including the path), no need to maintain consistency with > the > > > way we name the build target. > > > > > > another very minor nit: the star ('*') is slightly harder to notice than > > '...', > > > so various *nix manuals use the syntax like: > > > > > > tail [OPTION]... [FILE]... > > > > Done. > > OK, the 2 out of 3 suggestions were not necessary, but your thoughts about why > not taking them would have been appreciated :) My bad. I have just completely forgotten them.
still lgtm
lgtm. https://codereview.chromium.org/1767653002/diff/40001/net/tools/content_decod... File net/tools/content_decoder_tool/content_decoder_tool.cc (right): https://codereview.chromium.org/1767653002/diff/40001/net/tools/content_decod... net/tools/content_decoder_tool/content_decoder_tool.cc:23: << "Content-Encoding HTTP response header's value splitted by `,`." Nit: "split" not "splitted" and ' not `. https://codereview.chromium.org/1767653002/diff/40001/net/tools/content_decod... net/tools/content_decoder_tool/content_decoder_tool.cc:37: for (const auto& arg : wide_args) { Nit: braces not needed. https://codereview.chromium.org/1767653002/diff/40001/net/tools/content_decod... net/tools/content_decoder_tool/content_decoder_tool.cc:54: std::cerr << "Unsupported decoder `" << content_encoding << "`." nit: I think using ` here is a bit odd. Would ' make more sense?
Thanks Gavin! I have addressed your nits in the newly uploaded revision. Landing. https://codereview.chromium.org/1767653002/diff/40001/net/tools/content_decod... File net/tools/content_decoder_tool/content_decoder_tool.cc (right): https://codereview.chromium.org/1767653002/diff/40001/net/tools/content_decod... net/tools/content_decoder_tool/content_decoder_tool.cc:23: << "Content-Encoding HTTP response header's value splitted by `,`." On 2016/03/22 14:56:58, gavinp wrote: > Nit: "split" not "splitted" and ' not `. Done. https://codereview.chromium.org/1767653002/diff/40001/net/tools/content_decod... net/tools/content_decoder_tool/content_decoder_tool.cc:37: for (const auto& arg : wide_args) { On 2016/03/22 14:56:58, gavinp wrote: > Nit: braces not needed. Done. https://codereview.chromium.org/1767653002/diff/40001/net/tools/content_decod... net/tools/content_decoder_tool/content_decoder_tool.cc:54: std::cerr << "Unsupported decoder `" << content_encoding << "`." On 2016/03/22 14:56:58, gavinp wrote: > nit: I think using ` here is a bit odd. Would ' make more sense? Done.
The CQ bit was checked by gabadie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, gavinp@chromium.org Link to the patchset: https://codereview.chromium.org/1767653002/#ps60001 (title: "Addresses gavinp's nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767653002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767653002/60001
Message was sent while issue was closed.
Description was changed from ========== Implement content_decoder_tool.cc to decode offline any resources. Sandwich is going to use the HTMLPreloadScanner to get all the resources to prefetch instead of using the Clovis' dependency graph. However resources in the chrome HTTP cache or in the WPR archive are stored as transport layer content, implying that they might be stored using a compression algorithm, according to the Content-Encoding response header. This tools enable us to decode any resources using the same very code path used in chrome, implying that we will be able to uncompressed absolutely all resources that chrome can and is advertising in its Accept-Encoding request header. BUG=582080 ========== to ========== Implement content_decoder_tool.cc to decode offline any resources. Sandwich is going to use the HTMLPreloadScanner to get all the resources to prefetch instead of using the Clovis' dependency graph. However resources in the chrome HTTP cache or in the WPR archive are stored as transport layer content, implying that they might be stored using a compression algorithm, according to the Content-Encoding response header. This tools enable us to decode any resources using the same very code path used in chrome, implying that we will be able to uncompressed absolutely all resources that chrome can and is advertising in its Accept-Encoding request header. BUG=582080 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Implement content_decoder_tool.cc to decode offline any resources. Sandwich is going to use the HTMLPreloadScanner to get all the resources to prefetch instead of using the Clovis' dependency graph. However resources in the chrome HTTP cache or in the WPR archive are stored as transport layer content, implying that they might be stored using a compression algorithm, according to the Content-Encoding response header. This tools enable us to decode any resources using the same very code path used in chrome, implying that we will be able to uncompressed absolutely all resources that chrome can and is advertising in its Accept-Encoding request header. BUG=582080 ========== to ========== Implement content_decoder_tool.cc to decode offline any resources. Sandwich is going to use the HTMLPreloadScanner to get all the resources to prefetch instead of using the Clovis' dependency graph. However resources in the chrome HTTP cache or in the WPR archive are stored as transport layer content, implying that they might be stored using a compression algorithm, according to the Content-Encoding response header. This tools enable us to decode any resources using the same very code path used in chrome, implying that we will be able to uncompressed absolutely all resources that chrome can and is advertising in its Accept-Encoding request header. BUG=582080 Committed: https://crrev.com/4dc1dc6668a87620574dd43392046c3691914da2 Cr-Commit-Position: refs/heads/master@{#382599} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4dc1dc6668a87620574dd43392046c3691914da2 Cr-Commit-Position: refs/heads/master@{#382599}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1827483002/ by dgozman@chromium.org. The reason for reverting is: Broke iOS build: https://build.chromium.org/p/chromium.mac/builders/iOS_Device/builds/44002.
Message was sent while issue was closed.
On 2016/03/22 17:56:35, dgozman wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/1827483002/ by mailto:dgozman@chromium.org. > > The reason for reverting is: Broke iOS build: > https://build.chromium.org/p/chromium.mac/builders/iOS_Device/builds/44002. Don't you have to add this to a gn file too?
Message was sent while issue was closed.
On 2016/03/24 13:48:03, Nico (away until Mon Mar 27) wrote: > On 2016/03/22 17:56:35, dgozman wrote: > > A revert of this CL (patchset #4 id:60001) has been created in > > https://codereview.chromium.org/1827483002/ by mailto:dgozman@chromium.org. > > > > The reason for reverting is: Broke iOS build: > > https://build.chromium.org/p/chromium.mac/builders/iOS_Device/builds/44002. > > Don't you have to add this to a gn file too? Also I believe this doesn't build, see https://llvm.org/bugs/show_bug.cgi?id=27051 Consider reverting, and then reland once you have did .gn changes and have green try bots with that.
Message was sent while issue was closed.
On 2016/03/24 13:50:26, Nico (away until Mon Mar 27) wrote: > On 2016/03/24 13:48:03, Nico (away until Mon Mar 27) wrote: > > On 2016/03/22 17:56:35, dgozman wrote: > > > A revert of this CL (patchset #4 id:60001) has been created in > > > https://codereview.chromium.org/1827483002/ by mailto:dgozman@chromium.org. > > > > > > The reason for reverting is: Broke iOS build: > > > https://build.chromium.org/p/chromium.mac/builders/iOS_Device/builds/44002. > > > > Don't you have to add this to a gn file too? > > Also I believe this doesn't build, see > https://llvm.org/bugs/show_bug.cgi?id=27051 > > Consider reverting, and then reland once you have did .gn changes and have green > try bots with that. I'm fixing the gyp build here: https://codereview.chromium.org/1831953002/
Message was sent while issue was closed.
On 2016/03/24 13:50:26, Nico (away until Mon Mar 27) wrote: > On 2016/03/24 13:48:03, Nico (away until Mon Mar 27) wrote: > > On 2016/03/22 17:56:35, dgozman wrote: > > > A revert of this CL (patchset #4 id:60001) has been created in > > > https://codereview.chromium.org/1827483002/ by mailto:dgozman@chromium.org. > > > > > > The reason for reverting is: Broke iOS build: > > > https://build.chromium.org/p/chromium.mac/builders/iOS_Device/builds/44002. > > > > Don't you have to add this to a gn file too? > > Also I believe this doesn't build, see > https://llvm.org/bugs/show_bug.cgi?id=27051 > > Consider reverting, and then reland once you have did .gn changes and have green > try bots with that. I have just made https://codereview.chromium.org/1838443002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
