|
|
DescriptionMake sure snap_page combined iframe serialized dom
To try this out, run:
./telemetry/bin/snap_page --browser=system --url=https://www.cnn.com
BUG=chromium:763119
COMMIT=false # will split this to smaller CLs when it works end to end
Patch Set 1 #Patch Set 2 : . #
Total comments: 1
Patch Set 3 : . #
Total comments: 10
Patch Set 4 : Rebase & address review comments #
Total comments: 1
Patch Set 5 : . #
Messages
Total messages: 10 (3 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Make sure snap_page combined iframe serialized dom BUG=chromium:763119 COMMIT=false ========== to ========== Make sure snap_page combined iframe serialized dom To try this out, run: ./telemetry/bin/snap_page --browser=system --url=https://www.cnn.com BUG=chromium:763119 COMMIT=false # will split this to smaller CLs when it works end to end ==========
nednguyen@google.com changed reviewers: + pdr@chromium.org, wkorman@chromium.org
This CL is ready for review, PTAL
Exciting! https://codereview.chromium.org/3017573002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/web_contents.py (right): https://codereview.chromium.org/3017573002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/web_contents.py:248: ab.EvaluateJavaScript(..., context_id=context_id). Missing 't' https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/backends/chrome_inspector/inspector_runtime.py (right): https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/backends/chrome_inspector/inspector_runtime.py:72: return self._all_context_ids Is there a doc comment we should update or add here to note the change in return type/contents? Also, unit test? https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/web_contents.py (right): https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/web_contents.py:248: ab.EvaluateJavaScript(..., context_id=context_id). ab -> tab https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/snap_page_util.py (right): https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/snap_page_util.py:27: sys.stdout.write( Why switch to this from print? Just curious. To allow flush? https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/snap_page_util.py:73: step_size = 100000 Add comment noting why we break into step_size chunks and how we came up with the value for step_size (even if it's just, this seems within range of what http conn for dev api allows currently). I see the comment above at Navigate() but feels like a more detailed note here too would be useful. https://codereview.chromium.org/3017573002/diff/60001/telemetry/third_party/s... File telemetry/third_party/snap-it/HTMLSerializer.js (right): https://codereview.chromium.org/3017573002/diff/60001/telemetry/third_party/s... telemetry/third_party/snap-it/HTMLSerializer.js:729: asDict() { Add a unit test for this? May as well while we're moving it, or can do later as P3.
On 2017/09/20 20:11:05, wkorman wrote: > Exciting! > > https://codereview.chromium.org/3017573002/diff/40001/telemetry/telemetry/int... > File telemetry/telemetry/internal/browser/web_contents.py (right): > > https://codereview.chromium.org/3017573002/diff/40001/telemetry/telemetry/int... > telemetry/telemetry/internal/browser/web_contents.py:248: > ab.EvaluateJavaScript(..., context_id=context_id). > Missing 't' > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... > File telemetry/telemetry/internal/backends/chrome_inspector/inspector_runtime.py > (right): > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... > telemetry/telemetry/internal/backends/chrome_inspector/inspector_runtime.py:72: > return self._all_context_ids > Is there a doc comment we should update or add here to note the change in return > type/contents? Also, unit test? > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... > File telemetry/telemetry/internal/browser/web_contents.py (right): > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... > telemetry/telemetry/internal/browser/web_contents.py:248: > ab.EvaluateJavaScript(..., context_id=context_id). > ab -> tab > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... > File telemetry/telemetry/internal/snap_page_util.py (right): > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... > telemetry/telemetry/internal/snap_page_util.py:27: sys.stdout.write( > Why switch to this from print? Just curious. To allow flush? > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... > telemetry/telemetry/internal/snap_page_util.py:73: step_size = 100000 > Add comment noting why we break into step_size chunks and how we came up with > the value for step_size (even if it's just, this seems within range of what http > conn for dev api allows currently). > > I see the comment above at Navigate() but feels like a more detailed note here > too would be useful. > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/third_party/s... > File telemetry/third_party/snap-it/HTMLSerializer.js (right): > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/third_party/s... > telemetry/third_party/snap-it/HTMLSerializer.js:729: asDict() { > Add a unit test for this? May as well while we're moving it, or can do later as > P3. Thanks, I will split this to 3 CLs: 1) Fix the API of ContextID to return a set of context ids 2) Cl to snap-it to refactor asDict 3) this one
On 2017/09/21 15:09:49, nednguyen wrote: > On 2017/09/20 20:11:05, wkorman wrote: > > Exciting! > > > > > https://codereview.chromium.org/3017573002/diff/40001/telemetry/telemetry/int... > > File telemetry/telemetry/internal/browser/web_contents.py (right): > > > > > https://codereview.chromium.org/3017573002/diff/40001/telemetry/telemetry/int... > > telemetry/telemetry/internal/browser/web_contents.py:248: > > ab.EvaluateJavaScript(..., context_id=context_id). > > Missing 't' > > > > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... > > File > telemetry/telemetry/internal/backends/chrome_inspector/inspector_runtime.py > > (right): > > > > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... > > > telemetry/telemetry/internal/backends/chrome_inspector/inspector_runtime.py:72: > > return self._all_context_ids > > Is there a doc comment we should update or add here to note the change in > return > > type/contents? Also, unit test? > > > > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... > > File telemetry/telemetry/internal/browser/web_contents.py (right): > > > > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... > > telemetry/telemetry/internal/browser/web_contents.py:248: > > ab.EvaluateJavaScript(..., context_id=context_id). > > ab -> tab > > > > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... > > File telemetry/telemetry/internal/snap_page_util.py (right): > > > > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... > > telemetry/telemetry/internal/snap_page_util.py:27: sys.stdout.write( > > Why switch to this from print? Just curious. To allow flush? > > > > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... > > telemetry/telemetry/internal/snap_page_util.py:73: step_size = 100000 > > Add comment noting why we break into step_size chunks and how we came up with > > the value for step_size (even if it's just, this seems within range of what > http > > conn for dev api allows currently). > > > > I see the comment above at Navigate() but feels like a more detailed note here > > too would be useful. > > > > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/third_party/s... > > File telemetry/third_party/snap-it/HTMLSerializer.js (right): > > > > > https://codereview.chromium.org/3017573002/diff/60001/telemetry/third_party/s... > > telemetry/third_party/snap-it/HTMLSerializer.js:729: asDict() { > > Add a unit test for this? May as well while we're moving it, or can do later > as > > P3. > > Thanks, I will split this to 3 CLs: > 1) Fix the API of ContextID to return a set of context ids > 2) Cl to snap-it to refactor asDict > 3) this one (1) is landed https://github.com/progers/snap-it/pull/2 to for (2)
https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/backends/chrome_inspector/inspector_runtime.py (right): https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/backends/chrome_inspector/inspector_runtime.py:72: return self._all_context_ids On 2017/09/20 20:11:04, wkorman wrote: > Is there a doc comment we should update or add here to note the change in return > type/contents? Also, unit test? We have unittest for this https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/web_contents.py (right): https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/web_contents.py:248: ab.EvaluateJavaScript(..., context_id=context_id). On 2017/09/20 20:11:04, wkorman wrote: > ab -> tab Done in other CL https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/snap_page_util.py (right): https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/snap_page_util.py:27: sys.stdout.write( On 2017/09/20 20:11:04, wkorman wrote: > Why switch to this from print? Just curious. To allow flush? To allow inline print with \r below https://codereview.chromium.org/3017573002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/snap_page_util.py:73: step_size = 100000 On 2017/09/20 20:11:04, wkorman wrote: > Add comment noting why we break into step_size chunks and how we came up with > the value for step_size (even if it's just, this seems within range of what http > conn for dev api allows currently). > > I see the comment above at Navigate() but feels like a more detailed note here > too would be useful. Done. https://codereview.chromium.org/3017573002/diff/60001/telemetry/third_party/s... File telemetry/third_party/snap-it/HTMLSerializer.js (right): https://codereview.chromium.org/3017573002/diff/60001/telemetry/third_party/s... telemetry/third_party/snap-it/HTMLSerializer.js:729: asDict() { On 2017/09/20 20:11:04, wkorman wrote: > Add a unit test for this? May as well while we're moving it, or can do later as > P3. Acknowledged. I filed a tracking bug here https://github.com/progers/snap-it/issues/3
lgtm https://codereview.chromium.org/3017573002/diff/80001/telemetry/telemetry/int... File telemetry/telemetry/internal/snap_page_util.py (right): https://codereview.chromium.org/3017573002/diff/80001/telemetry/telemetry/int... telemetry/telemetry/internal/snap_page_util.py:79: while k < len(sub_dom_string): Worth adding a unit test to validate the chunking logic? |