Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1920563002:
Only trigger navigation in distiller if in print preview (Closed)
DescriptionOnly trigger navigation in distiller if in print preview
BUG=605432, 611161
Committed: https://crrev.com/80ef02b21fe174b3c6a4588b1f6984b13f4d88f4
Cr-Commit-Position: refs/heads/master@{#393065}
Patch Set 1 #
Total comments: 2
Patch Set 2 : nit #
Total comments: 9
Patch Set 3 : address comments #Patch Set 4 : address comments #
Messages
Total messages: 25 (7 generated)
mdjones@chromium.org changed reviewers: + wychen@chromium.org
ptal
lgtm with nits https://codereview.chromium.org/1920563002/diff/1/components/dom_distiller/co... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1920563002/diff/1/components/dom_distiller/co... components/dom_distiller/core/javascript/dom_distiller_viewer.js:46: if (isLastPage && typeof is_print_preview_distiller != 'undefined' !== feels safer, even though in this case it doesn't matter.
https://codereview.chromium.org/1920563002/diff/1/components/dom_distiller/co... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1920563002/diff/1/components/dom_distiller/co... components/dom_distiller/core/javascript/dom_distiller_viewer.js:46: if (isLastPage && typeof is_print_preview_distiller != 'undefined' On 2016/04/23 03:00:19, wychen wrote: > !== feels safer, even though in this case it doesn't matter. Done.
mdjones@chromium.org changed reviewers: + bauerb@chromium.org
+bauerb for owners approval.
https://codereview.chromium.org/1920563002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1920563002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:143: "var is_print_preview_distiller = true;"); Nit: Arguments should be aligned if they're broken across multiple lines, so you should break before web_contents->GetMainFrame() and indent those four spaces. Or just use clang-format :) Also, I would do the Javascript call via a static method. https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:46: if (isLastPage && typeof is_print_preview_distiller !== 'undefined' If you declare this at the global level and initialize it to false, you don't need this check. Also, the JS style guide doesn't seem to mention global variables, so I think by default the same naming style applies, i.e. lowerInitialCamelCase.
https://codereview.chromium.org/1920563002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1920563002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:143: "var is_print_preview_distiller = true;"); On 2016/04/25 15:36:38, Bernhard Bauer wrote: > Nit: Arguments should be aligned if they're broken across multiple lines, so you > should break before web_contents->GetMainFrame() and indent those four spaces. > Or just use clang-format :) > > Also, I would do the Javascript call via a static method. Done. https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:46: if (isLastPage && typeof is_print_preview_distiller !== 'undefined' On 2016/04/25 15:36:38, Bernhard Bauer wrote: > If you declare this at the global level and initialize it to false, you don't > need this check. Also, the JS style guide doesn't seem to mention global > variables, so I think by default the same naming style applies, i.e. > lowerInitialCamelCase. Updated var name. Unfortunately I can't pre-define the isPrintPreviewDistiller var because this script usually runs after it would be set in the c++ code.
https://codereview.chromium.org/1920563002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1920563002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:143: "var is_print_preview_distiller = true;"); On 2016/04/25 15:36:38, Bernhard Bauer wrote: > Nit: Arguments should be aligned if they're broken across multiple lines, so you > should break before web_contents->GetMainFrame() and indent those four spaces. > Or just use clang-format :) > > Also, I would do the Javascript call via a static method. Sorry, too many nested languages ;-) What I meant was, expose the Javascript functionality via a static method that you would call from here (instead of directly accessing the variable). https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:46: if (isLastPage && typeof is_print_preview_distiller !== 'undefined' On 2016/04/25 16:33:13, mdjones wrote: > On 2016/04/25 15:36:38, Bernhard Bauer wrote: > > If you declare this at the global level and initialize it to false, you don't > > need this check. Also, the JS style guide doesn't seem to mention global > > variables, so I think by default the same naming style applies, i.e. > > lowerInitialCamelCase. > > Updated var name. Unfortunately I can't pre-define the isPrintPreviewDistiller > var because this script usually runs after it would be set in the c++ code. But that's fine, because you'll have declared the variable beforehand at top-level, no? Or is it that this whole file will be evaluated only later? That would seem very strange to me.
https://codereview.chromium.org/1920563002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1920563002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:143: "var is_print_preview_distiller = true;"); On 2016/04/25 17:30:31, Bernhard Bauer wrote: > On 2016/04/25 15:36:38, Bernhard Bauer wrote: > > Nit: Arguments should be aligned if they're broken across multiple lines, so > you > > should break before web_contents->GetMainFrame() and indent those four spaces. > > Or just use clang-format :) > > > > Also, I would do the Javascript call via a static method. > > Sorry, too many nested languages ;-) What I meant was, expose the Javascript > functionality via a static method that you would call from here (instead of > directly accessing the variable). Reverted to just fix indent. The problem is that the script is not evaluated until later, so the function won't be defined by the time I call it here. https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:46: if (isLastPage && typeof is_print_preview_distiller !== 'undefined' On 2016/04/25 17:30:31, Bernhard Bauer wrote: > On 2016/04/25 16:33:13, mdjones wrote: > > On 2016/04/25 15:36:38, Bernhard Bauer wrote: > > > If you declare this at the global level and initialize it to false, you > don't > > > need this check. Also, the JS style guide doesn't seem to mention global > > > variables, so I think by default the same naming style applies, i.e. > > > lowerInitialCamelCase. > > > > Updated var name. Unfortunately I can't pre-define the isPrintPreviewDistiller > > var because this script usually runs after it would be set in the c++ code. > > But that's fine, because you'll have declared the variable beforehand at > top-level, no? Or is it that this whole file will be evaluated only later? That > would seem very strange to me. Yes, the whole file is parsed after the JavaScript is run. This behavior might be the result of how we send content to the page and the fact that we are using a custom scheme. Really this whole mechanism needs to be replaced by a proper signal from the renderer, but I don't currently have the time to implement this solution.
friendly ping
https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:46: if (isLastPage && typeof is_print_preview_distiller !== 'undefined' On 2016/04/25 18:24:08, mdjones wrote: > On 2016/04/25 17:30:31, Bernhard Bauer wrote: > > On 2016/04/25 16:33:13, mdjones wrote: > > > On 2016/04/25 15:36:38, Bernhard Bauer wrote: > > > > If you declare this at the global level and initialize it to false, you > > don't > > > > need this check. Also, the JS style guide doesn't seem to mention global > > > > variables, so I think by default the same naming style applies, i.e. > > > > lowerInitialCamelCase. > > > > > > Updated var name. Unfortunately I can't pre-define the > isPrintPreviewDistiller > > > var because this script usually runs after it would be set in the c++ code. > > > > But that's fine, because you'll have declared the variable beforehand at > > top-level, no? Or is it that this whole file will be evaluated only later? > That > > would seem very strange to me. > > Yes, the whole file is parsed after the JavaScript is run. This behavior might > be the result of how we send content to the page and the fact that we are using > a custom scheme. Really this whole mechanism needs to be replaced by a proper > signal from the renderer, but I don't currently have the time to implement this > solution. That does not seem like the right solution. From the context, it looks like you are trying to fix a flaky test, so you would really do well to fix the order in which things happen here (or find another way of doing it, like passing in the flag to the URL that gets loaded?), otherwise you're just going to run into similar issues later.
On 2016/04/28 16:24:04, Bernhard Bauer wrote: > https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... > File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): > > https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... > components/dom_distiller/core/javascript/dom_distiller_viewer.js:46: if > (isLastPage && typeof is_print_preview_distiller !== 'undefined' > On 2016/04/25 18:24:08, mdjones wrote: > > On 2016/04/25 17:30:31, Bernhard Bauer wrote: > > > On 2016/04/25 16:33:13, mdjones wrote: > > > > On 2016/04/25 15:36:38, Bernhard Bauer wrote: > > > > > If you declare this at the global level and initialize it to false, you > > > don't > > > > > need this check. Also, the JS style guide doesn't seem to mention global > > > > > variables, so I think by default the same naming style applies, i.e. > > > > > lowerInitialCamelCase. > > > > > > > > Updated var name. Unfortunately I can't pre-define the > > isPrintPreviewDistiller > > > > var because this script usually runs after it would be set in the c++ > code. > > > > > > But that's fine, because you'll have declared the variable beforehand at > > > top-level, no? Or is it that this whole file will be evaluated only later? > > That > > > would seem very strange to me. > > > > Yes, the whole file is parsed after the JavaScript is run. This behavior might > > be the result of how we send content to the page and the fact that we are > using > > a custom scheme. Really this whole mechanism needs to be replaced by a proper > > signal from the renderer, but I don't currently have the time to implement > this > > solution. > > That does not seem like the right solution. From the context, it looks like you > are trying to fix a flaky test, so you would really do well to fix the order in > which things happen here (or find another way of doing it, like passing in the > flag to the URL that gets loaded?), otherwise you're just going to run into > similar issues later. The test is flaking due to some additions that were made for the sake of print preview; namely "#loaded" gets added to the URL. This is not instrumental to the core functionality of dom-distiller and should not be included in the core tests. I agree that the print preview code generally needs re-working but the solution (band-aid) is re-implementing what was already there: https://codereview.chromium.org/1575473002/diff/200001/chrome/browser/ui/webu... Alternatively, I can revert that entire change above.
On 2016/04/28 18:03:58, mdjones wrote: > On 2016/04/28 16:24:04, Bernhard Bauer wrote: > > > https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... > > File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): > > > > > https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... > > components/dom_distiller/core/javascript/dom_distiller_viewer.js:46: if > > (isLastPage && typeof is_print_preview_distiller !== 'undefined' > > On 2016/04/25 18:24:08, mdjones wrote: > > > On 2016/04/25 17:30:31, Bernhard Bauer wrote: > > > > On 2016/04/25 16:33:13, mdjones wrote: > > > > > On 2016/04/25 15:36:38, Bernhard Bauer wrote: > > > > > > If you declare this at the global level and initialize it to false, > you > > > > don't > > > > > > need this check. Also, the JS style guide doesn't seem to mention > global > > > > > > variables, so I think by default the same naming style applies, i.e. > > > > > > lowerInitialCamelCase. > > > > > > > > > > Updated var name. Unfortunately I can't pre-define the > > > isPrintPreviewDistiller > > > > > var because this script usually runs after it would be set in the c++ > > code. > > > > > > > > But that's fine, because you'll have declared the variable beforehand at > > > > top-level, no? Or is it that this whole file will be evaluated only later? > > > That > > > > would seem very strange to me. > > > > > > Yes, the whole file is parsed after the JavaScript is run. This behavior > might > > > be the result of how we send content to the page and the fact that we are > > using > > > a custom scheme. Really this whole mechanism needs to be replaced by a > proper > > > signal from the renderer, but I don't currently have the time to implement > > this > > > solution. > > > > That does not seem like the right solution. From the context, it looks like > you > > are trying to fix a flaky test, so you would really do well to fix the order > in > > which things happen here (or find another way of doing it, like passing in the > > flag to the URL that gets loaded?), otherwise you're just going to run into > > similar issues later. > > The test is flaking due to some additions that were made for the sake of print > preview; namely "#loaded" gets added to the URL. This is not instrumental to the > core functionality of dom-distiller and should not be included in the core > tests. I agree that the print preview code generally needs re-working but the > solution (band-aid) is re-implementing what was already there: > > https://codereview.chromium.org/1575473002/diff/200001/chrome/browser/ui/webu... > > Alternatively, I can revert that entire change above. As Matthew said, the tests don't really care about the href part of the committed URL, so to fix the flaky tests, we could've just ignored it when comparing the URLs. This CL is more about fixing the behavior for all non-print-preview use cases, including the desktop experience, where two more navigations are injected, polluting the history stack. We can add another BUG to make the context clearer.
On 2016/05/10 20:34:41, wychen wrote: > On 2016/04/28 18:03:58, mdjones wrote: > > On 2016/04/28 16:24:04, Bernhard Bauer wrote: > > > > > > https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... > > > File components/dom_distiller/core/javascript/dom_distiller_viewer.js > (right): > > > > > > > > > https://codereview.chromium.org/1920563002/diff/20001/components/dom_distille... > > > components/dom_distiller/core/javascript/dom_distiller_viewer.js:46: if > > > (isLastPage && typeof is_print_preview_distiller !== 'undefined' > > > On 2016/04/25 18:24:08, mdjones wrote: > > > > On 2016/04/25 17:30:31, Bernhard Bauer wrote: > > > > > On 2016/04/25 16:33:13, mdjones wrote: > > > > > > On 2016/04/25 15:36:38, Bernhard Bauer wrote: > > > > > > > If you declare this at the global level and initialize it to false, > > you > > > > > don't > > > > > > > need this check. Also, the JS style guide doesn't seem to mention > > global > > > > > > > variables, so I think by default the same naming style applies, i.e. > > > > > > > lowerInitialCamelCase. > > > > > > > > > > > > Updated var name. Unfortunately I can't pre-define the > > > > isPrintPreviewDistiller > > > > > > var because this script usually runs after it would be set in the c++ > > > code. > > > > > > > > > > But that's fine, because you'll have declared the variable beforehand at > > > > > top-level, no? Or is it that this whole file will be evaluated only > later? > > > > That > > > > > would seem very strange to me. > > > > > > > > Yes, the whole file is parsed after the JavaScript is run. This behavior > > might > > > > be the result of how we send content to the page and the fact that we are > > > using > > > > a custom scheme. Really this whole mechanism needs to be replaced by a > > proper > > > > signal from the renderer, but I don't currently have the time to implement > > > this > > > > solution. > > > > > > That does not seem like the right solution. From the context, it looks like > > you > > > are trying to fix a flaky test, so you would really do well to fix the order > > in > > > which things happen here (or find another way of doing it, like passing in > the > > > flag to the URL that gets loaded?), otherwise you're just going to run into > > > similar issues later. > > > > The test is flaking due to some additions that were made for the sake of print > > preview; namely "#loaded" gets added to the URL. This is not instrumental to > the > > core functionality of dom-distiller and should not be included in the core > > tests. I agree that the print preview code generally needs re-working but the > > solution (band-aid) is re-implementing what was already there: > > > > > https://codereview.chromium.org/1575473002/diff/200001/chrome/browser/ui/webu... > > > > Alternatively, I can revert that entire change above. > > As Matthew said, the tests don't really care about the href part of the > committed URL, so to fix the flaky tests, we could've just ignored it when > comparing the URLs. > > This CL is more about fixing the behavior for all non-print-preview use cases, > including the desktop experience, where two more navigations are injected, > polluting the history stack. > > We can add another BUG to make the context clearer. Yeah, that would be good, I think. LGTM with that.
Description was changed from ========== Only trigger navigation in distiller if in print preview BUG=605432 ========== to ========== Only trigger navigation in distiller if in print preview BUG=605432, 611161 ==========
On 2016/05/11 16:44:31, Bernhard Bauer wrote: > On 2016/05/10 20:34:41, wychen wrote: > > As Matthew said, the tests don't really care about the href part of the > > committed URL, so to fix the flaky tests, we could've just ignored it when > > comparing the URLs. > > > > This CL is more about fixing the behavior for all non-print-preview use cases, > > including the desktop experience, where two more navigations are injected, > > polluting the history stack. > > > > We can add another BUG to make the context clearer. > > Yeah, that would be good, I think. LGTM with that. Thank you! Context bug added.
Thanks!
The CQ bit was checked by mdjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wychen@chromium.org Link to the patchset: https://codereview.chromium.org/1920563002/#ps60001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1920563002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920563002/60001
Message was sent while issue was closed.
Description was changed from ========== Only trigger navigation in distiller if in print preview BUG=605432, 611161 ========== to ========== Only trigger navigation in distiller if in print preview BUG=605432, 611161 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Only trigger navigation in distiller if in print preview BUG=605432, 611161 ========== to ========== Only trigger navigation in distiller if in print preview BUG=605432, 611161 Committed: https://crrev.com/80ef02b21fe174b3c6a4588b1f6984b13f4d88f4 Cr-Commit-Position: refs/heads/master@{#393065} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/80ef02b21fe174b3c6a4588b1f6984b13f4d88f4 Cr-Commit-Position: refs/heads/master@{#393065} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
