Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1737103002:
sandwich: Implements filter-cache sub-command. (Closed)
|
Created:
4 years, 10 months ago by gabadie Modified:
4 years, 9 months ago CC:
chromium-reviews, gabadie+watch_chromium.org, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@i10 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionsandwich: Implements filter-cache sub-command.
NoState-Prefetch won't be able to know all the resources to fetch.
This new sub-command creates a new cache archive by pruning all
resources that can't be discovered by the HTML parser of the main
HTML document.
BUG=582080
Committed: https://crrev.com/aeb69a7ef733515cc168e9cdd4edcb5642c1b3fc
Cr-Commit-Position: refs/heads/master@{#378486}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Addresses pasko's comment #
Total comments: 5
Patch Set 3 : Updates loading_trace_paths's help #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 18 (7 generated)
gabadie@chromium.org changed reviewers: + lizeb@google.com, mattcary@chromium.org, pasko@chromium.org
Hi guys, This CL introduces a sub-command that creates a new cache archive by pruning all resources that can't be discovered by the HTML parser of the main HTML document. PTAL.
Description was changed from ========== sandwich: Implements filter-cache sub-command. NoState-Prefetch won't be able to know all the ressources to fetch. This new sub-command creates a new cache archive by prunning all ressources that can't be discovred by the HTML parser of the main HTML document. BUG=582080 ========== to ========== sandwich: Implements filter-cache sub-command. NoState-Prefetch won't be able to know all the resources to fetch. This new sub-command creates a new cache archive by pruning all resources that can't be discovered by the HTML parser of the main HTML document. BUG=582080 ==========
gabadie@chromium.org changed reviewers: + lizeb@chromium.org - lizeb@google.com
lgtm https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... File tools/android/loading/chrome_cache.py (right): https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... tools/android/loading/chrome_cache.py:145: if os.path.isfile(archive_dest_path): This doesn't seem to be necessary---zipfile will silently overwrite existing files as far as I can tell. Seems harmless enough, though, even if my instinct is always to remove code.
https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... File tools/android/loading/chrome_cache.py (right): https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... tools/android/loading/chrome_cache.py:145: if os.path.isfile(archive_dest_path): is it really necessary to remove before writing? According to the code below we are not expecting a file to be there, so maybe this code is dead and not needed? https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... tools/android/loading/chrome_cache.py:305: shutil.rmtree(cache_temp_directory) the try..finally would avoid polluting the filesystem https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... tools/android/loading/chrome_cache.py:309: import argparse why import here? https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... tools/android/loading/chrome_cache.py:310: parser = argparse.ArgumentParser(description='Tests cache back-end.') nit: backend in one word https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich.py (right): https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich.py:383: filter_cache_parser.add_argument('loading_trace_paths', type=str, nargs='+', underscores -> dashes, please https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich.py:385: help='A loading trace path to generate the' + please document more details on how they are generated? is it everything except data urls. I have less confidence in trace putting all urls correctly than the cache tool reading entries one by one and deleting them if they are not data urls, this would depend on a smaller code surface, I think. WDYT?
pasko: new revision uploaded. PTAL. https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... File tools/android/loading/chrome_cache.py (right): https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... tools/android/loading/chrome_cache.py:145: if os.path.isfile(archive_dest_path): On 2016/02/26 17:10:06, pasko wrote: > is it really necessary to remove before writing? According to the code below we > are not expecting a file to be there, so maybe this code is dead and not needed? Done. https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... tools/android/loading/chrome_cache.py:305: shutil.rmtree(cache_temp_directory) On 2016/02/26 17:10:06, pasko wrote: > the try..finally would avoid polluting the filesystem Good point. I think we should just do a util like https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory. Done. https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... tools/android/loading/chrome_cache.py:309: import argparse On 2016/02/26 17:10:06, pasko wrote: > why import here? Because this is a lib manual test (in the mean time of the integrated test for sandwich). So we don't want to pollute the module import list when including into another module. https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... tools/android/loading/chrome_cache.py:310: parser = argparse.ArgumentParser(description='Tests cache back-end.') On 2016/02/26 17:10:06, pasko wrote: > nit: backend in one word Looks like the two are correct. Keeping it to be consistent within this directory. https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich.py (right): https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich.py:383: filter_cache_parser.add_argument('loading_trace_paths', type=str, nargs='+', On 2016/02/26 17:10:06, pasko wrote: > underscores -> dashes, please Nope. Because non optional argument. I would have added them if this was --loading-trace-paths meaning you need to write those word in the command line. But here you won't write it, you just directly list the paths of the loading traces. https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich.py:385: help='A loading trace path to generate the' + On 2016/02/26 17:10:06, pasko wrote: > please document more details on how they are generated? is it everything except > data urls. Done. > > I have less confidence in trace putting all urls correctly than the cache tool > reading entries one by one and deleting them if they are not data urls, this > would depend on a smaller code surface, I think. > > WDYT? You might be mixing up between traces and clovis loading traces. https://code.google.com/p/chromium/codesearch#chromium/src/tools/android/load.... The point of using a loading traces is to create the resource dependency graph that clovis guys have implemented to actually white list only resources discover-able by the html pre-scanner.
lgtm on the functionality, we can postpone renaming the args if you are not immediately happy with my suggested replacements. https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... File tools/android/loading/chrome_cache.py (right): https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... tools/android/loading/chrome_cache.py:309: import argparse On 2016/03/01 10:40:48, gabadie wrote: > On 2016/02/26 17:10:06, pasko wrote: > > why import here? > > Because this is a lib manual test (in the mean time of the integrated test for > sandwich). So we don't want to pollute the module import list when including > into another module. isn't this against the style guide? To avoid the pollution please prefer to create a separate file for manual testing. https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... tools/android/loading/chrome_cache.py:310: parser = argparse.ArgumentParser(description='Tests cache back-end.') On 2016/03/01 10:40:48, gabadie wrote: > On 2016/02/26 17:10:06, pasko wrote: > > nit: backend in one word > > Looks like the two are correct. Keeping it to be consistent within this > directory. They are both correct, but we should prefer using it as "backend" to be consistent with the rest of Chrome. Please also see Don Knuth's opinion on email vs e-mail: http://www-cs-faculty.stanford.edu/~uno/email.html But this is a nit for now, so you can leave it unchanged for now https://codereview.chromium.org/1737103002/diff/20001/tools/android/loading/s... File tools/android/loading/sandwich.py (right): https://codereview.chromium.org/1737103002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich.py:385: filter_cache_parser.add_argument('loading_trace_paths', type=str, nargs='+', nit: loading-trace-paths for consistency https://codereview.chromium.org/1737103002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich.py:387: help='A loading trace path generated by a sandwich run for a given url.' + I would say: "A list of extended traces generated by .." To avoid overloading the term "trace", shall we name the thing "extended trace"? Then I would actually name the argument as: --extended-traces. WDYT?
New revision the amended help uploaded. Commiting once the dependencies are landed. Thanks for your reviews ! =) https://codereview.chromium.org/1737103002/diff/20001/tools/android/loading/s... File tools/android/loading/sandwich.py (right): https://codereview.chromium.org/1737103002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich.py:385: filter_cache_parser.add_argument('loading_trace_paths', type=str, nargs='+', On 2016/03/01 16:01:11, pasko wrote: > nit: loading-trace-paths for consistency > This change would be point less because this is a positional argument. So the only place it is shown is in the man, with underscore, and it won't be in command line. And since I have not explicitly overridden the dest parameter, I would rather to keep here with the underscore for easier grep of the code for this variable name. https://codereview.chromium.org/1737103002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich.py:387: help='A loading trace path generated by a sandwich run for a given url.' + On 2016/03/01 16:01:11, pasko wrote: > I would say: "A list of extended traces generated by .." Done. > > To avoid overloading the term "trace", shall we name the thing "extended trace"? > > Then I would actually name the argument as: --extended-traces. WDYT? I agree the loading trace is terrible. But I would rather stick with the name of the python class (LoadingTrace) that can load this LoadingTrace.
https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... File tools/android/loading/chrome_cache.py (right): https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... tools/android/loading/chrome_cache.py:309: import argparse On 2016/03/01 16:01:11, pasko wrote: > On 2016/03/01 10:40:48, gabadie wrote: > > On 2016/02/26 17:10:06, pasko wrote: > > > why import here? > > > > Because this is a lib manual test (in the mean time of the integrated test for > > sandwich). So we don't want to pollute the module import list when including > > into another module. > > isn't this against the style guide? To avoid the pollution please prefer to > create a separate file for manual testing. According to Benoit, that is a common practice. https://code.google.com/p/chromium/codesearch#chromium/src/tools/android/load... https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... tools/android/loading/chrome_cache.py:310: parser = argparse.ArgumentParser(description='Tests cache back-end.') On 2016/03/01 16:01:11, pasko wrote: > On 2016/03/01 10:40:48, gabadie wrote: > > On 2016/02/26 17:10:06, pasko wrote: > > > nit: backend in one word > > > > Looks like the two are correct. Keeping it to be consistent within this > > directory. > > They are both correct, but we should prefer using it as "backend" to be > consistent with the rest of Chrome. > > Please also see Don Knuth's opinion on email vs e-mail: > http://www-cs-faculty.stanford.edu/~uno/email.html > > But this is a nit for now, so you can leave it unchanged for now Acknowledged.
The CQ bit was checked by gabadie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattcary@chromium.org, pasko@chromium.org Link to the patchset: https://codereview.chromium.org/1737103002/#ps40001 (title: "Updates loading_trace_paths's help")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737103002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737103002/40001
still lgtm https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... File tools/android/loading/chrome_cache.py (right): https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/chrom... tools/android/loading/chrome_cache.py:309: import argparse On 2016/03/01 16:49:09, gabadie wrote: > On 2016/03/01 16:01:11, pasko wrote: > > On 2016/03/01 10:40:48, gabadie wrote: > > > On 2016/02/26 17:10:06, pasko wrote: > > > > why import here? > > > > > > Because this is a lib manual test (in the mean time of the integrated test > for > > > sandwich). So we don't want to pollute the module import list when including > > > into another module. > > > > isn't this against the style guide? To avoid the pollution please prefer to > > create a separate file for manual testing. > > According to Benoit, that is a common practice. > https://code.google.com/p/chromium/codesearch#chromium/src/tools/android/load... I disagree with Benoit here. In the Chromium codebase the only occurrences of this are in tools/android/loading. And in this one weird place: https://goo.gl/SozSvM We should not do it. https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich.py (right): https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich.py:383: filter_cache_parser.add_argument('loading_trace_paths', type=str, nargs='+', On 2016/03/01 10:40:48, gabadie wrote: > On 2016/02/26 17:10:06, pasko wrote: > > underscores -> dashes, please > > Nope. Because non optional argument. I would have added them if this was > --loading-trace-paths meaning you need to write those word in the command line. > But here you won't write it, you just directly list the paths of the loading > traces. ah, I see, thanks, maybe move the definition of this non-optional argument above all optional ones? That is my implicit assumption, not sure where it is coming from, maybe protobufs I saw were all like that. https://codereview.chromium.org/1737103002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich.py:385: help='A loading trace path to generate the' + On 2016/03/01 10:40:48, gabadie wrote: > > I have less confidence in trace putting all urls correctly than the cache tool > > reading entries one by one and deleting them if they are not data urls, this > > would depend on a smaller code surface, I think. > > > > WDYT? > > You might be mixing up between traces and clovis loading traces. > https://code.google.com/p/chromium/codesearch#chromium/src/tools/android/load.... > The point of using a loading traces is to create the resource dependency graph > that clovis guys have implemented to actually white list only resources > discover-able by the html pre-scanner. No, I was not mixing them this time. We can name the latter as "extended traces" or "clovis traces" btw. The Traces of Mister Clovis :) Anyway, I have relatively low level of trust for the metadata devtools interface provides. It is less mission-critical than, say HTMLPreloadScanner, and there are known bugs which droger@ has been looking at, right? I added a task to check the equivalence later. https://codereview.chromium.org/1737103002/diff/20001/tools/android/loading/s... File tools/android/loading/sandwich.py (right): https://codereview.chromium.org/1737103002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich.py:385: filter_cache_parser.add_argument('loading_trace_paths', type=str, nargs='+', On 2016/03/01 16:40:00, gabadie wrote: > On 2016/03/01 16:01:11, pasko wrote: > > nit: loading-trace-paths for consistency > > > > This change would be point less because this is a positional argument. So the > only place it is shown is in the man, with underscore, and it won't be in > command line. And since I have not explicitly overridden the dest parameter, I > would rather to keep here with the underscore for easier grep of the code for > this variable name. Yes, we discussed it in another review. Good to go.
Message was sent while issue was closed.
Description was changed from ========== sandwich: Implements filter-cache sub-command. NoState-Prefetch won't be able to know all the resources to fetch. This new sub-command creates a new cache archive by pruning all resources that can't be discovered by the HTML parser of the main HTML document. BUG=582080 ========== to ========== sandwich: Implements filter-cache sub-command. NoState-Prefetch won't be able to know all the resources to fetch. This new sub-command creates a new cache archive by pruning all resources that can't be discovered by the HTML parser of the main HTML document. BUG=582080 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== sandwich: Implements filter-cache sub-command. NoState-Prefetch won't be able to know all the resources to fetch. This new sub-command creates a new cache archive by pruning all resources that can't be discovered by the HTML parser of the main HTML document. BUG=582080 ========== to ========== sandwich: Implements filter-cache sub-command. NoState-Prefetch won't be able to know all the resources to fetch. This new sub-command creates a new cache archive by pruning all resources that can't be discovered by the HTML parser of the main HTML document. BUG=582080 Committed: https://crrev.com/aeb69a7ef733515cc168e9cdd4edcb5642c1b3fc Cr-Commit-Position: refs/heads/master@{#378486} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/aeb69a7ef733515cc168e9cdd4edcb5642c1b3fc Cr-Commit-Position: refs/heads/master@{#378486} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
